Community
Participate
Working Groups
I just found the jface.dialogs.InputDialog class as an easy to use value requestor. This is just a minor improvement suggestion. Most dialogs and wizards show error messages with a leading red-X error icon. The InputDialog does not. I think, the validator's error message would be easier to spot for a user if it also has a red-X error icon.
Consider as a post 2.0 improvement
Reopen to investigate
marking for 3.3. I hope to clean up the various inconsistent presentations of error messages in the different dialog hierarchies.
*** Bug 57169 has been marked as a duplicate of this bug. ***
removing milestone...the idea was to provide a common image/message implementation to be used in multiple places, but we missed the API window for this release due to other higher prio items.
It would be nice to have a common status message area implementation for our dialogs. See also bug #148085
As per http://wiki.eclipse.org/Platform_UI/Bug_Triage_Change_2009
Created attachment 144289 [details] InputStatusDialog I'd like to suggest to consider an implementation of input dialog based on StatusDialog which already provides somewhat richer decorations namely error/warning/info icons. Here is possible implementation of such dialog InputStatusDialog. InputDialog and StatusDialog are not exactly compatible as far as interface hence a new class. I think this class is orthogonal to common status message area implementation and would use the common piece if ever developed.
Thanks for the patch. Assigning to myself to take a look at this early in 3.6.
(In reply to comment #8) > Created an attachment (id=144289) [details] > InputStatusDialog Hi Andrew, thanks for your contribution. Here are some comments after trying out your patch. 1. You've spec'd IInputStatusValidator to allow null as a return value but an NPE will actually be thrown by InputStatusDialog if you do so. I presume you spec'd null as an allowable return value because you were following the pattern set by IInputValidator. It's not clear to me as to whether having two different return values mean the same thing is a good idea or not. Perhaps Susan can comment on this when she makes her pass over this bug. java.lang.NullPointerException at org.eclipse.jface.dialogs.StatusDialog.updateButtonsEnableState(StatusDialog.java:185) at org.eclipse.jface.dialogs.StatusDialog.updateStatus(StatusDialog.java:162) at org.eclipse.jface.dialogs.InputStatusDialog.validateInput(InputStatusDialog.java:193) at org.eclipse.jface.dialogs.InputStatusDialog$1.modifyText(InputStatusDialog.java:139) 2. Specifying null for the 'dialogMessage' parameter in the constructor causes a runtime exception. Exception in thread "main" java.lang.IllegalArgumentException: Argument cannot be null at org.eclipse.swt.SWT.error(SWT.java:3868) at org.eclipse.swt.SWT.error(SWT.java:3802) at org.eclipse.swt.SWT.error(SWT.java:3773) at org.eclipse.swt.widgets.Widget.error(Widget.java:452) at org.eclipse.swt.widgets.Label.setText(Label.java:574) at org.eclipse.jface.dialogs.InputStatusDialog.createDialogArea(InputStatusDialog.java:128)
Created attachment 146500 [details] patch 2 (In reply to comment #10) Thanks for the scrutiny, any code intended for Platform of course deserves that. > 1. You've spec'd IInputStatusValidator to allow null as a return value but an > NPE will actually be thrown by InputStatusDialog if you do so. I presume you > spec'd null as an allowable return value because you were following the pattern > set by IInputValidator. It's not clear to me as to whether having two different > return values mean the same thing is a good idea or not. Perhaps Susan can > comment on this when she makes her pass over this bug. You are absolutely right that I followed copy/paste pattern here trying to be as much consistent with IInputValidator as I could. Looking at this class more closely I note that - while there is a distinction in IInputValidator.isValid() for the null vs empty string return value - there is no distinction in IInputStatusValidator. It seems that the reason would be just convenience. Perhaps misguided. Null is supposed to mean an absence of an object, in this case IStatus. The semantics of absence of IStatus is really not clear and may be confusing. So I suggest to change the specs not to allow null - considering that returning Status.OK_STATUS just as easy. However if somebody comes up with a good reason to use null as a return value I would not object. The change would be trivial. > 2. Specifying null for the 'dialogMessage' parameter in the constructor causes > a runtime exception. Thanks again for catching that. In the updated patch I corrected the problem using empty string as a default value. In this case the semantics of dialogMessage being absent is pretty clear.
I'm looking at this one now for M3. (There's something exciting about fixing such a low bug number!) I agree with you both that it doesn't make sense to allow a null return value from IInputStatusValidator.isValid(String text). In fact, I think the method name is misleading. We should probably just call it IStatus validate(String text) since the method is not returning a boolean and hence there is no "is" anything. What do you think? Did you per chance have any test cases that went along with this dialog? I will write some tomorrow if not.
(In reply to comment #12) > I'm looking at this one now for M3. (There's something exciting about fixing > such a low bug number!) Yes, there is! This submission does not resolve InputDialog problem though. > I agree with you both that it doesn't make sense to allow a null return value > from IInputStatusValidator.isValid(String text). > In fact, I think the method name is misleading. We should probably just call > it IStatus validate(String text) > since the method is not returning a boolean and hence there is no "is" > anything. What do you think? I think you are right and I like that name better. > Did you per chance have any test cases that went along with this dialog? I > will write some tomorrow if not. No. I am not sure how you write test cases like that and could use a good example.
Created attachment 150326 [details] current patch with tests Here's what I've got right now based on our discussion. Plus some test cases. Also, I opted to handle a null message value the same way that InputDialog does. Rather than make the value an empty string, I check for null before creating the label. Otherwise you'll get extra empty space in the input dialog when there is no message. However, I'm not sure I want to release a new class. The other approach would be to move InputDialog underneath StatusDialog, provide a second constructor that takes an IInputStatusValidator, and map the IInputValidator to an internal IInputStatusValidator. This would give existing InputDialog clients a way to get the error icon for free. But I have to think about what that means according to our compatibility rules. (Does this break binary compatibility?) For that reason i want to move this to M4 (this is my last day to work on M4 stuff) and pick this up again.
> For that reason i want to move this to M4 (this is my last day to work on M4 > stuff) and pick this up again. I meant to say this is my last day to work on M3.
Need to look at http://java.sun.com/docs/books/jls/second_edition/html/binaryComp.doc.html section 13.4.4 when my brain is not already full.
Hi, Andrew. We discussed this issue briefly in a UI status call. - We don't want to introduce this behavior "for free" in InputDialog because it introduces new visuals for products that may not want to adopt them. So there's no gain in putting binary compatibility at risk by changing the structure of something that's so stable/old. - Adding a new dialog, then, to JFace is of marginal value unless there is significant need/use in the SDK itself, or unless we see a bunch of copies of this dialog floating around the release train components. I realize there's a chicken/egg problem here. Adoption won't happen if the dialog is not there, but we would like to see some momentum from SDK components who wish to adopt this dialog before adding it. So I'm removing the milestone for now. Thanks for sharing the work you did.
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant.
