Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ToolItem with SWT.CHECK is not well displayed when checked and with background setted. #542

Merged
merged 2 commits into from
Apr 25, 2023

Conversation

RoiSoleil
Copy link
Contributor

ToolItem with SWT.CHECK is not well displayed when checked and with background setted.

Fix : #541

@github-actions
Copy link
Contributor

github-actions bot commented Jan 30, 2023

Test Results

     303 files       303 suites   6m 18s ⏱️
  4 048 tests   4 034 ✔️   8 💤 5  1 🔥
12 071 runs  11 994 ✔️ 70 💤 6  1 🔥

For more details on these failures and errors, see this check.

Results for commit 148eac2.

♻️ This comment has been updated with latest results.

@RoiSoleil
Copy link
Contributor Author

Failing tests doesn't seems to be related to my PR.

@vogella
Copy link
Contributor

vogella commented Jan 30, 2023

@SyntevoAlex could you review?

@mickaelistria
Copy link
Contributor

@RoiSoleil in Eclipse project, we try to avoid merge commits. Please prefer rebasing as much as possible.

@RoiSoleil
Copy link
Contributor Author

@RoiSoleil in Eclipse project, we try to avoid merge commits. Please prefer rebasing as much as possible.

@mickaelistria it should be ok now.

@RoiSoleil
Copy link
Contributor Author

Can someone review this change for Windows ?

@HannesWell @iloveeclipse @niraj-modi

@deepika-u
Copy link
Contributor

deepika-u commented Mar 20, 2023

I am seeing 3 issues(when checked across multiple icons on the menu bar) as listed below :

Before the fix, i am seeing this way =>
swt check_before_fix

After the fix, i am seeing this way =>
swt check_after_fix

So the fix looks fine for me on dark theme on windows 11.

@RoiSoleil
Copy link
Contributor Author

@deepika-u Thx for the test. This is exactly what i wanted to fix.

@vogella
Copy link
Contributor

vogella commented Mar 20, 2023

@deepika-u Thx for the test. This is exactly what i wanted to fix.

The white background of the sync icon in the package explorer looks bad. It this intended?

@RoiSoleil
Copy link
Contributor Author

@vogella In Windows 10, i get this :

image

If in Windows 11, it's not good, i think it should be fixed in another specific issue ... (I'm not sure this is an actual issue)

@deepika-u
Copy link
Contributor

@deepika-u Thx for the test. This is exactly what i wanted to fix.

The white background of the sync icon in the package explorer looks bad. It this intended?

@vogella
To be more specific on your query i see a consistency across this icon and other icons after the fix. This looks to be an obvious behavior for me.

The complete left set of icons are when hovered(abit whitish comparitively).
The complete right set of icons are when they are selected/enabled actually(abit bluish comparitively).
Again both look abit different and also wanted to point out that difference too(in the orange circle - you can make out that difference when same icon is selected.)

For your reference =>
swt check_after_fix_more_similar_icons

Please let me know if that clarifies. Thanks.

@deepika-u
Copy link
Contributor

@vogella In Windows 10, i get this :

image

If in Windows 11, it's not good, i think it should be fixed in another specific issue ... (I'm not sure this is an actual issue)

I have no clue on this issue.

@RoiSoleil
Copy link
Contributor Author

@vogella In Windows 10, i get this :
image
If in Windows 11, it's not good, i think it should be fixed in another specific issue ... (I'm not sure this is an actual issue)

I have no clue on this issue.

It's Windows version specific (difference between Windows 10 and 11), It's another problem for me (I'm not sure this is an actual issue).

@RoiSoleil
Copy link
Contributor Author

@vogella Any news ?

Copy link
Contributor

@deepika-u deepika-u left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My vote goes to the fix.

@RoiSoleil
Copy link
Contributor Author

@vogella @iloveeclipse @niraj-modi Tested on my personal Eclipse with the fix for 2 months and everything is good. Could you take à look ?

@vogella
Copy link
Contributor

vogella commented Apr 14, 2023

@SyntevoAlex could you test?

@RoiSoleil
Copy link
Contributor Author

@SyntevoAlex could you test?

@SyntevoAlex @vogella any news ?

@merks
Copy link
Contributor

merks commented Apr 25, 2023

Pulling this into my workspace leaves a sea of errors and I'm unable to launch. I think this needs to be squashed and rebased on master because for master the SWT version is 3.124.0 and this PR reduces that number.

Probably easiest if you switch to master, make sure it's pulled to the latest, apply these two changes, and force push your branch....

@merks
Copy link
Contributor

merks commented Apr 25, 2023

Trying just the one SWT change locally, it does look better in dark theme with this change on Windows 10.

@lshanmug
Copy link
Member

@niraj-modi Can you please take a look and merge this one?

Copy link
Member

@niraj-modi niraj-modi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code change LGTM.
Thanks @deepika-u @merks @RoiSoleil for verifying this PR.

@niraj-modi niraj-modi merged commit fbd70cd into eclipse-platform:master Apr 25, 2023
@RoiSoleil
Copy link
Contributor Author

Nice !

@deepika-u
Copy link
Contributor

Verified this on below build and looks good to me.

Eclipse SDK
Version: 2023-06 (4.28)
Build id: I20230515-1800
OS: Windows 11, v.10.0, x86_64 / win32
Java vendor: Eclipse Adoptium
Java runtime version: 17.0.6+10
Java version: 17.0.6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ToolItem with SWT.CHECK is not well displayed when checked and with background setted.
7 participants