Run in terminal
ClosedPublic

Authored by ederag on Dec 7 2017, 7:27 PM.

Details

Summary

This patch was last submitted as https://bugs.kde.org/show_bug.cgi?id=349756

It allows to "run current file" in terminal.
This just outputs to the terminal plugin
either the full file name or its basename,
depending on settings.

This is very useful when the current file is a script, for instance.
Especially combined with a keyboard shortcut.

For instance when octave is running in the terminal,
and "octave_script.m" is the current file,
a simple F4 issues "octave_script\n" in the octave console,
which accordingly makes octave run the script. Very handy.
Yes, I know that octave has an IDE now, but kate is nice,
and it could be useful for others too ?

The patch was against kate shipped with kde-4.12.

What do you think about it ?

Test Plan

Facultative (but really handy): assign the F4 shortcut to Tools>Run Current Document

With the kateconsole visible,
Hit F4
The current file name is issued in the terminal (kate console).
The file extension being stripped or not,
depending on the "Run in terminal: remove extension" in the kateconsole settings.

A prefix string (e.g. "python ") can be prepended, according to
Settings>Configure kate>Terminal>Run in Terminal>Prefix

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
ederag requested review of this revision.Dec 7 2017, 7:27 PM
ederag created this revision.
ederag edited the summary of this revision. (Show Details)

In principle I have no problem with this extension.
For the code: Could you change KateConsole::slotRun() to report some more meaningful error like "can only run local files".
And do we perhaps want some info message that hints that you should save your file if it has unsaved changes?

ederag updated this revision to Diff 23706.EditedDec 9 2017, 10:29 PM
ederag edited the test plan for this revision. (Show Details)

Thanks for the positive feedback.

In principle I have no problem with this extension.
For the code: Could you change KateConsole::slotRun() to report some more meaningful error like "can only run local files".

Then shouldn't we handle isValid() and isLocalFile() errors separately,
and make similar changes in slotSync (20 lines above) ?
Could this be done later ?

And do we perhaps want some info message that hints that you should save your file if it has unsaved changes?

Good catch. The patch was a bit old.
Actually it is even better to have an automatic save before run.
Then the workflow is really smooth: edit file / F4 / see result, repeat.

The diff has been updated, against 'v16.12.3', because
the latest revision failed with
error: ‘class QApplication’ has no member named ‘desktopFileName’

For the error message: Two different errors would be nice.

Btw., you could use the ktexteditor/message.h to show the error message inline in the view, that will make it easier to spot.

For the auto-save: Perhaps the action should then be "Save & run...". And I am not sure if we don't want a question box on first triggering that to make people aware what they do (at least for the first time).

For the error message: Two different errors would be nice.

Btw., you could use the ktexteditor/message.h to show the error message inline in the view, that will make it easier to spot.

This also would avoid sending any "#### ..." on the command line.

But I would appreciate some pointers there;
right now only the 4.x documentation shows up:
https://api.kde.org/4.x-api/kdelibs-apidocs/interfaces/ktexteditor/html/classKTextEditor_1_1Message.html
https://api.kde.org/4.x-api/kdelibs-apidocs/interfaces/ktexteditor/html/classKTextEditor_1_1MessageInterface.htm

For the auto-save: Perhaps the action should then be "Save & run...". And I am not sure if we don't want a question box on first triggering that to make people aware what they do (at least for the first time).

What about another checkbox "Autosave", off by default, instead ?

For the error message: Two different errors would be nice.

Btw., you could use the ktexteditor/message.h to show the error message inline in the view, that will make it easier to spot.

This also would avoid sending any "#### ..." on the command line.

But I would appreciate some pointers there;
right now only the 4.x documentation shows up:
https://api.kde.org/4.x-api/kdelibs-apidocs/interfaces/ktexteditor/html/classKTextEditor_1_1Message.html
https://api.kde.org/4.x-api/kdelibs-apidocs/interfaces/ktexteditor/html/classKTextEditor_1_1MessageInterface.htm

The

https://api.kde.org/frameworks/ktexteditor/html/classKTextEditor_1_1Message.html

page has some small example.

For the auto-save: Perhaps the action should then be "Save & run...". And I am not sure if we don't want a question box on first triggering that to make people aware what they do (at least for the first time).

What about another checkbox "Autosave", off by default, instead ?

I still think on first invoke you should be informed that you now will execute something.
But I could be a bit over-protective.

ederag updated this revision to Diff 23774.Dec 11 2017, 9:11 PM

The KateConsole::slotRun() function has been reworked,
with separate errors:
KMessageBox::sorry(0, i18n ("Invalid document URL"));
and
KTextEditor::Message(i18n("Not a local file: '%1'", u.path() )...);

Intermediate variables like document render the code much cleaner.
Are they OK ?

If fine, should the slotSync be reworked accordingly, or left for later ?

I still think on first invoke you should be informed that you now will execute something.
But I could be a bit over-protective.

Maybe one those nifty windows warning, with "do not show again" checkbox ?
Could you please give some guidance ?

Intermediate variables like document render the code much cleaner.
Are they OK ?

That is OK ;=)
But I don't understand why the "no valid url" is a message box but the other is a nice overlay message ;=)

KWidgetAddons KMessageBox... provides the "show not again" functionality:

https://api.kde.org/frameworks/kwidgetsaddons/html/namespaceKMessageBox.html

Intermediate variables like document render the code much cleaner.
Are they OK ?

That is OK ;=)
But I don't understand why the "no valid url" is a message box but the other is a nice overlay message ;=)

Because I thought that if the url itself was invalid,
then things were probably really bad,
and it was safer not to rely on the visibility of the document ?

KWidgetAddons KMessageBox... provides the "show not again" functionality:

https://api.kde.org/frameworks/kwidgetsaddons/html/namespaceKMessageBox.html

Thanks, the KMessageBox::warningContinueCancel seems adequate.

Because I thought that if the url itself was invalid, then things were probably really bad, and it was safer not to rely on the visibility of the document ?

I think the best is to skip that and just warn if no local file, we don't care why it is no one ;=)

ederag updated this revision to Diff 24046.Dec 17 2017, 8:35 PM

Because I thought that if the url itself was invalid, then things were probably really bad, and it was safer not to rely on the visibility of the document ?

I think the best is to skip that and just warn if no local file, we don't care why it is no one ;=)

But then a "Not a local file" error could be shown, when it is actually the url that would be invalid.
So it was left aside; or please clarify ?

And I am not sure if we don't want a question box on first triggering that to make people aware what they do (at least for the first time).

A warning dialog is now issued, for possible review of the command to be sent.
It can be suppressed by checking "Don't ask again", or brought back by the "Show warning next time" button in the settings.

But then a "Not a local file" error could be shown, when it is actually the url that would be invalid.
So it was left aside; or please clarify ?

I think that would be just fine enough.
A non-valid URL is no local file (and I doubt somebody will ever get one, beside "no" url).
But I can live with two different messages, as long as both are inline via KTextEditor::Message, it makes no sense that one is a messagebox.

ederag updated this revision to Diff 24083.Dec 18 2017, 7:54 PM

A non-valid URL is no local file (and I doubt somebody will ever get one, beside "no" url).
But I can live with two different messages, as long as both are inline via KTextEditor::Message, it makes no sense that one is a messagebox.

To me it still makes sense (shorter code, simpler to maintain, for the very unlikely event)
It would have been nice to explain why (documentation ?) you are so confident
that a file without any valid url
has for sure a valid display (for the error message) nonetheless.

Anyway, here it is, as requested.

cullmann accepted this revision.Dec 18 2017, 8:09 PM

To me it still makes sense (shorter code, simpler to maintain, for the very unlikely event)
It would have been nice to explain why (documentation ?) you are so confident
that a file without any valid url
has for sure a valid display (for the error message) nonetheless.

Sorry if I was to imprecise.

You get your document via:

if ( m_mw->activeView() ) {

KTextEditor::Document *document = m_mw->activeView()->document();

That means there is some valid view around.
That means you can use the message stuff.

For my comment, I did even propose to just remove the

if (! u.isValid()) {
}

block completely and only show the "is no local file" error.
A not valid URL is no local file anyways.
I don't remember we do any isValid() check inside ktexteditor anyways ;)

But I am happy as is, therefor accept.

Thanks for the contribution again, btw.!

I assume I shall apply the patch and push?

This revision is now accepted and ready to land.Dec 18 2017, 8:09 PM

Thanks you very much for the very enlightening feedback.

I assume I shall apply the patch and push?

Thanks, but please hold on, because

I don't remember we do any isValid() check inside ktexteditor anyways ;)

made me search through the kate project code, and indeed, the few other
places .isValid() is called for an url
make more sense, like

if (url.isValid()) {
        KIO::DeleteJob *job = KIO::del(url);

Reading the QUrl doc helped too. It is mainly about encoding.
But the document url has been checked, on opening the document,
so how could it be invalid ?
So I do agree now with one of your first statements:
remove this block altogether.

http://doc.qt.io/qt-5/qurl.html#isLocalFile
is a bit puzzling, as samba files are considered "local", with an example there
http://doc.qt.io/qt-5/qurl.html#toLocalFile
but this is a corner case; the command might just fail (or not), no big deal.

Then I'm not sure we should really check for isLocalFile either.
Here is a potential use case :
myscript <file url>
where myscript would download the file url and run it locally.

Any objection to remove this one too ?
(Not all work lost, since it will be useful to improve KateConsole::slotSync)

About coding style, any standard for kate ?
In particular, line length ?

Thanks you very much for the very enlightening feedback.

I assume I shall apply the patch and push?

Thanks, but please hold on, because

I don't remember we do any isValid() check inside ktexteditor anyways ;)

made me search through the kate project code, and indeed, the few other
places .isValid() is called for an url
make more sense, like

if (url.isValid()) {
        KIO::DeleteJob *job = KIO::del(url);

Reading the QUrl doc helped too. It is mainly about encoding.
But the document url has been checked, on opening the document,
so how could it be invalid ?
So I do agree now with one of your first statements:
remove this block altogether.

http://doc.qt.io/qt-5/qurl.html#isLocalFile
is a bit puzzling, as samba files are considered "local", with an example there
http://doc.qt.io/qt-5/qurl.html#toLocalFile
but this is a corner case; the command might just fail (or not), no big deal.

Then I'm not sure we should really check for isLocalFile either.
Here is a potential use case :
myscript <file url>
where myscript would download the file url and run it locally.

Any objection to remove this one too ?
(Not all work lost, since it will be useful to improve KateConsole::slotSync)

If you would remove that, you would first need to write code that does the download via KIO and stuff.
I think that would be overkill, normally you want to work with a local file.
Just let that check be like it is.

About coding style, any standard for kate ?
In particular, line length ?

Normally 4 space indent, line length is not of interest.

If you would remove that, you would first need to write code that does the download via KIO and stuff.

That might be awesome.
But a simpler use case is when switching between few remote files,
knowing exactly where they are on the server, so that
a simple script can curl the right one (based on its name), and execute it.

But it is better to work locally, and synchronize after (less traffic).
So let's leave the isLocalFile check.
Thank you very much for the feedback.

About coding style, any standard for kate ?
In particular, line length ?

Normally 4 space indent, line length is not of interest.

Thanks for the info.
In this diff, let's stick to the 2 space indent already used for all the files of the konsole addon.

This evening I'll just remove the u.isValid() check.

Ok, thanks.
And please add yourself to the copyright headers, I will need full name + email anyways for the commit.
Thanks!

ederag updated this revision to Diff 24191.EditedDec 20 2017, 9:38 PM

Done, but running ultimate tests too quickly,
I hit several times F4 before noticing the 'not local file' error.
Then i had to wait for it to disappear, hit somewhere, wait for the error to appear, ...
Would it be possible to catch that the error is still shown (for the same file),
and allow us to do nothing until the error is fixed ?
Or simpler: a way to collapse identical error messages for the same view into one ?
To the minimum, how to allow the user to quickly remove this error message (a delete cross) ?

ederag updated this revision to Diff 24269.Dec 21 2017, 7:59 PM

After reading the doc, and a bit of source code, it does not seem feasible.
But setting a shorter time of 2s, and Immediate autohide
yield a much better behavior.

If someone come up with a better solution, I'm ready to continue,
otherwise, ready to go ?

If someone come up with a better solution, I'm ready to continue,

otherwise, ready to go ?
I think this is good enough ATM, lets see what people think of it.

For the commit, shall I push?
Actually, we want full names for authorship, I assume you have some first/lastname, too?

For the commit, shall I push?

OK, thanks !

Actually, we want full names for authorship, I assume you have some first/lastname, too?

This is my usual signature for free software contributions.
Hope it is fine with you.
But you can split it as Ede Rag if it solves a technical problem.

This revision was automatically updated to reflect the committed changes.

Thank you very much for the commit, and for the feedback that improved the patch a lot.
Here is the corresponding diff for SlotSync: D9484