Review Request: bug fixes for the system-monitor applet

Wed May 12 10:42:21 CEST 2010

> On 2010-05-11 17:20:41, Aaron Seigo wrote:
> > looks like a good start. i've only read through the code, i haven't actually done any testing (have to run to a meeting now, actually ...) but i will do so later. there are some comments below, in any case. :)
> > 
> > thanks for the patch, and welcome to KDE & Plasma hacking!

Thank you very much :)

> On 2010-05-11 17:20:41, Aaron Seigo wrote:
> > /trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/applet.cpp, line 251
> > <http://reviewboard.kde.org/r/3950/diff/3/?file=26487#file26487line251>
> >
> >     centralizing the "meters" is a reasonable idea; however, the name of the methods and the data members isn't really accurate anymore and a bit misleading when reading the code.
> >     
> >     perhaps changing "meter" to "display" or "visualization"? e.g. addVisualization, deleteVisualizations, visualization(source), etc?

You're perfectly right. 

I did some refactoring of the source code: item -> source and meter -> visualization.

Concerning "item" becoming "source", it seems to be obvious since when we called addItem, the parameter was always a source.
Concerning "visualization", I like it because each element in the system-monitor applet in really a "visualization" of the "sources" (previously "items" :)).

> On 2010-05-11 17:20:41, Aaron Seigo wrote:
> > /trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/hdd.cpp, lines 279-282
> > <http://reviewboard.kde.org/r/3950/diff/3/?file=26491#file26491line279>
> >
> >     is clearing the layouts really necessary? doesn't deleting the top level layout also take all the layouts with it? hm...
> >     
> >     clearing the icons collection looks correct though.

You're right, I removed these lines.

> On 2010-05-11 17:20:41, Aaron Seigo wrote:
> > /trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/net.cpp, lines 167-169
> > <http://reviewboard.kde.org/r/3950/diff/3/?file=26494#file26494line167>
> >
> >     this seems to be a very common idiom, repeated a number of times. perhaps this should be moved into a method in the parent class?

The clear() method has been added to the SM::Applet class and is now called in every applet.

- Michel

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

On 2010-05-12 08:42:09, Michel Lafon-Puyo wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3950/
> -----------------------------------------------------------
> 
> (Updated 2010-05-12 08:42:09)
> 
> 
> Review request for Plasma.
> 
> 
> Summary
> -------
> 
> As my first work for KDE, I would like to add some power management monitoring features to the system-monitor applet. Monitoring the cpu clock frequency when using the on-demand or the conservative governor could be useful (at least it is for me :)).
> 
> Before going further in this development I tried to fix some little bugs with the applet. Here is the list of the changes this patch introduces:
> - the size of the applets when used in the panel and when no item is monitored was (0,0) but an icon were displayed and it overlapped the other applets (SM::Applet::CheckGeometry())
> - set the preferred height to MINIMUM when no item is monitored (SM::Applet::displayNoAvailableSources()). When all the meters of an applet were removed at once, the size were unchanged and a big icon could be displayed.
> - clear the content of the tooltip when nothing has to be displayed (SM::Applet::toolTipAboutToShow())
> - when one or many items were set "unmonitored", the corresponding widgets were not correctly deleted (SM::Applet::deleteMeters())
> - the removal of the layout and the meters were done on SM::Applet::connectToEngine(), I moved that part in a new method SM::Applet::removeLayout() that can be called more easily. This method is called by the applets on configuration change to achieve a clean update. 
> - the header of the applets is now correctly deleted on form factor change and should not be displayed when the applet is used in the panel
> - when used in the panel, the webview containing the information given by the hardware information applet was displayed under the icon because the webview and the icon were not correctly deleted on form factor changes. 
> - to be consistent with the other applets, the HDD applet has been changed to *not* populate the configuration with the list of the mounted volumes when there is no more item to monitor. Nevertheless, on the first launch (no configuration is present), the behaviour has not changed and the configuration is still populated with the list of the mounted volumes.
> 
> Additionnally, the HDD applet didn't use the SM::Applet to manage its meters. So I replaced the list of SM::Plotter (m_plotters) by a list of QGraphicsWidget (m_meters) and modified HDD to take advantage of the meters management already implemented in the SM::Applet class (in particular the removal of the meters/widgets on configuration change as mentioned above). 
> 
> Note: I was unable to find any bug reports corresponding to the fixes above. Should I create them myself?
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/applet.h 1125154 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/applet.cpp 1125154 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/cpu.h 1125154 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/cpu.cpp 1125154 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/hdd.h 1125154 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/hdd.cpp 1125154 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/hwinfo.h 1125154 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/hwinfo.cpp 1125154 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/net.h 1125154 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/net.cpp 1125154 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/ram.h 1125154 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/ram.cpp 1125154 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/temperature.h 1125154 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/temperature.cpp 1125154 
> 
> Diff: http://reviewboard.kde.org/r/3950/diff
> 
> 
> Testing
> -------
> 
> Basic testing
> 
> 
> Thanks,
> 
> Michel
> 
>