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

Improve vertical alignment of DialogDisplayer icon (question mark, warning symbol etc.) #5335

Closed
wants to merge 2 commits into from

Conversation

eirikbakke
Copy link
Contributor

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:

220930 Dialog Centering Screenshot

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.

@eirikbakke eirikbakke added Look and Feel Platform [ci] enable platform tests (platform/*) labels Jan 20, 2023
@eirikbakke eirikbakke requested a review from DevCharly January 20, 2023 22:26
@mbien
Copy link
Member

mbien commented Jan 21, 2023

@eirikbakke would you like to rebase this on delivery? I think this would be a good candidate for NB17 rc2.

@mbien mbien added this to the NB17 milestone Jan 21, 2023
@eirikbakke
Copy link
Contributor Author

Sure, let me see if I can figure out how to do that. Do I need to create a new PR?

@eirikbakke eirikbakke changed the base branch from master to release170 January 21, 2023 02:47
@mbien mbien changed the base branch from release170 to delivery January 21, 2023 02:52
@eirikbakke
Copy link
Contributor Author

OK, I think I figured it out...

@mbien
Copy link
Member

mbien commented Jan 21, 2023

Sure, let me see if I can figure out how to do that. Do I need to create a new PR?

@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 delivery since that is the right one :)

@eirikbakke
Copy link
Contributor Author

Oh, it's literally called "delivery". I see. Thank you! Will step aside now and not touch anything :-)

@mbien
Copy link
Member

mbien commented Jan 21, 2023

although this mitigates the issue, I am wondering if the actual problem is the vertical size of the window:

optionpane

(manually resized, this patch not included)

@eirikbakke
Copy link
Contributor Author

@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.

@mbien
Copy link
Member

mbien commented Jan 21, 2023

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:
https://dummyscodes.blogspot.com/2015/06/joptionpane-with-multiple-inputs.html
https://www.onlinetutorialspoint.com/java/java-swing-joptionpane-html-content-example.html
https://stackoverflow.com/questions/21059080/joptionpane-input-dialog-menu

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.

@eirikbakke
Copy link
Contributor Author

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.)

@mbien
Copy link
Member

mbien commented Jan 21, 2023

NbPresenter is already using pack(). The issue seems to be that JOptionPane sets the minimum height to 90. And looking at the code there is already a workaround added for Metal LAF.

if (UIManager.getLookAndFeel().getClass() == MetalLookAndFeel.class ||
UIManager.getLookAndFeel().getClass() == BasicLookAndFeel.class) {
optionPane.setUI(new javax.swing.plaf.basic.BasicOptionPaneUI() {
@Override
public Dimension getMinimumOptionPaneSize() {
if (minimumSize == null) {
//minimumSize = UIManager.getDimension("OptionPane.minimumSize");
// this is called before defaults initialized?!!!
return new Dimension(MinimumWidth, 50);
}
return new Dimension(minimumSize.width, 50);
}
});
}

which isn't active when FlatLAF is enabled.

@mbien
Copy link
Member

mbien commented Jan 21, 2023

here is an in-IDE example where the center alignment doesn't look very good in my opinion:
question

NB16:
question_baseline

(this window has it's height also incorrectly calculated)

NB16, manually adjusted:
fixed

@eirikbakke
Copy link
Contributor Author

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.

@mbien
Copy link
Member

mbien commented Jan 22, 2023

the minimum size influences how a single line message looks.
Min height of 90 for the JOptionPane is too much which makes it look like on your screenshots, while it should look like this. Since it is set to 50 on Metal and Basic LAF, I have the suspicion that someone tried to fix this before (but it reappeared with FlatLaf since the workaround is skipped there).

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.

@eirikbakke
Copy link
Contributor Author

The #5335 (comment) 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:

  1. If we don't do anything about the dialog size, then vertically centering the icons improves the appearance of the dialogs.
  2. If we do fix the dialog size, then the cases where vertically centering the icon looks bad go away.

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.

@mbien
Copy link
Member

mbien commented Jan 22, 2023

If we don't do anything about the dialog size, then vertically centering the icons improves the appearance of the dialogs.

to me it marginally improves the look of a certain type of dialog: one line messages
On the screenshot you provided, both before and after windows look to me as if someone placed labels on a frame without bothering to configure the layout or frame size. Both do look unfinished.

If we do fix the dialog size, then the cases where vertically centering the icon looks bad go away.

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:

thunder-pw

another example is the standard gnome keychain unlock dialog:

unlock

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.

@eirikbakke
Copy link
Contributor Author

eirikbakke commented Jan 22, 2023

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?

image

Mockup for the purpose of the example:

mockup

@neilcsmith-net
Copy link
Member

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.

I think rebasing is probably not even needed since the branches didn't diverge yet.

As clarification on that point, master diverges immediately on branch. Any PR that picked up the spec version change from master would require rebasing.

@neilcsmith-net neilcsmith-net changed the base branch from delivery to master January 23, 2023 09:54
@neilcsmith-net neilcsmith-net added the do not merge Don't merge this PR, it is not ready or just demonstration purposes. label Jan 23, 2023
@neilcsmith-net neilcsmith-net modified the milestones: NB17, NB18 Jan 23, 2023
@neilcsmith-net
Copy link
Member

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.

@eirikbakke eirikbakke force-pushed the pr-centerdialogicon branch 2 times, most recently from f9dc164 to 9f5fa5c Compare March 26, 2023 20:57
@eirikbakke
Copy link
Contributor Author

eirikbakke commented Mar 26, 2023

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:

newcentered

longer

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.
@eirikbakke eirikbakke force-pushed the pr-centerdialogicon branch from 9f5fa5c to 41546b5 Compare March 26, 2023 21:33
@eirikbakke eirikbakke changed the title Vertically center the icon (question mark or such) in DialogDisplayer dialogs Improve vertical alignment of DialogDisplayer icon (question mark, warning symbol etc.) Mar 26, 2023
@neilcsmith-net neilcsmith-net requested a review from mbien April 17, 2023 10:33
@mbien
Copy link
Member

mbien commented Apr 17, 2023

@eirikbakke is it ok to move this to NB 19?

@eirikbakke
Copy link
Contributor Author

@mbien Yeah, no problem!

@mbien mbien modified the milestones: NB18, NB19 Apr 17, 2023
@neilcsmith-net
Copy link
Member

neilcsmith-net commented Jul 6, 2023

@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.

@neilcsmith-net neilcsmith-net modified the milestones: NB19, NB20 Jul 17, 2023
@neilcsmith-net neilcsmith-net removed this from the NB20 milestone Oct 16, 2023
@eirikbakke
Copy link
Contributor Author

I think the "do not merge" label can be removed; this PR is ready to be committed except for a minor requested change.

@Chris2011
Copy link
Contributor

So reopening it?

@neilcsmith-net neilcsmith-net removed the do not merge Don't merge this PR, it is not ready or just demonstration purposes. label Oct 16, 2023
@eirikbakke
Copy link
Contributor Author

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...

@neilcsmith-net
Copy link
Member

Removed the do not merge as I realised that's something I added when moving things around with NB17.

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.

@neilcsmith-net
Copy link
Member

as switching/recompiling branches of the netbeans repo is a multi-hour operation on my Windows machine...

😮 what's going on there? It's a 7min operation on my machine. That might be something to look into?!

@mbien
Copy link
Member

mbien commented Oct 16, 2023

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)

@eirikbakke
Copy link
Contributor Author

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.

@eirikbakke
Copy link
Contributor Author

just my opinion but I am not convinced that the icon position needs to be adjusted.

I think this is clearly wrong:

Wrong

@mbien
Copy link
Member

mbien commented Oct 16, 2023

^ the dialog size is wrong I agree -> #5335 (comment)

(the layout does its job if the size is properly adjusted)

@eirikbakke
Copy link
Contributor Author

@mbien I think this was all discussed before, but if you prefer I can just leave this unmerged.

@mbien
Copy link
Member

mbien commented Oct 16, 2023

lets take a look at another dialog which displays a panel with sub-optimal layout management:
warning-before
->
warning-after
all I did was to quickly fix the content which used grid bag layout, the dialog is unchanged. It used empty panels for padding, couldn't properly calculate its default size and the gaps were not set for the purpose of it being included into a dialog etc

netbeans/ide/projectui/src/org/netbeans/modules/project/ui/problems/BrokenReferencesAlertPanel.java

actually... let me open a PR for this :) (edit: #6575)

@mbien
Copy link
Member

mbien commented Oct 17, 2023

draft for one-liners: #6577

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Look and Feel Platform [ci] enable platform tests (platform/*)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants