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

Fix Color of Toolbar Item on Dark Background

Open BeckerWdf opened this issue 1 year ago • 8 comments

Fixes: #1233

BeckerWdf avatar May 15 '24 15:05 BeckerWdf

This PR fixes the issue on macOS.

Before this change: old_dark old_light

With this change: new_dark new_light

BeckerWdf avatar May 15 '24 15:05 BeckerWdf

Test Results

   418 files  ±0     418 suites  ±0   8m 25s :stopwatch: -23s  4 121 tests ±0   4 113 :white_check_mark: ±0   8 :zzz: ±0  0 :x: ±0  16 313 runs  ±0  16 221 :white_check_mark: ±0  92 :zzz: ±0  0 :x: ±0 

Results for commit 33a96430. ± Comparison against base commit 567a8b40.

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

github-actions[bot] avatar May 15 '24 15:05 github-actions[bot]

For Windows I didn't find a corresponding implementation. Maybe there this triangle is drawn by the native widget library. I don't know.

BeckerWdf avatar May 15 '24 15:05 BeckerWdf

@BeckerWdf thank you for the fix for Mac.

I haven't looked at the code but I see that this PR doesn't really fix #1233 but only contributes to it (it only fixes the problem in 1 platform).

Please change the commit text and the description of this PR.

Sure. It's only macOS. At least it's a start. @fedejeanne: Wanna help with a fix on Windows or Linux?

BeckerWdf avatar May 16 '24 06:05 BeckerWdf

I tested this in macOS Sonoma, and the PR looks good visually. I haven't reviewed the code changes, but from a visual perspective, the change looks good.

elsazac avatar May 16 '24 08:05 elsazac

@niraj-modi: Can you provide help with windows?

BeckerWdf avatar May 16 '24 08:05 BeckerWdf

@niraj-modi: Can you provide help with windows?

@deepika-u Please evaluate for Windows.

niraj-modi avatar May 16 '24 09:05 niraj-modi

@niraj-modi: Can you provide help with windows?

@deepika-u Please evaluate for Windows.

Let me check on it.

deepika-u avatar May 16 '24 11:05 deepika-u

deepika-u

@deepika-u: Any news on this topic?

BeckerWdf avatar Jun 06 '24 14:06 BeckerWdf

@BeckerWdf thank you for the fix for Mac.

I haven't looked at the code but I see that this PR doesn't really fix #1233 but only contributes to it (it only fixes the problem in 1 platform).

Please change the commit text and the description of this PR.

@fedejeanne Just did that. Pls. re-review and approve if ok for you.

BeckerWdf avatar Jun 07 '24 06:06 BeckerWdf

I tested this in macOS Sonoma, and the PR looks good visually. I haven't reviewed the code changes, but from a visual perspective, the change looks good.

@elsazac: Can you pls. review the code?

BeckerWdf avatar Jun 07 '24 06:06 BeckerWdf

@BeckerWdf thank you for the fix for Mac. I haven't looked at the code but I see that this PR doesn't really fix #1233 but only contributes to it (it only fixes the problem in 1 platform). Please change the commit text and the description of this PR.

@fedejeanne Just did that. Pls. re-review and approve if ok for you.

@BeckerWdf thank you for changing the commit text, I just changed the description of this PR for you :-)

fedejeanne avatar Jun 07 '24 07:06 fedejeanne

I just changed the description of this PR for you :-)

Thanks a lot. I overlooked this.

BeckerWdf avatar Jun 07 '24 07:06 BeckerWdf

@elsazac: Can you pls. review the code?

For me, the code changes looks ok, I have tried some snippets as well from Toolbars and the changes looks good on them;

but should we get an opinion from @lshanmug ?

elsazac avatar Jun 07 '24 09:06 elsazac

but should we get an opinion from @lshanmug ?

Sure why not.

BeckerWdf avatar Jun 07 '24 11:06 BeckerWdf

@deepika-u: Any news on this topic?

Hello, i was occupied with another issue so i havent looked into this issue. Will check on it.

deepika-u avatar Jun 10 '24 07:06 deepika-u