[GTK] Fix GC#copyArea() for overlapping source and target areas #1756

[HeikoKlare]

The GC provides a #copyArea() method that copies an area inside the surface of the GC to another area in the same surface. When using a surface directly created via the Cairo API (or with the GDK API on Wayland), this operation is broken.
When the source and target area are overlapping and the source is above-left of the target area, the source area will appear replicated in the target area, as Cairo does not perform any buffering but does a linewise/chunkwise write from the source area to the target area.

This change fixes the behavior of GC#copyArea() by explicitly making Cairo first pipe the copied area into an intermediate surface to be painted into the actual target area afterwards. It also removes the existing workaround in images that avoids the instantiation of a surface via the Cairo API, as the reason for that workaround was the issue fixed by this change. The effect can be seen in the description of #1756. The operation now produces the result shown as the expected one in that issue, also on GTK4 and when using Wayland. An according regression test is added.

Fixes #1756

Can be tested in the line number ruler, which has also proper behavior on GTK4 and when using Wayland with this fix. Can also be tested together with eclipse-platform/eclipse.platform.ui#2754 resolving the workaround that was implemented to deal with the bug in SWT.

There might be better solutions than this (e.g., methods directly provided by Cairo that perform the buffering), but I did not find them yet and actually any solution will have to do something like the solution proposed here (buffering the area to be copied).

[github-actions]

Test Results

   494 files  + 1     494 suites  +1   11m 19s ⏱️ +48s
 4 331 tests +38   4 318 ✅ +36   13 💤 +3  0 ❌ ±0 
16 572 runs  +41  16 464 ✅ +38  108 💤 +4  0 ❌ ±0 

Results for commit 094abb8. ± Comparison against base commit 8d65ddc.

♻️ This comment has been updated with latest results.

[HeikoKlare]

@iloveeclipse can you please have a look at this? You were involved in the development of the workaround for the copyArea() issue (eclipse-platform/eclipse.platform.ui#2754), which this PR replaces with an actual solution of the underlying problem, as well as in the discussion on the regression we had because of that in the line number ruler this week. So would be great to have your opinion and/or testing of this (in the best case together with the removal of the Platform UI workaround eclipse-platform/eclipse.platform.ui#2754).

[tomaswolf]

I'm by no means a Cairo expert, so if I'm completely off here just say so and ignore my input :-)

Drawing into a temporary surface: isn't this what cairo_push_group and cairo_pop_group_to_source are for?

[iloveeclipse]

I'm by no means a Cairo expert, so if I'm completely off here just say so and ignore my input :-)

Me too :-)

I've tested this patch alone and also with reverted the UI workaround for line number ruler, everything seem to work

[iloveeclipse]

@sravanlakkimsetti : I believe you were involved in earlier fixes in this area, do you have time to look into this PR?

@HeikoKlare : if you want merge this, best before M3.

[HeikoKlare]

Drawing into a temporary surface: isn't this what cairo_push_group and cairo_pop_group_to_source are for?

Thank you for the pointer! I saw code using these methods in other cases of copyArea() as well, but did not further look into it. I will test whether that can be used for an even simpler solution.

I've tested this patch alone and also with reverted the UI workaround for line number ruler, everything seem to work

Thank you for testing! If we agree that this change (or an improvement incorporating the above mentioned pointer) is fine and has low risk of regression, I would be in favor of merging as soon as possible to have some testing of this in daily use until M3/release.

[HeikoKlare]

Drawing into a temporary surface: isn't this what cairo_push_group and cairo_pop_group_to_source are for?

Just implemented it that way (e89afc8) and it works perfectly fine for me (tested with line number ruler on X11, Wayland and by executing the added regression test). Much simpler and proper usage of Cairo API provided for this use case, so to me this now seems to be the right fix for the issue. Thank you, @tomaswolf!

[iloveeclipse]

Last patch seem to work too. Please rebase on master.

[tomaswolf]

Glad to see my idea wasn't misleading at all. And thanks for using it; the code is much cleaner now!

[HeikoKlare]

Tested again after updating to I20250124-1800 on Ubuntu 24.04: With X11 everything is working as before. Using Wayland the previous glitches with the line number ruler are fixed.

[ptziegler]

We seem to be facing a test failure in GEF, ever since the M3 went live (or to be more precise, starting with I20250124-1800, so I'm fairly certain this commit is responsible for this behavior.

The test in question is checking whether anti-aliasing is working correctly, as shown below. The first line is how it currently looks like, the second line is what it's supposed to look like. Note that the second line is also what's shown on e.g. Windows.

Weirdly enough, this issue only happens on the Jenkins/GitHub builds which are using Xvnc, but not when executed locally. Reason seems to be that Xvnc only supports up to 24 bit.

Specify the pixel depth in bits of the desktop to be created. Default is 24, other possible values are 8, 15, and 16 - anything else is likely to cause strange behaviour by applications.

So my question: @HeikoKlare is there a technical reason for going from 24 to 32 bit? Because if I understand it correctly, Windows creates at most a 24bit handle:

eclipse.platform.swt/bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java

Lines 1480 to 1487 in 857ac0b

	if (imageMetadata.handle == 0) {
	int bits = OS.GetDeviceCaps(hDC, OS.BITSPIXEL);
	int planes = OS.GetDeviceCaps(hDC, OS.PLANES);
	int depth = bits * planes;
	if (depth < 16) depth = 16;
	if (depth > 24) depth = 24;
	imageMetadata = new ImageHandle(createDIB(width, height, depth), getZoom());
	}

The same for MacOS:

eclipse.platform.swt/bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/graphics/Image.java

Line 1441 in 857ac0b

rep = rep.initWithBitmapDataPlanes(0, width, height, 8, 3, false, false, OS.NSDeviceRGBColorSpace, OS.NSAlphaFirstBitmapFormat | OS.NSAlphaNonpremultipliedBitmapFormat, width * 4, 32);

Would it make sense to go back to 24 bit for Linux as well, rather than being the odd one out? Especially if it risks causing visual glitches on systems that only support True Color?

[HeikoKlare]

Thank you for the catch, @ptziegler!

So my question: @HeikoKlare is there a technical reason for going from 24 to 32 bit? Because if I understand it correctly, Windows creates at most a 24bit handle:

I do not remember any technical reason for doing that. Actually, if I remember correctly, the intended change was to simply remove the else case (previously kept for GTK 3 compatibility). I have no clue why I changed the color type of the remaining case.

Since both previous cases (if/else, GTK4/3) used a 24 bit color type for the created surface, it should be safe to revert that change. Thus I proposed:

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

Maybe you want to check whether that PR fixes your test?