GNOME Bugzilla – Bug 786835
FileChooserButton refilters model in :popped-up handler, breaking assumptions of ComboBox
Last modified: 2018-05-02 18:57:14 UTC
GtkFileChooserButton has a [None] item, indicating nothing selected. The model it sets on its internal GtkComboBox is a GtkTreeModelFilter. This exists to - among other things - show the [None] item when the popup is popped-down but hide it while popping up, as it was judged to be unwanted in the popup list/menu. The problem here is that the ComboBox needs to do its own stuff with the model while popping up/down. If the user alters the model in a conflicting way, ComboBox uses invalid iterators to the model, causing crashes. FileChooserButton in both modes was breaking assumptions caused by ComboBox doing the following: * getting the active item BEFORE popping up the GtkMenu in menu mode, to centre it over the button, then selecting that item in the menu AFTER it pops up - worked around by commit e98e6f73be8ec7647d6eddeacf6d13cd96352589 * getting the clicked iter BEFORE popping down, but only using it AFTER popping down, to avoid potentially confusing grabs in users' ::changed handlers - worked around by commit 696b9a5df7fc89bf007a11c7f8b6a6a913229feb (hashes are for GTK+ 3, but these problems also exist in 2 and 4 where relevant) These patches may interfere with expectations elsewhere, and they shouldn't be necessary: FileChooserButton or any other user of ComboBox should not pull the model out from under the ComboBox while it's doing internal housekeeping. By letting them continue to do so, we incur the hassle of having to work around the resulting problems elsewhere, which may break different scenarios in the process. The real fix is to stop tolerating widgets that do this, and fix them. (filing under FileChooser as it's closest; hopefully the FCB is the only internal user that does this. If it's not, this will need to be a general bug)
The behaviour hiding the (None) item is from commit 67f5e595a796a8321d6dc7737c58476564998c07 IMO this clearly causes more problems than it solves. Besides, I think it would be useful to have a way to unselect the file, as that seems like a valid action - though the commit provides no context of whether that ever worked, i.e. whether the (None) item was removed due to functional issues or purely for cosmetics. That commit took a bunch of other changes along for the ride, so we couldn't just revert it if the decision is made, but it should be simple enough to disconnect the handler for ComboBox:popup-shown.
adding See Also links to related bugs
(In reply to Daniel Boles from comment #1) > IMO this clearly causes more problems than it solves. Besides, I think it > would be useful to have a way to unselect the file, as that seems like a > valid action [...] if the decision is made, but it should be simple enough to > disconnect the handler for ComboBox:popup-shown. If not, maybe we can move the ugly workaround to the other side, by making FCB's :popup-shown handler defer its changes of the model to an idle handler. If that works, it's better: FCB can bear the burden of ugly code, for its ugly decisions, and ComboBox can stop accounting for users doing silly things during popup/down.
(In reply to Daniel Boles from comment #3) > If not, maybe we can move the ugly workaround to the other side, by making > FCB's :popup-shown handler defer its changes of the model to an idle > handler. If that works Yes, this seems to work. In the popping-up case, there is of course a brief moment where the (None) item can be seen at the bottom of the popup, selected, before being removed. Though not ideal, I think this is benign enough - and tests indicate it can happen sometimes without the idle handler, anyway, if maybe not quite so noticeably. Also: * if the ComboBox is in menu mode, losing (None) means it has nothing selected in the menu once the idle runs, and the menu gets positioned below the button. Before, the top item would get focus, and the menu would pop above the button. * in list mode, as (None) gets hidden, the above item - Other - gets the focus. This was already the case. Nothing seems to change here with my proposal. These little side-effects seem harmless enough, I think, right?
(In reply to Daniel Boles from comment #4) > * if the ComboBox is in menu mode, losing (None) means it has nothing > selected > in the menu once the idle runs, and the menu gets positioned below the > button. > Before, the top item would get focus, and the menu would pop above the > button. correction: The menu is positioned above the button in both cases. So the change in default selection is the only real difference here.
Created attachment 358513 [details] [review] FileChooserButton: Don’t alter model in popup/down ComboBox historically did not, and should not have to, expect that users might alter its model in popup/down. If it gets an iter, pops up/down, then uses the iter, altering the model means the iter is now invalid. If FCB insists on hiding (None) when :popup-shown changes, let’s put the ugly workaround in FCB rather than expecting ComboBox to account for it. Specifically, defer altering the model to an idle handler, so ComboBox can do all of its stuff with the same model, and FileChooserButton does its own, questionable stuff after ComboBox returns to the main loop.
Review of attachment 358513 [details] [review]: ::: gtk/gtkfilechooserbutton.c @@ +2871,3 @@ + button); + g_source_set_name_by_id (priv->popup_shown_idle_id, + "[gtk+] FileChooser: combo_box_popup_shown_idle"); should be FileChooserButton, of course
As for the already inconsistent, now slightly different behaviour of the default focus in the popup - we could probably resolve that by using the private gtk_combo_box_get_popup() and using its type's API (either list or menu in GTK+ 3) to focus e.g. the first item - but the question is whether it's worth doing, as it looks like a bit of a hassle to get to the Menu or TreeView from there.
Created attachment 358530 [details] [review] FileChooserButton: Don’t alter model in popup/down ComboBox historically did not, and should not have to, expect that users might alter its model in popup/down. If it gets an iter, pops up/down, then uses the iter, altering the model means the iter is now invalid. If FCB insists on hiding (None) when :popup-shown changes, let’s put the ugly workaround in FCB rather than expecting ComboBox to account for it. Specifically, defer altering the model to an idle handler, so ComboBox can do all of its stuff with the same model, and FileChooserButton does its own, questionable stuff after ComboBox returns to the main loop. -- fixes the source name
Created attachment 358531 [details] [review] FileChooserButton: Always select 1st item in Popup …when in the idle handler that hides the (None) option when popped-up. Previously, list mode would select the last item (Other), whereas menu mode would select the 1st item before the previous commit and none after -- This works, but is it me, or is it too much hassle?
Review of attachment 358530 [details] [review]: ::: gtk/gtkfilechooserbutton.c @@ +198,2 @@ gint icon_size; + guint popup_shown_idle_id; g_signal_connect() returns a gulong, not a guint.
Created attachment 358718 [details] [review] FileChooserButton: Don’t alter model in popup/down ComboBox historically did not, and should not have to, expect that users might alter its model in popup/down. If it gets an iter, pops up/down, then uses the iter, altering the model means the iter is now invalid. If FCB insists on hiding (None) when :popup-shown changes, let’s put the ugly workaround in FCB rather than expecting ComboBox to account for it. Specifically, defer altering the model to an idle handler, so ComboBox can do all of its stuff with the same model, and FileChooserButton does its own, questionable stuff after ComboBox returns to the main loop.
Ping; any thoughts on Attachment 358718 [details]? This would let us revert two (1 in master as it lacks list mode) kludges in ComboBox and avoid setting an awful example in FileChooserButton (as now we'd be setting a silly-but-tolerable example instead).
Review of attachment 358718 [details] [review]: This works fine in GTK+ 3 and 4 but in GTK+ 2, it's problematic in menu mode because it causes this. * you click the button * the menu pops up * (None) gets hidden * the menu repositions so that its 1st item is directly under the cursor * you release the click * you accidentally select the 1st item. This is dodged in 3 and 4 because the menu pops up *above*, not over the button. I might just leave 2 how it is for now (assuming another release is going to happen there, anyway) ::: gtk/gtkfilechooserbutton.c @@ +2902,3 @@ + priv->popup_shown_idle_id = 0; + + return FALSE; => G_SOURCE_REMOVE
(In reply to Daniel Boles from comment #14) > Review of attachment 358718 [details] [review] [review]: > > This works fine in GTK+ 3 and 4 > > but in GTK+ 2, it's problematic in menu mode because it causes this. > > * you click the button > * the menu pops up > * (None) gets hidden > * the menu repositions so that its 1st item is directly under the cursor > * you release the click > * you accidentally select the 1st item. > > This is dodged in 3 and 4 because the menu pops up *above*, not over the > button. Also, even if the item appeared below the pointer, this would presumably block the inadvertent selection: https://bugzilla.gnome.org/show_bug.cgi?id=703069#c3
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gtk/issues/889.