GNOME Bugzilla – Bug 765019
Operations toolbar button doesn't always disappear when operations finish
Last modified: 2016-04-25 11:51:27 UTC
Created attachment 325887 [details] Button persists in other window The button doesn't disappear when it should (screenshot 1) one one of the windows, and when it's clicked you get an empty list of operations (screenshot 2) To reproduce: 1. Open 2 Nautilus windows 2. Start a long file operation (e.g. copy a big file) 3. Close the popover in each window by clicking in each of the windows in turn The operations button disappears (correctly) from the window you focused last, but not from the other window(s). Generally it happens whenever you have multiple windows open, and you start a long operation (or open multiple windows once the operation has started).
Created attachment 325888 [details] Demonstrates what happens when the button is clicked
Created attachment 326181 [details] [review] toolbar: fix ops button so it gets removed when multiple windows open In some cases, the operations button doesn't get removed from every Nautilus window. And if clicked, an empty popover will appear. One case is when the user starts a long operation, and then closes the popovers in all windows once the operations have completed (and before the buttons are due to be removed). All windows get notified that the operations have finished. But if there's a popover open in any window at that point, the windows don't schedule removal of the button - as the logic is to keep the buttons visible while there are popovers open. When the user then closes the popover in the last window, that window knows there are no popovers in the other windows, so it removes the button from it's toolbar. But there's nothing to notify the other windows to remove their buttons. The fix is to implement a more robust solution; instead of the windows checking the other windows for popovers (windows shouldn't know about the other windows anyway), the progress info manager maintains a list of viewers. When an popover is open or closed, the window tells the manager to update it's list of viewers. When there are no entries in the list, the info manager notifies all listeners (the windows), so they all know when to schedule removal of their buttons.
Review of attachment 326181 [details] [review]: Kudos, much better than that "look at all other windows" workaround I came with. A few insights: ::: libnautilus-private/nautilus-progress-info-manager.c @@ +113,3 @@ + G_TYPE_NONE, + 1, + G_TYPE_BOOLEAN); I would put no parameters at all, and let the clients to query the state directly with nautilus_progess_manager_has_any_viewers @@ +179,3 @@ + +void +nautilus_progess_manager_add_viewer (NautilusProgressInfoManager *self, GObject *viewer) one line per parameter. @@ +185,3 @@ + viewers = self->priv->current_viewers; + if (g_list_find (viewers, viewer) == NULL) { + viewers = g_list_append (viewers, g_object_ref (viewer)); This is dangerous. Taking references to objects without a clear time that you will unreference them. Let me explain with a case: Imagine that the window has a reference to the toolbar, and this manager has another reference to the toolbar. Now there is a really long operation, or even worse, the operation got stuck. Now the user closes the window. But unluckely, the toolbar is still alive because this manager has a reference to it. Therefore all the cleanups to disconnect are not done, and the toolbar will outlive the window. Or even worse, imagine that the toolbar gets destroyed, and you trigger the viewers changed signal with a widget that was destroyed but was not freed. So in C and Nautilus never take a reference if not necessary. You could argue we shouldn't rely on when there are not references to the object (dispose and finalize) to do proper cleanups, and I would agree, because any language with GC would fail to do it, but in C and Nautilus we do it in that way. If it is on your interest, in languages with GC or in any other application with better ownership handling we could rely on gtk_widget_destroy that always call dispose and do the cleanups there and always call gtk_widget_destroy from the owners. Or in case the source is not aware on someone having a reference to it, the owner can connect to the "destroy" GtkWidget signal and make sure to remove it from the list if that happen. In our case, you can use g_object_weak_ref and remove from the list if the object is freed. @@ +211,3 @@ + +gboolean +nautilus_progess_manager_has_any_viewers (NautilusProgressInfoManager *self) *_has_viewers ::: src/nautilus-toolbar.c @@ +476,2 @@ static gboolean +is_all_windows_operations_buttons_inactive (NautilusToolbar* self) remove this function and use directly nautilus_progess_manager_has_any_viewers @@ +737,3 @@ + G_OBJECT (self)); + } + else { } else { @@ +784,3 @@ g_signal_connect (self->priv->progress_manager, "new-progress-info", G_CALLBACK (on_new_progress_info), self); + g_signal_connect (self->priv->progress_manager, "viewers-changed", You are missing to disconnect it somewhere
Some good feedback - thanks, especially for elaborating! Will make those changes tonight. I used strong refs originally as a) no matter how I got rid of the window (whether the popover was open or not), the remove_viewer function was always called - although I wasn't convinced that was always going to be the case, and b) to be consistent with add_info function. Weak refs are a much better idea as they'll guarantee the pointer is removed from the list when the window object is freed. I don't think I'm missing a disconnect - there's already this line: g_signal_handlers_disconnect_by_data (self->priv->progress_manager, self);
(In reply to Neil Herald from comment #4) > Some good feedback - thanks, especially for elaborating! > > Will make those changes tonight. I used strong refs originally as a) no > matter how I got rid of the window (whether the popover was open or not), > the remove_viewer function was always called - although I wasn't convinced > that was always going to be the case, and b) to be consistent with add_info > function. Weak refs are a much better idea as they'll guarantee the pointer > is removed from the list when the window object is freed. And probably that's the current case and this code is safe right now. But we can ensure about that in the future, and this management issues are really really hard to spot. Is where my time sinks debugging when it happens. > > I don't think I'm missing a disconnect - there's already this line: > g_signal_handlers_disconnect_by_data (self->priv->progress_manager, self); Ah right :)
Created attachment 326358 [details] [review] toolbar: fix ops button so it gets removed when multiple windows open In some cases, the operations button doesn't get removed from every Nautilus window. And if clicked, an empty popover will appear. One case is when the user starts a long operation, and then closes the popovers in all windows once the operations have completed (and before the buttons are due to be removed). All windows get notified that the operations have finished. But if there's a popover open in any window at that point, the windows don't schedule removal of the button - as the logic is to keep the buttons visible while there are popovers open. When the user then closes the popover in the last window, that window knows there are no popovers in the other windows, so it removes the button from it's toolbar. But there's nothing to notify the other windows to remove their buttons. The fix is to implement a more robust solution; instead of the windows checking the other windows for popovers (windows shouldn't know about the other windows anyway), the progress info manager maintains a list of viewers. When an popover is open or closed, the window tells the manager to update it's list of viewers. When there are no entries in the list, the info manager notifies all listeners (the windows), so they all know when to schedule removal of their buttons.
(In reply to Carlos Soriano from comment #5) > And probably that's the current case and this code is safe right now. But we > can ensure about that in the future, and this management issues are really > really hard to spot. Is where my time sinks debugging when it happens. I know, I deserve the book to be thrown at me for that one! Only thing I was a bit unsure of was whether to unref all remaining entries in the list in the progress manager finalize method. There shouldn't be anything in the list when finalize is called, unless it's finalized before the windows are. In which case I'm pretty sure you'd need to do it.
Review of attachment 326358 [details] [review]: Looks mostly good now! Just some style nitpicks that I didn't caught before. ::: libnautilus-private/nautilus-progress-info-manager.c @@ +203,3 @@ + self->priv->current_viewers = viewers; + + if (g_list_length (viewers) == 1) This can be kinda confusing since the signal is named viewers-changed. It changes for every add/remove, so just unconditionally signal it just for code clarity sake, since it doesn't involve a performance issue (one signal per window) or name the signal has-viewers-changed @@ +213,3 @@ +{ + if (g_list_find (self->priv->current_viewers, viewer) != NULL) { + g_object_weak_unref (viewer, (GWeakNotify) remove_viewer, self); I think this would look better in remove_viewer (I think it's clearer to also remove the weak reference when the object is finalized, although I believe doesn't really matter) ::: src/nautilus-toolbar.c @@ +739,3 @@ +static void +on_progress_viewers_changed (NautilusProgressInfoManager *manager, + NautilusToolbar *self) if you go with changing the signal name, change also this to has_viewers_changed.
(In reply to Carlos Soriano from comment #8) > + if (g_list_length (viewers) == 1) > > This can be kinda confusing since the signal is named viewers-changed. It > changes for every add/remove, so just unconditionally signal it just for > code clarity sake, since it doesn't involve a performance issue (one signal > per window) or name the signal has-viewers-changed Yep, it is. Really the toolbar is only interested in when a) # of viewers has increased to 0 -> 1, and b) # viewers has decreased 1 -> 0. The other cases are irrelevant, and unfortunately just relying on has_viewers doesn't give enough info to be able to distinguish from other cases (e.g. a decrease of 2 -> 1). But thinking about it - the toolbar can handle these other cases, it just means it'll be calling unshedule_remove_operations_popover more times than needed. I can live with that though. > + g_object_weak_unref (viewer, (GWeakNotify) remove_viewer, > self); > > I think this would look better in remove_viewer (I think it's clearer to > also remove the weak reference when the object is finalized, although I > believe doesn't really matter) I had it this way originally - the code is much nicer that way. Unfortunately weak_unref generates a warning in the logs when it's called from the weak reference callback (yes - seriously! even though the docs don't mention anything about how it can/can't be used) :( Would happily revert though if you're fine with that?
(In reply to Neil Herald from comment #9) > (In reply to Carlos Soriano from comment #8) > > + if (g_list_length (viewers) == 1) > > > > This can be kinda confusing since the signal is named viewers-changed. It > > changes for every add/remove, so just unconditionally signal it just for > > code clarity sake, since it doesn't involve a performance issue (one signal > > per window) or name the signal has-viewers-changed > Yep, it is. Really the toolbar is only interested in when a) # of viewers > has increased to 0 -> 1, and b) # viewers has decreased 1 -> 0. The other > cases are irrelevant, and unfortunately just relying on has_viewers doesn't > give enough info to be able to distinguish from other cases (e.g. a decrease > of 2 -> 1). But thinking about it - the toolbar can handle these other > cases, it just means it'll be calling unshedule_remove_operations_popover > more times than needed. I can live with that though. > As you said, we are only interested on 0->1 and 1->0, so I guess renaming the signal to has-viewers-changed is the best way (not sure I understood your last part of the comment though) > > > + g_object_weak_unref (viewer, (GWeakNotify) remove_viewer, > > self); > > > > I think this would look better in remove_viewer (I think it's clearer to > > also remove the weak reference when the object is finalized, although I > > believe doesn't really matter) > I had it this way originally - the code is much nicer that way. > Unfortunately weak_unref generates a warning in the logs when it's called > from the weak reference callback (yes - seriously! even though the docs > don't mention anything about how it can/can't be used) :( Would happily > revert though if you're fine with that? aha! well, then the current patch is fine (warnings are never accepted).
Created attachment 326579 [details] [review] toolbar: fix ops button so it gets removed when multiple windows open In some cases, the operations button doesn't get removed from every Nautilus window. And if clicked, an empty popover will appear. One case is when the user starts a long operation, and then closes the popovers in all windows once the operations have completed (and before the buttons are due to be removed). All windows get notified that the operations have finished. But if there's a popover open in any window at that point, the windows don't schedule removal of the button - as the logic is to keep the buttons visible while there are popovers open. When the user then closes the popover in the last window, that window knows there are no popovers in the other windows, so it removes the button from it's toolbar. But there's nothing to notify the other windows to remove their buttons. The fix is to implement a more robust solution; instead of the windows checking the other windows for popovers (windows shouldn't know about the other windows anyway), the progress info manager maintains a list of viewers. When an popover is open or closed, the window tells the manager to update it's list of viewers. When there are no entries in the list, the info manager notifies all listeners (the windows), so they all know when to schedule removal of their buttons.
Review of attachment 326579 [details] [review]: Pushed as 2774b85
Hey, probably I didn't make it clear before... but never push until the patch is marked as accepted-commit-now even if they are just style nitpicks. You never know when you could introduce a small error or I didn't catch something. For me, the review before I mark it as accepted-commit-now is the one that I spend more time on to make sure everything is ok.
Apologies. It was a misinterpretation on my part
(In reply to Neil Herald from comment #14) > Apologies. It was a misinterpretation on my part No worries at all :) Note that even if some error is introduced, it is perfectly fine to revert the patch or just make a followup patch.