Bug 272997 - [Dialogs] Add getMessage and getErrorMessage to TitleAreaDialogAPI
Summary: [Dialogs] Add getMessage and getErrorMessage to TitleAreaDialogAPI
Status: VERIFIED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 3.5   Edit
Hardware: PC Windows XP
: P3 enhancement (vote)
Target Milestone: 3.6 M6   Edit
Assignee: Susan McCourt CLA
QA Contact: Susan McCourt CLA
URL:
Whiteboard:
Keywords: api
Depends on:
Blocks: 272794
  Show dependency tree
 
Reported: 2009-04-20 19:20 EDT by Kai Schlamp CLA
Modified: 2010-03-09 18:24 EST (History)
2 users (show)

See Also:


Attachments
Patch for TitleAreaDialog (6.72 KB, patch)
2009-04-20 19:25 EDT, Kai Schlamp CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kai Schlamp CLA 2009-04-20 19:20:12 EDT
Build ID:  I20090313-0100

Steps To Reproduce:
I suggest the following methods for a TitleAreaDialog:
setDialogComplete, getDialogComplete, setDescription, getDescription (and probably getMessage, getErrorMessage).
This would allow (when data binding is used) the TitleAreaDialog together with TitleAreaDialogSupport to behave the same way as a WizardPage with WizardPageSupport.
And this again provides a consistent GUI feel for the user.
Comment 1 Kai Schlamp CLA 2009-04-20 19:25:51 EDT
Created attachment 132523 [details]
Patch for TitleAreaDialog

This patch implements the above mentioned methods and allows the fix suggested in bug 272794 to work.
Comment 2 Matthew Hall CLA 2009-04-27 13:27:15 EDT
Gah, "select to remove" indeed.  Sorry Remy, and thanks Mylyn!  ;)
Comment 3 Matthew Hall CLA 2009-06-26 17:31:35 EDT
ping
Comment 4 Susan McCourt CLA 2009-06-29 15:17:36 EDT
I'm still triaging the backlog for 3.6, so am not assigning milestones to anything yet.  I'll assign this to myself and mark 3.6 for now...
Comment 5 Kai Schlamp CLA 2009-08-15 19:38:52 EDT
I just want to report that I am using this patch now for quite some time and it seems to work flawlessly. Any chance to have it in one of the next milestones?
Comment 6 Susan McCourt CLA 2009-08-17 14:17:18 EDT
I'll mark this one for M3
Comment 7 Susan McCourt CLA 2010-01-21 14:42:38 EST
It feels like a red flag to me to ask that WizardPage-like methods be introduced into TitleAreaDialog.

in my mind the mapping is
WizardDialog->TitleAreaDialog
WizardPage->DialogPage

In the dependent bug, you mentioned

> As I am using a lot of databinding lately, I also wanted to have the same
> support for my TiteAreaDialog as for the WizardPage.

Can you not use a DialogPage inside your TitleAreaDialog?  This would allow you to get the page description for free, and would be a bit more consistent, such that descriptions are provided at the page-level, not the dialog page level. 

I've used this technique when I have code that might live inside a wizard, a title area dialog, or even a preference page. 

To tie the page to the title area dialog, you need to do something like:

  protected Control createDialogArea(Composite parent) {
 	page = new MyDialogPage();
	page.createControl(parent);
        this.setMessage(page.getDescription())
        return page.getControl();
  }

It's even possible that data binding introduces a DialogPage subclass that handles whatever is needed here.  Thoughts?
Comment 8 Kai Schlamp CLA 2010-01-21 15:04:25 EST
Good point, but I still don't see how the DialogPage could emulate an WizardPage like description. TitleAreaDialog does not even support getting it's message field.
Comment 9 Kai Schlamp CLA 2010-01-21 15:16:38 EST
On the other side ... why not just give TitleAreaDialog (and therefore also WizardDialog) the same "description" feature as a page has?
It doesn't break anything and only makes things easier.

And I never read anywhere that by definition a dialog only can have messages and not descriptions ;-)
Comment 10 Susan McCourt CLA 2010-01-21 16:08:19 EST
(In reply to comment #8)
> Good point, but I still don't see how the DialogPage could emulate an
> WizardPage like description. TitleAreaDialog does not even support getting it's
> message field.

There is no specific support in TitleAreaDialog for pages, but it would work as demonstrated in my snippet above.  The dialog subclass would set its message to the dialog page's description.  I'm suggesting that this technique might be useful to solve your original problem, which I believe is that you want to treat wizards and title area dialogs similarly.  (I don't understand the specific data binding scenario, but I've had similar issues myself along these lines).

I think having a single-page title area dialog would give you the abstraction you want. 

(In reply to comment #9)
> On the other side ... why not just give TitleAreaDialog (and therefore also
> WizardDialog) the same "description" feature as a page has?
> It doesn't break anything and only makes things easier.

Are you suggesting that two descriptions would be shown at any one time (the dialog and the current page's description?), or that the page description would win over the dialog's description?  These are the kinds of things that would need to be worked out in a very specific API description, I don't think duplication necessarily makes things easier.

> 
> And I never read anywhere that by definition a dialog only can have messages
> and not descriptions ;-)
Comment 11 Kai Schlamp CLA 2010-01-22 06:20:22 EST
Yes, I absolutely see your point.
But I am not sure if it can be implemented without changing the TitleAreaDialog at all. The last time I tried, I came to the conclusion that it can't and rewrote the TitleAreaDialog for my application.
From a quick look I see that WizardDialog has methods and internal attributes a TitleAreaDialog does not have, like updateDescriptionMessage() and pageDescription.
I am sorry, I am very busy and can't look into it at the moment.
Can you come up with a runnable DialogPage/TitleAreaDialog example? I will try the binding then (with DialogPageSupport).
Comment 12 Susan McCourt CLA 2010-01-22 10:15:45 EST
(In reply to comment #11)
> Yes, I absolutely see your point.
> But I am not sure if it can be implemented without changing the TitleAreaDialog
> at all. The last time I tried, I came to the conclusion that it can't and
> rewrote the TitleAreaDialog for my application.
> From a quick look I see that WizardDialog has methods and internal attributes a
> TitleAreaDialog does not have, like updateDescriptionMessage() and
> pageDescription.

It's quite possible that there would need to be some changes.  

But what I was imagining was a subclass of TitleAreaDialog (SinglePageDialog?)...that could be tailored to your needs.  I'm not suggesting this goes into the platform, I'm suggesting that if you want to treat wizards and title area dialogs in the same way, using a paged dialog might be the way to go.  Maybe this belongs in data binding.  

> I am sorry, I am very busy and can't look into it at the moment.
> Can you come up with a runnable DialogPage/TitleAreaDialog example? I will try
> the binding then (with DialogPageSupport).

Sorry, equally busy, and I don't have the use case.  But really what I'm thinking of is not that much more interesting than the snippet I showed you in comment #7.  I sense you want more than that.  My suggestion is to try implementing what you need in terms of a single-page title area dialog (when you have time) and then attach a patch that demonstrates what is missing in TitleAreaDialog. I would hope that most of what is needed could be implemented in a subclass of TitleAreaDialog, but if there are method visiblity problems we could fix those.

I'm going to remove the milestone on this bug for now.
Comment 13 Kai Schlamp CLA 2010-01-22 10:36:07 EST
> But what I was imagining was a subclass of TitleAreaDialog
> (SinglePageDialog?)...that could be tailored to your needs.  I'm not suggesting
> this goes into the platform, I'm suggesting that if you want to treat wizards
> and title area dialogs in the same way, using a paged dialog might be the way
> to go.  Maybe this belongs in data binding.

In my opinion we should look for a solution that goes into the platform. For myself I have a solution. I use that patched TitleAreaDialog in my RCP application without any problem.
But data binding is a common task nowadays. And TitleAreaDialog is one of the most used custom dialogs of the platform.
So we should easily allow the user to add that feature to their plugins and RCP applications. At the moment the TitleAreaDialogSupport is simply broken.

Maybe we should support a TitleAreaDialogPage and allow to add or set it in the TitleAreaDialog. And then have a TitleAreaDialogPageSupport.
That would fulfill the mapping as you suggested in comment #7.
I do not think that another subclass of TitleAreaDialog just to have the data binding for people to work is any solution.
Comment 14 Kai Schlamp CLA 2010-01-22 10:37:11 EST
Matthew, what is your opinion on this?
Comment 15 Susan McCourt CLA 2010-01-22 11:04:06 EST
(In reply to comment #13)

> Maybe we should support a TitleAreaDialogPage and allow to add or set it in the
> TitleAreaDialog. And then have a TitleAreaDialogPageSupport.
> That would fulfill the mapping as you suggested in comment #7.
> I do not think that another subclass of TitleAreaDialog just to have the data
> binding for people to work is any solution.

That sounds reasonable, I just wouldn't want to force the use of a page on clients of TitleAreaDialog.
Comment 16 Kai Schlamp CLA 2010-01-22 13:53:31 EST
I gave that whole thing a second thought ... and came up as result with my first solution :-/ Here is why:
In my opinion TitleAreaDialog has implicitly one page. 
In comment #10 you wrote about duplication of the description attribute. I don't think that this is really true. The dialog is the view and the page is the content. So the page stores it's description and shows it by using the dialog when it is the current visible page. This could also be done by using a dialog.setDescription() method (and not just updating the message).
But even if you still think that this is duplication, how do you explain that both page and dialog already have a message and errorMessage attribute.
So it would just be fair for the dialog (TitleAreaDialog) to have the description attribute, too.
This is by far the most straight forward solution. It doesn't break anything and provides all needed features.
Comment 17 Kai Schlamp CLA 2010-02-13 08:21:16 EST
Susan, would it be possible to have two getters at least for message and error message (getMessage and getErrorMessage)?
That way the TitleAreaDialogSupport could emulate the description.
Comment 18 Matthew Hall CLA 2010-02-25 00:21:59 EST
Susan can we possibly have public getters for the message and errorMessage properties?
Comment 19 Susan McCourt CLA 2010-02-25 11:20:32 EST
will have to take another look later, marking M6 since it's API affecting.
Comment 20 Susan McCourt CLA 2010-03-02 22:15:45 EST
Hi, Kai and Matthew.
I don't see a technical problem with introducing getters here.  (ie, there's no confusion or funky timing issues about the values that make it confusing).  So the request seems reasonable.

Now, I put on my "stop API bloat" hat.  (I'm not trying to be difficult, at this point it would be much simpler for me to release the new API, but I don't feel good doing that without understanding the requirement.)

You want to be able to attach some data binding support code to any random TitleAreaDialog and have the data binding code get a result and set the message/error message accordingly.  You'd like to use the same code for WizardPageSupport and TitleAreaDialogSupport.

Both classes have set methods so setting the message is not your problem.
It has something to do with retreiving the message.  I looked briefly at DialogPageSupport and its subclasses and they don't seem to use the existing public getters, so I still don't understand the use case.  If this is some kind of "instrumentation" code where reflection or subclassing could be used instead, then I'm not sure adding API is appropriate.  If it seems like it's more general use, I'm not against adding those methods.
Comment 21 Matthew Hall CLA 2010-03-02 22:26:41 EST
The use case on question is that we want to remember any previously set dialog message so it can be restored when validation finally passes.
Comment 22 Susan McCourt CLA 2010-03-03 11:06:25 EST
(In reply to comment #21)
> The use case on question is that we want to remember any previously set dialog
> message so it can be restored when validation finally passes.

TitleAreaDialog keeps separate messages so that it can do this for you.   It remembers the message and restores it when the error message is set to null.  Is the issue that you have a different way of remembering the message (ie, other severities besides error that should replace the message?).  

If you like, we can talk about this on IRC (susanmccourt).  Maybe we can resolve it faster.
Comment 23 Matthew Hall CLA 2010-03-03 16:37:46 EST
DataBinding posts messages of all types, not just errors.  The use case is that the client manually sets an initial message, e.g. setMessage(IMessageProvider.INFO, "something informative"), and we want DataBinding to restore that manually set message when there is no DataBinding validation message.  DataBinding itself is using setMessage so calling it again with null clears the message rather than restoring the old value.
Comment 24 Susan McCourt CLA 2010-03-03 17:10:15 EST
thanks, Matt.  That's what I needed to understand.  Just trying to cut through the more general argument of "things should be the same."
Will put these in.
Comment 25 Susan McCourt CLA 2010-03-03 17:40:06 EST
fixed in HEAD >20100303.
Changing title of bug to reflect what happened.
Comment 26 Susan McCourt CLA 2010-03-03 17:40:35 EST
Comment on attachment 132523 [details]
Patch for TitleAreaDialog

patch marked obsolute - we went with a different approach
Comment 27 Susan McCourt CLA 2010-03-03 20:21:43 EST
.
Comment 28 Susan McCourt CLA 2010-03-09 18:24:28 EST
verified via source inspection.
win7, Build id: I20100309-0100