Review Request: Menubar and statusbar visibility actions: replace togglable "show" actions with "show/hide" actions

Parker Coates parker.coates at kdemail.net
Thu Oct 14 13:33:08 BST 2010 


> On 2010-10-13 22:59:25, Markus Slopianka wrote:
> > Hm..... I find the checkbox solution more intuitive.
> > If this patch gets accepted, one has to make sure, *all* KDE applications follow that method -- incl.Dolphin's "Show hidden file", KOffice's "Show grid", etc.
> 
> Hugo Pereira Da Costa wrote:
>     ... as well as 'lock toolbar positions'
> 
> Christoph Feck wrote:
>     We have to differentiate between true actions and just configurable options. As I understand it right, the KDualAction class has been created for the cases where no "checked" or "down" state is applicable, such as Play/Pause, Reload/Stop, etc. These would be actions.
>     
>     On the other hand there are options, such as whether certain UI elements should be visible. Since it is assumed that those things are saved between application invokations, they should keep the checkbox so that one quickly sees the state of the option, without even bothering to read the text.
>     
>     Just the fact that invoking the option immediately causes an action (in particular the hiding or showing of a widget) doesn't make these actions per se. Try putting them in a configuration dialog with a checkbox, and you will see the difference.
> 
> Parker Coates wrote:
>     I was part way through my own reply explaining the difference between "pure actions" and "options with implicit actions", when Christoph's comment hit my inbox. So I guess I'll just say "+1". :)
> 
> Aurélien Gâteau wrote:
>     @markus: Saying unchecking a "show" checkbox item is more intuitive than clicking an "hide" item sounds like saying "don't show" is more intuitive than "hide" to me. I do intend to fix all misuses of KToggleAction I find in kdelibs and KDE applications.
>     
>     @Christoph: As for quickly seeing the state of the option, it is even quicker to look at the bottom of the window to see if the status bar is there than to open the "Settings" menu.
>     
>     If you look at the way applications (mis)use KToggleAction, you will see that a lot of them want to have changing texts to toggle UI elements or UI content. I went through all uses of KToggleAction::setCheckedState() on lxr.kde.org before deciding to start this work. The results are enlightening:
>     
>     # Show/Hide UI elements
>     
>     'http://lxr.kde.org/source/KDE/kdeaccessibility/kmag/kmag.cpp#237' S/H Mouse Cursor
>     'http://lxr.kde.org/source/KDE/kdebase/apps/konqueror/src/konqguiclients.cpp#140'
>     'http://lxr.kde.org/source/KDE/kdepim/kaddressbook/mainwidget.cpp#415'
>     'http://lxr.kde.org/source/KDE/kdepim/kaddressbook/mainwidget.cpp#422'
>     'http://lxr.kde.org/source/KDE/kdepim/kaddressbook/mainwidget.cpp#429'
>     'http://lxr.kde.org/source/KDE/kdepim/knode/knmainwidget.cpp#760' S/H Quick Search
>     'http://lxr.kde.org/source/KDE/kdesdk/umbrello/umbrello/uml.cpp#408' S/H Grid
>     'http://lxr.kde.org/source/extragear/graphics/kiconedit/kiconedit.cpp#380' S/H Grid
>     'http://lxr.kde.org/source/extragear/graphics/kuickshow/src/kuickshow.cpp#289' S/H File Browser
>     'http://lxr.kde.org/source/koffice/karbon/ui/KarbonView.cpp#814' S/H Page Margins
>     'http://lxr.kde.org/source/koffice/karbon/ui/KarbonView.cpp#881' S/H Rulers
>     'http://lxr.kde.org/source/koffice/krita/ui/canvas/kis_perspective_grid_manager.cpp#74' S/H Perspective Grid
>     'http://lxr.kde.org/source/koffice/krita/ui/kis_painting_assistants_manager.cc#88' S/H Painting Assistants
>     'http://lxr.kde.org/source/koffice/krita/ui/kis_selection_manager.cc#193' S/H Selection
>     'http://lxr.kde.org/source/koffice/kword/part/KWView.cpp#264' S/H Document Headers
>     'http://lxr.kde.org/source/koffice/kword/part/KWView.cpp#273' S/H Document Footers
>     'http://lxr.kde.org/source/koffice/libs/main/KoGridData.cpp#210' S/H Grid
>     'http://lxr.kde.org/source/koffice/libs/main/KoMainWindow.cpp#343' S/H Dockers
>     'http://lxr.kde.org/source/koffice/libs/main/KoStandardAction.cpp#32' S/H Guides
>     
>     ## Should use std actions
>     'http://lxr.kde.org/source/KDE/kdeaccessibility/kmag/kmag.cpp#192'
>     'http://lxr.kde.org/source/KDE/kdeaccessibility/kmag/kmag.cpp#198'
>     'http://lxr.kde.org/source/KDE/kdeaccessibility/kmag/kmag.cpp#180'
>     'http://lxr.kde.org/source/KDE/kdeaccessibility/kmag/kmag.cpp#186'
>     'http://lxr.kde.org/source/KDE/kdeedu/klettres/src/klettres.cpp#156'
>     'http://lxr.kde.org/source/koffice/kword/part/KWView.cpp#469' S/H Status Bar
>     'http://lxr.kde.org/source/koffice/libs/main/KoMainWindow.cpp#1625' S/H Toolbars
>     
>     # Show/Hide content
>     'http://lxr.kde.org/source/KDE/kdeadmin/kuser/ku_mainwidget.cpp#118' S/H System Users/Groups
>     'http://lxr.kde.org/source/KDE/kdeedu/marble/src/marble_part.cpp#826' S/H shadow of the sun
>     'http://lxr.kde.org/source/KDE/kdeedu/marble/src/marble_part.cpp#834' S/H zenith location
>     'http://lxr.kde.org/source/KDE/kdepim/knode/knmainwidget.cpp#760' S/H threads
>     'http://lxr.kde.org/source/KDE/kdesdk/kate/plugins/symbolviewer/plugin_katesymbolviewer.cpp#90' S/H Symbols (inverted!)
>     'http://lxr.kde.org/source/KDE/kdesdk/kbugbuster/gui/kbbmainwindow.cpp#270' S/H Closed Bugs
>     'http://lxr.kde.org/source/KDE/kdesdk/kbugbuster/gui/kbbmainwindow.cpp#277' S/H Wishes
>     'http://lxr.kde.org/source/extragear/graphics/kpovmodeler/pmshell.cpp#119' S/H Path
>     'http://lxr.kde.org/source/playground/graphics/krita-exp/kis_layer_manager.cc#192' S/H Layers
>     
>     All those apps should be using KDualAction instead (and I intend to fix them or get their authors to fix them). I believe the current implementation of the showMenubar() and showStatusbar() actions is what it is because their authors knew one should not use KToggleAction with setCheckedState() but there was no simple alternative to implement actions with changing text.
>     
>     And last (even if I agree we should not copy everything from them), Apple HIG states "show/hide" should be used instead of "[ ] show/[x] show":
>     
>     http://developer.apple.com/library/mac/#documentation/UserExperience/Conceptual/AppleHIGuidelines/XHIGMenus/XHIGMenus.html#//apple_ref/doc/uid/TP30000356-TPXREF121

@Aurélien
"If you look at the way applications (mis)use KToggleAction, you will see that a lot of them want to have changing texts to toggle UI elements or UI content."

I think there's general agreement that having both changing text and a checkbox is confusing and inconsistent. You listed 35 examples of setCheckedState misuse, but we have to consider the (I'm guessing) hundreds of places where KToggleAction is used correctly. If you commit this patch and convert these standard KToggleActions to KDualActions, you'll be introducing inconsistencies in all those applications.

So I agree that the uses above are broken, but whether they'd be better off as "Show/Hide" KDualActions or "[X] Show" KToggleActions will probably have to be determined on a case by case basis.

"I believe the current implementation of the showMenubar() and showStatusbar() actions is what it is because their authors knew one should not use KToggleAction with setCheckedState() but there was no simple alternative to implement actions with changing text."

I assumed the opposite: that they believed static text with a checkbox was superior to changing text. Either way, it is definitely the style used most commonly throughout KDE4, so it should not be changed in standard UI elements without a major effort to change it everywhere.

By the way, I do think KDualAction is a great idea and I have plans to replace some questionable uses of KToggleAction with it. I just don't feel that these standard actions are great candidates for conversion.


- Parker


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/5622/#review8129
-----------------------------------------------------------


On 2010-10-13 22:19:16, Aurélien Gâteau wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/5622/
> -----------------------------------------------------------
> 
> (Updated 2010-10-13 22:19:16)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> -------
> 
> The standard actions to toggle the visibility of the menubar and the statusbar are togglable "show" actions: their label is always "show {menu,status}bar" and they have a checkbox next to them. This means when the user wants to hide the statusbar, he must click an item which says "[x] Show Statusbar". I believe this is less intuitive than if the item label were "Hide Statusbar".
> 
> Attached patch implements this by introducing KStandardAction::showHideMenubar(), KStandardAction::showHideStatusbar() and using showHideStatusbar() in KXmlGuiWindow. It also deprecates KStandardAction::showMenubar() and KStandardAction::hideMenubar().
> (I already have a patch for Konqueror to make use of KStandardAction::showHideMenubar() and will probably patch other applications if this request is approved)
> 
> 
> Diffs
> -----
> 
>   trunk/KDE/kdelibs/kdeui/actions/kstandardaction.h 1185607 
>   trunk/KDE/kdelibs/kdeui/actions/kstandardaction.cpp 1185607 
>   trunk/KDE/kdelibs/kdeui/xmlgui/kxmlguiwindow.cpp 1185607 
> 
> Diff: http://svn.reviewboard.kde.org/r/5622/diff
> 
> 
> Testing
> -------
> 
> Tested with Konqueror, KWrite and other KDE applications (more patches to come to provide consistency among applications)
> 
> 
> Thanks,
> 
> Aurélien
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20101014/57f9e75f/attachment.htm>

More information about the kde-core-devel mailing list