Review Request 107264: Fix Presence Applet settings

[Dan Vrátil]

> On Nov. 9, 2012, 4:09 p.m., David Edmundson wrote:
> > That's not true, they're in the context menu
> > 
> > I would argue that: 
> > 1) they're not settings relating to the plasmoid, so don't belong there
> > 2) they don't physically fit without resizing
> > 
> > (did you know the battery applet only shows the configs there because the author didn't know how to add it to a context menu)
> > 
> > Justify the decision to me.
> 
> Dan Vrátil wrote:
>     Look on it from this POV: left click on "Instant Messaging Presence settings" - now do you see anything that would allow you to configure instant messaging presence?
>     
>     Secondly, there is no way to access the KDE Integration configuration page from the applet context menu.
>
> 
> David Edmundson wrote:
>     1) Left clicking on presence applet doesn't let you get to the plasmoid config (and therefore this) either.
>     2) Well that's just a bug. It doesn't justify doing it differently.
>     
>     I made a list:
>     
>      - Battery applet shows Solid settings in the plasmoid settings 
>      - Network Manager has a link in the main applet to the networking KCM and shows it in the plasmoid settings
>      - Device Notifier does not link to hotplug KCM
>      - Pager shows virtual desktop settings in the plasmoid settings.
>      - Clock has to "adjust date & time" KCM link in the context menu
>      - Nepomuk Controller "applet" links to KCM in the context menu.
>     
>     Personally, I'm against applets configuring things other than the applet and think the clock applet has it right, but it's not particularly consistent across the standard widgets. Maybe we should email plasma-devel?
>     
>     So, I'm not really in favour of this patch but it's not "wrong" either so I'm not against it either. 
>     
>     If you get a ship it from someone else, go for it.
> 
> Kai Uwe Broulik wrote:
>     Concerning the battery plasmoid settings (which now resize properly, so it is an issue in the telepathy KCM, not the plasmoid!), I think you cannot add entries to the context menu when it is a pure QML plasmoid?
>     
>     I'd say it is percetly valid to add related settings to the plasmoid. I like that I am able to configure the power management settings from the battery plasmoid or device actions from the device notifier.
> 
> David Edmundson wrote:
>     To clarify, the KTp config does resize properly with this patch . I assumed (before testing) that it would not because of the battery one. 
>     My initial comment that it would not resize, is incorrect.
>
> 
> Kai Uwe Broulik wrote:
>     Ah, okay. Yeah that bug in the battery plasmoid was due to some issues in the PowerDevil KCM, it had nested containers (QWidget inside QScrollArea inside a layout inside the KCM) and so probably some Qt bug caused the embedded KCM to not set a proper sizehint and so the dialog was not properly sized. I fixed that, and as a side-effect that fixed the missing gradient in the KCM as well. Just FYI :)

Since Dave does not like this patch, is there anyone else who would give me 'Ship it', or should I just close this review?


- Dan


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


On Nov. 9, 2012, 2:13 p.m., Dan Vrátil wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107264/
> -----------------------------------------------------------
> 
> (Updated Nov. 9, 2012, 2:13 p.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Description
> -------
> 
> Atm the applet configuration dialog shows only the Plasma default pages, which is silly. Let's add the KCM Accounts and KDE Integration pages there!
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt 7becc7e 
>   src/presenceapplet.h 5b462fc 
>   src/presenceapplet.cpp 33fc8cc 
> 
> Diff: http://git.reviewboard.kde.org/r/107264/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Dan Vrátil
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-telepathy/attachments/20130211/6740f3e3/attachment.html>