Review Request: PopupApplet : add some stuffs

[Loic Marteau]

> On 2008-07-27 09:05:13, Aaron Seigo wrote:
> > /trunk/KDE/kdebase/workspace/libs/plasma/popupapplet.h, line 61
> > <http://reviewboard.vidsolbach.de/r/109/diff/4/?file=555#file555line61>
> >
> >     delayedShowPopup()?

in fact the idea is to show the popup during a determinate time so :
showPopup(uint popupDuration)
and
hideTimedPopup() for the slot called by the timer

hope that this is ok :p


> On 2008-07-27 09:05:13, Aaron Seigo wrote:
> > /trunk/KDE/kdebase/workspace/libs/plasma/popupapplet.cpp, lines 141-143
> > <http://reviewboard.vidsolbach.de/r/109/diff/4/?file=556#file556line141>
> >
> >     i really dislike assignment+test in a conditional like this. it's much less readable and doesn't gain you anything at compile time. in such situations, i'd suggest something like:
> >     
> >     QLayoutItem *child = d->dialog->layout()->taskAt(0);
> >     
> >     while (child) {
> >         d->dialog->layout()->removeItem(child);
> >         child = d->dialog->layout->taskAt(0);
> >     }
> >     
> >     or even a for loop ... anyways, in this case this doesn't actually make sense anyways since QLayout::takeAt already removes the item from the layout.
> >     
> >     what you probably want here is something like:
> >     
> >     QLayout *l = d->dialog->layout();
> >     for (int i = 0; i < l->count(); ++i) {
> >         if (l->itemAt(i) == widget()) {
> >             l->takeAt(i);
> >             --i;
> >          }
> >     }

hum, i have a little too much anticipate for the case we have correct way to deal with Graphics stuff
if we only deal with widget is it ok to just do this ? d->dialog->layout()->removeWidget(widget());


> On 2008-07-27 09:05:13, Aaron Seigo wrote:
> > /trunk/KDE/kdebase/workspace/libs/plasma/popupapplet.cpp, line 155
> > <http://reviewboard.vidsolbach.de/r/109/diff/4/?file=556#file556line155>
> >
> >     you know graphicsWidget() is not null, so you don't need to check for it here

sorry to waste your time with dummy stuff like this...


> On 2008-07-27 09:05:13, Aaron Seigo wrote:
> > /trunk/KDE/kdebase/workspace/libs/plasma/popupapplet.cpp, line 193
> > <http://reviewboard.vidsolbach.de/r/109/diff/4/?file=556#file556line193>
> >
> >     should be passing in d->dialog as the parent

QGraphicsScene *scene = new QGraphicsScene(d->dialog);
QGraphicsView *view = new QGraphicsView(d->dialog);
view->setScene(scene);

not really sure that is that you asked for...


> On 2008-07-27 09:05:13, Aaron Seigo wrote:
> > /trunk/KDE/kdebase/workspace/libs/plasma/popupapplet.cpp, line 237
> > <http://reviewboard.vidsolbach.de/r/109/diff/4/?file=556#file556line237>
> >
> >     put in PopupAppletPrivate

in fact device notifier need to reset a scheduled timer for popupHide.
i have remove it completely and just make the necessary in showPopup to reset an existing timer if you ask again for showPopup.
Applet who want reset the schedule timer for popupHide have to call showPopup again.

Thanks !


> On 2008-07-27 09:05:13, Aaron Seigo wrote:
> > /trunk/KDE/kdebase/workspace/libs/plasma/popupapplet.cpp, line 131
> > <http://reviewboard.vidsolbach.de/r/109/diff/4/?file=556#file556line131>
> >
> >     why this aspect ratio?

good question !

i have add in the private class
    Plasma::AspectRatioMode savedAspectRatio;
    bool restoreAspectRatio;

so we backup the aspect ratio in panel mode and we restore it in desktop mode
dont know if it is a bit too complicate...


- Loic


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.vidsolbach.de/r/109/#review98
-----------------------------------------------------------


On 2008-07-27 07:19:54, Loic Marteau wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.vidsolbach.de/r/109/
> -----------------------------------------------------------
> 
> (Updated 2008-07-27 07:19:54)
> 
> 
> Review request for Plasma.
> 
> 
> Summary
> -------
> 
> Here is a patch to adapt Device Notifier to use Plasma::PopupApplet.
> 
> I have add some timer stuff in popupApplet so we can say how time we want popup the applet in panel.
> 
> I have change a little the way popupApplet work on Desktop mode by using the same Plasma::dialog widget than we use in Panel Mode.
> Perhaps it is not a good idea ?!
> 
> If we prefer not using Plasma:Dialog in Desktop mode i have to found a correct way to use the plasma theme inside the proxyWidget, which is not the case in the actual code.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdebase/workspace/libs/plasma/popupapplet.h
>   /trunk/KDE/kdebase/workspace/libs/plasma/popupapplet.cpp
>   /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicenotifier.h
>   /trunk/KDE/kdebase/workspace/plasma/applets/devicenotifier/devicenotifier.cpp
> 
> Diff: http://reviewboard.vidsolbach.de/r/109/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Loic
> 
>