Review Request: Activities KCM

[David Edmundson]

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


UI review based on the screenshots in your email to plasma-devel, plus a few fairly minor code comments.

http://ivan.fomentgroup.org/blog/wp-content/uploads/2012/08/kamdsettings.png

Try to avoid nesting GroupBoxes, especially inside a TabWidget, you get this box inside boxes inside boxes effect.
A list of radio buttons should have a question above. In this case you're using the groupbox title as the question.

You may find the first one works better simply as a label.

Remember Open Documents:
   [ ] For all applications
   [ ] Only for specific applications
   [ ] Don't remember open documents

Also rules dictate all forms should be in a form layout.


http://img833.imageshack.us/img833/341/kamdsettings1.png

I'm afraid you're not going to like this, and this is going to cause an active discussion I think. 

QML in KDE applications stands out massively, it's inconsistent with the separation we're having and inconsistent means wrong, (IMHO) we can't do it at all until QML QtStyles. 

To pick this particular screenshot apart:
 - You're using a different font in your delegates. It needs to be the ones the user set in Application Appearance -> Fonts. 
 - You'll (possibly) find a 1 or 2 px vertical padding on the large icon which is on the left of the delegate will help, you won't get it touching it at the top. And you'll avoid a clash with highlights.

 - If this is mimicking a list view it needs to behave the same
   - Scrollbar should be on the right hand side of the of the list view, and needs to look like the conventional 
   - Keyboard navigation
   - Highlight on mouseover.

There's some code in KWin that does (most of) the above, after a similar discussion. Maybe we( or I?) should work on some standard library code for this as it seems quite a common thing to want to do. 

http://img94.imageshack.us/img94/8151/kamdsettings2.png

Same as all the above, plasma widgets inside an application breaks the workspace, application boundaries. You've got no choice but to use the "wrong widgets", and behaving in a manner inconsistent with other KCMs or even KDE applications with the slide in shouldn't be done. 

Ignoring that and focussing on particulars:

A checkbox with the label "Privacy" doesn't make sense to me. "Private" may work better. It definitely needs a tooltip if it's not got one already. Possibly even needs a full sentence label next to it to explain what this actually means. If I didn't read your blogs I'm not sure I'd know.

The icon needs to look more like a button so I know I can click it. I assume it's a Plasmoid ToolButton currently, a Button would make it clearer

Padding between buttons. Qt forms are almost always 4px apart. 



src/workspace/settings/BlacklistedApplicationsModel.cpp
<http://git.reviewboard.kde.org/r/106130/#comment14119>

    if applications.length is 0, you remove 0 to -1, and that gives you a crash (well qFatal)
    
    



src/workspace/settings/BlacklistedApplicationsModel.cpp
<http://git.reviewboard.kde.org/r/106130/#comment14120>

    const & 



src/workspace/settings/CMakeLists.txt
<http://git.reviewboard.kde.org/r/106130/#comment14121>

    capital letter in class filename isn't typically usual, and doesn't match any other cpp in kactivities repo.


- David Edmundson


On Aug. 22, 2012, 10:04 p.m., Ivan Čukić wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106130/
> -----------------------------------------------------------
> 
> (Updated Aug. 22, 2012, 10:04 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Description
> -------
> 
> System settings module for activities
> 
> 
> Diffs
> -----
> 
>   cmake/modules/c++11-test-initializer-lists-N2672.cpp PRE-CREATION 
>   src/CMakeLists.txt 63ed737 
>   src/config-features.h.cmake bde6b66 
>   src/service/Application.cpp 5dadbca 
>   src/service/plugins/sqlite/StatsPlugin.cpp 6f24be8 
>   src/service/plugins/virtualdesktopswitch/virtualdesktopswitch.cpp 568e0f9 
>   src/workspace/CMakeLists.txt 8a6e1bb 
>   src/workspace/settings/BlacklistedApplicationsModel.h PRE-CREATION 
>   src/workspace/settings/BlacklistedApplicationsModel.cpp PRE-CREATION 
>   src/workspace/settings/CMakeLists.txt PRE-CREATION 
>   src/workspace/settings/MainConfigurationWidget.h PRE-CREATION 
>   src/workspace/settings/MainConfigurationWidget.cpp PRE-CREATION 
>   src/workspace/settings/kcm_activities.cpp PRE-CREATION 
>   src/workspace/settings/kcm_activities.desktop PRE-CREATION 
>   src/workspace/settings/qml/BlacklistApplicationView.qml PRE-CREATION 
>   src/workspace/settings/ui/MainConfigurationWidgetBase.ui PRE-CREATION 
>   src/workspace/settings/ui/kdeclarativeview.h PRE-CREATION 
>   src/workspace/settings/ui/kdeclarativeview.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/106130/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Ivan Čukić
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20120822/beaef918/attachment-0001.html>