Version: HEAD (using KDE KDE 3.5.1) OS: Linux PROBLEM: To be more consistent with Hide/Show Toolbar/Statusbar menu items the Hide JavaSscript Console should not have a check mark associated with it. STEPS TO REPRODUCE: Start Kst Enable the JavaScript extension if necessary Select Tools... Show JavaScript Console Select Tools... menu RESULT: The menu item for Hide JavaScript Console has an associated checkmark EXPECTED RESULTS: The menu item for Hide JavaScript Console should have no associated checkmark
Created attachment 20072 [details] Proposed patch Use KAction instead of KToggleAction
I think the problem here is just a missing connection. Your patch implements KToggleAction without using KToggleAction. -- George Staikos KDE Developer http://www.kde.org/ Staikos Computing Services Inc. http://www.staikos.net/
As I understand it a KToggleAction is checkbox-like. However, this is now what we want as the menu item should never be checked. Hence, we need to keep track of the state ourselves instead of passing it off to KToggleAction. That said, if you can supply the missing connection that fixes this bug it would be more elegant.
Created attachment 20122 [details] Keep checkmark, don't change label How about we keep the architecture as it is, but chose to not change the label.... Is this clear/standard in KDE?
I think we already had this discussion in the past. What we currently have is standard - we just need to lose the checkmark.
The checkmark/toggle action is definitely the standard KDE approach. I suspect this patch doesn't solve the synchronization problem though. I think there's a missing signal connection somewhere. -- George Staikos KDE Developer http://www.kde.org/ Staikos Computing Services Inc. http://www.staikos.net/
The Hide/Show approach is used by KolourPaint, KIconEdit, KSig, KBabel, etc.
What sync problem? I thought that the bug was either just the wording of the menu item, or the existence of a checkmark (depending on how you looked at it). On Thursday 29 March 2007 1:06:05 pm George Staikos wrote: [bugs.kde.org quoted mail]
The checkmark approach seems to be used by Konqueror and Kmail/Kontact. Assertion: Konq should be considered the standard for all things KDE. so... Lets go with Konq/Kmail and use the Check/No Check approach. cbn On Thursday 29 March 2007 1:22:25 pm Andrew Walker wrote: [bugs.kde.org quoted mail]
On 29-Mar-07, at 1:49 PM, Barth Netterfield wrote: > The checkmark approach seems to be used by Konqueror and Kmail/ > Kontact. > Assertion: Konq should be considered the standard for all things KDE. > > so... > Lets go with Konq/Kmail and use the Check/No Check approach. Agreed - those are the only things I even checked against too. Simply because some apps do some things doesn't mean it's the right approach. -- George Staikos KDE Developer http://www.kde.org/ Staikos Computing Services Inc. http://www.staikos.net/
> What sync problem? I thought that the bug was either just the > wording of the > menu item, or the existence of a checkmark (depending on how you > looked at > it). Oh, I thought it was sometimes not synchronizing its state with the state of the console itself. Andrew? -- George Staikos KDE Developer http://www.kde.org/ Staikos Computing Services Inc. http://www.staikos.net/
I agree with Barth. There is no sync problem.
This makes the JavaScript Console checkmark approach inconsistent with Kst's own Show/Hide Statusbar and Toolbar. If we're okay with that then Barth's patch looks good, but I would delete the obsolete lines rather than comment them out.
SVN commit 648294 by netterfield: BUG: 143233 Make consistent w/ konqueror/kmail and with tools menue. M +0 -3 js.cpp --- trunk/extragear/graphics/kst/src/extensions/js/js.cpp #648293:648294 @@ -232,7 +232,6 @@ void KstJS::showConsole() { #ifdef KST_HAVE_READLINE - _showAction->setText(i18n("Hide &JavaScript Console")); if (!_konsolePart) { strcpy(shellStr, "SHELL=kstcmd"); putenv(shellStr); @@ -266,7 +265,6 @@ void KstJS::shellExited() { #ifdef KST_HAVE_READLINE _showAction->setChecked(false); - _showAction->setText(i18n("Show &JavaScript Console")); _konsolePart = 0L; QTimer::singleShot(0, this, SLOT(restoreUI())); // konsole crashes otherwise #endif @@ -285,7 +283,6 @@ void KstJS::hideConsole() { #ifdef KST_HAVE_READLINE - _showAction->setText(i18n("Show &JavaScript Console")); if (_konsolePart) { _konsolePart->widget()->hide(); }