Community
Participate
Working Groups
Created attachment 282584 [details] Space between parts This bug arises from the following forum thread: https://www.eclipse.org/forums/index.php/m/1826428/ As a summary, I'm trying to create a compact theme (similar to Bug 506042) and wanted to reduce the space between the various parts (see attachment). The sash width isn't configurable directly and even overriding the hardcoded value (e.g. using reflection) does not seem to have any impact on the space between the various parts and instead disables the resize cursor. Making this configurable and allowing to set smaller values would be great for custom themes, but could also benefit Eclipse's default themes!
*** Bug 565490 has been marked as a duplicate of this bug. ***
+1
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/167143
With the old implementation of SashLayout, reducing the sash width would result in "hiding" the composite container: indeed, you're reducing the border and making the children of the MPartSashContainer take up all the space over it. This means that the MPartSashContainer to which the mouse listeners are attached to would no longer receive any events, which in turn would prevent any of the parts to be resized. The above Gerrit attaches the mouse listeners to the display instead, meaning that we always get the events no matter what. One tricky part is to convert the x and y coordinates of mouse events to make them relative to the MPartSashContainer host. To test the Gerrit, simply modify the sashWidth value in SashLayout.java. For example you can set it to 0. As I'm not very familiar with SWT, I don't know if there are any negative side effects to having expanded the listeners to the whole display, but my own testing has not highlighted any issues. If people are happy and think that this approach makes sense, me or someone else can then add a CSS handler to allow modifying the sashWidth field easily. Once that it done, we can use the new functionality in themes!
Adding Rolf and Mike to the CC list, as you were both part of it in the duplicate bug 565490. :)
Created attachment 283789 [details] No spaces between views I've actually solved this in our product. It's not a straightforward solution because the sash area is where we do all the mouse listeners... Anyways the solution I came up with was to create my own SashRenderer and SashLayout classes. These classes actually use a padding of -1 between the views, this allows the borders between two views to only be a single pixel, with 0 padding, then we end up with two 1px lines butting up together and it forms a 2px divider between views. As I was saying, the big challenge is the mouse listeners, in order to accomplish this, I did something that's considered unconventional and I added some event filters on the Display which are responsible for managing the mouse when it enters the regions of the screen that are the 'invisible sash' area. We used filters so the events are dispatched to the sash logic and not to the underlying controls. I'm not entirely convinced that my solution is the solution that we would want to use in the Platform... but I don't an alternative solution to this problem.
For additional context, the screenshot is from a version of our software that's still using v4.10 of the Eclipse Platform.
(In reply to Mike Marchand from comment #7) > I added some event filters on the Display Yes, I've had to do the same with the patch that I've linked to this Gerrit. I'll attach a screenshot to show how it looks with the standard IDE, though I must say your custom product does look neat!
Created attachment 283790 [details] Platform with sash width set to -1, on Windows
Gerrit change https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/167143 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=337a0aaa692b794167a713d4af25ebcc86f492fb
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/167574
(In reply to Eclipse Genie from comment #12) > New Gerrit change created: > https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/167574 This is the second part of the work: it adds the required CSS handling. To test it, you can add the following CSS snippet in a theme: .MPartSashContainer { swt-sash-width: 2px; }
Gerrit change https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/167574 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=6dcb5e8d493e11f0bcb045cfd7e8df67105a375d
The CSS related patch is now merged. Awesome work Pierre-Yves :) And thanks for all the help & review Mike! IMO this should probably be added to the new and note worthy? Something similar to the N&N entry about the active tab highlight CSS maybe?
(In reply to Andrew Obuchowicz from comment #15) > The CSS related patch is now merged. Awesome work Pierre-Yves :) And thanks > for all the help & review Mike! > > IMO this should probably be added to the new and note worthy? Something > similar to the N&N entry about the active tab highlight CSS maybe? +1 for n&n. I suggest to also use this in your default light and dark theme. Can you please create a bug for that also?
(In reply to Andrew Obuchowicz from comment #15) > The CSS related patch is now merged. Awesome work Pierre-Yves :) And thanks > for all the help & review Mike! > > IMO this should probably be added to the new and note worthy? Something > similar to the N&N entry about the active tab highlight CSS maybe? Thank you as well for the reviews and testing! :) (In reply to Lars Vogel from comment #16) > +1 for n&n. > > I suggest to also use this in your default light and dark theme. Can you > please create a bug for that also? Will create the N&N. Will also open a bug targeted at 4.18 for changing the default Platform themes. I'm guessing different people may have different opinions, so I'll create screenshots for a variety of widths and prepare a small vote form so that people can choose their preferred look & feel. :)
(In reply to Pierre-Yves B. from comment #17) > Will create the N&N. Will also open a bug targeted at 4.18 for changing the > default Platform themes. I'm guessing different people may have different > opinions, so I'll create screenshots for a variety of widths and prepare a > small vote form so that people can choose their preferred look & feel. :) Default should be as small as possible. We want our default theme to be as much as possible space efficient.
Last days I often observe (on windows) that the 'resize cursor' is not always reset when no longer hovering over the sashes. The mouse pointer remains incorrect until passing over the sash again. I am assuming that this can be caused by this change, though I don't really understand how this is possible.
(In reply to Rolf Theunissen from comment #19) > Last days I often observe (on windows) that the 'resize cursor' is not > always reset when no longer hovering over the sashes. The mouse pointer > remains incorrect until passing over the sash again. > > I am assuming that this can be caused by this change, though I don't really > understand how this is possible. I can trigger this behavior by releasing the mouse button while dragging the sash. The sash is no longer moved, but the cursor does not change back.
Thanks for reporting Rolf. I've got an idea of why this may be happening, will submit a patch a little later.
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/167870
Note: if bug 566152 (same as comment 19) is a regression from this one, then this patch must be fixed for M3 or the original patch reverted.
I found another regression see Bug 566171. In general, I have some strange mouse behavior lately. First I was blaming my hacks while debugging some layout-out issues. But they still seem around. For instance, the mouse feel sluggish moves slow, or after clicking in a text file suddenly a selection is dragged with the mouse (with no buttons pushed anymore). Can these issues be related to this change?
(In reply to Rolf Theunissen from comment #24) > I found another regression see Bug 566171. > > In general, I have some strange mouse behavior lately. First I was blaming > my hacks while debugging some layout-out issues. But they still seem around. > For instance, the mouse feel sluggish moves slow, or after clicking in a > text file suddenly a selection is dragged with the mouse (with no buttons > pushed anymore). > Can these issues be related to this change? The main point for me now is: we have M3 and soon RC builds. The change here seem to be very invasive and obviously lead to multiple regressions, may be to some not known yet. Should we simply revert all the changes on SashLayout and start experimenting on it in 4.18? IMHO the severity of the feature here does not justify the severity of possible regressions in the SDK. Shipping M3 with mouse cursor occasionally not working on all platforms is a NO GO.
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/167933
(In reply to Andrey Loskutov from comment #25) > (In reply to Rolf Theunissen from comment #24) > > I found another regression see Bug 566171. > > > > In general, I have some strange mouse behavior lately. First I was blaming > > my hacks while debugging some layout-out issues. But they still seem around. > > For instance, the mouse feel sluggish moves slow, or after clicking in a > > text file suddenly a selection is dragged with the mouse (with no buttons > > pushed anymore). > > Can these issues be related to this change? > > The main point for me now is: we have M3 and soon RC builds. The change here > seem to be very invasive and obviously lead to multiple regressions, may be > to some not known yet. Should we simply revert all the changes on SashLayout > and start experimenting on it in 4.18? > > IMHO the severity of the feature here does not justify the severity of > possible regressions in the SDK. Shipping M3 with mouse cursor occasionally > not working on all platforms is a NO GO. I have no official involvement in the Platform project, so it's not up to me to decide whether we should revert or keep on polishing the implementation with the couple of open patches that were submitted, and potentially extra ones. As a side note, the second patch is harmless and could be left as is, given that it modifies an already existing field in SashLayout through optional CSS. However, without the first patch, it's less interesting, you can tweak the width but you probably don't want to assign it values below 2 or 3 pixels.
+1 for revert for 4.17 and to polish the patches within 4.18
Gerrit change https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/167933 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=d18e02c30c6e33ad335fd343c85e2a1cfd1a7f5f
(In reply to Eclipse Genie from comment #29) > Gerrit change > https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/167933 was merged > to [master]. > Commit: > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/ > ?id=d18e02c30c6e33ad335fd343c85e2a1cfd1a7f5f This was the revert of all changes related to this bug. @Pierre: please check the two additional pending patches and provide *one* patch that includes everything needed for the feature to work properly. Please note: the original patch uses Display.addFilter() extensively, despite the fact that this API is explicitly marked as dangerous: "They should generally be avoided for performance, debugging and code maintenance reasons". Honestly speaking, I would prefer the updated patch would not use display filters at all.
(In reply to Andrey Loskutov from comment #30) > Please note: the original patch uses Display.addFilter() extensively, > despite the fact that this API is explicitly marked as dangerous: "They > should generally be avoided for performance, debugging and code maintenance > reasons". > > Honestly speaking, I would prefer the updated patch would not use display filters at all. Yes, we know about this, however, as he pointed out earlier, Mike does not have an alternative solution to this problem, nor do I. I'm also assuming that the people involved in the discussions here or the reviews would have reacted if they had an alternative in mind. :) The big challenge is that if you reduce the sash width, you're effectively hiding the underlying sash containers. The 4 pixel wide borders were previously the areas where these containers were visible and therefore you could attach mouse listeners to them. With a sash width of -1 or 0, you end up with the views and other UI elements next to one another without separators, there's nothing tangible you can attach your mouse listeners to. That's why the patch listens to mouse events on the whole display, and then checks whether the mouse comes near the width-less borders between the UI parts. Rolf pointed out that he observed "the mouse feel sluggish moves slow". I did not personally get the same impression whilst testing nor did any of the other testers report this, but I'm guessing different setups may lead to different behaviours. The described performance impact can however be explained quite simply by the presence of those filters on the whole display. Previously, processing was done only when the mouse interacted with the visible borders between parts. Now, similar processing is done no matter where the mouse is on the display. Additionally, that processing is slightly heavier, there are a few of conditional checks to make sure that we're only reacting to events we're interested in. Lifting the limitations of the UI comes with those two combined performance hits, which seemed completely negligible initially, but may not be in light of Rolf's comment. I see 4 paths forward here: * we deem the usage of display filters acceptable and decide that the performance trade-off is acceptable. * we deem the usage of display filters acceptable, but decide that the performance trade-off is not acceptable. We can then attempt to optimise the code paths within the display filters to make things smoother. * we deem the usage of display filters not acceptable, and someone else manages to come up with an alternative. * we keep the existing UI limitations for all foreseeable future and we abandon this enhancement as well as dependent ones, for example bug 506042.
For information, without Eclipse running, my mouse seem to be sluggish too. Quiting Microsoft PowerToys seem to improve here. So the overall problems observed are probably not be related to this change, nor to Eclipse.
Have a look at Bug 114749 for another possible solution to the problem. I haven't checked out the code though, so I don't know if its applicable in this case.
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/170913
(In reply to Eclipse Genie from comment #34) > New Gerrit change created: > https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/170913 As requested, this is the one patch including all previous patches (167143, 167574, 167870 and a refined version of 167923). All known defects should be addressed.
Mass change to 4.19 M1, please update the target if you have other plans.
Mass move 4.19 M1 bugs to M3
Removing milestone from 4.19 M3 to 4.19, please re-target accordingly.
Mass move out of 4.19
Mass Move out of 4.20
