[win32] Add workaround for missing selection indicator for menu item with image on Win11 (#501) #2143
Conversation
with image on Win11 (eclipse-platform#501) For menu items of type CHECK and RADIO with an image, it create an second image with a checkmark/circle over the original image.
|
|
|
This problem is the only reason why I have not upgraded by system to Windows 11. Can you show a screen capture of how it looks with and without your changes? I'm not sure I can really test/see that on Windows 10. This problem has festered for years and I don't think Windows 11 itself will ever make the problem go away. It would be great for this to be reviewed to make any necessary changes or improvements. |
|
Oh, I think this is the change:
|
|
What should happen is that the menu item image is highlighted by a surrounding square (blue in light theme). Wasn't this possible as POC in #501? |
The should happen is based on how it looked before, which was quite (very) nice. In fact kind of nicer visually than this proposal and more familiar. But, at this point 6 months later, I'm willing to say what should happen is something, maybe even anything reasonable, because that is better than nothing at all which we have currently and have had for far too long. I really hope to see movement here... |
Yes, I think that approach is prettier. 😍 I just don't know if it's harder to implement and maybe harder to match the native styling. I don't know much, except that I'm willing to compromise at this point... |
|
Thank you for this proposal! At first glance, the change looks quite straightforward and should hopefully be of low risk. Still the question is if we want to process this for the upcoming release given that we are short before M3 (and at least I will not have time to have a more thorough look into this until Thursday). |
I scanned that very long issue and I'm not sure exactly where the POC code actually exists. So while I like visually what I see now on Windows 10, if no one can spend the effort to emulate that (which seems tricky with respect to theme adjustments) then I like that this PR provides a working solution that we can actually commit. In my opinion, we need to move forward one way or the other before M1. So if someone has a better way and is willing to create a PR for it, that would be a good thing. |
Fully agree. From UX perspective the solution looks quite fine and I don't expect that anyone proposes a better one soon. |
No code but @wahlbrink proposed possible solution "(A) Mimic Win 10 / Add blue box (as proposed)" here - is that possible or a no go? |
|
I think anything that changes the size needed to render the decoration is disruptive and will likely lead to visual inconsistencies. |
There was a problem hiding this comment.
Thank you for this concrete proposal on addressing the long-standing issue with missing selection indicators in menu items!
The implementation seems to work fine. I have tested it also with different monitor zooms and it adapts as expected. Also the different configuration options work as designed. I did not find any functional issues within the code as well. So regarding all that, the PR is fine to me.
I am just a bit concerned that the implementation is scattered across the MenuItem implementation. This makes it difficult to understand even in the review and will become even more difficult when reading the code fragment without the context of a PR.
I don't want to bother with rewriting the adaptation, but in my opinion encapsulating all the custom selection image functionality at a single place might significantly improve the quality. It might be an (inner) Selection class (or the like) encapsulating:
- the toggle state for which kind of customization to use based on the system propery
- the initialization logic for the custom selected image
- the custom selected image (currently the
imageSelectedfield), which can be initialized on demand - the selected bitmap handle (currently the
hBitmapSelectedfield), which can also be initialized on demand - the dispose functionality for all data related to the selection image (to be called when setting a new image on the menu item or releasing the menu item widget)
- the functionality to fill the menu item info (called in
updateImage()andsetSelection())
This would serve two means:
- Locality: the functionality would be encapsulated at a single place so it can be understood, maintained and, if necessary, removed at that single place.
- Comprehensibility: as a side effect of putting all partial functionalities into methods of that encapsulating class, the purpose of the single code blocks becomes more clear (just like when extracting the code blocks into methods with self-explaining names).
But to point it out again: even without such a design refinement the change is fine and it will definitely improve UX compared to the given state by showing selected states again anyway.
| int y0 = imageHeight / 2 - 8; | ||
| if (((style & SWT.CHECK) != 0)) { | ||
| int[] points = new int[] { x0 + 4, y0 + 10, x0 + 6, y0 + 12, x0 + 12, y0 + 6 }; | ||
| gc.setAntialias(SWT.ON); |
There was a problem hiding this comment.
nit: alias is set in if and else clause, so it could be moved outside
| gc.drawImage(image, imageWidth - imageBounds.width, (imageHeight - imageBounds.height) / 2); | ||
| int x0 = imageWidth - 16; | ||
| int y0 = imageHeight / 2 - 8; | ||
| if (((style & SWT.CHECK) != 0)) { |
There was a problem hiding this comment.
nit: to make the method a bit simpler and code a bit more comprehensive, you might extract the if and else blocks into methods drawCheck and drawRadio or the like
For menu items of type CHECK and RADIO with an image, it create an second image with a checkmark/circle over the original image.