Community
Participate
Working Groups
MessageDialog.open could IMHO benefit from a fluent API, like the one we created for SWT. This API should allow to: set the icon (optional) set the title set the message set the buttons text
Marcus, something for you as our new master of fluent API?
(In reply to Lars Vogel from comment #1) > Marcus, something for you as our new master of fluent API? Maybe... But I'm currently busy with other things. Hope I can manage to look into it within the next weeks.
While I started to work on a fix I recognized that there are a lot of static methods in MessageDialog to create dialogs of any kind. So my question is, does it really add value if we add yet another possibility to create MessageDialogs? W.r.t.to clean code it is worth to do it since the mentioned methods have a lot of parameters (four or more) and have int parameters e.g. for the message kind. A fluent api would bring benefits here. But on the other hand it may confuse implementors to have another method?! To make it more clear: I'm thinking for something like this: MessageDialog.information().title("Do you really want another method?").message("Click ok if you want to have yet another method for MessageDialogs.").open(theShell); WDYT?
(In reply to Marcus Höpfner from comment #3) > While I started to work on a fix I recognized that there are a lot of static > methods in MessageDialog to create dialogs of any kind. > > So my question is, does it really add value if we add yet another > possibility to create MessageDialogs? > > WDYT? +1 for adding another type for MessageDialog fluent API, like "MessageDialogs"
The static methods do not allow to specify the text of the buttons and are therefore useless for real usage. Also we want dialogs without icons, the usage of the help icon for question dialogs is wrong according to Microsoft UX guidelines.
ok, thanks for clarification. So I continue.
(In reply to Marcus Höpfner from comment #3) > MessageDialog.information().title("Do you really want another > method?").message("Click ok if you want to have yet another method for > MessageDialogs.").open(theShell); I'm currently facing problems in testing this. I thought I call the above code in a unit test, do the assertions for the created controls (buttons, title, description) and close the dialog. Any idea how I can do this? How can I recognize that the dialog has opened? I tried a ShellListener on the original shell but this was not called.
(In reply to Marcus Höpfner from comment #7) > I'm currently facing problems in testing this. > I thought I call the above code in a unit test, do the assertions for the > created controls (buttons, title, description) and close the dialog. > Any idea how I can do this? How can I recognize that the dialog has opened? > I tried a ShellListener on the original shell but this was not called. Indeed not an easy task. Will be even more difficult since the open method kind of block the main thread. The best I came up spontaneous is the following snippet. It will open a message dialog and check if the dialog is actual opened. ------------------------- public void testMessageDialog() { Display display = Display.getDefault(); Shell shell = new Shell(display); AtomicBoolean found = new AtomicBoolean(false); String title = "Title"; display.asyncExec(() -> { for (Shell s : display.getShells()) { if (title.equals(s.getText())) { found.set(true); } } display.dispose(); }); MessageDialog.open(MessageDialog.CONFIRM, shell, title, "Message", 0); if (!display.isDisposed()) { display.readAndDispatch(); } assertTrue(found.get()); } -------------------------
New Gerrit change created: https://git.eclipse.org/r/155044
Regarding Lars' comment in the Gerrit commit to have a simpler builder api in order to have the possibility to just open a dialog maybe even without an image... For this, the API of MessageDialog must also be changed. It has a bunch static methods, two huge constructors with many parameters. Adding new API methods to satisfy the new requirements would make it worth. I have the impression that the static methods have been introduced to overcome the constructors and to have an easier API. Now, as it is not sufficient anymore we add a factory or a builder (how ever you call it) on top of that in a separate type. For me, this somehow feels wrong. It will result in a mixture of patterns and there is no clean API anymore. In some cases the static methods are ok, in others the Builder/Factory is needed. My proposal is to start from scratch with a new MessageDialog with the same capabilities as the old one but with one constructor, just setter methods and an open methods. No overloaded static methods or huge constructors. Then, as an easier to consume API have a builder/factory in a separate type to have an easier API. But adding another public methods to MessageDialog to satisfy the Builder requirements feels wrong and makes MessageDialog worse IMHO. WDYT?
(In reply to Marcus Höpfner from comment #10) > Regarding Lars' comment in the Gerrit commit to have a simpler builder api > in order to have the possibility to just open a dialog maybe even without an > image... > > For this, the API of MessageDialog must also be changed. It has a bunch > static methods, two huge constructors with many parameters. > Adding new API methods to satisfy the new requirements would make it worth. > I have the impression that the static methods have been introduced to > overcome the constructors and to have an easier API. Now, as it is not > sufficient anymore we add a factory or a builder (how ever you call it) on > top of that in a separate type. > For me, this somehow feels wrong. It will result in a mixture of patterns > and there is no clean API anymore. In some cases the static methods are ok, > in others the Builder/Factory is needed. > > My proposal is to start from scratch with a new MessageDialog with the same > capabilities as the old one but with one constructor, just setter methods > and an open methods. No overloaded static methods or huge constructors. > Then, as an easier to consume API have a builder/factory in a separate type > to have an easier API. > > But adding another public methods to MessageDialog to satisfy the Builder > requirements feels wrong and makes MessageDialog worse IMHO. > > WDYT? +1
New Gerrit change created: https://git.eclipse.org/r/156156
I started to change the https://git.eclipse.org/r/#/c/156156/ locally and to incorporate the feedback. If I remove all the constants for the kind which determine the button texts and the image, what is left then? Just a plain message dialog with a title, message, buttons and icon. But nothing is predefined, it is completely free-style. And I guess will be used like free-style, so in every UI the dialog may look different. Imagine an error dialog with error image and button "OK". Another one with error image and buttons "OK", "Cancel" (which is obviously wrong). I would like to clarify how many guidance do we want to provide and how many free-style?
(In reply to Marcus Höpfner from comment #13) > > If I remove all the constants for the kind which determine the button texts > and the image, what is left then? > Several simple final dialog classes that encapsulates this information. Something is wrong with idea to expose internals (text, image). What is the case when we need these kind constants?
(In reply to Alexander Fedorov from comment #14) > (In reply to Marcus Höpfner from comment #13) > > > > If I remove all the constants for the kind which determine the button texts > > and the image, what is left then? > > > > Several simple final dialog classes that encapsulates this information. > Something is wrong with idea to expose internals (text, image). What is meant by expose? Where do we expose it? It should be just setters to set these information. > What is the case when we need these kind constants? E.g. if you want to create an error dialog with an error image and a button "Ok". Or a question dialog with a question mark icon and "Yes" and "No" buttons.
(In reply to Marcus Höpfner from comment #15) > (In reply to Alexander Fedorov from comment #14) > > (In reply to Marcus Höpfner from comment #13) > > > > > > If I remove all the constants for the kind which determine the button texts > > > and the image, what is left then? > > > > > > > Several simple final dialog classes that encapsulates this information. > > > Something is wrong with idea to expose internals (text, image). > What is meant by expose? Where do we expose it? It should be just setters to > set these information. If we are talking about good OOP - getters/setters are evil, because they degrades the idea of object to a "struct of data, that everyone can change from everywhere. > > > What is the case when we need these kind constants? > E.g. if you want to create an error dialog with an error image and a button > "Ok". I would expect several constructors with good defaults 'new ErrorDialog(title, message)` 'new ErrorDialog(title, message, buttonLabel)' > Or a question dialog with a question mark icon and "Yes" and "No" buttons. 'new QuestionDialog(title, question)` 'new QuestionDialog(title, question, <other arguments to configure buttons>` If the continue our design with respect to OOP we can come up with some additional object to configure each button: public final class Choice { private final String label; private final String tooltip; private final SomethingFunctional action; }
Marcus and I had a short email conversation. I think this new API should: - Introduce a new dialog - Allow to create a dialog without icons - Allow to create error / question / etc dialogs via a build method for examle type(SWT.Error) - The usage / adaption of this API should be handled via a new bug
I have uploaded a new patch set in https://git.eclipse.org/r/c/156156/. Kindly let me know if this is the direction we wanna go. Open issues: - should the dialog be final? - should it still support SWT.SHEET? - in the builder SWT.ERROR is used but there is nor SWT.INFO neither SWT.WARNING
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/170943
Mass move 4.19 M1 bugs to M3
Too late for 4.19. Remove target milestone. Please set one when you start working on it.
Gerrit change https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/170943 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=4ccc686b15a8190380e29cb62ef16307e9295c52
Thanks, Marcus for this contribution. Could you add this new API to the N&N?
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/182104
Gerrit change https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/182104 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=dc6641214e62e59f1862bf9a03c450d9168f181b
(In reply to Eclipse Genie from comment #22) > Gerrit change > https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/170943 was merged > to [master]. > Commit: > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/ > ?id=4ccc686b15a8190380e29cb62ef16307e9295c52 This introduces compilation error in the IDE. Javadoc: The method of(Object[]) is undefined for the type List PlainMessageDialog.java /org.eclipse.jface/src/org/eclipse/jface/dialogs line 137 Java Problem
New Gerrit change created: https://git.eclipse.org/r/c/www.eclipse.org/eclipse/news/+/182163
(In reply to Andrey Loskutov from comment #26) > This introduces compilation error in the IDE. > > Javadoc: The method of(Object[]) is undefined for the type List > PlainMessageDialog.java /org.eclipse.jface/src/org/eclipse/jface/dialogs > line 137 Java Problem Marcus, can you have a look?
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/182174
(In reply to Lars Vogel from comment #28) > (In reply to Andrey Loskutov from comment #26) > > This introduces compilation error in the IDE. > > > > Javadoc: The method of(Object[]) is undefined for the type List > > PlainMessageDialog.java /org.eclipse.jface/src/org/eclipse/jface/dialogs > > line 137 Java Problem > > Marcus, can you have a look? Ok, I fixed it in a new commit. Pls. review and merge. I find it somehow bad that I did not get a build error for my first commit. Isn't that something Jenkins should find and prevent the merge?
Gerrit change https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/182174 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=aeb53a9ce52ee4e3b283c86a8de3e2b90316463e
Thanks Marcus for fixing and Andrey for finding. Marcus, I think we could change the project settings to error for Javadoc errors, see https://bugs.eclipse.org/bugs/show_bug.cgi?id=570430
Gerrit change https://git.eclipse.org/r/c/www.eclipse.org/eclipse/news/+/182163 was merged to [master]. Commit: http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/?id=2737f1c87946f887ad6c334a2fb73df3e72197a2
Windows tests are failing, see https://download.eclipse.org/eclipse/downloads/drops4/I20210811-1800/testresults/html/org.eclipse.jface.tests_ep421I-unit-win32-java11_win32.win32.x86_64_11.html createsDialogWithButtons Failure expected:<[No]> but was:<[Yes]> org.junit.ComparisonFailure: expected:<[No]> but was:<[Yes]> at org.junit.Assert.assertEquals(Assert.java:117) at org.junit.Assert.assertEquals(Assert.java:146) at org.eclipse.jface.tests.dialogs.PlainMessageDialogTest.createsDialogWithButtons(PlainMessageDialogTest.java:63) createsDialogWithDefaultButton Failure expected:<[Cancel]> but was:<[No]> org.junit.ComparisonFailure: expected:<[Cancel]> but was:<[No]> at org.junit.Assert.assertEquals(Assert.java:117) at org.junit.Assert.assertEquals(Assert.java:146) at org.eclipse.jface.tests.dialogs.PlainMessageDialogTest.createsDialogWithDefaultButton(PlainMessageDialogTest.java:79)
(In reply to Andrey Loskutov from comment #34) > Windows tests are failing @Marcus, Lars: please investigate & provide fixes.
(In reply to Andrey Loskutov from comment #35) > (In reply to Andrey Loskutov from comment #34) > > Windows tests are failing > > @Marcus, Lars: please investigate & provide fixes. I try to look at that tomorrow morning.
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/184117
Gerrit change https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/184117 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=6c22e76788bcdced94fea1de8cf0a6937fbf9124
Verified that the tests pass in the windows build https://download.eclipse.org/eclipse/downloads/drops4/I20210818-0600/testResults.php
(In reply to Andrey Loskutov from comment #34) > Windows tests are failing, see > https://download.eclipse.org/eclipse/downloads/drops4/I20210811-1800/ > testresults/html/org.eclipse.jface.tests_ep421I-unit-win32-java11_win32. > win32.x86_64_11.html > > createsDialogWithButtons Failure expected:<[No]> but was:<[Yes]> > > org.junit.ComparisonFailure: expected:<[No]> but was:<[Yes]> > at org.junit.Assert.assertEquals(Assert.java:117) > at org.junit.Assert.assertEquals(Assert.java:146) > at > org.eclipse.jface.tests.dialogs.PlainMessageDialogTest. > createsDialogWithButtons(PlainMessageDialogTest.java:63) > > createsDialogWithDefaultButton Failure expected:<[Cancel]> but was:<[No]> > > org.junit.ComparisonFailure: expected:<[Cancel]> but was:<[No]> > at org.junit.Assert.assertEquals(Assert.java:117) > at org.junit.Assert.assertEquals(Assert.java:146) > at > org.eclipse.jface.tests.dialogs.PlainMessageDialogTest. > createsDialogWithDefaultButton(PlainMessageDialogTest.java:79) The tests pass in https://download.eclipse.org/eclipse/downloads/drops4/I20210818-0600/testresults/html/org.eclipse.jface.tests_ep421I-unit-win32-java11_win32.win32.x86_64_11.html
