Bug 498217 (swtDnDPartsRectangle) - [GTK3][DnD] Dragging parts does not show rectangle
Summary: [GTK3][DnD] Dragging parts does not show rectangle
Status: VERIFIED FIXED
Alias: swtDnDPartsRectangle
Product: Platform
Classification: Eclipse Project
Component: SWT (show other bugs)
Version: 4.8   Edit
Hardware: PC Linux
: P2 major with 2 votes (vote)
Target Milestone: 4.8 RC3   Edit
Assignee: Leo Ufimtsev CLA
QA Contact:
URL:
Whiteboard: RHT
Keywords: helpwanted, triaged
: 414119 436211 (view as bug list)
Depends on: swtWaylandImproveOverlay
Blocks: 492861
  Show dependency tree
 
Reported: 2016-07-20 12:06 EDT by Leo Ufimtsev CLA
Modified: 2018-06-15 15:32 EDT (History)
11 users (show)

See Also:
akurtakov: review+
sravankumarl: review+
ericwill: review+


Attachments
Video of issue. (6.80 MB, application/octet-stream)
2016-07-20 12:10 EDT, Leo Ufimtsev CLA
no flags Details
gtk commit that broke things. (5.36 KB, patch)
2018-05-15 12:41 EDT, Leo Ufimtsev CLA
no flags Details | Diff
Snippet to reproduce tracker issue. (1.38 KB, text/plain)
2018-05-15 15:15 EDT, Leo Ufimtsev CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Leo Ufimtsev CLA 2016-07-20 12:06:08 EDT
We seem to have a major usability issue in GTK3 with Tracker for views.

When dragging views around, on GTK3 there is no preview of where the view would be:
- When dragging tabs, it shoudl show where tabs should be dropped.
- When dragging tabs onto a plane, it should show how the plane should be split.

This is reproducible on Gtk3.20, Gtk3.21(newest dev branch) and maybe previous Gtk3.x versions (have not tested extensivley). 
This does not reproduce on Gtk2.24.
Comment 1 Leo Ufimtsev CLA 2016-07-20 12:10:11 EDT
Created attachment 263215 [details]
Video of issue.

Tracker doesn't seem to get painted on GTK3. Or sometimes it appears when you let go of the mouse, not while you drag things around.
Comment 2 Leo Ufimtsev CLA 2016-07-20 12:12:41 EDT
I need to investigate if it's related to my DND patch:
428852: [GTK3] SWT - DND broken
https://bugs.eclipse.org/bugs/show_bug.cgi?id=428852

During DND, GTK didn't  emit motion events, maybe lack of motion events is causing tracker to mailfunction also... 

Assigning to self for now.
Comment 3 Leo Ufimtsev CLA 2016-07-20 12:20:17 EDT
Might be related to:
492783: [GTK2] Tracker doesn't work properly when using the constructor that accepts a Display
https://bugs.eclipse.org/bugs/show_bug.cgi?id=492783
Comment 4 Stefan Xenos CLA 2016-07-20 13:56:30 EDT
Not related to bug 492783.

The drag affordance used for views in the E4 workbench is actually a transparent ON_TOP shell, not a Tracker.

A Tracker is present but it is hidden offscreen and you're not supposed to see it.
Comment 5 Stefan Xenos CLA 2016-07-20 13:58:18 EDT
Also, notice that your drop cursor is changing to an X in addition to the missing drag rectangles.

This looks to me like a bug in the workbench, not SWT.
Comment 6 Leo Ufimtsev CLA 2016-07-21 09:39:24 EDT
(In reply to Stefan Xenos from comment #5)
> Also, notice that your drop cursor is changing to an X in addition to the
> missing drag rectangles.
> 
> This looks to me like a bug in the workbench, not SWT.

Thank you for feedback.

Hmm, I sort of wonder why it only occurs on Gtk3 though..?
Comment 7 Stefan Xenos CLA 2016-07-22 14:04:12 EDT
> Hmm, I sort of wonder why it only occurs on Gtk3 though..?

Yes, that does normally point to an SWT bug, which may be evidence against my hypothesis from comment 4.

It occurs to me that we've been having many problems with Tracker-based drag & drop over the years and we could easily make this work using an event filter (like I did in the patch for the "select control" button in bug 498030). The only downside of the event filter is that we'd lose control over the custom cursor... but that may be a small price to pay in exchange for getting reliable and bug-free drag & drop on all platforms.

Maybe we could implement both and use a hidden preference to flip between the two. Then whenever folks report Drag & Drop problems, we could ask them to toggle the preference. If flipping the preference makes a difference, we'd know that the Tracker was to blame.

Another possible cause: when I was working on bug 498030 I had to add a special case to my target detection in order to prevent my own ON_TOP shell from being considered as a possible drop target. I can't find any similar special case in the workbench code, so it's possible that the drag rectangle itself is blocking possible drop targets.
Comment 8 Leo Ufimtsev CLA 2016-07-25 10:06:08 EDT
(In reply to comment #7)
> > Hmm, I sort of wonder why it only occurs on Gtk3 though..?
> 
> Yes, that does normally point to an SWT bug, which may be evidence against my
> hypothesis from comment 4.
> 
> It occurs to me that we've been having many problems with Tracker-based drag &
> drop over the years and we could easily make this work using an event filter
> (like I did in the patch for the "select control" button in bug 498030). The
> only downside of the event filter is that we'd lose control over the custom
> cursor... but that may be a small price to pay in exchange for getting reliable
> and bug-free drag & drop on all platforms.
> 
> Maybe we could implement both and use a hidden preference to flip between the
> two. Then whenever folks report Drag & Drop problems, we could ask them to
> toggle the preference. If flipping the preference makes a difference, we'd know
> that the Tracker was to blame.
> 
> Another possible cause: when I was working on bug 498030 I had to add a special
> case to my target detection in order to prevent my own ON_TOP shell from being
> considered as a possible drop target. I can't find any similar special case in
> the workbench code, so it's possible that the drag rectangle itself is blocking
> possible drop targets.

I see. 
So the suggestion is to implement an option that would switch to using the tracker instead of an invisible shell?

Just before we go down that road, do you happen to know if there is a (swt/jface) snippet that reproduces the behaviour via invisible shell?
(or one that could potentially be customized to do so..)
Maybe it's just a matter of the boarder not being drawn properly or some CSS property that needs to be applied on GTK side.
Comment 9 Stefan Xenos CLA 2016-07-25 13:30:30 EDT
> So the suggestion is to implement an option that would switch to using
> the tracker instead of an invisible shell?

No. Both would use an invisible Shell. One option would use a Tracker to capture mouse events and set the cursor and the other would use an event filter attached to the Display (with no Tracker).


> Just before we go down that road, do you happen to know if there is a
> (swt/jface) snippet that reproduces the behaviour via invisible shell?
> (or one that could potentially be customized to do so..)

If you suspect the low-level code that draws drag-over affordances, check out the layout spy dialog from the lastest code in master in the PDE repository. It's bound to ctrl-alt-shift-F9. 

Click the "select control" button and you'll get the same sort of semitransparent shell, but it uses an event filter rather than a Tracker for capturing mouse movement. The code for that bit is in ControlSelector.java.

I suspect the problem may be related to bug 492861. While messing with that code, I've discovered a bunch of bugs related to leaking state during drag and drop... and it seems plausible that some of that state may have broken the dragover affordances in some cases.

I'm working on a rewrite that is purely functional (I've deleted all the member variables from the drop targets), which makes it impossible for any side-effects to occur.
Comment 10 Leo Ufimtsev CLA 2016-09-01 17:26:51 EDT
(In reply to Stefan Xenos from comment #9)
> > So the suggestion is to implement an option that would switch to using
> > the tracker instead of an invisible shell?
> 
> No. Both would use an invisible Shell. One option would use a Tracker to
> capture mouse events and set the cursor and the other would use an event
> filter attached to the Display (with no Tracker).
> 
> 
> > Just before we go down that road, do you happen to know if there is a
> > (swt/jface) snippet that reproduces the behaviour via invisible shell?
> > (or one that could potentially be customized to do so..)
> 
> If you suspect the low-level code that draws drag-over affordances, check
> out the layout spy dialog from the lastest code in master in the PDE
> repository. It's bound to ctrl-alt-shift-F9. 
> 
> Click the "select control" button and you'll get the same sort of
> semitransparent shell, but it uses an event filter rather than a Tracker for
> capturing mouse movement. The code for that bit is in ControlSelector.java.

I tested ControlSelector, it works well. The yellow tracker wraps niceley around all controls. But the tracker for parts doesn't work properly in the same eclipse instance.

I tested with various GTK versions. I found that up to and including gtk3.8, the part-dragging works well. In Gtk3.10 and onwards it broke. I suspect some underlying gtk caching might be the culprit.

I will investigate further.
Comment 11 Leo Ufimtsev CLA 2016-09-01 17:41:28 EDT
(In reply to Stefan Xenos from comment #4)
> Not related to bug 492783.
> 
> The drag affordance used for views in the E4 workbench is actually a
> transparent ON_TOP shell, not a Tracker.
> 
> A Tracker is present but it is hidden offscreen and you're not supposed to
> see it.

I can confirm this. The part-dragging still works in Gtk3.6, Gtk3.8, but breaks on Gtk3.10.
But the tracker doesn't work on Gtk3.6, Gtk3.8 or any further Gtk3.* version.

I will try to bisect gtk's codebase between gtk3.8 and gtk3.10 to find which gtk commit broke the part-dragging.
Comment 12 Stefan Xenos CLA 2016-09-01 19:57:09 EDT
Leo, although I have nothing wrong with fixing the Tracker class, I'm not sure that using a Tracker is really the best solution here.

AFAIK, the only reason the workbench uses a Tracker is to get exclusive control over the mouse cursor in a way that won't be overridden when you move the cursor over controls that use a custom cursor.

This is kind-of an abuse of the Tracker class to begin with since the workbench doesn't actually want the tracker rectangle and intentionally moves it offscreen to make it invisible.

IMO, a more elegant solution for Drag & Drop would be to introduce new API into SWT that provides direct control over the mouse cursor and to do away with the Tracker entirely.
Comment 13 Leo Ufimtsev CLA 2016-09-02 10:48:46 EDT
(In reply to Stefan Xenos from comment #12)
> Leo, although I have nothing wrong with fixing the Tracker class, I'm not
> sure that using a Tracker is really the best solution here.
> 
> AFAIK, the only reason the workbench uses a Tracker is to get exclusive
> control over the mouse cursor in a way that won't be overridden when you
> move the cursor over controls that use a custom cursor.
> 
> This is kind-of an abuse of the Tracker class to begin with since the
> workbench doesn't actually want the tracker rectangle and intentionally
> moves it offscreen to make it invisible.
> 
> IMO, a more elegant solution for Drag & Drop would be to introduce new API
> into SWT that provides direct control over the mouse cursor and to do away
> with the Tracker entirely.

Thank you for your reply.

Interesting proposition. I've never looked into platform.ui deeply, I don't have an understanding of how part-dragging works beyond the SWT layer. If you think new api is better, maybe we should investigate further down that road.

I don't have a good understanding of what you mean by 'direct control over the mouse cursor', can you elaborate on what functions the api should provide, what input/output the functions would have?
Comment 14 Stefan Xenos CLA 2016-09-02 13:16:39 EDT
> can you elaborate on what functions the api should provide, what
> input/output the functions would have?

I'm referring specifically to the Tracker.setCursor(Cursor) method. It sets the mouse cursor in a way that is different from all other setCursor methods, since it continues to have an effect no matter where you move the mouse on screen.

One possible alternative might be to create a Display.overrideCursor(Cursor) method which either accepts a cursor or null. Passing it a Cursor causes it to use that cursor everywhere on screen, and passing it null returns it to the default mode of getting the cursor appearance from the control below the mouse.

As I'm writing this, I've thought of a third option for fixing drag & drop. It could continue to use the Tracker for its setCursor method and nothing else, but switch from using the Tracker's event loop to using a global event filter like ControlSelector does. From your tests, above, it seems as though the event filter approach should work fine... and by ignoring any events fired from the Tracker, the workbench would no longer be susceptible to flakey Tracker behaviors.
Comment 15 Leo Ufimtsev CLA 2016-09-02 16:23:11 EDT
(In reply to comment #14)
> > can you elaborate on what functions the api should provide, what
> > input/output the functions would have?
> 
> I'm referring specifically to the Tracker.setCursor(Cursor) method. It sets the
> mouse cursor in a way that is different from all other setCursor methods, since
> it continues to have an effect no matter where you move the mouse on screen.
> 
> One possible alternative might be to create a Display.overrideCursor(Cursor)
> method which either accepts a cursor or null. Passing it a Cursor causes it to
> use that cursor everywhere on screen, and passing it null returns it to the
> default mode of getting the cursor appearance from the control below the mouse.
> 
> As I'm writing this, I've thought of a third option for fixing drag & drop. It
> could continue to use the Tracker for its setCursor method and nothing else, but
> switch from using the Tracker's event loop to using a global event filter like
> ControlSelector does. From your tests, above, it seems as though the event
> filter approach should work fine... and by ignoring any events fired from the
> Tracker, the workbench would no longer be susceptible to flakey Tracker
> behaviors.

Hmmm. Interesting. I'll do some reading on Tracker.setCursor(Cursor) .
In the mean time I found the GTK commit that broke part-dragging:
http://osdir.com/ml/commits.gnome/2013-05/msg04138.html
In the mean time I will investigate if we can fix this on the gtk side of things.
Comment 16 Leo Ufimtsev CLA 2016-09-07 17:28:53 EDT
(In reply to Leo Ufimtsev from comment #15)
> Hmmm. Interesting. I'll do some reading on Tracker.setCursor(Cursor) .
> In the mean time I found the GTK commit that broke part-dragging:
> http://osdir.com/ml/commits.gnome/2013-05/msg04138.html
> In the mean time I will investigate if we can fix this on the gtk side of
> things.

I'm not having much luck with resolving this on the gtk side at the moment.

The offending GTK+ commit is:
a60ccd3672467efb454b121993febc36f33cbc79   (#a60ccd36)

If I checkout maybe 100 commits after #a60ccd36, then revert a60ccd36, then Drag-part still works.
However, if I checkout master, attempting to revert causes many merge conflicts. I tried to revert by manually editing code, but drag-part issue still persists with that approach.

I will either need to reproduce this on a snippet level to peruse a gtk solution further, or we should alter the part-drag mechanism as per your suggestion.
Or go implement rectangle drawing similar to how "layout spy dialog" does it at the moment, because that seems to work.
Hmm...
Comment 17 Leo Ufimtsev CLA 2016-09-13 12:32:04 EDT
It seems either way fixing this would take a while.

It might be a while before I'm able commit a big chunk of time to this.
(Have to finish webkit2 port first, then stabilise Eclipse on Wayland, could work on this after).

If someone wants to work on it in the meantime, feel free to do so. Assigning to platform-swt for now.
Comment 18 Leo Ufimtsev CLA 2017-07-07 11:04:13 EDT
I've been meaning to work on this for a while, but webkit2 port is taking some more time than expected.
I'd be really great to have this fixed in near future...
Comment 19 Eric Williams CLA 2018-05-11 15:21:01 EDT
*** Bug 436211 has been marked as a duplicate of this bug. ***
Comment 20 Leo Ufimtsev CLA 2018-05-15 12:41:28 EDT
Created attachment 274052 [details]
gtk commit that broke things.

Found the gtk commit that caused this breakage.

It's a change in transparent shell handling.
Gtk implemented an optimization where transparent shells no longer have their clip tracked anymore. Since the partTracker is a transparent shape, things broke for us.

Will investigate solution.
Comment 21 Leo Ufimtsev CLA 2018-05-15 15:15:26 EDT
Created attachment 274054 [details]
Snippet to reproduce tracker issue.

org.eclipse.e4.ui.workbench.addons.dndaddon.(DnDManager) has an offscreen Tracker:

> private static final Rectangle offScreenRect = new Rectangle(10000, -10000, 1, 1);

As of one of Gtk3.9.1's commits: a60ccd3672467efb454b121993febc36f33cbc79, clipping for off-screen elements is not computed anymore for optimization purposes. This results in off-screen widgets not emitting events.

However, DnDManager relies on the off-Screen Tracker to send MouseMove events as trigger for it's custom drawing. So the result is that due to the offScreen tracker no longer sending mouseMove events, custom drawnings are no longer done.

I fiddled around with it, if the tracker is put on Screen, then everything works again.

This issue can be observed in the attached snippet.

I will look for a solution.
Comment 22 Eclipse Genie CLA 2018-05-24 14:52:57 EDT
New Gerrit change created: https://git.eclipse.org/r/123290
Comment 24 Eclipse Genie CLA 2018-05-25 15:32:34 EDT
New Gerrit change created: https://git.eclipse.org/r/123370
Comment 27 Lars Vogel CLA 2018-05-28 10:35:50 EDT
Awesome! Thanks for the fix.
Comment 28 Lars Vogel CLA 2018-05-29 17:51:33 EDT
Verified in Eclipse SDK
Version: Photon (4.8)
Build id: I20180529-0600
OS: Linux, v.4.15.0-22-generic, x86_64 / gtk 3.22.30
Java version: 1.8.0_161


Thanks a bunch, I was really missing this drag indicator.
Comment 29 Leo Ufimtsev CLA 2018-05-30 14:12:07 EDT
(In reply to Lars Vogel from comment #28)
> Verified in Eclipse SDK
> Version: Photon (4.8)
> Build id: I20180529-0600
> OS: Linux, v.4.15.0-22-generic, x86_64 / gtk 3.22.30
> Java version: 1.8.0_161
> 
> 
> Thanks a bunch, I was really missing this drag indicator.

Thanks for verification. Yea, it's been bugging me for 4 years also =).
Comment 30 Eric Williams CLA 2018-06-15 15:32:31 EDT
*** Bug 414119 has been marked as a duplicate of this bug. ***