> 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 > > _______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
