'Re: Review Request: bug fixes for the system-monitor applet'

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-panel-devel
Subject:    Re: Review Request: bug fixes for the system-monitor applet
From:       "Aaron Seigo" <aseigo () kde ! org>
Date:       2010-05-11 17:20:36
Message-ID: 20100511172036.13445.89297 () localhost
[Download RAW message or body]

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

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!

/trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/applet.cpp
<http://reviewboard.kde.org/r/3950/#comment5303>

    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?

/trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/applet.cpp
<http://reviewboard.kde.org/r/3950/#comment5301>

    should be on same line: } else {

/trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/applet.cpp
<http://reviewboard.kde.org/r/3950/#comment5302>

    would a check for m_meters.contains(source) first (and clearing it out if it does \
exist) make sense and prevent possible errors in the future?

/trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/cpu.cpp
<http://reviewboard.kde.org/r/3950/#comment5304>

    personally, i'd probably write this line as:

    SM::Plotter *plotter = qobject_cast<SM::Plotter *>(meter(source));

    qobject_cast is generally prefered when it's a QObject, though it otherwise \
behaves as a dynamic_cast.

/trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/hdd.cpp
<http://reviewboard.kde.org/r/3950/#comment5300>

    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.

/trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/net.cpp
<http://reviewboard.kde.org/r/3950/#comment5299>

    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?

- Aaron

On 2010-05-11 16:25:30, Michel Lafon-Puyo wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3950/
> -----------------------------------------------------------
> 
> (Updated 2010-05-11 16:25:30)
> 
> 
> 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.cpp 1125154 
> /trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/ram.cpp 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
> 
> 

_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

[prev in list] [next in list] [prev in thread] [next in thread] 