Review Request: Plasma::DialogManager

[Marco Martin]

> On 2010-04-03 19:35:12, Aaron Seigo wrote:
> > /trunk/KDE/kdelibs/plasma/abstractdialogmanager.h, line 53
> > <http://reviewboard.kde.org/r/3474/diff/2/?file=22486#file22486line53>
> >
> >     my concern with using virtual methods for this is that we only get one release in which to add more virtuals to it. and i half-expect that this class easily gain more methods.
> >     
> >     what if instead of virtual methods, they were just "normal" methods which emitted corresponding signals? then the shell would be responsible for creating, registering with Corona and connecting the signals of the DialogManager.
> >     
> >     then it's future proof: we just add more public methods and more signals.
> >     
> >     thoughts?

yes, seems a good idea


> On 2010-04-03 19:35:12, Aaron Seigo wrote:
> > /trunk/KDE/kdelibs/plasma/abstractdialogmanager.h, line 42
> > <http://reviewboard.kde.org/r/3474/diff/2/?file=22486#file22486line42>
> >
> >     it also makes it nice for memory management, and we may want to take a signals approach to this anyways?

yep, what i tought


> On 2010-04-03 19:35:12, Aaron Seigo wrote:
> > /trunk/KDE/kdelibs/plasma/applet.h, line 811
> > <http://reviewboard.kde.org/r/3474/diff/2/?file=22488#file22488line811>
> >
> >     this could actually help a number of types of dialog showing, e.g. to prevent modal dialogs in plasma-desktop.
> >     
> >     i wonder if instead of showConfigurationInterface(QWidget *widget) it should be showDialog(QWidget *widget, Plasma::DialogType type) where DialogType would be things like ConfigurationDialog, ... uhm. i can't actually think of another example atm.
> >     
> >     perhaps that is just over-engineering it then. :)

i can kinda see we wanting things like the applet config dialog, usually pretty big anyways mostly full screen at least in some shells, while other dialogs should probably have a different look.

so i can see we maybe want at least two, can't think about good names now, but something like
showDialog (if we go signals showDialogRequested i think)
and
showFullScreenDialogRequested (FullScreen is bad, suffests a particular implementation should be something more generic)
but perhaps i'm overengineering even more, but i'm thinking, if we want to add a new public class, at least be worth it eheh :p


> On 2010-04-03 19:35:12, Aaron Seigo wrote:
> > /trunk/KDE/kdelibs/plasma/corona.cpp, line 271
> > <http://reviewboard.kde.org/r/3474/diff/2/?file=22491#file22491line271>
> >
> >     as this gets passed in from the outside world, i think it might be a good idea to keep a QWeakPointer to it.

yep.


- Marco


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/3474/#review4864
-----------------------------------------------------------


On 2010-04-03 19:09:42, Marco Martin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3474/
> -----------------------------------------------------------
> 
> (Updated 2010-04-03 19:09:42)
> 
> 
> Review request for Plasma.
> 
> 
> Summary
> -------
> 
> this is the DialogManager class, as discussed before on irc with aaron. will let plasma-netbook and plasma mobile show the config uis in a bit fancier way
> the base class happily does exactly nothing, actual implementations will be only in shells.
> there are still some doubts, expressed by the various todo/fixme :)
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdelibs/plasma/CMakeLists.txt 1110066 
>   /trunk/KDE/kdelibs/plasma/abstractdialogmanager.h PRE-CREATION 
>   /trunk/KDE/kdelibs/plasma/abstractdialogmanager.cpp PRE-CREATION 
>   /trunk/KDE/kdelibs/plasma/applet.h 1110066 
>   /trunk/KDE/kdelibs/plasma/applet.cpp 1110066 
>   /trunk/KDE/kdelibs/plasma/corona.h 1110066 
>   /trunk/KDE/kdelibs/plasma/corona.cpp 1110066 
> 
> Diff: http://reviewboard.kde.org/r/3474/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Marco
> 
>