> 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 > > _______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
