-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.vidsolbach.de/r/331/#review318
-----------------------------------------------------------
patch looks ok, though we'll want to spiffy up the checkboxes a bit.
"jobs" and "notifications" are perhaps a bit "jargony" for the average user?
perhaps we could add a little bit of text at the top of the config page?
"Select which kinds of application feedback should be integrate with the sytem
tray below" or something along those lines?
and then:
File Transfers [ ]
Processing Jobs [ ]
Desktop Announcements [ ]
i'm not sure i like any of the above, actually ;) splitting out file transfers
and "other" jobs would mean a bit of work in the applicationjobs dataengine i
think, but would be a neat feature to look at adding after this patch goes in.
the configuration is something we can sort out after this patch goes in,
however. let's get the functionality in first and then we can all
hyperventilate together over the config dialog. ;)
just a few small issues in the comments below and then you can commit this
patch. welcome to plasma devel! =)
/trunk/KDE/kdebase/workspace/plasma/applets/systemtray/ui/applet.cpp
<http://reviewboard.vidsolbach.de/r/331/#comment263>
in case you are wondering why this doesn't need a parent, it's because when
you call addPage on it below, a parent is implicitly set on it.
not the prettiest aspect of that API imho (a bit "magical") but there you
go =)
/trunk/KDE/kdebase/workspace/plasma/applets/systemtray/ui/applet.cpp
<http://reviewboard.vidsolbach.de/r/331/#comment261>
checkbox is not given a parent and will thus leak memory. it should be:
d->showJobs = new QCheckBox(i18n("Show Jobs"), d->notificationInterface);
/trunk/KDE/kdebase/workspace/plasma/applets/systemtray/ui/applet.cpp
<http://reviewboard.vidsolbach.de/r/331/#comment262>
checkbox is not given a parent and will thus leak memory.
/trunk/KDE/kdebase/workspace/plasma/applets/systemtray/ui/applet.cpp
<http://reviewboard.vidsolbach.de/r/331/#comment259>
missing space: } else
/trunk/KDE/kdebase/workspace/plasma/applets/systemtray/ui/applet.cpp
<http://reviewboard.vidsolbach.de/r/331/#comment260>
missing space: } else
- Aaron
On 2009-01-17 09:53:04, distortedlogic wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.vidsolbach.de/r/331/
> -----------------------------------------------------------
>
> (Updated 2009-01-17 09:53:04)
>
>
> Review request for Plasma.
>
>
> Summary
> -------
>
> This patch allows one to turn on and off jobs and notifications in the panel
> via the systemtray configuration dialog. This is in response to Aaron
> Seigo's blog entry:
> http://aseigo.blogspot.com/2009/01/todays-tip-turning-off-fancy-schmancy.html
>
> Within manager.cpp, functions to unregister the notification and job
> protocols was created, as well as a function to unset a protocol (disconnect
> it). These were based on the already present functions to register and setup
> protocols.
>
> Within applet.cpp, a new page was added to the configuration dialog called
> "Notifications." This page includes the two check boxes that allow one to
> turn on and turn off notifications. A possible area where that may need to
> be fixed is the strings for the check boxes. Presently they are "Show Jobs"
> and "Show Notifications."
>
> Thank you for your time and review.
>
> Update: I have attached a screenshot of what the configuration dialog looks
> like and I also made a change to the patch: I added the i18n function around
> the check box label strings which I had forgotten the first time around.
>
>
> Diffs
> -----
>
> /trunk/KDE/kdebase/workspace/plasma/applets/systemtray/core/manager.h
> /trunk/KDE/kdebase/workspace/plasma/applets/systemtray/core/manager.cpp
> /trunk/KDE/kdebase/workspace/plasma/applets/systemtray/ui/applet.cpp
>
> Diff: http://reviewboard.vidsolbach.de/r/331/diff
>
>
> Testing
> -------
>
> Testing of suppression of job progress.
> Testing of suppression of notifications.
> Testing of allowing job progress.
> Testing of allowing notifications.
>
>
> Screenshots
> -----------
>
> Configuration Dialog
> http://reviewboard.vidsolbach.de/r/331/s/101/
>
>
> Thanks,
>
> distortedlogic
>
>
_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
