Version: HEAD (using KDE KDE 3.5.1) Installed from: Compiled From Sources OS: Linux PROBLEM: Exiting the JavaScript console removes all tabs already created and does not allow any new ones to be created STEPS TO REPRODUCE: Start Kst Select File... New tab... several times to create some new tabs Enable the JavaScript extension if necessary Select Tools... Show JavaScript Console In the JavaScript console type: exit<Enter> RESULTS: The JavaScript console and all exisitng tabs disappear. No more tabs can be created. EXPECTED RESULTS: Only the JavaScript console disappears. New tabs can be created.
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; }