Bug 143233

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();
   }