Bug 317292 - `Empty areas' misdetection in Gwyddion
Summary: `Empty areas' misdetection in Gwyddion
Status: RESOLVED FIXED
Alias: None
Product: Oxygen
Classification: Plasma
Component: gtk2-engine (other bugs)
Version First Reported In: 4.8
Platform: Other Linux
: NOR normal
Target Milestone: ---
Assignee: Hugo Pereira Da Costa
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-24 20:20 UTC by David Nečas
Modified: 2013-04-14 12:41 UTC (History)
3 users (show)

See Also:
Latest Commit:
Version Fixed/Implemented In:
Sentry Crash Report:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description David Nečas 2013-03-24 20:20:36 UTC
As suggested in bug 273637, comment 19, opening a separate issue.

Problem: active widgets/areas are considered empty and thus windows are moved when the user tries to select shapes in 2D false colour views or control 3D views.

Steps to reproduce:
1) Get and run gwyddion (http://gwyddion.net/).
2) Create some data: Select Data Process → Synthetic → Objects. Press OK button. A new window appears with false colour view of some random stuff.
3) Give focus to another window (see below).
4) Click and drag on the previously created window (somewhere in the displayed data).

Expected behaviour: Mouse cursor is changed to cross, dragging creates a `selected point' that will appear as a cross on the view after finishing the dragging.

Actual behaviour: The entire window starts to being dragged.

Notes:
1) Clicking on the affected window, waiting a short while and then dragging seems to prevent the window dragging. But with (3) it should be reproducible reliably.
2) I am namely summarising reports of others because although I have seen the behaviour myself I do not have KDE installed anywhere at this moment. Recent reports are from SuSE 12.2 users.
3) I tried to reproduce the problem with a trivial program that just created an empty GtkWindow and connected to its button press and release events -- but it worked fine. The widget must be doing something differently but I have no theory what.
4) The false colour display widget construction code is around here: https://sourceforge.net/p/gwyddion/code/HEAD/tree/trunk/gwyddion/libgwydgets/gwydataview.c#l398 (pretty ancient but otherwise standard Gtk+ widget code).
Comment 1 Hugo Pereira Da Costa 2013-03-24 20:37:44 UTC
Thanks david,
I'll investigate asap and will keep you posted
(then will post 'instructions' provided this is not an actual bug in our code)
Comment 2 Hugo Pereira Da Costa 2013-03-24 20:38:53 UTC
PS: 
on point 3)
yes. That's pretty much what I need to find out for the so called "instructions"
Comment 3 Hugo Pereira Da Costa 2013-04-08 20:23:57 UTC
Git commit f387973ed00c2c1232573d41d4d9c49b0eb943d5 by Hugo Pereira Da Costa.
Committed on 08/04/2013 at 22:19.
Pushed by hpereiradacosta into branch '1.3'.

Check cursor shape before accepting event for window dragging.
This is consistent with what is done for oxygen-qt

M  +9    -3    src/oxygenwindowmanager.cpp

http://commits.kde.org/oxygen-gtk/f387973ed00c2c1232573d41d4d9c49b0eb943d5
Comment 4 Hugo Pereira Da Costa 2013-04-08 20:25:17 UTC
Git commit bb73c9d660d366345101899b0b4926e1428b810b by Hugo Pereira Da Costa.
Committed on 08/04/2013 at 22:19.
Pushed by hpereiradacosta into branch 'gtk3'.

Check cursor shape before accepting event for window dragging.
This is consistent with what is done for oxygen-qt

M  +9    -3    src/oxygenwindowmanager.cpp

http://commits.kde.org/oxygen-gtk/bb73c9d660d366345101899b0b4926e1428b810b
Comment 5 Hugo Pereira Da Costa 2013-04-08 20:25:36 UTC
@David
Comment 6 Hugo Pereira Da Costa 2013-04-08 20:34:34 UTC
@David
The commit above fixes the issue.
I will make a new release of oxygen gtk that includes the fix during the week. In the meanwhile, would be nice if you could confirm by compiling oxygen gtk from source (either the master of 1.3 branch). 

Also, while browsing through Gwyddion's code I figured the following patch would also fix the issue. I think it is a sane thing to do (to use gtk_widget_set_events, to pass the event mask, rather than passing it to the underlying gdk window). It 'should' have no side effect, and would not require the change above (which I pushed nonetheless, for consistency with the oxygen-qt code), since oxygen-gtk does catch this setting on the gtk_widget and prevents the window grab when this is caught.
 
Can you double check on your side ? if it indeed does work, I believe the same recipie could be used elsewhere where similar 'conflicts' occur. 

Cheers,

Hugo

The patch:
--- libgwydgets/gwydataview-sv.c        2013-04-08 22:31:01.243282020 +0200
+++ libgwydgets/gwydataview.c   2013-04-08 22:31:05.711047532 +0200
@@ -432,6 +432,9 @@
                                     &attributes, attributes_mask);
     gdk_window_set_user_data(widget->window, widget);
 
+    // Force widget to listen to relevant events
+    gtk_widget_add_events( widget, attributes.event_mask );
+
     widget->style = gtk_style_attach(widget->style, widget->window);
     gtk_style_set_background(widget->style, widget->window, GTK_STATE_NORMAL);
Comment 7 David Nečas 2013-04-08 22:03:38 UTC
(In reply to comment #6)
> +    // Force widget to listen to relevant events
> +    gtk_widget_add_events( widget, attributes.event_mask );

OK, thanks a lot, this indeed makes sense.  I will try your Gwyddion fix ASAP.  Also the oxygen fix but that is not completely straightforward for me so don't hold your breath for this.

Just to be sure I understand the issue and will be able to avoid the problem in future: Gtk+ has "window" and "no-window" widgets (depending on whether they have their own Gdk/X/... window).  I can see for "window" widgets that gtk_widget_add_events(...) is a sane thing to do.  A "window" widget was also the reported case.

Now to avoid a similar problem for "no-window" widgets. They draw on the parent window but may create some invisible input-only windows to receive events. Widgets such as GtkButton seem to work, so doing either

gdk_window_set_user_data(input_window, widget);  // older Gtk+

or

gtk_widget_register_window(widget, input_window);  // recent Gtk+

is sufficient, right?  I don't see any standard "no-window" widgets doing any gtk_widget_add_events()-like stuff but I may be missing something else. Calling gdk_window_set_user_data(window, widget); was not sufficient for GwyDataView.

I must say I am still at loss concerning the additional rules this oxygen feature creates. From a widget developer perspective, if I create an Gdk/X/... window responding to button press events (by whatever means), that window represents an `active' area as far as X/... is concerned so it should be sufficient for detection.  But it is not and the means apparently matter.
Comment 8 David Nečas 2013-04-10 10:11:12 UTC
I tried your Gwyddion patch with widgets that caused problems and it seems to work.  Thanks.
Comment 9 Hugo Pereira Da Costa 2013-04-10 10:18:57 UTC
@David,
Great ! so we have a double fix.

Concerning the other comment: 
"I must say I am still at loss concerning the additional rules this oxygen feature creates. From a widget developer perspective, if I create an Gdk/X/... window responding to button press events (by whatever means), that window represents an `active' area as far as X/... is concerned so it should be sufficient for detection. But it is not and the means apparently matter."

... I agree, and I'm not 100% sure I understand how it works either.
The thing is: it is somewhat hard to tell (from oxygen-gtk's side) whether a widget indends to use a given type of event or not (and thus whether it should or not use it). 

This gtk_widget_set_masks() command does the trick. 
the other code you had (on the gdk window) should also have done the trick, but for some reason, it does not work: I explicitely do not catch the Window mask in oxygen-gtk, because if I do so, then many "empty" areas which are effectively not using the events I would need become ignored. So for some reason: gdk_window mask is somehow abused in gtk (at least in many apps), which makes it useless for me, whereas gdk_widget_set/get_events() is not. Hence the suggested patch.

Does that 'sort of' make sense ?
Comment 10 David Nečas 2013-04-14 12:41:43 UTC
Thanks for the explanation.  While it makes sense it simultaneously looks like a path to hell.  It can already be seen here.  Essentially, the conclusion for a Gtk+ widget developer is:

If I want widgets taking mouse events to work properly under oxygen, I must use gtk_widget_add_events().

So he will set exactly the same event flags this way they might have set in a more fine-grained manner for the input windows before (because they may cover only a part of the widget). KDE will not gain anything and Gtk+ will lose some semantic distinctions.

This follows from the decision of enabling it by default the aggressive variant of the feature.  With a conservative approach, there could be widgets or areas actually ‘empty’ but not movable just by dragging due to over-use of event flags in Gtk+ widgets (some widgets may need special treatment, e.g. to detect something like the empty parts of menubars, but that's the same in all cases). This would make the feature somewhat less useful for those actually using it. With an aggressive approach you err on the side of breaking widgets and inevitably developers will start to fight oxygen and create workarounds since they need to have the last say concerning what areas are active.

Only two possible outcomes exist: (a) widget developers are able to have the last say -- then making the feature aggressive achieved nothing at the expense more effort from everyone involved (b) oxygen has the last say -- then some widgets will always be broken.

Well, these general thoughts probably belongs more to bug 273637...