Bugzilla – Full Text Bug Listing
| Summary: | [wayland] StyledText DnD not signalling, move drag detection to mouse_move | ||
|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Ian Pun <ipun> |
| Component: | SWT | Assignee: | Ian Pun <ipun> |
| Status: | CLOSED FIXED | QA Contact: | |
| Severity: | normal | ||
| Priority: | P3 | CC: | akurtakov, loskutov, lufimtse, sxenos, xcariba |
| Version: | 4.7 | ||
| Target Milestone: | 4.7 M6 | ||
| Hardware: | PC | ||
| OS: | Linux | ||
| See Also: |
https://git.eclipse.org/r/88784 https://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=157a34b45432073f47e48a5b828912872b7af91a https://git.eclipse.org/r/94395 https://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=9dc0136dddbf36d56a54a14430749427e4bd5a81 https://git.eclipse.org/r/94703 https://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=3ba7df63994d8cf308a7dde930738fe87a55badf https://bugs.eclipse.org/bugs/show_bug.cgi?id=541635 |
||
| Whiteboard: | |||
| Bug Depends on: | 514659 | ||
| Bug Blocks: | 514776 | ||
|
Description
Ian Pun
New Gerrit change created: https://git.eclipse.org/r/88784 Latest patch will move the drag detection over to on mouse movement instead of on mouse click. Our initial implementation of drag detection was to hold the mouse click event, go into a while loop looking for mouse release/movement events, and compare position offsets to see if it was breaking a threshold. In Wayland, we cannot retrieve new release/movement events while holding onto the initial click, causing DnD to not work. This change will have movement events (conditionally only when it is being initiated by a mouse click and is inside a selection) do the drag detection and should not effect any other OSes. Gerrit change https://git.eclipse.org/r/88784 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=157a34b45432073f47e48a5b828912872b7af91a This patch introduced a serious regression. See bug 514659 for details. (In reply to Stefan Xenos from comment #4) > This patch introduced a serious regression. See bug 514659 for details. Looking on the patch I wonder if the IS_GTK should be changed to IS_WAYLAND because the complicated logic changes it introduce are only required for wayland backend. I'm not sure but recently I've seen really weird editor selection issues which entirely broke selection in Java editor instance so that I had to reopen it. This was with one of recent builds where the dnd was broken due the bug 503431, so I didn't reported it yet. This was on GTK3. In this sense (and also scratching the head about the logic in the patch) I would vote for reverting it. My problem with the patch is that there are 3 new global variables spread over the ifdef sections with unclear life cycle and unclear responsibilities of the state transitions. I'm not sure the previous styled text code (which was not maintainable anyway) will became even more unmaintainable after the patch. (In reply to Andrey Loskutov from comment #5) > (In reply to Stefan Xenos from comment #4) > > This patch introduced a serious regression. See bug 514659 for details. > > Looking on the patch I wonder if the IS_GTK should be changed to IS_WAYLAND > because the complicated logic changes it introduce are only required for > wayland backend. I'm not sure but recently I've seen really weird editor > selection issues which entirely broke selection in Java editor instance so > that I had to reopen it. This was with one of recent builds where the dnd > was broken due the bug 503431, so I didn't reported it yet. This was on > GTK3. > > In this sense (and also scratching the head about the logic in the patch) I > would vote for reverting it. My problem with the patch is that there are 3 > new global variables spread over the ifdef sections with unclear life cycle > and unclear responsibilities of the state transitions. I'm not sure the > previous styled text code (which was not maintainable anyway) will became > even more unmaintainable after the patch. Hi Andrey, As much as I would've like to introduce GTK calls into StyledText, this is not possible as this class is very much linked to all platforms (macOSX, win32, linux). There only seem to be a way to find out if it is running GTK or not, but we are not able to tell if it is running wayland backend as we cannot refer to GTK calls in this class. Also, could you detail me some of the issues you had with the java editor in terms of selection? > I would vote for reverting it
+1 for a temporary revert, at least until such time as the regressions can be sorted out.
To go meta for a moment:
IMO, it would be a good policy to always immediately revert patches that cause regressions. There's no need to leave the builds in a bad state while the patch is being fixed and it's really easy to un-revert the change once the regressions are addressed.
The exception would be one of those rare cases where a patch is expected to break something and where the plan for dealing with breakage is described in the bug - or those cases where too much depends on the patch to revert it. I'm not sure either case applies here.
(In reply to Ian Pun from comment #7) > Also, could you detail me some of the issues you had with the java editor in > terms of selection? One issue I'm observing right now - double clicking a symbol in the Java editor selects this word for the first time and selects the first visible line in the editor second time. Weird, and not reproducible in 4.6.3! (In reply to Ian Pun from comment #7) > Also, could you detail me some of the issues you had with the java editor in > terms of selection? Next issue (the blocker I've mentioned before): I don't know how to reproduce, but I see again that speed of selecting text starts to crawl up to the state where no selection is possible at all and so I have to close the editor. This happened today for the first time after 1 minute of working on a small interface while rewriting the javadoc part, where I've selected the javadoc to re-format it. Next issue: modify text area selection repeatably from the end of the text to the beginning, and after releasing the mouse if the cursor is over the selection, you will see a "Copy" cursor showing up and disappearing OR instead the cursor will jump out of the text selection again to the first visible line in the editor. (In reply to Ian Pun from comment #7) > Also, could you detail me some of the issues you had with the java editor in > terms of selection? Next issue: in Java editor, hold "Ctrl" key and hover over an interface method call to see the "Open implementation" pupup menu. Now try to click it - *nothing happens* and menu stays open. On 4.6.3, the right type is opened. BTW I'm on GTK 3.14, and using build I20170402-2000. I think I've submitted enough issues with styled text now for reverting the patch, and I was only working about 1 hour with IDE. Ian, please revert the Java parts only. Let's keep the binaries as they are for easier testing of the next patches Alex, I've created a patch that reverts the .java-only portions of the problematic change along with all subsequent changes that depend on it: https://git.eclipse.org/r/#/c/94419/ List of changes affected by the revert: 5e9b298245d8ad0d72cf2b1bcb529dbcf46384e4 9659869e56fa7cdebe9375440bfb50466651a6a3 b515cd1fa98fe02aacaf2d2e1a49b77ad4ed5aff 81633b430a87caf2f0c020f10e108754d81a4415 157a34b45432073f47e48a5b828912872b7af91a 3f92612774ff0dda4e8e9c86127bb0f3a880c329 860641a84d915424de3c6c214008ed5fc87c3a6f Could I trouble you (or someone else on SWT) for a quick review so that we can get our nightly builds stable again? Gerrit change https://git.eclipse.org/r/94395 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=9dc0136dddbf36d56a54a14430749427e4bd5a81 (In reply to Stefan Xenos from comment #13) > Alex, I've created a patch that reverts the .java-only portions of the > problematic change along with all subsequent changes that depend on it: > > https://git.eclipse.org/r/#/c/94419/ > > List of changes affected by the revert: > > 5e9b298245d8ad0d72cf2b1bcb529dbcf46384e4 > 9659869e56fa7cdebe9375440bfb50466651a6a3 > b515cd1fa98fe02aacaf2d2e1a49b77ad4ed5aff > 81633b430a87caf2f0c020f10e108754d81a4415 > 157a34b45432073f47e48a5b828912872b7af91a > 3f92612774ff0dda4e8e9c86127bb0f3a880c329 > 860641a84d915424de3c6c214008ed5fc87c3a6f > > Could I trouble you (or someone else on SWT) for a quick review so that we > can get our nightly builds stable again? Hi Stephan, sorry for the delay, but as you may have known, Leo and I have come to a comprimise and fixed it so that the nightly builds should be stable again while not having to revert anything. We have reverted the styledText changes so that everything remains the same for x11, but styledtext is still broken in Wayland, but at least the nightly builds will be working fine again while we figure out another way. Thanks for your patience! Yep. Thanks for the quick fix. I just tested the revert and it appears to address my issues. New Gerrit change created: https://git.eclipse.org/r/94703 Gerrit change https://git.eclipse.org/r/94703 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=3ba7df63994d8cf308a7dde930738fe87a55badf DnD works for StyledText on Wayland now. *** Bug 514826 has been marked as a duplicate of this bug. *** |