Review Request: Plasma::DialogManager

[Aaron Seigo]

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



/trunk/KDE/kdelibs/plasma/abstractdialogmanager.h
<http://reviewboard.kde.org/r/3474/#comment4347>

    it also makes it nice for memory management, and we may want to take a signals approach to this anyways?



/trunk/KDE/kdelibs/plasma/abstractdialogmanager.h
<http://reviewboard.kde.org/r/3474/#comment4348>

    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?



/trunk/KDE/kdelibs/plasma/applet.h
<http://reviewboard.kde.org/r/3474/#comment4346>

    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. :)



/trunk/KDE/kdelibs/plasma/corona.cpp
<http://reviewboard.kde.org/r/3474/#comment4345>

    as this gets passed in from the outside world, i think it might be a good idea to keep a QWeakPointer to it.


- Aaron


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
> 
>