Bug 143854

Yikes!  Can 'Exit' be disabled in kstscript for 1.4.0?  'Exit' as it is now, 
is much worse than nothing.

On 5-Apr-07, at 10:29 AM, Barth Netterfield wrote:

> Yikes!  Can 'Exit' be disabled in kstscript for 1.4.0?  'Exit' as  
> it is now,
> is much worse than nothing.


   Not really.  CTRL+D will do the same.  It's a known problem with  
KMDI.

--
George Staikos
KDE Developer				http://www.kde.org/
Staikos Computing Services Inc.		http://www.staikos.net/

Can we override ctrl-D with a global hot key (like w and 2 were recently)?  

The fact that we know about it doesn't reduce how bad it is by very much at 
all...

cbn

On Thursday 05 April 2007 10:29:43 am George Staikos wrote:
[bugs.kde.org quoted mail]

On 5-Apr-07, at 10:49 AM, Barth Netterfield wrote:

> Can we override ctrl-D with a global hot key (like w and 2 were  
> recently)?
>
> The fact that we know about it doesn't reduce how bad it is by very  
> much at
> all...


    Well it's been around for a long time now.  It was in 1.2 and  
1.3.  There are still more ways to make it exit too.  This was just  
an example.

--
George Staikos
KDE Developer				http://www.kde.org/
Staikos Computing Services Inc.		http://www.staikos.net/

OK... we can't fix it totally... but having a trivial way to render kst dead 
that the user wouldn't expect would kill kst is not a good thing.

Is there anything we can do to reduce the cross-section for this bug?
eg, 
1) is it easy to disable the command "exit" from javascript, so you wont 
trigger the bug that way.

2) is it easy to make ctrl^d a global shortcut to do nothing at all?

Do either of these choices have any obvious problems?

cbn

On Thursday 05 April 2007 11:33:28 am George Staikos wrote:
[bugs.kde.org quoted mail]

> OK... we can't fix it totally... but having a trivial way to render  
> kst dead
> that the user wouldn't expect would kill kst is not a good thing.
>
> Is there anything we can do to reduce the cross-section for this bug?
> eg,
> 1) is it easy to disable the command "exit" from javascript, so you  
> wont
> trigger the bug that way.


    It's not a javascript command, it's a kstcmd command.  However  
then there is no way to exit it if run from the commandline if we  
remove exit and ctrl+d.  We're trading one of many bad situations for  
another.

> 2) is it easy to make ctrl^d a global shortcut to do nothing at all?


    I think so.

> Do either of these choices have any obvious problems?


    All of this is probably more work than

    Also, for example, the user can access anything in the UI and do  
all kinds of whacky things with KstUI.  Remember you can't protect  
users from themselves and there is always going to be a way to make  
Kst crash or behave strangely.  At least we don't crash.  If this  
(very) corner case happens, the user can File->Save As, exit, reload,  
and continue.  And we can fix KMDI in 1.4.1 or later.

   Making it impossible to exit the command prompt as a workaround  
for a bug in a KDE component really doesn't make any sense to me.

--
George Staikos
Staikos Computing Services Inc.      http://www.staikos.net/

right... now I remember... this doesn't happen if you type 'exit' from an 
external kstcmd.  So disabling exit breaks the working case.

1.4.1 (or later) then.

Created attachment 20189 [details]
Proposed patch

Created attachment 20190 [details]
Proposed patch

Can you explain why Konsole crashed before and why it no longer  
crashes since you removed this documented fix?

--
George Staikos
KDE Developer				http://www.kde.org/
Staikos Computing Services Inc.		http://www.staikos.net/

No. Do you have any problems with the proposed patch?

Yes, it backs out a fix.  I think this has to be deferred.
--
George Staikos
KDE Developer				http://www.kde.org/
Staikos Computing Services Inc.		http://www.staikos.net/

So you're saying that Konsole crashes with the proposed patch?

I'm saying that you removed a fix that explicitly claimed to fix a  
specific problem.  You left no trace of the comment or fix and have  
been unable to provide an explanation as to why you removed it.   
Furthermore you have been unable to diagnose the problem or explain  
the proposed solution.  Even if we ignore the fact that we're far too  
late for 1.4.0 it definitely would not be appropriate to apply this  
change into SVN based on the above.

--
George Staikos
KDE Developer				http://www.kde.org/
Staikos Computing Services Inc.		http://www.staikos.net/

So what would you like to see before this fix goes in?

An explanation of the fix, what it does, why it is correct, and why  
the fix for konsolepart crashes is no longer necessary.
--
George Staikos
KDE Developer				http://www.kde.org/
Staikos Computing Services Inc.		http://www.staikos.net/

Previously there was an attempt to remove the splitter window and reparent the main widget after exiting the JavaScript console by typing 'exit'. This did not work properly.

The revised code does not do this but simply remembers that it does need to recreate the splitter window. In this respect it is closer in functionality to simply hiding the JavaScript console, as is done when the user selects Hide javaScript Console from the top menu. 

You've eliminated the check to see if the part is recreated when the splitter survived, this means no message box if the part isn't created properly.

You could simplify the code by removing the unnecessary if/else statement.

I think the if/else statement is needed as the first time through _splitter is NULL, but on all subsequent times (unless an error occurred with the creation of the part) it is non-NULL.

I agree that an error message was missing - this was partly deliberate as I assumed that if it was created once then it will be recreated without problem on subsequent tries. This may well not be valid.

Created attachment 20251 [details]
Proposed patch

Add message suggested by Adam.

Any other reasons not to check this in?

Created attachment 20253 [details]
Simplified

This ensures the part is created or returns with message and also simplifies
the whole thing.  I can't get javascript console to show on my current system,
so please test.

Adam, your patch is functionality somewhat different in that the first time through the splitter will be created regardless of whether or not the part is created. In my patch the splitter will be deleted if the part is not created - leaving the system in an unaltered state.

But your code is cleaner.

I have no particular preference.

No, the splitter is deleted if the part isn't created.  Look again.

But I think it should only be deleted if we haven't already set it to be the central widget. If it is already the central widget then we should just leave it (as we do when we select Hide JavaScript Console).

Your patch is better without the delete - as I originally and mistakenly read it. Then my previous comment stands.

SVN commit 653178 by treat:

* Don't mess up tabs

BUG:143854


 M  +10 -18    js.cpp  
 M  +0 -1      js.h  


--- branches/work/kst/1.5/kst/src/extensions/js/js.cpp #653177:653178
@@ -240,23 +240,26 @@
       KMessageBox::sorry(app(), i18n("Could not load konsole part.  Please install kdebase."));
       return;
     }
-    _splitter = new QSplitter(Qt::Vertical, app());
+
+    if (!_splitter) {
+      _splitter = new QSplitter(Qt::Vertical, app());
+      _oldCentralWidget = app()->centralWidget();
+      _oldCentralWidget->reparent(_splitter, QPoint(0, 0));
+      _splitter->show();
+      app()->setCentralWidget(_splitter);
+    }
+
     KParts::Part *p = dynamic_cast<KParts::Part*>(f->create(_splitter, "kstcmd"));
     if (!p) {
       KMessageBox::sorry(app(), i18n("Konsole part appears to be incompatible.  Please install kdebase correctly."));
-      delete _splitter;
       return;
     }
 
-    _oldCentralWidget = app()->centralWidget();
-    _oldCentralWidget->reparent(_splitter, QPoint(0, 0));
     _splitter->moveToLast(p->widget());
-    app()->setCentralWidget(_splitter);
-
     connect(p, SIGNAL(destroyed()), this, SLOT(shellExited()));
     _konsolePart = p;
   }
-  _splitter->show();
+
   _konsolePart->widget()->show();
 #endif
 }
@@ -266,21 +269,10 @@
 #ifdef KST_HAVE_READLINE
   _showAction->setChecked(false);
   _konsolePart = 0L;
-  QTimer::singleShot(0, this, SLOT(restoreUI())); // konsole crashes otherwise
 #endif
 }
 
 
-void KstJS::restoreUI() {
-  if (_oldCentralWidget) {
-    _oldCentralWidget->reparent(app(), QPoint(0, 0));
-    app()->setCentralWidget(_oldCentralWidget);
-  }
-  delete _splitter;
-  _splitter = 0L;
-}
-
-
 void KstJS::hideConsole() {
 #ifdef KST_HAVE_READLINE
   if (_konsolePart) {
--- branches/work/kst/1.5/kst/src/extensions/js/js.h #653177:653178
@@ -56,7 +56,6 @@
 
   private slots:
     void shellExited();
-    void restoreUI();
     void doArgs();
 
   private:

SVN commit 653187 by arwalker:

CCBUG:143854 Set the 'Show JavaScript Console' toggle state appropriately if we failed to create the part

 M  +2 -0      js.cpp  


--- branches/work/kst/1.5/kst/src/extensions/js/js.cpp #653186:653187
@@ -238,6 +238,7 @@
     KLibFactory *f = KLibLoader::self()->factory("libkonsolepart");
     if (!f) {
       KMessageBox::sorry(app(), i18n("Could not load konsole part.  Please install kdebase."));
+      _showAction->setChecked(false);
       return;
     }
 
@@ -252,6 +253,7 @@
     KParts::Part *p = dynamic_cast<KParts::Part*>(f->create(_splitter, "kstcmd"));
     if (!p) {
       KMessageBox::sorry(app(), i18n("Konsole part appears to be incompatible.  Please install kdebase correctly."));
+      _showAction->setChecked(false);
       return;
     }