Commit 68b09bca authored by Eric Gregory's avatar Eric Gregory

1502 deadlock in slideshow fix

This eliminates a deadlock in BackgroundJob and moves the next/previous controls into SinglePhotoPage along with the hacky fix we have for when the user holds down the next/prev key.

This does uncover two more issues, which I've filed tickets for.
1. The hacky fix is entirely time-dependent. On a slow enough computer it won't work. Ticket #3892
2. Slideshow transitions shouldn't be occurring when I use next/prev. It should just jump to the next photo asap. Ticket #3891
parent 3d33f199
......@@ -1827,6 +1827,7 @@ public abstract class CheckerboardPage : Page {
public abstract class SinglePhotoPage : Page {
public const Gdk.InterpType FAST_INTERP = Gdk.InterpType.NEAREST;
public const Gdk.InterpType QUALITY_INTERP = Gdk.InterpType.BILINEAR;
public const int KEY_REPEAT_INTERVAL_MSEC = 200;
public enum UpdateReason {
NEW_PIXBUF,
......@@ -1853,6 +1854,7 @@ public abstract class SinglePhotoPage : Page {
private bool zoom_high_quality = true;
private ZoomState saved_zoom_state;
private bool has_saved_zoom_state = false;
private uint32 last_nav_key = 0;
public SinglePhotoPage(string page_name, bool scale_up_to_viewport) {
base(page_name);
......@@ -2300,6 +2302,49 @@ public abstract class SinglePhotoPage : Page {
protected override bool on_context_keypress() {
return popup_context_menu(get_page_context_menu());
}
protected virtual void on_previous_photo() {
}
protected virtual void on_next_photo() {
}
public override bool key_press_event(Gdk.EventKey event) {
// if the user holds the arrow keys down, we will receive a steady stream of key press
// events for an operation that isn't designed for a rapid succession of output ...
// we staunch the supply of new photos to under a quarter second (#533)
bool nav_ok = (event.time - last_nav_key) > KEY_REPEAT_INTERVAL_MSEC;
bool handled = true;
switch (Gdk.keyval_name(event.keyval)) {
case "Left":
case "KP_Left":
case "BackSpace":
if (nav_ok) {
on_previous_photo();
last_nav_key = event.time;
}
break;
case "Right":
case "KP_Right":
case "space":
if (nav_ok) {
on_next_photo();
last_nav_key = event.time;
}
break;
default:
handled = false;
break;
}
if (handled)
return true;
return (base.key_press_event != null) ? base.key_press_event(event) : true;
}
}
//
......
......@@ -370,7 +370,6 @@ public abstract class EditingHostPage : SinglePhotoPage {
public const int TOOL_WINDOW_SEPARATOR = 8;
public const int PIXBUF_CACHE_COUNT = 5;
public const int ORIGINAL_PIXBUF_CACHE_COUNT = 5;
public const int KEY_REPEAT_INTERVAL_MSEC = 200;
private class EditingHostCanvas : PhotoCanvas {
private EditingHostPage host_page;
......@@ -403,7 +402,6 @@ public abstract class EditingHostPage : SinglePhotoPage {
private EditingTool current_tool = null;
private Gtk.ToggleToolButton current_editing_toggle = null;
private Gdk.Pixbuf cancel_editing_pixbuf = null;
private uint32 last_nav_key = 0;
private bool photo_missing = false;
private PixbufCache cache = null;
private PixbufCache master_cache = null;
......@@ -1685,32 +1683,9 @@ public abstract class EditingHostPage : SinglePhotoPage {
if (on_zoom_slider_key_press(event))
return true;
// if the user holds the arrow keys down, we will receive a steady stream of key press
// events for an operation that isn't designed for a rapid succession of output ...
// we staunch the supply of new photos to under a quarter second (#533)
bool nav_ok = (event.time - last_nav_key) > KEY_REPEAT_INTERVAL_MSEC;
bool handled = true;
switch (Gdk.keyval_name(event.keyval)) {
case "Left":
case "KP_Left":
case "BackSpace":
if (nav_ok) {
on_previous_photo();
last_nav_key = event.time;
}
break;
case "Right":
case "KP_Right":
case "space":
if (nav_ok) {
on_next_photo();
last_nav_key = event.time;
}
break;
// this block is only here to prevent base from moving focus to toolbar
case "Down":
case "KP_Down":
......@@ -2129,7 +2104,7 @@ public abstract class EditingHostPage : SinglePhotoPage {
tool_window.present();
}
public void on_next_photo() {
protected override void on_next_photo() {
deactivate_tool();
if (!has_photo())
......@@ -2162,7 +2137,7 @@ public abstract class EditingHostPage : SinglePhotoPage {
}
}
public void on_previous_photo() {
protected override void on_previous_photo() {
deactivate_tool();
if (!has_photo())
......
......@@ -27,13 +27,11 @@ public class PixbufCache : Object {
public FetchJob(PixbufCache owner, BackgroundJob.JobPriority priority, Photo photo,
Scaling scaling, CompletionCallback callback) {
base(owner, callback, new Cancellable());
base(owner, callback, new Cancellable(), null, new Semaphore());
this.priority = priority;
this.photo = photo;
this.scaling = scaling;
set_completion_semaphore(new Semaphore());
}
public override BackgroundJob.JobPriority get_priority() {
......
......@@ -199,7 +199,7 @@ class SlideshowPage : SinglePhotoPage {
Gtk.ToolButton previous_button = new Gtk.ToolButton.from_stock(Gtk.Stock.GO_BACK);
previous_button.set_label(_("Back"));
previous_button.set_tooltip_text(_("Go to the previous photo"));
previous_button.clicked.connect(on_previous);
previous_button.clicked.connect(on_previous_photo);
toolbar.insert(previous_button, -1);
......@@ -213,7 +213,7 @@ class SlideshowPage : SinglePhotoPage {
Gtk.ToolButton next_button = new Gtk.ToolButton.from_stock(Gtk.Stock.GO_FORWARD);
next_button.set_label(_("Next"));
next_button.set_tooltip_text(_("Go to the next photo"));
next_button.clicked.connect(on_next);
next_button.clicked.connect(on_next_photo);
toolbar.insert(next_button, -1);
......@@ -319,7 +319,7 @@ class SlideshowPage : SinglePhotoPage {
timer.start();
}
private void on_previous() {
protected override void on_previous_photo() {
DataView view = controller.get_view_for_source(current);
Photo? prev_photo = null;
......@@ -343,7 +343,7 @@ class SlideshowPage : SinglePhotoPage {
advance(prev_photo, Direction.BACKWARD);
}
private void on_next() {
protected override void on_next_photo() {
DataView view = controller.get_view_for_source(current);
Photo? next_photo = null;
......@@ -389,7 +389,7 @@ class SlideshowPage : SinglePhotoPage {
if (timer.elapsed() < Config.Facade.get_instance().get_slideshow_delay())
return true;
on_next();
on_next_photo();
return true;
}
......@@ -401,16 +401,6 @@ class SlideshowPage : SinglePhotoPage {
on_play_pause();
break;
case "Left":
case "KP_Left":
on_previous();
break;
case "Right":
case "KP_Right":
on_next();
break;
default:
handled = false;
break;
......
......@@ -111,11 +111,13 @@ public abstract class BackgroundJob {
private int notification_priority = Priority.DEFAULT_IDLE;
public BackgroundJob(Object? owner = null, CompletionCallback? callback = null,
Cancellable? cancellable = null, CancellationCallback? cancellation = null) {
Cancellable? cancellable = null, CancellationCallback? cancellation = null,
AbstractSemaphore? completion_semaphore = null) {
this.owner = owner;
this.callback = callback;
this.cancellable = cancellable;
this.cancellation = cancellation;
this.semaphore = completion_semaphore;
}
public abstract void execute();
......@@ -144,22 +146,12 @@ public abstract class BackgroundJob {
notification_priority = priority;
}
// This method is thread-safe, however, because of race conditions between setting a semaphore
// and another thread waiting on it, it's best to set this before enqueuing the job.
public void set_completion_semaphore(AbstractSemaphore semaphore) {
lock (this.semaphore) {
this.semaphore = semaphore;
}
}
// This method is thread-safe, but only waits if a completion semaphore has been set, otherwise
// exits immediately. Note that blocking for a semaphore does NOT spin the event loop, so a
// thread relying on it to continue should not use this.
public void wait_for_completion() {
lock (semaphore) {
if (semaphore != null)
semaphore.wait();
}
if (semaphore != null)
semaphore.wait();
}
public Cancellable? get_cancellable() {
......@@ -177,11 +169,8 @@ public abstract class BackgroundJob {
// This should only be called by Workers. Beware to all who fail to heed.
public void internal_notify_completion() {
// notify anyone waiting
lock (semaphore) {
if (semaphore != null)
semaphore.notify();
}
if (semaphore != null)
semaphore.notify();
if (callback == null && cancellation == null)
return;
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment