Community
Participate
Working Groups
Hello, EGit UI's RefSpecPanel class (used to configure push/pull settings) displays a table which has several columns. One of them is a "force" column, which displays a check box. However further inspection of the source code shows that this is not actually an SWT button, but rather an image of a SWT.CHECK Button which is displayed as an image in the Table's column. This approach is highly unusual and is the source of bugs -- there are some hardcoded colors in the check button image that can cause problems and make the "button" look out of place in certain themes. I'd like to suggest using an actual SWT.CHECK Button in this column -- a good example would be the file permissions table (right click any file in the IDE -> Properties -> observe the file permissions table). This will allow the panel to look more consistent on different platforms, and probably have fewer bugs.
The ResourceInfoPage uses an SWT table and the technique shown in SWT Snippet126. The RefSpecPanel uses a TableViewer. I don't see off-hand how to combine the two to put real checkbox controls into that column without having to rewrite the whole thing to use a plain SWT Table instead of a JFace TableViewer? JFace Snippet061 doesn't look appealing to me either. That's a hack. (Taking a screenshot of a real checkbox and then using that image in the label provider -- ugh.) Any suggestions how one could put a real checkbox into that column and have it editable?
New Gerrit change created: https://git.eclipse.org/r/150546
I had a response typed out but then I saw your Gerrit added to the ticket. :)
Would like your comments on that way to mix SWT and JFace. Seems to work fine and correctly on OS X, but haven't tested yet other platforms. (Could do only OS X 10.14 and CentOS 7/gtk 3.22.30/X11 in a virtual machine. Have no Windows machine.)
Okay, my comments were: I believe you can also use a TableEditor (CellEditor in JFace) widget, maybe even an actual SWT.CHECK Button as the editor widget. At a quick glance, "Snippet056 - Boolean Cell Editor" looks useful. If checkboxes are an absolute no-go, maybe something like a simple combo drop down CellEditor widget. Of course you can probably re-write the thing in pure SWT, but I don't know if it's worth the upfront cost. I'll take a look at the patch sometime today.
Created attachment 280157 [details] issue in patchset 3 This is a screenshot from gerrit patchset 3 showing a minor issue on Windows: When editing the ref spec name, the full row is selected. Changing that row selection to another row leads to the checkbox background of the old row not updated, but still showing the selection color.
(In reply to Michael Keppler from comment #6) > Created attachment 280157 [details] > issue in patchset 3 > > This is a screenshot from gerrit patchset 3 showing a minor issue on > Windows: When editing the ref spec name, the full row is selected. Changing > that row selection to another row leads to the checkbox background of the > old row not updated, but still showing the selection color. Doesn't happen on Mac. In GTK in dark mode in an Eclipse Neon child, that bit has a different gray than the rest of the cell, but it's independent of the selection, and I attributed it to a bug in the Neon dark mode CSS. Perhaps calling super.update(cell); at the beginning of NativeCheckboxLabelProvider.update(ViewerCell) helps.
(In reply to Thomas Wolf from comment #7) > Perhaps calling super.update(cell); at the beginning of > NativeCheckboxLabelProvider.update(ViewerCell) helps. Unfortunately no. I've added a workaround in the gerrit change, redraw all checkboxes on tableviewer selection changes. With that it looks good (and in fact better then before) in all cases that I played through. There is just one minor difference to before: The checkbox widget itself no longer shows the hint that is shown on the cell. @Erik: How did you find the issue? Anyone reported actual buggy behaviour or did you somehow stumble over the code pattern? Just asking because I can imagine this kind of pattern is used in more eclipse projects if there is even a snippet recommending that screenshot practice. Would be nice if we could fix those too by generalizing the solution of Thomas.
(In reply to Michael Keppler from comment #8) > Would be nice if we > could fix those too by generalizing the solution of Thomas. It can be generalized (I've done so already, since I want to use it also for the "Remove" column). But there's one big caveat: this will work only for small tables. Windows has a limit of 10000 handles per application, so adding 10000 SWT widgets is a hard upper limit on that platform. Compare bug 550555. So don't try this with a table that can have many items.
(In reply to Michael Keppler from comment #8) > one minor difference to before: The checkbox widget itself no longer shows > the hint that is shown on the cell. Yes, that's also the case on Mac and GTK. We could try to also set the tooltip text on the button, but that would show an SWT tooltip, not the JFace tooltip shown on the cell. I don't know if they're styled the same on all platforms/themes. If not, I think it's preferrable to leave as is; getting differently styled tooltip popups depending on where the mouse pointer is would be too confusing.
(In reply to Thomas Wolf from comment #9) > Windows has a limit of 10000 handles per application, so > adding 10000 SWT widgets is a hard upper limit on that platform. I'm aware of that, and I have even fixed some handle related bugs in the SWT equivalent in VisualAge Smalltalk several years ago. Should anyone have to inspect such handle issues without being able to directly debug the handle creation, I recommend bear.exe to inspect externally what's going on: https://www.the-sz.com/products/bear/ Sorry for sidetracking the bug, but I could not resist. :)
(In reply to Michael Keppler from comment #8) > @Erik: How did you find the issue? Anyone reported actual buggy behaviour or > did you somehow stumble over the code pattern? Just asking because I can > imagine this kind of pattern is used in more eclipse projects if there is > even a snippet recommending that screenshot practice. Would be nice if we > could fix those too by generalizing the solution of Thomas. Initially I was poking around in the EGit source code to reproduce bug 511133. I stumbled across this code and thought "what a hack!", but didn't pursue anything further because there were more pressing issues to fix in SWT at the time. Recently I was dealing with some checkbox oddities due to a change in the default GTK theme, and this same place in EGit was showing some odd behaviour. I figured the time is best spent finally fixing this in EGit, as using a native button is far more effective at ensuring proper platform/theme consistency. There are definitely other areas in the IDE that do this, and when I come across them I'll be reporting them. I even plan to use this fix as an example. A big thanks to you guys for looking at this issue, by the way!
(In reply to Thomas Wolf from comment #9) > (In reply to Michael Keppler from comment #8) > > Would be nice if we > > could fix those too by generalizing the solution of Thomas. > > It can be generalized (I've done so already, since I want to use it also for > the "Remove" column). Although generalizing this is possible and I'll push that, I'm not going to use this for the "Remove" column. Too many problems: * There's funny redraw problems when a row is removed. JFace somehow manages to move up the rows below, but the TableEditors stay. Can be worked around, but the work-around is ugly. * The only way to make that image act as a TableEditor is to use a ToolBar with SWT.FLAT and one ToolItem with the image. Looks and works great on Mac. Can be made to look and work on GTK with some hacks (style the toolbar and toolitem via CSS to have no padding, margins, and background-color transparent). But it cannot be made to look good on Windows. The background is wrong, and remains wrong in dark mode if made transparent, one cannot get rid of internal padding on the ToolItem, and the table rows are not high enough and thus one needs a MeasureItem listener to set a somewhat larger row height. Windows has redraw problems as pointed out by Michael anyway. The work-around (force a redraw of all buttons on selection changes) doesn't work in all cases: select a row, then click directly in a checkbox in another row. The selected row gets the "inactive selection" color (light gray), but the bits above and below the checkbox in the selected row remain blue.
Gerrit change https://git.eclipse.org/r/150546 was merged to [master]. Commit: http://git.eclipse.org/c/egit/egit.git/commit/?id=37cd14d6bf0d86705af91cdaba8e42e218fd1950
(In reply to Eclipse Genie from comment #14) > Gerrit change https://git.eclipse.org/r/150546 was merged to [master]. > Commit: > http://git.eclipse.org/c/egit/egit.git/commit/ > ?id=37cd14d6bf0d86705af91cdaba8e42e218fd1950 Rendering problems on Windows are dealt with as good as possible.
Thanks for this, I really appreciate it!
