Review Request: bug fixes for the system-monitor applet

[Michel Lafon-Puyo]

> On 2010-05-12 22:07:10, Aaron Seigo wrote:
> > a few minor coments below. the biggest issue now is that with your patch, the icon for the drives in the disk monitor appear _below_ the meters rather than beside them. are you seeing this as well?

Le jeudi 13 mai 2010 14:00:25, Petri Damstén a écrit :
> Haven't actually tested the diff but appendVisualization already adds
> widget to main layout. hdd calls it with meter only. I guess it should
> call appendVisualization with layout (icon + meter) and remove
> mainLayout()->addItem(layout); line.

Thanks for your help.
In fact, adding the layout as a visualization is not really possible because the meters must be widgets (in the current implementation). If we want to do that (manage layouts), it complexifies the removal of the meters in the SM::Applet class because we should browse the nested layouts to find the widgets and destroy them (or let the sub applets do that but it is error prone to manage all these widgets in every applet).

So I used a little trick: first I call appendVisualization() to add the visualization to the list (the widget is added to the main layout by doing that) then I reparent the widget by adding it to the sub-layout managed by the applet. 


- Michel


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


On 2010-05-14 08:21:03, Michel Lafon-Puyo wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3950/
> -----------------------------------------------------------
> 
> (Updated 2010-05-14 08:21:03)
> 
> 
> 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 1126529 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/applet.cpp 1126529 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/cpu.h 1126529 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/cpu.cpp 1126529 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/hdd.h 1126529 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/hdd.cpp 1126529 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/hwinfo.h 1126529 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/hwinfo.cpp 1126529 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/net.h 1126529 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/net.cpp 1126529 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/ram.h 1126529 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/ram.cpp 1126529 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/temperature.h 1126529 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/temperature.cpp 1126529 
> 
> Diff: http://reviewboard.kde.org/r/3950/diff
> 
> 
> Testing
> -------
> 
> Basic testing
> 
> 
> Thanks,
> 
> Michel
> 
>