Replace polyline/Region drawing with direct line and rect calls in CT…

[vogella]

…abFolderRenderer

Now that all tab corners are square, the polyline-based shape approach is unnecessary. Replace with direct gc.drawLine() / gc.drawRectangle() calls:

• Remove EMPTY_CORNER constant (was {0,0}, i.e. no corner offset)
• Remove drawBorder(GC, int[] shape) — callers now draw lines directly
• Simplify drawLeftUnselectedBorder / drawRightUnselectedBorder to a single gc.drawLine() each
• Remove Region-based polygon clipping from drawBackground(); since all shapes are rectangular the clipping was a no-op — remove the shape parameter from the signature entirely
• Remove fillRegion() — no longer called
• Simplify shape arrays in drawSelected and drawTabArea from loop-built polylines (that iterated over EMPTY_CORNER) to inline int[] literals
• Remove the Region.subtract(shape) / fillRegion block in drawTabArea that painted parent background into curved-corner cutouts (corners are now square so this region was always empty)

[vogella]

@HeikoKlare this can be done in additional to your changes, please review

[github-actions]

Test Results

  170 files  ±0    170 suites  ±0   25m 30s ⏱️ -37s
4 679 tests ±0  4 658 ✅ ±0   21 💤 ±0  0 ❌ ±0 
6 592 runs  ±0  6 437 ✅ ±0  155 💤 ±0  0 ❌ ±0 

Results for commit bbef8ad. ± Comparison against base commit c83ed3c.

♻️ This comment has been updated with latest results.

[HeikoKlare]

In general, the changes look sound to me. But this PR combines quite many changes/concerns into a single one where it's easy to oversee suspicious parts.

What I do not understand are the changes related to drawBackground calls. When testing with the snippets from #3168, the behavior (regarding gradients) looks fine to me, but I could not have anticipated that from looking at the code change. Thus I would appreciate an explanation why the changes are correct.

[HeikoKlare]

I do not understand how the different cases became obsolete and were thus removed.

[vogella]

The three branches applied gradient fill to the highlight_margin L-shaped frame. That frame is 2 pixels wide (highlight_margin = 2 for non-FLAT; 0 for FLAT). Rendering a horizontal gradient across a 2-pixel strip is visually indistinguishable from a solid color — the gradient collapses to a single color sample at that scale. gc.fillPolygon(shape) with the solid background color produces an equivalent visual result.

[HeikoKlare]

I do not easily understand why this is equivalent to the previous call either.

[vogella]

The SWT.BACKGROUND case repaints the close button area to erase a previously drawn hot/selected close icon and restore the background. The tab background (including any gradient) was already painted by drawTabArea/drawSelected before drawClose is called. The old drawBackground(gc, shape, false) applied parent.gradientColors over a 10×10 pixel area — a gradient fragment at that scale is visually indistinguishable from a solid color. Using parent.getBackground() correctly restores the area with no perceptible difference.

[HeikoKlare]

Contributed by Claude:

Thank you for this cleanup PR. The approach is sound overall, and I have verified several of the shape simplifications are equivalent. Here is a detailed analysis:

Verified as Correct

Shape simplifications in drawTabArea: Tracing through the EMPTY_CORNER ({0,0}) loop iterations confirms the new inline literals produce identical coordinate arrays. The special handling with borderLeft == 0 ? 1 : 0 is preserved correctly in the bottomY variable.

fillRegion removal: The old code computed r = rect.subtract(shape) where shape was exactly that same rectangle — the resulting region was always empty, so fillRegion was never called. Safe to remove.

drawBorder → drawLine: The EMPTY_CORNER-based shapes in drawLeftUnselectedBorder / drawRightUnselectedBorder each produced a two-point polyline (a single line segment), so the replacement with gc.drawLine() is equivalent.

drawTabArea shape-based clipping removal: The shape passed to drawBackground(gc, shape, bkSelected) was the tab header rectangle itself, so the Region clip was identical to the draw area — a no-op. The new call to drawBackground(gc, bkSelected) draws the same thing.

drawSelected non-firstIndex shape literals: Verified the EMPTY_CORNER loop produces the same coordinate arrays as the new inline literals for both onBottom and !onBottom cases.

Apparent Bug Fix (Good)

drawSelected onBottom, firstIndex shape: The old code set left = new int[]{x, y+height} (where x is the tab's left edge in absolute coordinates) and then used it as a corner offset, computing the point (x + left[0], y + height + left[1] - 1) = (2x, 2y + 2*height - 1). For x > 0 this would place the corner far outside the tab. The new shape correctly places the bottom-left corner at (x, y+height). This looks like a pre-existing bug fix hidden in the simplification — worth calling out explicitly in the commit message.

Concerns

1. drawBody — gradient silently dropped (already commented inline)

The highlight_margin L-shaped frame previously received horizontal gradient fill when selectionGradientColors or gradientColors was configured. The new code always uses solid color fill (gc.fillPolygon(shape)). Since the L-shape is not rectangular, the Region-based clipping was not a no-op here — it actively restricted the gradient to the margin frame. Whether this is intentional simplification or an unintended regression needs clarification.

2. drawSelected highlight header — draw area widened

In the base code:

int[] shape = new int[] {xx, yy, xx+ww, yy, xx+ww, yy+hh, xx, yy+hh};
drawBackground(gc, shape, parent.shouldHighlight());

The shape clip restricted the gradient to the narrow highlight_header - 1 strip between the tabs and the body area (height ≈ 2px for non-FLAT). The new drawBackground(gc, parent.shouldHighlight()) draws to the full tab header area (tabHeight + highlight_header high). This is functionally a wider paint, though the tabs themselves are rendered on top so it may not be visible.

3. drawClose — gradient explicitly removed (already commented inline)

The old call drawBackground(gc, shape, false) passed parent.gradientColors/parent.gradientPercents through. The new call passes explicit null for colors, so gradient is never applied to the close-button background area. Please explain whether this is intentional.

Overall

The drawTabArea and unselected-border simplifications are clean and correct. The issues above all relate to drawBackground call sites, consistent with your existing PR-level comment asking for an explanation of those changes. I would appreciate clarification on the three points above before merging.

[HeikoKlare]

I've merged #3172, so this could be rebased on top and then implicitly becomes a bit more simple.

[vogella]

Thank you for the detailed review (and the AI-assisted analysis)!

Addressing all three drawBackground concerns:

drawBody — gradient on highlight_margin frame
The gradient was applied to an L-shaped frame that is 2px wide (highlight_margin = 2 for non-FLAT, 0 for FLAT). A horizontal gradient across 2 pixels is visually indistinguishable from a solid color — the gradient collapses to essentially a single color sample at that scale. Solid fill is equivalent here. (Also replied inline.)

drawClose SWT.BACKGROUND — gradient removed
This case erases a previously drawn hot/selected close icon by repainting the background. The tab's actual gradient background was already painted by drawTabArea/drawSelected before drawClose is invoked. The old path applied parent.gradientColors over a 10×10 pixel area — a gradient fragment that small is also indistinguishable from solid. Using parent.getBackground() is correct. (Also replied inline.)

drawSelected highlight header — draw area widened
The old shape-clipped call drew into a strip of height highlight_header - 1 = 2px. The new drawBackground(gc, parent.shouldHighlight()) draws over the full tab header height, but tab items are rendered on top of that area immediately afterward — the extra painted area is never visible.

All three cases involve rendering gradients into areas of ≤2px height or ≤10px total size, or areas immediately overdrawn by tab content — which is consistent with your own observation that visual testing shows no regression.

[github-actions]

Test Results (macos)

   54 files  ±0     54 suites  ±0   6m 58s ⏱️ - 2m 17s
4 531 tests ±0  4 287 ✅ ±0  244 💤 ±0  0 ❌ ±0 
2 130 runs  ±0  2 068 ✅ ±0   62 💤 ±0  0 ❌ ±0 

Results for commit f121784. ± Comparison against base commit d820f7e.

[github-actions]

Test Results (linux)

   55 files   -    33     55 suites   - 33   6m 8s ⏱️ - 7m 56s
4 421 tests  -   136  4 175 ✅  -   162  246 💤 +26  0 ❌ ±0 
1 135 runs   - 2 140  1 096 ✅  - 2 110   39 💤  - 30  0 ❌ ±0 

Results for commit f121784. ± Comparison against base commit d820f7e.

This pull request removes 136 tests.

org.eclipse.swt.tests.gtk.snippets.Bug336238_ShellSetBoundFailTest ‑ testSetBounds
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_browser_Browser ‑ test_ClearAllSessionCookies
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_browser_Browser ‑ test_OpenWindowListener_openHasValidEventDetails
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_browser_Browser ‑ test_OpenWindowListener_open_ChildPopup
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_browser_Browser ‑ test_OpenWindow_Progress_Listener_ValidateEventOrder
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_browser_Browser ‑ test_StatusTextListener_hoverMouseOverLink
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_browser_Browser ‑ test_VisibilityWindowListener_eventSize
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_browser_Browser ‑ test_VisibilityWindowListener_multiple_shells
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_browser_Browser ‑ test_get_set_Cookies
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_browser_Browser ‑ test_isFocusControl
…

This pull request skips 35 tests.

org.eclipse.swt.tests.junit.Test_org_eclipse_swt_dnd_ByteArrayTransfer ‑ test_nativeToJava
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_dnd_Clipboard ‑ test_AfterNewDisplay_RemoteGetContents
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_dnd_Clipboard ‑ test_Remote(int)[1]
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_dnd_Clipboard ‑ test_Remote(int)[2]
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_dnd_Clipboard ‑ test_getAvailableTypeNames(int)[1]
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_dnd_Clipboard ‑ test_getAvailableTypeNames(int)[2]
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_dnd_Clipboard ‑ test_getAvailableTypes(int)[1]
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_dnd_Clipboard ‑ test_getAvailableTypes(int)[2]
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_dnd_Clipboard ‑ test_getContents(int)[1]
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_dnd_Clipboard ‑ test_getContents(int)[2]
…

[vogella]

@HeikoKlare any additional feedback?

[HeikoKlare]

I am currently on vacation and will not be able to have another look before Thursday. If you are confident with the changes, don't wait for me and just move on. IIRC my feedback was just on not understanding certain changes. If you consider them correct, the changes are fine for me.

[vogella]

Thursday is not to far away, enjoy your vacation. I will be in forced absense Thursday and Friday so feel free to merge if you reviewed.

[HeikoKlare]

I am sorry but I have to admit that I am still not able to do a proper review of this without investing quite some effort. This PR still contains several changes at once. Copilot has also identified multiple categories of changes, which should probably be done by separate PRs:

How I understand the latest comments, the changes regarding drawBackground actually change behavior but in a non-noticable way. That should, in my opinion, be properly reflected in the commit / PR message. All the evaluations done in drawBackground(GC, boolean) are so difficult to understand that I cannot assess whether skipping them in so many cases a replaced in this PR is appropriate.

If you feel confident with the changes and fully understand those changes (if I am not mistaken, the answer to my questions was also AI generated?), feel free to move on. If you want to split this PR up, I can further assist with reviews.

[vogella]

I split this PR up into multiple smaller changes. I pretty confident that the change here is correct but giving this beautiful spagetti code, its good to have individual commits which can be reverted to reworked if necessary.

[vogella]

#3217 and #3218 are the first two split outs. Small and save I would say.

[vogella]

Converting this to draft, as we want to split the changes into smaller bits.

[HeikoKlare]

#3217 and #3218 are the first two split outs. Small and save I would say.

Agreed. I've merged them.

[vogella]

Closing this for now, this takes at the moment to much time from me, may investigate this later again.