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.

And you of course tested that the original bug (#767468) is still fixed... :)

I of course did not.

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.

(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.

(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.

(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.

(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.

(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.

Roman, does my patch in bug 770130 comment 8 fix this bug?

That patch was not accepted. If you test a patch in bug 770130, you'd better
test the one in comment 11.

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;
 }

(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.