GNOME Bugzilla – Bug 115348
can we port the gnome druid (assistant/wizard like widget) to gtk?
Last modified: 2011-02-04 16:10:23 UTC
Currently there is no way to do a wizard type function with the widgets available unless you use a GtkNoteBook using buttons to show and hide tabs as Next and Previous buttons are clicked. Gnome already has a druid which does this, can we not port it to GTK, or create a new widget for this task in GTK+?
Created attachment 19323 [details] gtkassistant.c
Created attachment 19324 [details] gtkassistant.h
Created attachment 19325 [details] testassistant.c
Created attachment 19326 [details] [review] glue
I've just attached a first attempt at such a port. For a discussion, see http://mail.gnome.org/archives/gtk-devel-list/2003-August/msg00105.html
with linux trying to be more user friendly this looks like a must have. Not planned for gtk 2.6? :(
added the official term "Assistant" into the summary to make this easier to find. (I was checking to to see if this would be in the next version of Glade but if it hasn't made it Gtk+ yet then obviously not)
Hi, I'd like to propose the assistant implementation I've done, it has some features I think they're worth to consider, the implementation is the following: EggAssistantPage ================ Inherits from GtkBin, it's just a container which provides title, header pixmap and watermark, but is not responsible of drawing them. Also provides the "prepared" signal. EggAssistant ============ Inherits from GtkDialog, and already provides the buttons and all the frame for the contained EggAssistantPages. It's responsible of drawing pages' title, header or watermark. For drawing the top and side colors it just inverses the GTK_STATE_NORMAL and GTK_STATE_SELECTED colors, thus adapting to theme changes. It uses egg_assistant_set_forward_page_func() and egg_assistant_set_back_page_func() for handling page switches instead of "next" and "back" signals. By default, the forward function moves to the next page, and back function returns to the previously shown page, independently of the forward function (maintains a list of visited pages), so unless people want to make very crazy things, this is a very sane default. In general terms, The API to handle pages is quite similar to the GtkNotebook one. issues left =========== - color drawing doesn't work with pixmap themes (but this isn't specific to this implementation) - inverted color still doesn't play fine with unsensitive assistants - maybe it could be worth adding a method in EggAssistant for handling watermark alignment, like in Matthias' implementation I'd thank any comment on this implementation :)
Created attachment 49059 [details] eggassistant.c
Created attachment 49060 [details] eggassistant.h
Created attachment 49061 [details] eggassistantpage.c
Created attachment 49062 [details] eggassistantpage.h
Created attachment 49063 [details] sample test
Hmm, after looking over the api and implementation, I have to admit that I still prefer my older proposal, which gets rid of the separate page objects, and does not use a notebook internally.
Matthias, I see 2 different concepts here: 1) Global header and side image, possibly handle changes through the "prepare" signal 2) per page images, setting it is implicit in the GtkAssistant code I did 2) because (as far as I've seen) it's quite usual to change those images depending on the assistant stage, and as you may notice, the GtkAssistantPage widget is a really thin layer to provide that per-page data. another solution could be to attach that data internally to the inserted widgets, so we can get rid of GtkAssistantPage, but I don't know how well would this get along with Glade. Regarding the GtkNotebook use, after implementing my own code to draw pages and so, I felt that it could be a lot cleaner and less error prone if I used a widget that already handles that
carlos, look how my older code does the page titles. Child properties are the GTK+ mechanism for things like this. I think the images should be handled in that way, too. So, after thinking some more about the different approaches here, I think I want to try and merge the best aspects of both patches. - no page objects in the api, use child properties for title, image, etc - ditch the "forward" and "back" signals, and use your approach of specifying a function to compute the next page. - derive the assistant from GtkWindow (I don't think the GtkDialog response API is a very good match for an assistant) - use a stack of visited pages as you do to implement "back". I don't think there is a need to specify a custom back function, it should always go back to the previous page - keep the page types of my patch - add a per-page boolean property "complete", which indicates whether all required fields on the page have been filled in I think by combining the "forward" function, the page types and the "complete" information, we can do a very good job with flow control. E.g if all pages following the current page are already marked complete (because they only contain optional fields), we can show a "finish" button (if the sequence of following pages ends in a page of type "confirm", we would show a "last" button instead, which would jump to the confirm page).
To explain a bit more, the idea of the "complete" property is that you don't set the sensitivity of the buttons directly, but instead mark a page as complete when it is sufficently filled in, and let the assistant figure out how that translates into visibility and sensitivity of buttons. Oh, and of course we should keep the possibility to hide all standard buttons in the prepare signal, and add some way to add custom widgets to the action area. Such custom widgets can be used e.g. to show a progressbar if some activity on the page takes a long time, or to add custom navigation buttons, if that should be possible. (To enable custom navigation buttons, we need a set_current_page() function).
Nice, I didn't know a lot about child properties, thanks for pointing that out :) Regarding the rest of the proposal, it looks quite good (reimplementing action_area for GtkAssistant sounds like duplicate work, but I may understand your GtkDialog inheritance point), Do you plan to implement it? or may I work on this?
reimplementing action_area could be as simple as adding void gtk_assistant_{add,remove}_action_widget (GtkAssistant *assistant, GtkWidget *widget) Sure, you can work on it if you want. Otherwise I will revisit it in approx. 2 weeks. If you want to look into it, let me point out a few more things I noticed in your first implementation: - we still need an "apply" signal (this was called "finish" in the libgnomeui druid), so that the application knows when to act on the data collected by the wizard. Its probably a good idea to also keep the "cancel" and "close" signals. Note that pressing the "finish" button in a simple assistant should probably emit both "apply" and "close", since more complicated assistants may have a summary page (and possibly also a progress page) after the one with the "finish" button. - the list of visited pages must be emptied when the assistant is done. "unmap" might be good place to do that. - a destroy notify is needed for the data registered with the forward_page_func There were some more things I noticed, but I forgot to write then down...
Nice, I'll begin working on it tonight, thanks for the new comments, and don't forget to drop here your feedback :) Regarding the "apply" signal emission, I was handling it diferently, as the widget inherited from GtkDialog, I thought that people would do something like: GtkAssistant *assistant = get_assistant (); if (gtk_dialog_run (GTK_DIALOG (assistant)) == GTK_RESPONSE_APPLY) apply_changes (assistant); but anyway, emitting an "apply" signal would have been good too...
> if (gtk_dialog_run (GTK_DIALOG (assistant)) == GTK_RESPONSE_APPLY) > apply_changes (assistant); Thats actually one of the reasons why I think the response api is not a good fit for the assistant. There are cases where you want to apply the collected data before unmapping the dialog. If applying the data takes a long time, you might swich to another page displaying a progress bar, and once the changes are complete, swith to a summary page which lists what has been changed. The summary page only has a close button. (all this is handled by the page type api in my proposal)
Created attachment 52170 [details] first attempt to the proposed solution, .c file
Created attachment 52171 [details] first attempt to the proposed solution, .h file
Created attachment 52172 [details] some glue
Created attachment 52221 [details] tests for the attached implementation
Feedback from reading the files (I haven't actually tried it yet): I think we prefer to put enumerations in the same header file as the class they belong to, so I think GtkAssistantPageType should be in gtkassistant.h (I notice my earlier code got that wrong too). > gint gtk_assistant_insert_page (GtkAssistant *assistant, GtkWidget *sibling, gint position); The second parameter should probably be named "page", not "sibling" > _("Whether the page contents are correctly filled"), Maybe better "Whether all required fields on the page have been filled" It would probably be good to add a longer doc comment for this property and explain that the property should be set to TRUE for pages which have no required fields and point out that the value of the property is used to control the visibility and sensitivity of the "Last" button (if we add support for a "Last" button, that is) > static gint > default_forward_function (gint current_page, gpointer data) > { > return ++current_page; > } I think the default forward function should take page visibility into account, and skip invisible pages. Or is that taken care of somewhere else ? The switch statement in _set_assistant_buttons_state() needs a break for the last case, and just to be safe, a default: g_assert_not_reached(); case. In the else branch in compute_next_step(), don't you miss an if (do_apply) there ? Why is the apply signal emitted from compute_next_step(), for that matter ? Wouldn't it be cleaner to emit it from on_assistant_apply() ? _remove_assistant_page() needs to deal with removing a page that has already been visited, I think. > /* if there's no default page, pick the first one */ Should take visibility into account here, too. gtk_assistant_forall() needs to include the buttons when include_internals is TRUE More when I have tried the examples...
Created attachment 53304 [details] second try, .c file
Created attachment 53305 [details] second try, .h file
the new patches fixes those issues, and implement the "Last" button (plus other bugs/memleaks fixing), I've got some answers to your comments: - The compute_next_step() signals emission was mostly for handling more or less elegantly assistants with a broken flow (i.e.: not ending in a page of type "confirm" or "summary"), now I just throw a g_critical() in suck cases, is that ok? - the assistant buttons are children of priv->action_area, which is handled in gtk_assistant_forall()
The latest files appear to be missing a definition for GTK_TYPE_ASSISTANT_PAGE_TYPE (and from that, an implementation probably for a gtk_assistant_page_type_get_type() method). Also, it includes gtk/gtkmarshalers.h but does not seem to use it anywhere. And that #include is not available when trying to use it outside of GTK+ compilation (as an EggAssistant like thingy). Similarly gtk/gtkintl.h is not available outside GTK+ either, but I am not sure what the policy would be regarding inclusion of glib/gi18n.h instead.
The last implementation doesn't need a GtkAssistantPage type, it uses widgets and child properties as pages. The inclussion of gtkmarshalers.h should probably be removed (unless there is some obscure reason for using _gtk_marshal_* instead of g_cclosure_marshal_* I haven't seen), but the use of gtkintl.h is necessary for using the P_() macro for properties (which I've realized I havent used)
Created attachment 56197 [details] c file, now using P_() macro
"The last implementation doesn't need a GtkAssistantPage type, it uses widgets and child properties as pages." ... and uses a GTK_TYPE_ASSISTANT_PAGE_TYPE macro when adding these properties, which macro was not available anywhere. I now realize this is because it is likely generated if you build this inside GTK+ with the necessary glue; I tried using it as a libegg alike drop-in in an application. I generated the glue code myself now. Something else: I noticed that the page titles are top-aligned with the (top header) logo, whereas GnomeDruid used to center-align them alongside each other. IMO the center alignment looked better ;-) And the assistant does not seem to accommodate for page title length when calculating window size, which means that if the second (or a subsequent page) is larger than the first it gets wider when clicking next. GnomeDruid handled this ok, too - it calculated the maximum required width in advance.
Created attachment 57356 [details] c file, fourth try Oops, sorry, I thought you were referring to a GtkAssistantPage GType. The new C file takes into account page title and header image sizes during size allocation, uses cairo for drawing the colored box, adds the "content_padding" style property and adds some minor improvements to the code
I have committed this now, it seems good enough, and we can fix remaining small issues in-tree. A few things still need to be done: - Add some GtkAssistant examples to gtk-demo, add a standalone tests/testassistant - Flesh out docs, add some examples - Write a porting guide for porting from GnomeDruid to GtkAssistant
Detailing this a bit more, I think the tests in #25 are a great starting point for a comprehensive testassistant. Possible additions: - add some invisible pages, to test that they are properly skipped - add a page with a sidebar image - test "complete" handling, by adding a page with an entry which must be filled out - a progress page with a progressbar - add a custom page, where header and sidebar are not used, and the page itself contains some decorations - test a custom page forward function For gtk-demo, we should probably just add one somewhat representative assistant, which has some bells and whistles, without being excessively complicated
I see that gtkassistant.h (unlike the C file) was committed with DOS-style CR/LF pairs. Not sure if this matters...
Should be fixed
In trying to use GtkAssistant in a new application, I found the following issues: * Child property CHILD_PROP_PAGE_TYPE is named inconsistently: Enum value Name CHILD_PROP_PAGE_TYPE page-type CHILD_PROP_PAGE_TITLE title CHILD_PROP_PAGE_HEADER_IMAGE header-image CHILD_PROP_PAGE_SIDEBAR_IMAGE sidebar-image CHILD_PROP_PAGE_COMPLETE complete Should probably be simply "title". * Last button is misnamed under some circumstances: Page Type Contents Last button 0 Intro Finding modems - 1 Content (c) Port (defaults) Will skip to page 3 2 Content (c) Type (defaults) - 3 Progress Detecting... - 4 Content Modem specific - 5 Progress Configuring... - 6 Summary Done - (c) Page is marked as complete Instead, we need something like "_Jump forward" (with jump-to icon?) where the destination page isn't the last page. * Cancel, apply and close signals are documented as passing a page parameter, but don't. * It would be very useful if summary pages could be made to display the back button to allow users to change their minds where appropriate (rather than having to close the assistant and re-start it from the beginning as at present).
be simply "type", is of course what I meant to say.
Well, page-type is consistant with GtkAssistantPageType. What is inconsistent here is probably that the PROP constants for the other properties all have PAGE in their name, but I don't think its something to worry about. Regarding Jump, maybe. I'm not sure if it would be confusing. Wouldn't people wonder what steps they are skipping. The idea with the Last button was that you organize your wizard so that all optional steps go at the end, and then offer an early exit via the Last button. Maybe we should display a jump button in situations like you described, but then we probably need to add a way to turn it off. We should probably fix the signals to behave as specified. The idea of the Summary page is that it comes after applying whatever settings the wizard collected. So the damage is already done at that point. If you want to allow the user to inspect the collected settings before deciding to apply them or go back and make changes, use a confirm page.
I have fixed the signal docs, and added the testcases from #13 as tests/testassistant.c
Re: Jump-Forward, I certainly accept that it isn't perfect but I was working from the assumption that the current code to decide when the "last" button should be displayed was correct and that just changing the button name & icon would be an improvement. If we follow your argument in comment #41, then we shouldn't display any button at all in these cases. Ie., we stick with just the "last" button and change the check to display from (count > 1) to (count > 1 && page_num < 0). I'm just as happy with this, if you prefer it. Re: Summary page. Granted the damage has already been done, but the user may well only realise on reading the summary page that they've got it wrong and want to redo the operation. This is semantically equivalent to the undo menu item which many applications go to great lengths to provide. I would certainly agree that a back button may not be appropriate for all assistants, but it would be nice if we could allow applications to provide this functionality if it makes sense.
Another minor niggle I've just noticed with the last button is that its visibility depends on the forward-chain. If an application sets its own forward page function which varies the chain depending on the values entered by the user then there is no mechanism in place for the application to tell the assistant that the forward chain has been changed so that it can re-calculate whether to show or hide the last button. Perhaps we need a gtk_assistant_queue_rechain() or something?
A user-specified forward function should be prepared to calculate the next page for every page based, on the current values. For "future" pages, that would presumably be based on the default values.
A small problem with current compute_last_button_state implementation is that it does not check for cycles.
Re: Comment #45 which I assume is a reply to comment #44: That's all true, but it doesn't really help. Consider the code in ::set_complete() that is designed to cope with the complete property changing value on either the current page or a "future" page which might require that the last button be either hidden or shown. If it's right for the assistant to track changes to complete that affect the visibility of the last button, then why isn't it also right that it should track changes in the forward chain which have the same effect? Since it's not possible for the assistant to track such changes without help from the application, we need to provide a means by which this can be done.
As far as I can see, the assistant code determines the visibility of the last button when the completeness of any page changes. It should probably also track the visibility of pages. Oh, but you are worrying about cases where the user changes a value on a page which might cause the forward chain to change, making it necessary for the last button to change visibility. Yes, we don't handle that currently, and you are right that it might be simplest to have some recompute() function for such cases. I also notice another fine point of last button handling we don't currently get right. If all future pages are complete, but the current page is not yet, we should show the last button, but keep it insensitive until the current page is complete.
If the assistant is supposed to be able to cope with pages changing visibility, then yes, we need to track this too. I had originally altered the visibility of pages to modify the forward chain rather than using a forward page function, but I found that while calling gtk_widget_hide() wasn't a problem, calling gtk_widget_show() caused widgets on the "future" pages to be mapped instantly. If this is supposed to work, then gtkassistant!_set_current_page() probably needs to call gtk_widget_set_child_visible() to make this work. This at least is a nice easy bug with no decisions to be made. I'll start working on a patch tomorrow. I might do the tracking of page visibility also.
good catch.
Created attachment 57973 [details] [review] doc and fixes patch Hello, finally had some time to work on this patch, here's the list of changes: - Adds a doc chapter about porting from GnomeDruid to GtkAssistant - Adds an assistant sample to gtk-demo - Adds keyboard focus handling (sorry, didn't notice this before) - Improves some documentation and fixes some typos - Fixes a possible crash produced when there aren't more visible pages in the assistant flow - Avoids cicles in compute_last_button_state, and makes it to show the "last" button insensitive when the current page isn't complete (as suggested in #48) - Does a couple of minor improvements As far as I can see, ATM there are only left the issues commented in #44 and #49, a patch would be great for these, if not, I can work on them
Great work. You may have to redo the patch against current cvs though, since I made some changes to gtkassistant.c today, and also fixed the looping problem in compute_last_button_state() in a different way.
Created attachment 58008 [details] [review] patch against current cvs
Carlos, please be my guest. It's not as if I don't have other things to do :) Can I raise another point: We often want to run quite lengthly processes in assistants (sometimes several minutes). I presume these should be implemented using progress pages in the new API. However, neither the behaviour of incomplete progress pages (where cancel, back and forward buttons are insensitive) nor of complete progress pages (where all three are sensitive) seems to meet our needs. I can envision a situation where the current implementation of complete makes sense - if you have a process that is optional and can by skipped by the user (eg., a delay or polling for something that the user knows will never be found). However, where the process can't be skipped, I can't see what rational there is for concluding that it also can't be cancelled. Indeed the HIG (section 7 - Allowing Interruptions) would seem to preclude the current behaviour. I suggest that the back and cancel buttons always be sensitibe for progress pages. It would be nice if we could follow the HIG's recommendation further and provide some means to replace cancel with stop if this is more appropriate, perhaps most easily implemented with a new page type.
all we are discussing here is only default behaviour anyway. If you need to, you can frob the visibility/sensitivity of the buttons in the prepare handler. That is supposed to work. But lets discuss the default behaviour. Your proposal is to - make back and cancel sensitive by default on progress pages - treat progress pages like content pages wrt to last button handling - add an optional stop button on progress pages that would stop the currently progressing activity without canceling the assistant. For that we probably need a stop signal. If a progress page is stoppable, we could also allow the user to use the Forward button to stop the action and move to the next page. But maybe it is clearer to only make the stop button sensitive while the action is running, and enable cancel/back/forward when the action is stopped or complete. Comments ?
Ah, I hadn't understood that such frobbing was supported. Indeed, I've only just realized that the buttons are available as public members of GtkAssistant. Very nice. Presumably we'll need to take steps in due course to prevent buttons flashing up if they would be visible by default but the prepare handler is used to hide them. The first part of the proposal listed is certainly mine. I'm not clear where the second part come from. Perhaps it's implied by something I've said, but I'm missing it at the momemnt. Regarding a stop button, the HIG recommends that this button is used instead of cancel and should have a very similar meaning except that the operation in progress can't be backed out of. The application is supposed to put up an alert detailing the options available to the user. On this basis, I think it would be better if the assistant acted just as if it were a cancel button (ie., generate the cancel signal plus the WM close handling) and leave it to the application to take whatever action if necessary. Whether it is worth supporting the switch from cancel to stop in the assistant or whether applications should just use the prepare handler to hide the cancel button and add the stop button as an action widget, I'm not clear. One advantage of doing it inside the assistant is the buttons will appear in the right order. I'm not a usability guy, but from what I understand of the HIG, normal progress windows are supposed to automatically close once the operation is completed (the exception being checklist windows). thus I usually arrange that assistant progress pages are only shown while the operation is in progress and automatically advance to the next page once it's done. This allows me to think of "back" as meaning stop the operation and let me re-consider (after which I can use "forward" to mean start the operation again) and "forward" as meaning skip the rest of the operation and continue. For me, requiring that I stop the operation and then go back (ie., two seperate steps) just complicates things.
(In reply to comment #55) > - make back and cancel sensitive by default on progress pages > - treat progress pages like content pages wrt to last button handling > - add an optional stop button on progress pages that would stop > the currently progressing activity without canceling the assistant. > > For that we probably need a stop signal. If a progress page is stoppable, > we could also allow the user to use the Forward button to stop the > action and move to the next page. But maybe it is clearer to only make > the stop button sensitive while the action is running, and enable > cancel/back/forward when the action is stopped or complete. > > Comments ? > hmmm, IMHO an "stop" signal is quite out of the scope of an assistant widget, the current code works fine for the case of non-stoppable actions, and for the other case it adds other features: - buttons as public variables - gtk_assistant_add_action_widget () to add other buttons to the action area with the current behavior we cover the most common case, but the widget is lax enough to let developers handle the other one without big worries
I have committed the last patch. Here is my current TODO list for GtkAssistant: - implement visibility handling (see #48) - add long description - add some more test (see #36)
Created attachment 58340 [details] [review] another patch This is the summary of changes: - Adds migrating-GtkAssistant.sgml to Makefile.am in the gtk reference directory, it seems it was missing. - Adds long description to docs, also documents GtkAssistantPageFunc and GtkAssistantPageType. With these, I think the API is 100% covered. - Adds GtkAssistant to doc-shooter - implements children visibility handling - fixes assistant_paint_colored_box() to take container border width into account - adds another test to testassistant to use all untested features (hidden pages handling, sidebar and header images and widgets in the action area)
Thanks for the patch, I have committed it. I think we need to watch out for changes in the visibility of pages, and recalculate button state in that case. It has also been mentioned before that it might be a good idea to have a function to manually trigger a recalculation of the button state (user input on page A may influence the page sequence ahead of A, requiring an update of the visibility/sensitivity of the last button while the user interacts with page A) Also, did you see the bug about HIG compliant spacing etc ?
bug 328082 is the one about spacing issues.
Created attachment 58464 [details] [review] patch to handle visibility changes, also adds gtk_assistant_update_buttons_state()
2006-01-31 Matthias Clasen <mclasen@redhat.com> * gtk/gtk.symbols: * gtk/gtkassistant.h: * gtk/gtkassistant.c: Actually implement visibility handling, and add gtk_assistant_update_buttons_state. * tests/testassistant.c: Test visibility handling.
I think we are ready to close this now. Please open a new bug if I overlooked something.