[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: Petri_Damstén <petri.damsten () gmail ! com>
Date: 2010-09-25 8:31:29
Message-ID: 20100925083129.23979.75856 () vidsolbach ! de
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/3950/#review7773
-----------------------------------------------------------
Ship it!
Looks good to me.
- Petri
On 2010-05-14 08:21:03, Michel Lafon-Puyo wrote:
> =
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.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 monit=
oring features to the system-monitor applet. Monitoring the cpu clock frequ=
ency 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 moni=
tored was (0,0) but an icon were displayed and it overlapped the other appl=
ets (SM::Applet::CheckGeometry())
> - set the preferred height to MINIMUM when no item is monitored (SM::Appl=
et::displayNoAvailableSources()). When all the meters of an applet were rem=
oved 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 widget=
s were not correctly deleted (SM::Applet::deleteMeters())
> - the removal of the layout and the meters were done on SM::Applet::conne=
ctToEngine(), 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 con=
figuration change to achieve a clean update. =
> - the header of the applets is now correctly deleted on form factor chang=
e 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 w=
ebview and the icon were not correctly deleted on form factor changes. =
> - to be consistent with the other applets, the HDD applet has been change=
d 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 (n=
o configuration is present), the behaviour has not changed and the configur=
ation is still populated with the list of the mounted volumes.
> =
> Additionnally, the HDD applet didn't use the SM::Applet to manage its met=
ers. So I replaced the list of SM::Plotter (m_plotters) by a list of QGraph=
icsWidget (m_meters) and modified HDD to take advantage of the meters manag=
ement already implemented in the SM::Applet class (in particular the remova=
l of the meters/widgets on configuration change as mentioned above). =
> =
> Note: I was unable to find any bug reports corresponding to the fixes abo=
ve. Should I create them myself?
> =
> =
> Diffs
> -----
> =
> /trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/appl=
et.h 1126529 =
> /trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/appl=
et.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/hwin=
fo.h 1126529 =
> /trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/hwin=
fo.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/temp=
erature.h 1126529 =
> /trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/temp=
erature.cpp 1126529 =
> =
> Diff: http://svn.reviewboard.kde.org/r/3950/diff
> =
> =
> Testing
> -------
> =
> Basic testing
> =
> =
> Thanks,
> =
> Michel
> =
>
[Attachment #5 (text/html)]
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 \
solid;"> <tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://svn.reviewboard.kde.org/r/3950/">http://svn.reviewboard.kde.org/r/3950/</a>
</td>
</tr>
</table>
<br />
<p>Ship it!</p>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Looks good to me.</pre> \
<br />
<p>- Petri</p>
<br />
<p>On May 14th, 2010, 8:21 a.m., Michel Lafon-Puyo wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" \
style="background-image: \
url('http://svn.reviewboard.kde.orgrb/images/review_request_box_top_bg.png'); \
background-position: left top; background-repeat: repeat-x; border: 1px black \
solid;"> <tr>
<td>
<div>Review request for Plasma.</div>
<div>By Michel Lafon-Puyo.</div>
<p style="color: grey;"><i>Updated 2010-05-14 08:21:03</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">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?</pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">Basic testing</pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/applet.h \
<span style="color: grey">(1126529)</span></li>
<li>/trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/applet.cpp \
<span style="color: grey">(1126529)</span></li>
<li>/trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/cpu.h <span \
style="color: grey">(1126529)</span></li>
<li>/trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/cpu.cpp <span \
style="color: grey">(1126529)</span></li>
<li>/trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/hdd.h <span \
style="color: grey">(1126529)</span></li>
<li>/trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/hdd.cpp <span \
style="color: grey">(1126529)</span></li>
<li>/trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/hwinfo.h \
<span style="color: grey">(1126529)</span></li>
<li>/trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/hwinfo.cpp \
<span style="color: grey">(1126529)</span></li>
<li>/trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/net.h <span \
style="color: grey">(1126529)</span></li>
<li>/trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/net.cpp <span \
style="color: grey">(1126529)</span></li>
<li>/trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/ram.h <span \
style="color: grey">(1126529)</span></li>
<li>/trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/ram.cpp <span \
style="color: grey">(1126529)</span></li>
<li>/trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/temperature.h \
<span style="color: grey">(1126529)</span></li>
<li>/trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/temperature.cpp \
<span style="color: grey">(1126529)</span></li>
</ul>
<p><a href="http://svn.reviewboard.kde.org/r/3950/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
_______________________________________________
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]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic