Community
Participate
Working Groups
Created attachment 136244 [details] Extensions to WizardDialog to support optional callbacks on WizardPages Since early 2005, we've had an internal subclass of org.eclipse.jface.wizard.WizardDialog which adds support for optional callbacks for the WizardPages on the nextPressed() and backPressed() occurring in the WizardDialog. I had a good dig around the code back then looking for how this is best done, and ended up writing the attached classes. If there is now a better way, please just close this bug - if not, I thought I might contribute this change (or at least the concept) back. :-) This is really useful in cases where you use one pages data inputted by the user as input data for following pages. For example, the first page might ask for customer details, after pressing the next button the callback might call a web service, go to a database, etc. to get the details for that customer, which are then displayed on the following page. This is pretty much the use case that we use it for. We've got an XML-RPC based architecture, and large portions of our application depend on our callback style wizards. I'll attach two files: 1. ZenkaiWizardDialog.java is the extensions to WizardDialog 2. ZenkaiRpcCallable.java is the callback interface that WizardPage's optionally implement. In the case where they don't implement ZenkaiRpcCallable, then the functionality for that WizardPage is unchanged. I'd suggest taking the ideas from ZenkaiWizardDialog and incorporating them into the standard WizardDialog, and having an similar interface to ZenkaiRpcCallable as well. Again, let me know if this has already been done - I couldn't see anything in SWT/JFace that did this in 2005 when we implemented this.
Created attachment 136245 [details] interface that WizardPage's optionally implement to receive callbacks
Does the following API not suffice? org.eclipse.jface.wizard.WizardDialog.addPageChangingListener(IPageChangingListener) org.eclipse.jface.wizard.WizardDialog.addPageChangedListener(IPageChangedListener)
I wasn't aware of that API. Looking at it, it could almost be made to do the same thing, but in comparison addPageChangingListener() and addPageChangedListener() are quite clumsy. Specifically, 1. It does add extra code to each time you create a Wizard: foreach (page) addPageChangingListener() addPageChangedListener() whereas ours detects a supporting page and deals with it transparently. Ideally, the callbacks in the interface would be abstract methods in the base WizardPage, but it's a bit anti-social to force people to implement methods they don't need, and would also break backwards compatibility in all existing code. So implementing an optional interface is the best way out from here of providing both the backwards compatibility for existing apps, and implementing the functionality that always should have been there at the same time. 2. The listeners get called for every page every time in both the changing and changed case, regardless of whether it's a page that is being changed to or from, or if it's actually an innocent third party page in the same Wizard. Hence, you need to add yet more code to every WizardPage which should be in a framework to say "am I really the page that should have got this event?". It's very rare that you'd care that another page got an event, unless it's an event that causes the given page itself to get context. If there is a case for needing that extra event, that's when the extra flexibility of the generic listener would come in handy. We certainly haven't come across a case where that would be useful, and IMO it would be bad design anyway, as it breaks the separation contract between pages, and makes them less reusable as discrete components of other, unrelated wizards (which we do regularly). 3. Telling the difference between a back button and a next button press would be much more painful, if possible. From what I can tell, the WizardPage would need to getCurrentPage(), then getTargetPage(), and then have to know the ordering of the pages (which is the responsibility of the Wizard, not the WizardPage). So it breaks the separation contract quite badly. 4. Our error handling is more elegant. Simply returning false (in the case of doTransaction()) or a String error message (in the case of doPrepatoryTransaction()) is much better, as the WizardDialog can deal with what to do in the case of an error transparently, without repeat coding in the WizardPages. Having thought about this some more, I think both should really return a string in both cases (rather than a boolean) and leave error display to the host WizardDialog to do. I can see somebody has had a good go at solving this, and has used standard event handlers, which I guess are more flexible, as it means you can have a third party part of the application monitoring what is happening in a Wizard. So the two ways could co-exist, and I don't think they really represent a double implementation - the PageChang[ing|ed]Listener is a generic listener; ours is about sending the most common events to the specific page that they affect in the most obvious possible way. We currently have 62 wizard pages in our application which use the attached framework. I'd be really happy if an equivalent was implemented in the base JFace, and would happily port our code over to it and off our custom API, as I've been burnt in the past extending Eclipse APIs which have later been declared "final" - it adds supportability, and removes a risk that has burnt us before. But the addPageChangingListener() / addPageChangedListener() is long winded, more error prone and makes WizardPages do things that Wizards / WizardDialogs should deal with. It just isn't as good for the 99% of cases which don't require the full flexibility of the Listener based approach, and does so with a much cleaner API interface. We did make some mistakes: 1. I hate the naming of doTransaction() / doPrepatoryTransaction() - they really should have been doForward() / prepareForwardLoading() or something like that. 2. doTransaction() should return a String with the error message in the way doPrepatoryTransaction() does - it's much cleaner, and leaves error handling display to the framework, meaning less repeated code. Currently, we set the error message in the WizardPage in the case of doTransaction(), but the WizardDialog does it in doPrepatoryTransaction(). 3. There are four cases that need covering: (a) Next button pressed on previous page, (b) Next button pressed on this page, (c) Back button pressed on this page, (d) Back button pressed on following page. We only currently do the first three; we've never had a need for (d), however as a reference API, it wouldn't be complete without it. 4. doTransaction() doesn't get called in the case of the Finish button being pressed, because Finish fires performFinish(), not doNext(). This is inconsistent, and I'm not sure whether we should solve this or not. We work around this at the moment by manually calling "if (!lastPage.doTransaction()) return false;" as the first line in our performFinish() implementations, which is less than ideal. Really, the framework should call doTransaction() on the last page before calling performFinish() on the hosting Wizard. I'm happy to hear suggestions on the best way to solve this one. If you want me to spend the time writing a patch, I'm happy to do it. I wanted to present the concept first before spending the time actually integrating what we have done. If a committer says "provide a patch and if it's good enough quality, I'll merge it" then I'll do the work to write the patch.
Thank you for the detailed explanation. I agree that the current wizard framework does not make implementing wizards easy enough when each page needs to do some processing based on the inputs entered so far. This is going to require some thought, and we should invite people with relevant background. Tonny, who else should be on this bug?
Prakash is now responsible for watching bugs in the [Wizards] component area.
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.
