-
Notifications
You must be signed in to change notification settings - Fork 874
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
Improve vertical alignment of DialogDisplayer icon (question mark, warning symbol etc.) #5335
Conversation
@eirikbakke would you like to rebase this on delivery? I think this would be a good candidate for NB17 rc2. |
49f090e
to
87b511e
Compare
Sure, let me see if I can figure out how to do that. Do I need to create a new PR? |
87b511e
to
f94a8b6
Compare
OK, I think I figured it out... |
@eirikbakke no its fine. I think rebasing is probably not even needed since the branches didn't diverge yet. But it doesn't hurt. i changed target to |
Oh, it's literally called "delivery". I see. Thank you! Will step aside now and not touch anything :-) |
@mbien Well, if the text is center-aligned vertically, then so should the icon in any case. And there are cases where dialogs can contain a lot more text. |
I don't have a strong opinion on this, its just that I believe that it is intended for the icon to be in the NW edge in larger dialogs. found some screenshots with larger dialogs: If there is more content in the window, it doesn't look misaligned to me and if there is only one line it can be centered by packing the window. In this particular case the window is simply too big, even after the icon is centered to my eyes. |
I'd be nervous about changing the size or using pack(). With pack(), you often get an awkward size when there are components with Swing HTML in them, JScrollPanes, text wrapping or such. Vertically centering the icon seems like a simple and safe way to make things look OK in all cases. (Even in the screenshots you shared the top-aligned icon looks mis-aligned to me; it would need more top margin. Very hard to get this right for all dialogs.) |
netbeans/platform/core.windows/src/org/netbeans/core/windows/services/NbPresenter.java Lines 511 to 524 in 31028d2
which isn't active when FlatLAF is enabled. |
Hmm, that "Working copy" example looks a lot taller than 90 pixels (the image is 376 pixels tall). What's going on there? If we resize the dialog, though, the centered icon will still look better. If you really want it on top, there would need to be some top margin inserted. |
the minimum size influences how a single line message looks. The last screenshots were an example to demonstrate that centering the icon doesn't look good (to me) in dialogs which more content in them. |
If the dialog is properly sized, then it would look good, no? This means:
In other words, vertically centering the icon is a good thing to do in any case. Resizing the dialog is a separate issue with more potential to break things. |
to me it marginally improves the look of a certain type of dialog: one line messages
It makes the situation worse for already existing dialogs which display more content. Fixing the size there would still have the icon centered which looks non-standard. This is of course just my opinion. However, I do believe that it is fairly common to anchor an icon to the NW corner in dialogs. I can see the same layout in the master pw window of thunderbird or firefox for example: another example is the standard gnome keychain unlock dialog: Since this UI pattern works for both, small dialogs and big dialogs, it probably became popular and also made it into JDK's dialog utilities. I don't see it as a bug, the bug appears to be the incorrect sizing of the content inserted right from the icon which causes this weird behavior since the window can't pack correctly. A 1 line label isn't 90 pixels tall, yet it is what its preferred size is for some reason and someone tried to fix this before. |
OK, this requires a different approach, then. No hurry to get this PR into NetBeans 17. To continue the discussion, in your example below, I'd say that at the very least the top of the message text should align with the top of the icon (i.e. pushing the icon further down than in this screenshot). Agree/disagree? Mockup for the purpose of the example: |
That Thunderbird mockup looks worse than the original in my opinion. I think I agree with @mbien here that vertically centring the icon is not the answer - fixing sizing probably is. I don't think this is a good fit for the release given ongoing discussion. Putting this back to master with do-not-merge on pending resolution of discussion and testing across LAFs.
As clarification on that point, master diverges immediately on branch. Any PR that picked up the spec version change from master would require rebasing. |
Waking this discussion up! We're a few weeks from freeze for NB18 now. We should make a decision if there is anything to address here, and if so, how. |
f9dc164
to
9f5fa5c
Compare
I pushed a new adjustment to this PR, which adds some top margin to the icon instead of vertically centering it. The specific margin height is chosen so that the icon ends up appearing centered next to single-line messages while appearing (almost) top-aligned next to taller messages: With @mbien's adjustments from #5625 , the minimum height of the dialog also ends up a little smaller (note: only the top screenshot above includes the latter adjustment; I'm not sure where in NetBeans I can easily trigger a taller dialog for testing), and single-line messages end up looking more vertically centered relative to the title bar and buttons than before. (Edited:) After trying various values for the minimum height, I ended up preferring a value of 50, which is what was previously in place for MetalLookAndFeel. This adjustment is now in place for FlatLAF as well. |
Instead of vertically centering the icon, add some margin above the icon, so that it appears centered next to single-line messages and (almost) top-aligned next to taller content. The minimum height adjustment from MetalLookAndFeel is now also applied on FlatLAF.
9f5fa5c
to
41546b5
Compare
@eirikbakke is it ok to move this to NB 19? |
@mbien Yeah, no problem! |
@mbien @eirikbakke are we at the "is it OK to move this to NB 20" point now? 😄 Be good to decide whether this is going in rather than pushing back milestones. Did try and leave two comments on specific lines, which now seem to have disappeared from the UI?! Be good to see if the LaF specific logic can be removed (I wonder why the comment on minimum size from the LaF is not working), and feels like the boolean flag should be configurable or removed. |
I think the "do not merge" label can be removed; this PR is ready to be committed except for a minor requested change. |
So reopening it? |
Yes please, sorry--it takes me a while to get back to PRs, as switching/recompiling branches of the netbeans repo is a multi-hour operation on my Windows machine... |
Removed the If @eirikbakke @DevCharly and/or @mbien want to reopen and re-review, then it can be reopened. I'm just closing a few things that have bumped along from milestone to milestone without getting any further towards resolution. |
😮 what's going on there? It's a 7min operation on my machine. That might be something to look into?! |
just my opinion but I am not convinced that the icon position needs to be adjusted. If you run a picture search for "error dialog", or take a look at the examples I posted somewhere above, you will see that it is fairly common that the icon is anchored in the NW corner. NB has many layout bugs though, some dialogs might be too large and need to be adjusted so that the components don't float around, some might contain hidden components which break the layout manager (see screenshot of #6355). (and yes building NB takes about 7,5 mins on my PC I built ~2016 -> there must be something wrong) |
It's WSL 1 on Windows, on a 2018 laptop, with the NetBeans repo on a Windows file system (WSL 1 is actually faster than WSL 2 in this case). "git checkout" takes several minutes, "git status" takes 18 seconds, and a clean build will hog up the machine long enough that I usually break for dinner... I'll be switching to a new MacBook soon, though. Having a native UNIX terminal, no Windows Defender, and new hardware will make everything much faster. |
^ the dialog size is wrong I agree -> #5335 (comment) (the layout does its job if the size is properly adjusted) |
@mbien I think this was all discussed before, but if you prefer I can just leave this unmerged. |
lets take a look at another dialog which displays a panel with sub-optimal layout management:
actually... let me open a PR for this :) (edit: #6575) |
draft for one-liners: #6577 |
In DialogDisplayer dialogs, there's typically an "information", "question mark" or "warning" type icon next to the text. It shows up top-aligned by default, which makes it look mis-aligned relative to the message text. This PR centers the icon vertically. See below:
As the dialog is constructed by JOptionPane, the tweakIconLabelVerticalAlignment method applies the fix by descending into the child components and finding the one conveniently named "OptionPane.iconLabel".
This is purely a cosmetic UI improvement.