Revert change of Image surface color type from 24 to 32 bit

[HeikoKlare]

In a previous change (5c2611d), the case distinction in creation of an image's surface depending on using GTK 4 or not has been removed. By accident, this change replaced the previously used 24 bit color format (for both cases) with a 32 bit color format.
This change revert that accidental modification.

[github-actions]

Test Results

   502 files  + 3     502 suites  +3   11m 27s ⏱️ + 1m 29s
 4 334 tests +19   4 320 ✅ +19   14 💤 +1  0 ❌  - 1 
16 575 runs  +37  16 465 ✅ +35  110 💤 +3  0 ❌  - 1 

Results for commit 42a6916. ± Comparison against base commit da081e9.

[ptziegler]

I've set up a test environment where I can run the problematic GEF test with an SWT version that contains this fix. But it seems like the test fails, regardless of whether a 32-bit or a 24-bit color depth is chosen.

Only when I switch back to the previous implementation via gdk_window_create_similar_surface() does the test succeed. This give me the impression that this method copies more attributes from the window than just color depth. The documentation even explicitly says so:

Create a new surface that is as compatible as possible with the given window. For example the new surface will have the same fallback resolution and font options as window.

In the image I posted in the other PR, the text of the second line is slightly shorter than the text in the first line. So missing "font options" would explain those differences. Conversely, this then also means that the GTK4 implementation is incomplete, as there I would expect the same behavior...

What about reverting the changes to the Image class altogether? From what I can tell, the decision to move away from cairo_image_surface_create was done intentionally with 5f4db6c which to me sounds unrelated to the clipping issue you fixed in the other PR.

[HeikoKlare]

In the image I posted in the other PR, the text of the second line is slightly shorter than the text in the first line. So missing "font options" would explain those differences. Conversely, this then also means that the GTK4 implementation is incomplete, as there I would expect the same behavior...

That's interesting. I do not know what other properties are copied by the call to gdk_window_create_similar_surface() causing the difference to using the Cairo API. But we should be aware that this is the only place in Image where this method is still used, and this was only (re-)introduced as a workaround because the actual reason for the issue with the copyArea() method was not identified back then (https://bugs.eclipse.org/bugs/show_bug.cgi?id=571166) but is now fixed with #1757. All other places in Image use cairo_image_surface_create(), so relying on some specify behavior of copying properties from the root surface when using the constructor calling this init() method with gdk_window_create_similar_surface() seems to be wrong (or at least error prone) to me.

What about reverting the changes to the Image class altogether? From what I can tell, the decision to move away from cairo_image_surface_create was done intentionally with 5f4db6c which to me sounds unrelated to the clipping issue you fixed in the other PR.

The call to gdk_window_create_similar_surface() has already been removed several years ago, together with all the others (b84f006). Only at this single place the change was reverted (af9f5ce) because of a bug introduced to the line number ruler (which is the bug I addressed with the mentioned PR).
So even though I am not deep into the GTK implementation of SWT, it seems to me like getting rid of gdk_window_create_similar_surface() is the right thing to do (also regarding GTK 4 compatibility). Note that we have currently no single consumer of that method in the GTK SWT implementation.

Can you detail what exactly you (i.e., the GEF code consuming this method) expect from it? Maybe we can directly address that requirement rather than reverting to a workaround. But in case it's necessary to revert, I don't see any reasons against that (except that we need to find a proper solution to still get rid of that workaround as a follow-up task). We only need to make sure that the line number ruler still works when reverting the change to the Image class (see eclipse-platform/eclipse.platform.ui#2754), but from a quick test I did locally it still seems to work fine.

[ptziegler]

The issue stems from one of our test cases failing because of this change.

In short: In our Graphics object, we push the current state of the GC object on a stack, modify the text anti-aliasing and then afterwards restore the old state with a pop operation.

This no longer works and even though we call setTextAntialiasing() with the value originally returned by getTextAntialiasing(), we can now see a different behavior when actually paining strings.

[ptziegler]

Or maybe to rephrase the problem: When creating a GC object, getTextAntialiasing() returns SWT.DEFAULT but may behave as if SWT.ON is set.

[HeikoKlare]

So I guess the same issue occurs if you paint via GC on any image not created with this specific initialization logic (e.g., an image created via copy constructor), as they all use this initialization function of Cairo. Maybe it even occurs when you paint via GC on any other drawable (like a Canvas or other Controls).

even though we call setTextAntialiasing() with the value originally returned by getTextAntialiasing(), we can now see a different behavior when actually paining strings.
When creating a GC object, getTextAntialiasing() returns SWT.DEFAULT but may behave as if SWT.ON is set.

So might also be that there is an issue wie getTextAntialiasing() or setTextAntialiasing() not being properly implemented.

Can we somehow reproduce the failure of the added test locally? On my system it succeeds even on the master state, so I think it's up to the specifics of the GitHub Actions environment you mentioned in the other PR, isn't it?

[ptziegler]

Can we somehow reproduce the failure of the added test locally? On my system it succeeds even on the master state, so I think it's up to the specifics of the GitHub Actions environment you mentioned in the other PR, isn't it?

That's the reason why this a real pain to figure out... The only way I see this problem is when an Xvnc server is running. I also don't see the issue locally but only when the tests are executed on either the Jenkins or as a GitHub action.

[ptziegler]

Out of interest: What system are you using?

I've just booted up my Ubuntu VM and there I see the same issue that is reported on the build nodes:

That said, I think I see what the problem is. Let's consider the following situation:

• On Gnome, the default anti-aliasing setting seems to be "Grayscale", which corresponds to SWT.ON.
• A GC object might be created without Cairo context (i.e. data.cairo == null)

In this case, getTextAntialias() wrongfully returns SWT.DEFAULT because the method is unable to read the proper value and uses this value as fallback.

eclipse.platform.swt/bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/graphics/GC.java

Lines 2532 to 2547 in da081e9

	public int getTextAntialias() {
	if (handle == 0) SWT.error(SWT.ERROR_GRAPHIC_DISPOSED);
	if (data.cairo == 0) return SWT.DEFAULT;
	int antialias = Cairo.CAIRO_ANTIALIAS_DEFAULT;
	if (data.context != 0) {
	long options = OS.pango_cairo_context_get_font_options(data.context);
	if (options != 0) antialias = Cairo.cairo_font_options_get_antialias(options);
	}
	switch (antialias) {
	case Cairo.CAIRO_ANTIALIAS_DEFAULT: return SWT.DEFAULT;
	case Cairo.CAIRO_ANTIALIAS_NONE: return SWT.OFF;
	case Cairo.CAIRO_ANTIALIAS_GRAY:
	case Cairo.CAIRO_ANTIALIAS_SUBPIXEL: return SWT.ON;
	}
	return SWT.DEFAULT;
	}

I think what this boils down to is that the default value for anti-aliasing might not necessarily match the default value of the system. In case of Ubuntu where Grayscale is the default, Cairo will still using RGB, unless you explicitly call setTextAntialiasing(SWT.DEFAULT), which the switches to the system default.