Community
Participate
Working Groups
Created attachment 132324 [details] TitleAreaDialog Patch Build ID: I20090313-0100 Steps To Reproduce: As I am using a lot of databinding lately, I also wanted to have the same support for my TiteAreaDialog as for the WizardPage. For a WizardPage there is WizardPageSupport to automatically disable the Ok button and set the error/information message when data binding brings these things up. TitleAreaDialog does miss this support. There are some things why WizardPageSupport can not be so easily transferred to a TitleAreaDialogSupport. 1. The TitleAreaDialog misses the methods setDialogComplete and isDialogComplete. 2. TitleAreaDialog doesn't have a setDescription (it only has setMessage and setErrorMessage). But to get the above mentioned support to work, there must be also description. When the support sets message to null, the original description should show up. This is the quite bigger problem (than 1.) as this can't be implemented by extending the current TitleAreaDialog and adding a few lines. That is cause there is no getMessage and getErrorMessage for the TitleAreaDialog. Why are those missing? WizardPage has them! I feel somehow limited. So my suggestion, please support an TitleAreaDialogSupport for data binding and rework TitleAreaDialog by adding getDialogComplete, setDialogComplete, setDescription, getDescription, getErrorMessage, getMessage, getMessageType methods (this would NOT break any API). If you don't see any value for that, then please at least implement getErrorMessage, getMessage and getMessageType for TitleAreaDialog, so that people that like the data binding support for TitleAreaDialog can knock it together fast and easily by extending the current TitleAreaDialog. I also created a enhancement request and added the code of a revised TitleAreaDialog and a new TitleAreaDialogSupport. More information:
Created attachment 132326 [details] TitleAreaDialogSupport
Created attachment 132327 [details] TitleAreaDialog Patch Forgot to remove a little debug message. Removed it now.
I think there is already a TitleAreaDialogSupport per bug 239900?
I looked at Bug 239900 and also checked the TitleAreaDialogSupport in Eclipse 3.5M6, but it has some flaws (I will put a snippet up that demonstrates this). When an errorMessage shows up (triggered by an Error ValidationStatus) and is then removed again, then the original message doesn't show up anymore (the message under the title simply gets empty). This is cause TitleAreaDialog (in contrast to WizardPage) doesn't have a setDescription, that is shown when message == null and errorMessage == null. It would also be beneficial if the TitleAreaDialog would behave similiar to a WizardPage by disabling the ok button, when there is an error message. Both in my opinion can only be solved by redesigning the TitleAreaDialog a bit (see the TitleAreaDialog patch). As I already said, this won't break any API or harm any programs using the old TitleAreaDialog.
Created attachment 132339 [details] TitleAreaDialogSupport Test Snippet A snippet that shows the flaws of the original TitleAreaDialogSupport from 3.5M6 (resp. Bug 239900).
Created attachment 132407 [details] Snippet that shows another problem with the existing TitleAreaDialogSupport from 3.5M6 I tested a bit my implemented TitleAreaDialog + TitleAreaDialogSupport and came up with a strange problem I wasn't able to solve yet. I created a snippet to demonstrate it. If you click just the "Cancel" button of the dialog of that snippet then a NPE occurs. I tried to track it down with the debugger, but I really don't understand if fully. It only occurs when the following conditions are met. 1. The dialog is "watched" by the TitleAreaDialogSupport. 2. The message is first set with setMessage (line 66). The problem does not occur when setDescription is used! 3. The exception only occurs when using a TabFolder inside the dialog (yep, sounds strange, I know ;-)) I would be very happy if someone (perhaps with a bit more knowledge of SWT and JFace internals) can lend me a hand here as I am a bit stuck right now.
Moving bug to DataBinding.
I just tested the snippet that demonstrates the NPE problem when having an TabFolder with the original TitleAreaDialog + TitleAreaDialogSupport from 3.5M6 and it also shows up there. So it doesn't have to do with my implementation. The problem exists in the already released code of TitleAreaDialogSupport (resp. the TitleAreaDialog itself). But still not able to track it down :-(
Created attachment 132520 [details] Fixes all issues. I finally found a solution for that NPE. The problem seems that there was something like a thread race condition. Some things were already disposed, but the AggregateValidationStatus still induced a layout calculation. I wonder a bit why the AggregateValidationStatus (or better to say the ValueChangeListener) is called at all when closing a dialog?! But that's another story. This patch fixes the problems of the NPE, the problem of the empty title message (see Comment #4) and adds the feature that the Ok button is automatically disabled when there is an error validation status.
Thanks for the patch Kai, I will try to review it tonight.
I took a quick glance and noticed that your patch adds new public methods to TitleAreaDialog. Unfortunately we are well past the API freeze for the Galileo release so this will have to wait until the next development cycle. This is an unfortunate oversight, it's too bad this wasn't noticed earlier. Please ping immediately after the Galileo release and I'll try to get this pushed into 3.6 M1. Boris, Kai's patch affects both org.eclipse.jface.databinding and org.eclipse.jface. Since you are the only committer on both projects, do you want to take QA on this one.
Oops, I didn't see you had created a separate bug for the TitleAreaDialog changes. I think we can fix the dispose error, let's split that out into a separate bug though.
Hy Matthew. Are you sure the new "Summary" represents the bug correctly? 1. I would suggest isDialogComplete for the TitleAreaDialog. There is no page in a TitleAreaDialog. 2. Also the fix for this bug resolves the problem with the null message. When you run the "TitleAreaDialogSupport Test Snippet" (https://bugs.eclipse.org/bugs/attachment.cgi?id=132339) with the current TitleAreaDialogSupport from HEAD, you see that automatically the message of the dialog is cleared when you enter anything in the text field. Should we split this also into a seperate bug? But this can also be only solved in an appropriate way by changing the TitleAreaDialog API (adding setDescription).
(In reply to comment #13) > 1. I would suggest isDialogComplete for the TitleAreaDialog. There is no page > in a TitleAreaDialog. I've corrected the summary. > 2. Also the fix for this bug resolves the problem with the null message. When > you run the "TitleAreaDialogSupport Test Snippet" > (https://bugs.eclipse.org/bugs/attachment.cgi?id=132339) with the current > TitleAreaDialogSupport from HEAD, you see that automatically the message of the > dialog is cleared when you enter anything in the text field. I'm happy to entertain an intermediate solution within TitleAreaDialogSupport > Should we split this also into a seperate bug? But this can also be only solved > in an appropriate way by changing the TitleAreaDialog API (adding > setDescription). I was referring to the exception you noted when the dialog is disposed. This problem is distinct from the problem of clearing the dialog message during validation, and it could be fixed in time for the 3.5M7 milestone. Hence my suggestion to break off that piece into a separate bug.
(In reply to comment #14) > I'm happy to entertain an intermediate solution within TitleAreaDialogSupport Accidentally submitted before I could finish my thought. I'm happy to entertain an intermediate solution as long as no new API is introduced in TitleAreaDialogSupport. My current idea is that whenever we set a message on TitleAreaDialog we should check whether the current message was set by TitleAreaDialogSupport, and if not we save it to a field such that when the status returns to IStatus.OK we restore the saved message.
(In reply to comment #15) > My current idea is that whenever we set > a message on TitleAreaDialog we should check whether the current message was > set by TitleAreaDialogSupport Hmm, TitleAreaDialog does not have a method to query the current message--so this may not work.
Exactly, that was also my problem (the missing getters). Otherwise the TitleAreaDialog could also be extended to support all those new features. A workaround for now could be to to store the starting message in the TitleAreaDialogSupport and then restore that when no error and no message comes from the databinding. This is rather ugly, but the only way to solve this problem in the meantime.
But the above suggestion would also change the API (that of TitleAreaDialogSupport). Then I don't see any solution :-( But using the fix for myself and making a big entry in my schedule for late June to bring this up again.
(In reply to comment #18) > But the above suggestion would also change the API (that of > TitleAreaDialogSupport). Then I don't see any solution :-( Yes, I think we are stuck with this until the 3.6 development cycle opens.
I opened another bug for the NPE problem in the current CVS Head of TitleAreaDialogSupport. See here: https://bugs.eclipse.org/bugs/show_bug.cgi?id=275058
*** Bug 306866 has been marked as a duplicate of this bug. ***
Removed from 3.6. Owners can re-assess. PW
Just blogged about this problem: http://tomsondev.bestsolution.at/2011/01/08/enhanced-rcp-usage-of-titlearedialogsupport/
Since a lot of things happened since this bug was initially created, let me ask a few details about the default message to restore (I try to already respond to them but feel free to correct me :) ): 1. What exactly should be considered the default message? The message which is set when the TitleAreaDialgoSupport is created or should we even try to track changes in the default message when the user changes it manually? => I think that we should only consider the initial message our "default message" without tracking later changes. As an alternative, we might even introduce a new API method TitleAreaDialogSupport#setDefaultMessage(String) to make things clearer and more explicit. 2. When exactly should the default message be set? => Most probably when IValidationMessageProvider#getMessage() returns null (to make it consistent with the behavior of a WizardPage and it's description). @Tom: Somehow, I think that your solution for setting a default message by implementing your own IValidationMessageProvider is also a valid approach or do you feel that this is hacky (the part with the enablement of the button is more obviously a workaround :o) )?
Jens, please have a look if that one is still relevant.
(In reply to Lars Vogel from comment #25) > Jens, please have a look if that one is still relevant. I have very little time right now, and I keep starting more tasks than I am able to finish. But I'll add this to my list of things that would be nice to do in the future.
