Review Request: PATCH: Configuration Dialog for Turning off Notifications in the Panel

[Aaron Seigo]

-----------------------------------------------------------
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
> 
>