Skip to content

Redesign Message Dialog#2934

Open
SougandhS wants to merge 1 commit intoeclipse-platform:masterfrom
SougandhS:SaveBoxAlignment
Open

Redesign Message Dialog#2934
SougandhS wants to merge 1 commit intoeclipse-platform:masterfrom
SougandhS:SaveBoxAlignment

Conversation

@SougandhS
Copy link
Copy Markdown
Contributor

@SougandhS SougandhS commented Apr 23, 2025

Removes unwanted space and aligns message content & buttons to center

Before
image

After
Screenshot 2025-04-22 at 1 02 49 PM

Fixes : #2929

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 23, 2025

Test Results

 1 824 files  ±0   1 824 suites  ±0   1h 30m 39s ⏱️ - 7m 15s
 7 918 tests ±0   7 690 ✅ +1  228 💤 ±0  0 ❌  - 1 
23 841 runs  ±0  23 093 ✅ +1  748 💤 ±0  0 ❌  - 1 

Results for commit cd6224c. ± Comparison against base commit e6eb975.

@mickaelistria
Copy link
Copy Markdown
Contributor

mickaelistria commented Apr 23, 2025

As the discussion on the bug reveals, there is no obvious consensus to find between aligned left vs centered; however, all OSs recommend to get the width fit the content (ie no extra space), so I think for a first iteration, we should focus on just saving the extra space (changing the min width to 0 or DEFAULT), and consider whether to center or not later; maybe if all OSs align on the same recommendation some day.

@SougandhS
Copy link
Copy Markdown
Contributor Author

SougandhS commented Apr 24, 2025 •
edited
Loading

Updated one

image

@mickaelistria
Copy link
Copy Markdown
Contributor

mickaelistria commented Apr 25, 2025 •
edited
Loading

I'm seeing an issue with the given patch: the MessageDialog now tends to become too large:

Before
Screenshot From 2025-04-25 13-18-19

After
Screenshot From 2025-04-25 13-16-31

@SougandhS
Copy link
Copy Markdown
Contributor Author

SougandhS commented Apr 26, 2025

I'm seeing an issue with the given patch: the MessageDialog now tends to become too large:

Made minor adjustment for these cases

merks
merks requested changes Apr 26, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@merks merks left a comment

Choose a reason for hiding this comment

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

I think changing the horizontalSpan is a bad idea. I'm not even sure why it's needed.

Comment thread bundles/org.eclipse.jface/src/org/eclipse/jface/dialogs/MessageDialog.java
layout.marginWidth = 0;
composite.setLayout(layout);
GridData data = new GridData(GridData.FILL_BOTH);
data.horizontalSpan = 2;
Copy link
Copy Markdown
Contributor

@merks merks Apr 26, 2025

Choose a reason for hiding this comment

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

I thought there was previously already feedback about this change affecting all subclasses. There are a great many so this seems not a good idea at all.

image

Copy link
Copy Markdown
Contributor Author

@SougandhS SougandhS Apr 28, 2025

Choose a reason for hiding this comment

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

Sorry, I had only configured the platform UI code in my workspace, which caused me to overlook the impact on the subclasses. I've now reverted the change and adjusted the value to 1 to remove the extra space.

Copy link
Copy Markdown
Contributor

@mickaelistria mickaelistria Apr 28, 2025

Choose a reason for hiding this comment

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

See
Screenshot From 2025-04-28 11-03-54
about a regression caused by changing the span.

Comment thread bundles/org.eclipse.jface/src/org/eclipse/jface/dialogs/IconAndMessageDialog.java Outdated
if (message != null) {
messageLabel = new Label(composite, getMessageLabelStyle());
messageLabel.setText(message);
int minWidthHint = convertHorizontalDLUsToPixels(IDialogConstants.MINIMUM_MESSAGE_AREA_WIDTH - 50);
Copy link
Copy Markdown
Contributor

@merks merks Apr 26, 2025

Choose a reason for hiding this comment

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

It's odd to compute this in advance and then only use it conditionally later.

Copy link
Copy Markdown
Contributor Author

@SougandhS SougandhS Apr 28, 2025

Choose a reason for hiding this comment

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

Fixed now

Comment thread bundles/org.eclipse.jface/src/org/eclipse/jface/dialogs/IconAndMessageDialog.java Outdated
convertHorizontalDLUsToPixels(IDialogConstants.MINIMUM_MESSAGE_AREA_WIDTH),
SWT.DEFAULT).applyTo(messageLabel);
.indent(5, 0)
.hint(message.length() > 40 ? minWidthHint : SWT.DEFAULT, SWT.DEFAULT)
Copy link
Copy Markdown
Contributor

@merks merks Apr 26, 2025

Choose a reason for hiding this comment

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

The number 40 is quite magic. I wonder how things like font size and scaling might affect what this number should be?

Copy link
Copy Markdown
Contributor Author

@SougandhS SougandhS Apr 28, 2025

Choose a reason for hiding this comment

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

I tested the implementation with few different font sizes and found the results to be satisfactory, which is why I proceeded with this approach.

@SougandhS SougandhS force-pushed the SaveBoxAlignment branch 2 times, most recently from 4c05420 to 4045e5d Compare April 28, 2025 04:15
@BeckerWdf
Copy link
Copy Markdown
Member

BeckerWdf commented Apr 28, 2025

I asked our inhouse UX-Experts. This is what they say:

We would not recommend the centered alignment of dialog content in Eclipse – even for short texts like "Save ‘NAME.filetype‘?" – for several reasons:

  • This dialog is displayed for only a few seconds and does not obscure any relevant content. Removing the empty line between the text and the buttons, as well as centering the text, worsens readability – we do not see any advantages in this approach.
  • Left-aligned dialogs with right-aligned buttons are common desktop practices (e.g., Windows, macOS). Centered dialogs are more typical in mobile UIs where space is limited. On the desktop, space shouldn't be an issue.
  • Prompts would wrap to two lines in the centered dialog quite easily. The current "airy issue" with short names could potentially turn into a "squeezed issue" with long names.

@SougandhS
Copy link
Copy Markdown
Contributor Author

SougandhS commented Apr 28, 2025

I asked our inhouse UX-Experts. This is what they say:

Thanks for checking ✨

@akurtakov
Copy link
Copy Markdown
Member

akurtakov commented Apr 29, 2025

@SougandhS So are you going to continue with some other ideas here or are you giving up on the idea (and the PR should be closed)?

@SougandhS
Copy link
Copy Markdown
Contributor Author

SougandhS commented Apr 29, 2025

@SougandhS So are you going to continue with some other ideas here or are you giving up on the idea (and the PR should be closed)?

I have a small idea.. [ Not sure everyone agrees :) ]
For small texts can we reduce the width a bit atleast ?
image

@SougandhS
Copy link
Copy Markdown
Contributor Author

SougandhS commented Apr 29, 2025 •
edited
Loading

I have a small idea.. [ Not sure everyone agrees :) ] For small texts can we reduce the width a bit atleast ?

image image

@BeckerWdf
Copy link
Copy Markdown
Member

BeckerWdf commented Apr 29, 2025

I have a small idea.. [ Not sure everyone agrees :) ] For small texts can we reduce the width a bit atleast ?

image image

Where's the difference?

@SougandhS
Copy link
Copy Markdown
Contributor Author

SougandhS commented Apr 29, 2025

Where's the difference?

Before

image

after

image

@BeckerWdf
Copy link
Copy Markdown
Member

BeckerWdf commented Apr 29, 2025

and how does this change look for other dialogs?

@SougandhS
Copy link
Copy Markdown
Contributor Author

SougandhS commented Apr 29, 2025

and how does this change look for other dialogs?

Yes did checked one
Before
image


After

image

@SougandhS
Make message dialog more compact
21f7226 
Removed unnecessary spacing in the message dialog to improve visual
alignment and create a more compact, cleaner layout.

Fixes : eclipse-platform#2929
@mickaelistria
Copy link
Copy Markdown
Contributor

mickaelistria commented Apr 29, 2025

I don't see a clear benefit with this change. IMO, the desired format is #2934 (comment) and anything else is not worth being pursued.
I don't know the details about why #2934 (comment) cannot be implemented reliably. Is it because changing the horizontal span is required? Aren't there alternative that can be found? eg is the 2 columns layout used because of image? If so, then couldn't we dynamically decide of the span or layout according to whether image is null or not?

@SougandhS
Copy link
Copy Markdown
Contributor Author

SougandhS commented Apr 29, 2025

eg is the 2 columns layout used because of image? If so, then couldn't we dynamically decide of the span or layout according to whether image is null or not?

I tried modifying IconAndMessageDialog.getColumnCount() to return 1, but the extra space remained regardless of Image. It only disappeared after I also updated data.horizontalSpan = 1 in MessageDialog.createDialogArea(Composite).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mickaelistria mickaelistria mickaelistria left review comments

@merks merks merks requested changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Center text/save space on message dialogs?

5 participants

@SougandhS @mickaelistria @BeckerWdf @akurtakov @merks