KAssistantDialog proposal

[Aaron J. Seigo]

On Thursday 24 August 2006 8:13, Matthias Lechner wrote:
> based on the KStepList widget I proposed before I have created a
> KAssistantDialog class which extends the existing class by the following
> features:
> - an intuitive step list which displays the assistant's steps and allows to
> easily jump back to already configured steps
> - a customizable image on the left side
> - support for hierarchical page design for more complex scenarios
> - a unified way of displaying help texts (though this part still needs some
> work)

ok.. i've now looked at the header files and have a number of 
thoughts/questions..

KStepList
------------
as Michel noted, the name isn't the greatest. perhaps KStepsWidget. i don't 
think "list" is needed here actually.

widgetAttribute should be replaced with internal handling of the colours based 
on the QPalette of the app. perhaps instead of setColor the step could simply 
have one of three states: past, present and future. based on the state (which 
could be updated by next() and back()) the buttons could colour themselves 
rationally.

next(), back() ... shouldn't that be forward() and back() or next() and 
previous()? and why no jumpToStep(int index)?

KAssistantDialog
----------------------
setImage ... could be more descriptive? setBannerImage or some such so it's 
obvious what image is being referred to?

const QString pageHelp( KPageWidgetItem *item ) <-- i think you have the const 
on the wrong side of that definition. it should be: QString 
pageHelp(KPageWidgetItem* item) const

there's a bit of a non-OO nature to things.. e.g. there is a "setHelp" and 
a "setAppropriate" both of which take a KPageWidgetItem*. since it's setting 
an attribute associated with the KPageWidgetItem, wouldn't they belong in 
KPageWidgetItem itself? so one would simply call myWidgetItem->setHelp("some 
helpful text")? setAppropriate might be too specific to the KAssistantDialog, 
but at least Help shouldn't be?

there's a way to the get the current KPageWidgetItem, but no way to get a 
random item? perhaps a "KPageWidgetItem* findPage(const QString& name)"?

i'll also echo Thomas' concerns about the subpages thing. it seems rather 
convoluted. especially the whole bit about "can only remove subpages" and 
what not. currently wizards tend to just add pages as needed, they don't lay 
out the entire tree of widgets aforehand. in fact, doing so can actually be 
stupidly expensive since it often results in creating complex sets of 
widgets, loading plugins (again, c.f. kopete) and taking care of various bits 
of initialization...

finally, any reason the signal currentPageChanged is listed after the 
protected slots? all signals are public and traditionally are kept with the 
public interface. personally i really, really dislike class headers that mix 
public, protected, private together .. makes it much harder to go through and 
get a feel for the interface.

also, why is the before page included? and is it emitted before or after the 
page is shown? if after, then a before signal is certainly needed so as to do 
delayed initialization both for performance reasons as well as because you 
often have no choice but to wait for the user to fill in things in previous 
pages. you don't want to do this -after- the widget is on-screen for obvious 
reasons.

-- 
Aaron J. Seigo
Undulate Your Wantonness
GPG Fingerprint: 8B8B 2209 0C6F 7C47 B1EA  EE75 D6B7 2EB1 A7F1 DB43

Full time KDE developer sponsored by Trolltech (http://www.trolltech.com)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 191 bytes
Desc: not available
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20060824/529a530d/attachment.sig>