<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://svn.reviewboard.kde.org/r/5622/">http://svn.reviewboard.kde.org/r/5622/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On October 13th, 2010, 10:59 p.m., <b>Markus Slopianka</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
<p>On October 13th, 2010, 11:03 p.m., <b>Hugo Pereira Da Costa</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">... as well as 'lock toolbar positions'</pre>
</blockquote>
<p>On October 13th, 2010, 11:32 p.m., <b>Christoph Feck</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
<p>On October 13th, 2010, 11:49 p.m., <b>Parker Coates</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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". :)</pre>
</blockquote>
<p>On October 14th, 2010, 9:01 a.m., <b>Aurélien Gâteau</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">@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</pre>
</blockquote>
<p>On October 14th, 2010, 12:33 p.m., <b>Parker Coates</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">@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.</pre>
</blockquote>
<p>On October 15th, 2010, 7:22 a.m., <b>Aurélien Gâteau</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">> 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.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">> 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.</pre>
<br />
<p>- Thomas</p>
<br />
<p>On October 13th, 2010, 10:19 p.m., Aurélien Gâteau wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://svn.reviewboard.kde.orgrb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for kdelibs.</div>
<div>By Aurélien Gâteau.</div>
<p style="color: grey;"><i>Updated 2010-10-13 22:19:16</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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)</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Tested with Konqueror, KWrite and other KDE applications (more patches to come to provide consistency among applications)</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>trunk/KDE/kdelibs/kdeui/actions/kstandardaction.h <span style="color: grey">(1185607)</span></li>
<li>trunk/KDE/kdelibs/kdeui/actions/kstandardaction.cpp <span style="color: grey">(1185607)</span></li>
<li>trunk/KDE/kdelibs/kdeui/xmlgui/kxmlguiwindow.cpp <span style="color: grey">(1185607)</span></li>
</ul>
<p><a href="http://svn.reviewboard.kde.org/r/5622/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>