Keep theme coloring of cells in FocusCellOwnerDrawHighlighter

[Christopher-Hermann]

In dark theme, the cell back ground color are colored in light colors when using a FocusCellOwnerDrawHighlighter. By setting the background color alpha to 0, the color is not even draw and the initial color is kept.

Coloring on macOS before the fix:

Coloring on macOS after the fix:

Reproduce/Test
To test the issue on your platform, import the project focusCellDemo.zip and execute command "Open focus cell demo" to open a dialog with a demo viewer.

[BeckerWdf]

The JavaDoc of the class says:

A concrete implementation of {@link FocusCellHighlighter} using by setting the control into owner draw mode and highlighting the currently selected cell.

Does your change now no longer highlight the current selected cell?
Where does the getBackground(cell.getColumnIndex()) call get it's color from? Maybe we only need to override a default color in the dark theme.

[github-actions]

Test Results

   918 files  +    1     918 suites  +1   47m 33s ⏱️ + 9m 44s
 7 507 tests ±    0   7 354 ✅ ±    0  151 💤 ±  0  2 ❌ ±0 
23 670 runs  +1 573  23 163 ✅ +1 454  505 💤 +119  2 ❌ ±0 

For more details on these failures, see this check.

Results for commit cbf9737. ± Comparison against base commit 5bd3301.

♻️ This comment has been updated with latest results.

[Christopher-Hermann]

Does your change now no longer highlight the current selected cell?

The cell is still highlighted, see the screenshot. The problem was that the rest of the row should be colored as if it wasn't selected. This was done by setting the background to cell.getViewerRow().getBackground(cell.getColumnIndex()), but somehow this returns the color of the light theme. I did not find out why this is the case. By setting the background alpha to 0, nothing is draw and the color is kept as it was never selected.

Where does the getBackground(cell.getColumnIndex()) call get it's color from? Maybe we only need to override a default color in the dark theme.

Yes, good point. The problem is that the table rows are displayed in black and a very dark grey in alternating order. And as said, cell.getViewerRow().getBackground(cell.getColumnIndex()) does somehow returns the wrong color. I did not find where the actuall cell color is set for dark mode. I will check again if I can find some more information.

[Christopher-Hermann]

The problem is that the color for table row is never set, neither in the light mode nor in the dark mode. So the default color of the viewer is used, which is just a pure white:

Color defaultBackground () {
	return display.getWidgetColor (SWT.COLOR_LIST_BACKGROUND);
}

So if possible the best solution would be to set the color into the viewer row, but as said, the alternating color must be followed and I cannot find any coding where this is done. So if someone has an idea or a hint where I can find coding related to this it would be nice.

[BeckerWdf]

The problem is that the color for table row is never set, neither in the light mode nor in the dark mode. So the default color of the viewer is used, which is just a pure white:

Color defaultBackground () {
	return display.getWidgetColor (SWT.COLOR_LIST_BACKGROUND);
}

So if possible the best solution would be to set the color into the viewer row, but as said, the alternating color must be followed and I cannot find any coding where this is done. So if someone has an idea or a hint where I can find coding related to this it would be nice.

Where is this alternating of the color done?

[Christopher-Hermann]

The problem is that the color for table row is never set, neither in the light mode nor in the dark mode. So the default color of the viewer is used, which is just a pure white:

Color defaultBackground () {
	return display.getWidgetColor (SWT.COLOR_LIST_BACKGROUND);
}

So if possible the best solution would be to set the color into the viewer row, but as said, the alternating color must be followed and I cannot find any coding where this is done. So if someone has an idea or a hint where I can find coding related to this it would be nice.

Where is this alternating of the color done?

I'm not sure, I cannot find the coding for this one. I think this is a Mac only feature, at least on windows the alternating of the color is not done.

[BeckerWdf]

The problem is that the color for table row is never set, neither in the light mode nor in the dark mode. So the default color of the viewer is used, which is just a pure white:

Color defaultBackground () {
	return display.getWidgetColor (SWT.COLOR_LIST_BACKGROUND);
}

So if possible the best solution would be to set the color into the viewer row, but as said, the alternating color must be followed and I cannot find any coding where this is done. So if someone has an idea or a hint where I can find coding related to this it would be nice.

Where is this alternating of the color done?

I'm not sure, I cannot find the coding for this one. I think this is a Mac only feature, at least on windows the alternating of the color is not done.

Ah ok. So this may be in the native part of SWT.

[BeckerWdf]

This issue also exists in the light theme on macOS but it's not so apparent there: For lines with light background it's correct and for grey lines the difference between grey and white can easily been overlooked.
I find the fix of making the cell transparent (so that the correct background shines through) instead of setting the background very elegant.
I propose to merge this once master is open for the next release.
@HeikoKlare: Can you test this on windows and linux? Just to be sure we don't break anything there?

[HeikoKlare]

I have tested the change using the provided demo shell on Windows (Windows 11 Enterprise, Version 21H2, Build 22000.2777) without noticing any difference in behavior in highlighting behavior. Below, you can find two example screenshots with and without the patch.

Without the patch:

With the patch:

I'll also test on Linux and post the results.

[HeikoKlare]

Here are my test results with the provided demo shell on Linux (Ubuntu 22.04.4 LTS, Gnome 42.9, X11). There is a noticable difference on Linux, and I think the existing implementation is actually motivated by the behavior on GTK: It seems like on GTK the whole row is highlighted and setting the individual background color for the cells ensures that for non-selected cells the highlighting is overwritten. You can see in the screenshots below, that this is a kind of hacky solution, as you can still see the row highlighted behind the cell, but at least the user can see which cell is selected. After applying this patch, it will become impossible to know which cell is selected.

Without the patch:

With the patch:

[BeckerWdf]

Here are my test results with the provided demo shell on Linux (Ubuntu 22.04.4 LTS, Gnome 42.9, X11). There is a noticable difference on Linux, and I think the existing implementation is actually motivated by the behavior on GTK: It seems like on GTK the whole row is highlighted and setting the individual background color for the cells ensures that for non-selected cells the highlighting is overwritten. You can see in the screenshots below, that this is a kind of hacky solution, as you can still see the row highlighted behind the cell, but at least the user can see which cell is selected. After applying this patch, it will become impossible to know which cell is selected.

Without the patch:

With the patch:

So should we limit @Christopher-Hermann patch to only set the transparency on macOS as on windows the problem doesn't exist and on GTK it's even harmfull?

[HeikoKlare]

So should we limit @Christopher-Hermann patch to only set the transparency on macOS as on windows the problem doesn't exist and on GTK it's even harmfull?

If there is no alternative, I think that would be the way to go. However, I would try to avoid OS-specific behavior whenever possible. Do we know why the original line is working on every OS except Mac? More precisely, do we know why the background color returned by the viewer row is "wrong" on Mac? Since it works on the other OS', the returned color does not seem to be non-themed in general.

eclipse.platform.ui/bundles/org.eclipse.jface/src/org/eclipse/jface/viewers/FocusCellOwnerDrawHighlighter.java

Line 109 in ea87817

gc.setBackground(cell.getViewerRow().getBackground(cell.getColumnIndex()));

[Christopher-Hermann]

cell.getViewerRow().getBackground(cell.getColumnIndex()) is returning the background color of the viewer (on every OS). The background color is explicit set to unset on MacOS to keep the alternating coloring that is provided in MacOS (can also be seen in the files explorer on Mac):

eclipse.platform.ui/bundles/org.eclipse.ui.themes/css/e4-dark_mac.css

Lines 43 to 49 in cb121fe

	Table[swt-lines-visible=true] {
	background-color: unset;
	}
	Tree[swt-lines-visible=true] {
	background-color: unset;
	}

By unsetting the color for Trees/Tables on MacOs, the default color is returned which is just white.

[HeikoKlare]

Thanks for explanation, @Christopher-Hermann!

I currently don't see any generic, i.e., OS-independent, solution. My understanding (from some experiments I made) is as follows: On Windows and Mac, it is sufficient to remove the SWT.SELECTED flag from the event details (and to restore the text color, i.e., the foreground color). On GTK, however, the line is highlighted even though the SWT.SELECTED flag is removed, which is why the background has to be overwritten explicitly. This also results in weird appearance, as only the cell backgrounds are corrected but not the borders cell and row borders.

So in conclusion: the "special case" for me is GTK and not Mac, which is why I would not propose to apply the alpha-value solution for Mac. Instead I would restrict the application of the background color to GTK (or even better to ensure that the row is not highlighted on GTK at all, like it is done on the other OS').

[BeckerWdf]

Instead I would restrict the application of the background color to GTK (or even better to ensure that the row is not highlighted on GTK at all, like it is done on the other OS').

At @Christopher-Hermann is this something you think can do?

[Christopher-Hermann]

Yes, done. Thanks for testing and the hint @HeikoKlare. The fix works fine on MacOS

[HeikoKlare]

Thanks for updating the PR! I've tested the current changes on all three platforms and they work as expected.

Just one minor proposal for improvement: instead of adding further platform dependencies, I suggest to use the abilities for GTK identification provided by SWT, as the dependency to SWT exists anyway.

[HeikoKlare]

Still works as expected. Thank you!
Ready to be merged from my side.

[BeckerWdf]

Still works as expected. Thank you! Ready to be merged from my side.

Same for me. I will merge once the validation-build looks good.

[BeckerWdf]

Test failures are pre-existing and already reported in
#1427
#294

Merging this.
Thanks @Christopher-Hermann for providing the fix.