eclipse.platform.swt icon indicating copy to clipboard operation
eclipse.platform.swt copied to clipboard
verified publisher icon eclipse-platform

selection background drawn with the line spacing for multiline selection

Open tobiasmelcher opened this issue 1 year ago • 6 comments

Line selection background is drawn without the line spacing value in the StyledText control. For a multiline selection, a zebra effect occurs which looks questionable from my point of view. In this change, the line spacing height is included when drawing the selection background.

Before with line spacing value 60%: multiline_selection_line_spacing_before

After with line spacing value 60%: multiline_selection_line_spacing_after

tobiasmelcher avatar Jul 08 '24 16:07 tobiasmelcher

Test Results

   486 files  ±0     486 suites  ±0   7m 52s :stopwatch: -33s  4 151 tests ±0   4 143 :white_check_mark: ±0   8 :zzz: ±0  0 :x: ±0  16 358 runs  ±0  16 266 :white_check_mark: ±0  92 :zzz: ±0  0 :x: ±0 

Results for commit d97c17a8. ± Comparison against base commit 2374dcf1.

:recycle: This comment has been updated with latest results.

github-actions[bot] avatar Jul 08 '24 16:07 github-actions[bot]

contributes to https://github.com/eclipse-platform/eclipse.platform.swt/issues/1286

BeckerWdf avatar Jul 09 '24 07:07 BeckerWdf

if nobody objects I will merge this today EoB.

BeckerWdf avatar Jul 12 '24 09:07 BeckerWdf

@sratz: Do you have more insights into that? Do you see problems / side effects with this change?

BeckerWdf avatar Jul 12 '24 09:07 BeckerWdf

@sratz: Do you have more insights into that? Do you see problems / side effects with this change?

This change looks good.

By not extending the selection of the last line to the bottom, this avoids many other minor visual issues (e.g. if there is also an other highlight applied on the last line, such as a marked occurrence or a search result).

However: This change only fixes it for Win32. What about macOS and Linux? Can the same fix be applied there as well?

sratz avatar Aug 07 '24 08:08 sratz

the latest patch request contains implementations for windows, linux and mac.

Windows: windows

Linux: linux

Mac: mac

tobiasmelcher avatar Aug 07 '24 15:08 tobiasmelcher

I missed to merge this change for the M3 deadline :(

Any chance this could go in for RC1?

The coding is rather safe, especially in the default 'line spacing 0' case, in which no behavior is changed at all.

sratz avatar Aug 21 '24 12:08 sratz

@sratz

It is fixing a bug and I trust in your good judgement, so I will defer the decision to you with a +1 from me (the PMC).

merks avatar Aug 21 '24 13:08 merks

@sratz

It is fixing a bug and I trust in your good judgement, so I will defer the decision to you with a +1 from me (the PMC).

Thanks!

sratz avatar Aug 21 '24 14:08 sratz