<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>
</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;">@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>
<br />
<p>- Parker</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>