Review Request: Menubar and statusbar visibility actions: replace togglable "show" actions with "show/hide" actions
Aurélien Gâteau
agateau at kde.org
Fri Oct 15 21:41:35 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
>
> Parker Coates wrote:
> @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.
>
> Aurélien Gâteau wrote:
> > 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.
>
> They all are about showing/hiding UI elements so they all should behave in the same way. Of course I volunteer 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.
>
> > I assumed the opposite: that they believed static text with a checkbox was superior to changing text."
>
> SVN history agrees with me:
> http://websvn.kde.org/trunk/KDE/kdelibs/kdeui/actions/kstandardaction.cpp?r1=657382&r2=661863
>
> """
> KToggleAction::setCheckedState doesn't get rid of the checkbox.
>
> Update APIDOX to reflect this, as well as removing (improper) uses
> of it in kdelibs and kdebase.
>
> --- trunk/KDE/kdelibs/kdeui/actions/kstandardaction.cpp 2007/04/23 22:14:12 657382
> +++ trunk/KDE/kdelibs/kdeui/actions/kstandardaction.cpp 2007/05/06 20:43:31 661863
> @@ -459,10 +459,6 @@
>
> ret->setWhatsThis( i18n( "Show Menubar<p>"
> "Shows the menubar again after it has been hidden" ) );
> - KGuiItem guiItem( i18n("Hide &Menubar"), 0 /*same icon*/, QString(),
> - i18n( "Hide Menubar<p>"
> - "Hide the menubar. You can usually get it back using the right mouse button inside the window itself." ) );
> - ret->setCheckedState( guiItem );
>
> ret->setChecked( true );
>
> @@ -479,10 +475,6 @@
>
> ret->setWhatsThis( i18n( "Show Statusbar<p>"
> "Shows the statusbar, which is the bar at the bottom of the window used for status information." ) );
> - KGuiItem guiItem( i18n( "Hide St&atusbar" ), QString(), QString(),
> - i18n( "Hide Statusbar<p>"
> - "Hides the statusbar, which is the bar at the bottom of the window used for status information." ) );
> - ret->setCheckedState( guiItem );
>
> ret->setChecked( true );
> """
>
> My understanding is that in the KDE3 days, KToggleAction did not imply a checkbox so this code was ported as is, using "show/hide". When KToggleAction problem was discovered, KStandardAction code was patched.
>
> I believe "show/hide" is more adapted than togglable "show" actions for UI elements and UI content and I am willing to make the necessary changes so that applications consistently uses this style. If the decision is that togglable "show" actions are the way to go, I am of course not going to fix those 35 examples because I consider the fix broken, and being consistently broken is only marginally better than inconsistently broken. Will someone step up to do the work? The good news is that the required changes to make them consistently broken is quite lighter than to make them use KDualAction: one only need to remove the "setCheckedState()" lines.
>
> Thomas Friedrichsmeier wrote:
> > They all are about showing/hiding UI elements so they all should behave in the same way. Of course I volunteer to fix them.
>
> I know there are not many KDE applications outside of the KDE repository, these days. But there are, so please everybody stop assuming that it's enough to fix all occurrences in Git/SVN. For applications such as RKWard, which try to support more than just the most recent release of the KDE Platform, behavior changes like this can cause a significant headache.
>
> > My understanding is that in the KDE3 days, KToggleAction did not imply a checkbox so this code was ported as is, using "show/hide".
>
> Yes, so historically show/hide was first, but it's been a checkbox ever since KDE 4.0. Personally, I like the checkbox much better, but ultimately this sort of thing is for the usability experts to decide. Yet, even if there is general agreement on changing this, please consider delaying the change until KDE 5.
I used to maintain a KDE application outside KDE SVN so I agree with you fixing applications in KDE SVN does not mean all KDE applications are fixed, but that's the best I can do. I don't think it should prevent us from fixing bugs as it is possible and not very difficult to support multiple versions of KDE. This is often done even for applications inside KDE SVN, all extragear applications come to mind.
Having said that, experience tells me this discussion is going nowhere, so I am going to discard this request and file this patch together with his Konqueror and KWrite friends in my rejected/ dir. That will teach me to write code before asking.
- Aurélien
-----------------------------------------------------------
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/20101015/57f6d163/attachment.htm>
More information about the kde-core-devel
mailing list