Initial custom Group widget implementation #120

lathapatil · 2025-02-27T13:33:23Z

Known Issues :

A filled rectangle is drawn to erase the part of the Group where the title is present. This rectangle is slightly visible in the control example when the background color is white, but it is programmatically treated as grey.
Text Rendering: Text orientation works as expected, but text direction does not behave correctly.
HiDPI Scaling: Unable to test controlExample when scaled to 175% because the control example does not fit within the screen.
PrintWidget not yet worked on.
SWT.Border is not working as expected.

Any suggestions are welcome.

github-actions · 2025-02-27T13:36:49Z

Test Results

136 files - 202 136 suites - 202 1m 11s ⏱️ - 1m 18s
3 935 tests - 19 3 611 ✅ - 59 324 💤 + 40 0 ❌ ±0
3 973 runs - 7 717 3 649 ✅ - 7 156 324 💤 - 561 0 ❌ ±0

Results for commit 6b16d26. ± Comparison against base commit fb29be9.

This pull request removes 19 tests.

org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicASCII_dollarSign
org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicASCII_emptyString
org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicASCII_letterA
org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicASCII_letters
org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicUTF16LE_null
org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicUTF16_AsciiLetters
org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicUTF16_Asciiletter
org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicUTF16_LotsOfLetters
org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicUTF16_letter
org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicUTF16_letters
…

This pull request skips 41 tests.

org.eclipse.swt.tests.junit.Test_org_eclipse_swt_custom_CCombo ‑ test_setForegroundAlphaLorg_eclipse_swt_graphics_Color
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_custom_CLabel ‑ test_setForegroundAlphaLorg_eclipse_swt_graphics_Color
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_custom_CTabFolder ‑ test_setForegroundAlphaLorg_eclipse_swt_graphics_Color
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_custom_StyledText ‑ test_setForegroundAlphaLorg_eclipse_swt_graphics_Color
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_widgets_Button ‑ test_setForegroundAlphaCheckButton
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_widgets_Button ‑ test_setForegroundAlphaLorg_eclipse_swt_graphics_Color
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_widgets_Button ‑ test_setForegroundAlphaRadiokButton
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_widgets_Canvas ‑ test_setForegroundAlphaLorg_eclipse_swt_graphics_Color
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_widgets_Combo ‑ test_setForegroundAlphaDropDownCombo
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_widgets_Combo ‑ test_setForegroundAlphaLorg_eclipse_swt_graphics_Color
…

♻️ This comment has been updated with latest results.

bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/widgets/Group.java

tmssngr · 2025-02-28T08:28:42Z

bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/widgets/Group.java

+	private String text = "";
+	static final int CLIENT_INSET = 3;
+	private Listener listener;
+	private boolean enabled = true;


The enabled state should not be tracked here, but instead isEnabled() should be used, because it should be drawn disabled even if it is set enabled, but a parent is disabled.

bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/widgets/Group.java

tmssngr · 2025-02-28T08:37:50Z

bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/widgets/Group.java

+
+@Override
+void enableWidget(boolean enabled) {
+	this.enabled = enabled;


Don't remember here.

bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/widgets/Group.java

lathapatil · 2025-03-04T13:04:32Z

@DenisUngemach

I see a performance issue in this branch when running ControlExample, especially when the system's display is scaled up to 175%.

Additionally, the JMC performance tool highlights the following method as a concern, and I also noticed a heap space issue, as shown in the attached image. Is it known issue or should there be some issue in my code ?

Method Profiling

The most sampled method was void org.eclipse.swt.graphics.ImageData.blit(byte[], int, int, int, int, int, int, int, int, byte[], int, int, int, int, int, int, int, int, boolean, boolean), with 3.45 % of the maximum possible samples for 3/4/2025, 6:03:42.000 PM – 6:04:12 PM, and 64.8 % of the actual samples.

The methods that used the most CPU are:
•org.eclipse.swt.graphics.ImageData.blit(byte[], int, int, int, int, int, int, int, int, byte[], int, int, int, int, int, int, int, int, boolean, boolean) (65.3 % of samples) 3/4/2025, 6:03:57.000 PM – 6:04:27 PM
•org.eclipse.swt.graphics.ImageData.blit(byte[], int, int, int, int, int, int, int, int, byte[], int, int, int, int, int, int, int, int, boolean, boolean) (65.6 % of samples) 3/4/2025, 6:07:57.000 PM – 6:08:27 PM
•org.eclipse.swt.graphics.ImageData.blit(byte[], int, int, int, int, int, int, int, int, byte[], int, int, int, int, int, int, int, int, boolean, boolean) (64.8 % of samples) 3/4/2025, 6:03:42.000 PM – 6:04:12 PM

The most common stack trace was: at
void org.eclipse.swt.graphics.ImageData.blit(byte[], int, int, int, int, int, int, int, int, byte[], int, int, int, int, int, int, int, int, boolean, boolean)at org.eclipse.swt.graphics.ImageData org.eclipse.swt.graphics.Image.directToDirect(org.eclipse.swt.graphics.ImageData, int, org.eclipse.swt.graphics.PaletteData, int)at long[] org.eclipse.swt.graphics.Image.init(org.eclipse.swt.graphics.Device, org.eclipse.swt.graphics.Image, org.eclipse.swt.graphics.ImageData, java.lang.Integer)at void org.eclipse.swt.graphics.Image.init(org.eclipse.swt.graphics.ImageData, java.lang.Integer)at void org.eclipse.swt.graphics.Image.<init>(org.eclipse.swt.graphics.Device, java.io.InputStream)at void org.eclipse.swt.graphics.SkijaGC.commit()at void org.eclipse.swt.graphics.GC.commit()at void org.eclipse.swt.graphics.Drawing.drawWithGC(org.eclipse.swt.widgets.Control, org.eclipse.swt.graphics.GC, java.util.function.Consumer)at void org.eclipse.swt.widgets.Group.onPaint(org.eclipse.swt.widgets.Event)at void org.eclipse.swt.widgets.Group.lambda$0(org.eclipse.swt.widgets.Event)at void org.eclipse.swt.widgets.Group$$Lambda$29+0x000002bd9308b348.212628335.handleEvent(org.eclipse.swt.widgets.Event)at void org.eclipse.swt.widgets.EventTable.sendEvent(org.eclipse.swt.widgets.Event)at void org.eclipse.swt.widgets.Display.sendEvent(org.eclipse.swt.widgets.EventTable, org.eclipse.swt.widgets.Event)at void org.eclipse.swt.widgets.Widget.sendEvent(org.eclipse.swt.widgets.Event)at void org.eclipse.swt.widgets.Widget.sendEvent(int, org.eclipse.swt.widgets.Event, boolean)at void org.eclipse.swt.widgets.Widget.sendEvent(int, org.eclipse.swt.widgets.Event)at org.eclipse.swt.internal.win32.LRESULT org.eclipse.swt.widgets.Composite.WM_PAINT(long, long)at long org.eclipse.swt.widgets.Control.windowProc(long, int, long, long)at long org.eclipse.swt.widgets.Display.windowProc(long, long, long, long)at long org.eclipse.swt.internal.win32.OS.DispatchMessage(org.eclipse.swt.internal.win32.MSG)at boolean org.eclipse.swt.widgets.Display.readAndDispatch()at void org.eclipse.swt.examples.controlexample.ControlExample.main(java.lang.String[])

DenisUngemach · 2025-03-04T13:16:43Z

Yes, it is not a suprise, that the SkijaGC.commit call requires a lot of performance.

We want to get rid of all these image conversion calls in the future either with OpenGL or the direct use of windows GDI.

See issue #107

Test Snippet Review points fix Review points fixed

tmssngr · 2025-03-11T13:22:31Z

bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/widgets/Group.java

+}
+
+private void drawGroup(GC gc) {
+


Please remove this empty line.

tmssngr · 2025-03-11T13:23:22Z

bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/widgets/Group.java

+	checkWidget();
+	Point size = DPIUtil.autoScaleUp(super.computeSize(wHint, hHint, changed));
+
+	if (!changed && new Point(wHint, hHint).equals(size)) {


Instead of creating a new Point, why not just compare the x and y values?

tmssngr · 2025-03-11T13:24:16Z

bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/widgets/Group.java

+		return size;
+	}
+	Layout layout = getLayout();
+	Point layoutSize = (layout != null) ? layout.computeSize(this, wHint, hHint, changed)


Hm, the above super.computeSize() already invokes the layout manager. Why here again?

Thanks for this finding . I was able to fix other scaling issue as well while fixing this

tmssngr · 2025-03-11T13:25:26Z

bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/widgets/Group.java

+}
+
+@Override
+void enableWidget(boolean enabled) {


Is that method necessary?

It may not be necessary since I do not have any additional implementation compared to the parent (Control), and it is called solely by the parent (Control) in the setEnabled method in SWT. I retained this method as it was in the original Group widget implementation. I will remove it.

tmssngr · 2025-03-11T13:25:54Z

bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/widgets/Group.java

+	checkWidget();
+	Rectangle rect = super.getClientArea();
+	if (rect.isEmpty())
+		return rect;


Please surround the return statement with {}.

tmssngr · 2025-03-11T13:26:19Z

bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/widgets/Group.java

+boolean mnemonicMatch(char key) {
+	char mnemonic = findMnemonic(getText());
+	if (mnemonic == '\0')
+		return false;


tmssngr · 2025-03-11T13:27:37Z

bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/widgets/Group.java

+private void drawGroup(GC gc) {
+
+	int width = getBounds().width;
+	int height = getBounds().height;


Don't invoke getBounds() two times, but rather cache the result.

tmssngr · 2025-03-11T13:28:45Z

bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/widgets/Group.java

+	int width = getBounds().width;
+	int height = getBounds().height;
+	int titleWidth = textExtent.x;
+	int titleHeight = textExtent.y;


Are you sure, the textExtent is alway set correctly here?

textExtent is calculated whenever text/group title is changed using setText(String text) . Do you find any loophole here ?

If the font or the font size changes.

But basically it is right to cache these information and use them directly in the drawing, instead of doing all these calculations in the drawing.

In the end the efficiency of the drawing method defines the speed of the widget toolkit, because the drawings of controls cluster in the redraw.

It is more the question, how can we make sure these calculations are always up to date without recalculate them all the time (general question... :-) ).

Caching is fine, but if a method is using the cached values, you need to ensure that they are valid at this point. BTW, better indicate cached values in their variable names and before using them, check whether these values are valid.

tmssngr · 2025-03-11T13:30:26Z

bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/widgets/Group.java

+	int textX = inset + groupInset;
+	int textY = titleHeight;
+	int newWidth = width - (inset * 2);
+	int newHeight = height - (inset * 5);


Please use better variable name, e.g. borderWidth/Height.

tmssngr · 2025-03-11T13:33:01Z

The tests fail and the border is truncated at the bottom for your example:

DenisUngemach · 2025-03-12T14:20:47Z

Yes, it is not a suprise, that the SkijaGC.commit call requires a lot of performance.

We want to get rid of all these image conversion calls in the future either with OpenGL or the direct use of windows GDI.

See issue #107

The more i think about this:

-> first drawing the group widget with SkijaGC + commit
-> then drawing the child widgets with SkijaGC + commit

and this happens at each redraw() and these "SkijaGC + commit" of parent and child are separated calls.

Of course the bigger our widgets become, the more lag the SkijaGC + commit causes.

Generally:
We have to think about an efficient concepts for redrawing.

lathapatil · 2025-03-13T05:15:37Z

Of course the bigger our widgets become, the more lag the SkijaGC + commit causes.

I had unnecessarily scaled up some dimensions in the implementation. After fixing it, I was able to launch controlExample, and the performance improved.
However, the issue mentioned above is a matter of concern and needs to be fixed. Perhaps the approach to fixing this can be broken down into multiple issues so that different people can take them up for resolution.