After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 770508 - [REGRESSION][BISECTED] Recent change in GtkTreeView::grab_focus_and_unset_draw_keyfocus() breaks shortcut assignment
[REGRESSION][BISECTED] Recent change in GtkTreeView::grab_focus_and_unset_dra...
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Widget: GtkTreeView
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gtktreeview-bugs
gtktreeview-bugs
Depends on:
Blocks:
 
 
Reported: 2016-08-28 12:35 UTC by Roman Lebedev
Modified: 2016-09-11 15:50 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Revert "treeview: Ensure the treeview has the implicit grab before grabbing focus" (1.06 KB, patch)
2016-08-28 12:36 UTC, Roman Lebedev
rejected Details | Review

Description Roman Lebedev 2016-08-28 12:35:27 UTC
This is a regression.

Commit e33e23a6d9403f634003e6fc611ba7d02b5cf950 e.g. breaks shortcut re-assignment view. (darktable -> gear box to open preferences -> shortcuts tab, try to change any shortcut.)

~/src/gtk$ git bisect log
git bisect start
# bad: [b05ed13710dc52c11bb1fde6f6aaf064d8ce5c58] GDK W32: Remove obsolete assertions
git bisect bad b05ed13710dc52c11bb1fde6f6aaf064d8ce5c58
# good: [8c9e611b269877a713ae418253789fda5e423002] 3.14.15
git bisect good 8c9e611b269877a713ae418253789fda5e423002
# good: [09da977dc92ad63ded1a2323a41612302c7d19cf] 3.14.1
git bisect good 09da977dc92ad63ded1a2323a41612302c7d19cf
# good: [541926089ae8e1997360f2547adba1e933a15689] notebook: Set prelight state properly
git bisect good 541926089ae8e1997360f2547adba1e933a15689
# good: [1a524f374c2baeb035a1dd5e51a42cbf92b1d586] Updated Polish translation
git bisect good 1a524f374c2baeb035a1dd5e51a42cbf92b1d586
# good: [89973118f3a3624e36cdbe6ca8235f75e8281f8e] Add some more visual tests
git bisect good 89973118f3a3624e36cdbe6ca8235f75e8281f8e
# good: [0c37b057167f5da7fb4be1e182d8dda3de413264] cssimage: Make it possible to shrink builtin check/optionmarks
git bisect good 0c37b057167f5da7fb4be1e182d8dda3de413264
# bad: [38fbe68e83133fbbe4487fc9e8f77dfee5bbba00] gdk: do not provide display command line argument on windows
git bisect bad 38fbe68e83133fbbe4487fc9e8f77dfee5bbba00
# good: [7c397c621cb6ba7ba3575083b1510f226e7e25f4] headerbar: do not show buttons for modals/transients
git bisect good 7c397c621cb6ba7ba3575083b1510f226e7e25f4
# good: [2d38c40f789ecc5df4de0720c13496eee0429cd7] gdk: Explicitly create a cairo context inside GdkDrawingContext
git bisect good 2d38c40f789ecc5df4de0720c13496eee0429cd7
# bad: [dff61b02baf67d3f21d4c3920cbf710ded2dc7d0] Updates
git bisect bad dff61b02baf67d3f21d4c3920cbf710ded2dc7d0
# good: [bb2ca3b94d53c009c78d0f364c41ae0220ea2c9e] wayland: fix error handling for memfd_create
git bisect good bb2ca3b94d53c009c78d0f364c41ae0220ea2c9e
# bad: [e33e23a6d9403f634003e6fc611ba7d02b5cf950] treeview: Ensure the treeview has the implicit grab before grabbing focus
git bisect bad e33e23a6d9403f634003e6fc611ba7d02b5cf950
# good: [ed2bb7e012989804eae3a02968944cbd70568ae5] Updated Brazilian Portuguese translation
git bisect good ed2bb7e012989804eae3a02968944cbd70568ae5
# good: [76bacfde6e5305021c765ae690972efb1832682d] style cascade: Allow cascades with more ancestors
git bisect good 76bacfde6e5305021c765ae690972efb1832682d
# good: [51799d41e46553633a7e98a3b721eadba67bd5ba] GtkActionHelper: Change a message to a warning
git bisect good 51799d41e46553633a7e98a3b721eadba67bd5ba
# first bad commit: [e33e23a6d9403f634003e6fc611ba7d02b5cf950] treeview: Ensure the treeview has the implicit grab before grabbing focus
# bad: [e33e23a6d9403f634003e6fc611ba7d02b5cf950] treeview: Ensure the treeview has the implicit grab before grabbing focus
git bisect bad e33e23a6d9403f634003e6fc611ba7d02b5cf950
# first bad commit: [e33e23a6d9403f634003e6fc611ba7d02b5cf950] treeview: Ensure the treeview has the implicit grab before grabbing focus
# good: [51799d41e46553633a7e98a3b721eadba67bd5ba] GtkActionHelper: Change a message to a warning
git bisect good 51799d41e46553633a7e98a3b721eadba67bd5ba
# first bad commit: [e33e23a6d9403f634003e6fc611ba7d02b5cf950] treeview: Ensure the treeview has the implicit grab before grabbing focus
Comment 1 Roman Lebedev 2016-08-28 12:36:35 UTC
Created attachment 334306 [details] [review]
Revert "treeview: Ensure the treeview has the implicit grab before grabbing focus"

As suggested in IRC, i'm attaching a patch that fixes the regression.
Comment 2 Carlos Garnacho 2016-08-28 14:19:16 UTC
And you of course tested that the original bug (#767468) is still fixed... :)
Comment 3 Roman Lebedev 2016-08-28 14:21:33 UTC
I of course did not.
Comment 4 Carlos Garnacho 2016-08-28 15:31:11 UTC
Comment on attachment 334306 [details] [review]
Revert "treeview: Ensure the treeview has the implicit grab before grabbing focus"

Well, then you surely expected this. A smaller testcase than "all of darktable" is welcome.
Comment 5 Roman Lebedev 2016-08-28 15:59:18 UTC
(In reply to Carlos Garnacho from comment #4)
> Comment on attachment 334306 [details] [review] [review]
> Revert "treeview: Ensure the treeview has the implicit grab before grabbing
> focus"
> 
> Well, then you surely expected this.
Then let's not talk about the this specific patch, but about regression in general :)
Or is that now an acceptable thing in gtk+?

> A smaller testcase than "all of darktable" is welcome.
Sure, feel free to add.
Comment 6 Roman Lebedev 2016-08-28 16:35:13 UTC
(In reply to Carlos Garnacho from comment #4)
> Comment on attachment 334306 [details] [review] [review]
> Revert "treeview: Ensure the treeview has the implicit grab before grabbing
> focus"
> 
> Well, then you surely expected this.
BTW, simple answer: i'm quite sure my patch re-introduces https://bugzilla.gnome.org/show_bug.cgi?id=767468 because it simply reverts it's fix, because the fix causes a regression.
Comment 7 Carlos Garnacho 2016-08-28 16:36:53 UTC
(In reply to Roman Lebedev from comment #5)
> (In reply to Carlos Garnacho from comment #4)
> > Comment on attachment 334306 [details] [review] [review] [review]
> > Revert "treeview: Ensure the treeview has the implicit grab before grabbing
> > focus"
> > 
> > Well, then you surely expected this.
> Then let's not talk about the this specific patch, but about regression in
> general :)
> Or is that now an acceptable thing in gtk+?

I haven't closed this bug, have I? But that patch as is introduces regressions, and I just won't trade a regression for another.

> 
> > A smaller testcase than "all of darktable" is welcome.
> Sure, feel free to add.

Thanks for your cooperation, I'll look into this, at some point.
Comment 8 Roman Lebedev 2016-08-30 10:25:56 UTC
(In reply to Carlos Garnacho from comment #7)
> (In reply to Roman Lebedev from comment #5)
> > (In reply to Carlos Garnacho from comment #4)
> > > Comment on attachment 334306 [details] [review] [review] [review] [review]
> > > Revert "treeview: Ensure the treeview has the implicit grab before grabbing
> > > focus"
> > > 
> > > Well, then you surely expected this.
> > Then let's not talk about the this specific patch, but about regression in
> > general :)
> > Or is that now an acceptable thing in gtk+?
> 
> I haven't closed this bug, have I?

>But that patch as is introduces regressions, and I just won't trade a regression for another.
Just for the history, i feel like pointing out that it is the exactly what your commit did.
Commit e33e23a6d9403f634003e6fc611ba7d02b5cf950 introduced regression, while fixing another one.
While the original #767468 regression is a simple display issue, it was traded for input issue...

> > 
> > > A smaller testcase than "all of darktable" is welcome.
> > Sure, feel free to add.
> 
> Thanks for your cooperation, I'll look into this, at some point.
Comment 9 LRN 2016-08-30 10:37:06 UTC
(In reply to Roman Lebedev from comment #8)
> (In reply to Carlos Garnacho from comment #7)
> > (In reply to Roman Lebedev from comment #5)
> > > (In reply to Carlos Garnacho from comment #4)
> > > > Comment on attachment 334306 [details] [review] [review] [review] [review] [review]
> > > > Revert "treeview: Ensure the treeview has the implicit grab before grabbing
> > > > focus"
> > > > 
> > > > Well, then you surely expected this.
> > > Then let's not talk about the this specific patch, but about regression in
> > > general :)
> > > Or is that now an acceptable thing in gtk+?
> > 
> > I haven't closed this bug, have I?
> 
> >But that patch as is introduces regressions, and I just won't trade a regression for another.
> Just for the history, i feel like pointing out that it is the exactly what
> your commit did.
> Commit e33e23a6d9403f634003e6fc611ba7d02b5cf950 introduced regression, while
> fixing another one.
> While the original #767468 regression is a simple display issue, it was
> traded for input issue...

As the person who filed that bug, i can tell you that it was not a "simple display issue". It was a "shit, i can't access the popover menu, which is the only way to access a particular piece of functionality" issue.

> 
> > > 
> > > > A smaller testcase than "all of darktable" is welcome.
> > > Sure, feel free to add.
> > 
> > Thanks for your cooperation, I'll look into this, at some point.

I feel like pointing out that this remark probably means "It will be *much* easier to fix this if we had a small testcase instead of the whole 3rd-party application. See how previous bug had a testcase? Provide something like that". Reproducing the bug in gtk3-demo or gtk3-widget-factory should also be helpful. These are the things that you can do to help.
Comment 10 Kjell Ahlstedt 2016-09-10 07:42:08 UTC
Roman, does my patch in bug 770130 comment 8 fix this bug?
Comment 11 Kjell Ahlstedt 2016-09-11 08:20:37 UTC
That patch was not accepted. If you test a patch in bug 770130, you'd better
test the one in comment 11.
Comment 12 Matthias Clasen 2016-09-11 14:11:26 UTC
I agree that a testcase or a place inside gtk3-demo or gtk3-widget-factory to reproduce this problem would be very useful. In the meantime, here is a simplification of the fix in question here. Let me know if this helps for your case


diff --git a/gtk/gtktreeview.c b/gtk/gtktreeview.c
index a7463cc..8bfb5cd 100644
--- a/gtk/gtktreeview.c
+++ b/gtk/gtktreeview.c
@@ -3050,13 +3050,10 @@ static void
 grab_focus_and_unset_draw_keyfocus (GtkTreeView *tree_view)
 {
   GtkWidget *widget = GTK_WIDGET (tree_view);
-  GtkWidget *grab_widget = gtk_grab_get_current ();
 
-  if (gtk_widget_get_can_focus (widget) &&
-      !gtk_widget_has_focus (widget) &&
-      (!grab_widget || grab_widget == widget))
+  if (gtk_widget_get_can_focus (widget) && !gtk_widget_has_focus (widget) &&
+      !_gtk_widget_get_shadowed (widget))
     gtk_widget_grab_focus (widget);
-
   tree_view->priv->draw_keyfocus = 0;
 }
Comment 13 Roman Lebedev 2016-09-11 14:29:20 UTC
(In reply to Matthias Clasen from comment #12)
> I agree that a testcase or a place inside gtk3-demo or gtk3-widget-factory
> to reproduce this problem would be very useful. In the meantime, here is a
> simplification of the fix in question here. Let me know if this helps for
> your case


> diff --git a/gtk/gtktreeview.c b/gtk/gtktreeview.c
> index a7463cc..8bfb5cd 100644
> --- a/gtk/gtktreeview.c
> +++ b/gtk/gtktreeview.c
> @@ -3050,13 +3050,10 @@ static void
>  grab_focus_and_unset_draw_keyfocus (GtkTreeView *tree_view)
>  {
>    GtkWidget *widget = GTK_WIDGET (tree_view);
> -  GtkWidget *grab_widget = gtk_grab_get_current ();
>  
> -  if (gtk_widget_get_can_focus (widget) &&
> -      !gtk_widget_has_focus (widget) &&
> -      (!grab_widget || grab_widget == widget))
> +  if (gtk_widget_get_can_focus (widget) && !gtk_widget_has_focus (widget) &&
> +      !_gtk_widget_get_shadowed (widget))
>      gtk_widget_grab_focus (widget);
> -
>    tree_view->priv->draw_keyfocus = 0;
>  }

Yes, this patch (like https://bugzilla.gnome.org/show_bug.cgi?id=770130#c11) also seems to fix this issue.