Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add commands to insert/run selected text #23

Merged
merged 8 commits into from
May 24, 2020
Merged

Conversation

UziTech
Copy link
Member

@UziTech UziTech commented Mar 6, 2020

Add two commands

  • x-terminal:insert-selected-text
  • x-terminal:run-selected-text

The text comes from the selected text in a text editor or the line that the cursor is on if no text is selected.

The active terminal is the last focused terminal.

fixes #20

TODO:

  • add keymaps
  • add tests
  • update docs

@UziTech UziTech added enhancement ⚙️ Improvements or feature being added W.I.P. 🚧 In progress and not ready for merging labels Mar 6, 2020
@UziTech
Copy link
Member Author

UziTech commented Mar 6, 2020

I'm not sure what to do if there are multiple cursors or selections, currently it uses the last cursor or selection.

The active terminal is just the last focused but it stays active even when the dock is hidden. I'm not sure of the best way to choose which terminal should be active if the current active terminal is closed or hidden.

@Kabouik @the-j0k3r any ideas?

@the-j0k3r
Copy link
Member

the-j0k3r commented Mar 7, 2020

@the-j0k3r any ideas?

I'm not sure what to do if there are multiple cursors or selections, currently it uses the last cursor or selection.

Yes that's it same as terminusl, no need to over complicate things the terminal can only run one command at any time.

If you really want more multiple cursors/selections that would probably have to be from top to bottom order. Terminal can only run one selection at a time anyway. Better not complicate things.

The active terminal is just the last focused but it stays active even when the dock is hidden. I'm not sure of the best way to choose which terminal should be active if the current active terminal is closed or hidden.

Well, here I'm unsure, terminus maps to folders or files, so if you have multiple terminals + associated folders and files to each terminal it will probably use the terminal associated with that folder or file.

https://github.com/bus-stop/terminus#insert-selected-text seems to indicate that https://github.com/bus-stop/terminus#map-terminal-to

Edit:

Currently you cant visualise which terminal belongs where anyway, I think that in order to be able to see what terminal is being used, it would be made easy by #9 so user is not running the commands in wrong terminal.

I'm not sure if we need to also support mapping to folder/file in order to make this foll proof though.

Thoughts on these?

@@ -251,7 +251,6 @@ class XTerminalSingleton {
if (cursor) {
const line = editor.lineTextForBufferRow(cursor.row)
selectedText = line
editor.moveDown(1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not the way it works in terminus see https://github.com/bus-stop/terminus#insert-selected-text

the last two at cursor position, the cursor shifts down to the next line, this way you can run any commands in sequence, by disabling this you remove that ability.

generic

with is better

Copy link
Member Author

@UziTech UziTech Mar 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw that but as I was testing it out I realized I didn't usually want it to move to the next line. I figured pressing Ctrl + Shift + i just to insert and Ctrl + Shift + i + Down Arrow to insert and move to the next line works better.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about making it optional, default on? SO it suits both

Copy link
Member Author

@UziTech UziTech Mar 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like 1 extra key press isn't worth a setting. What is a situation where someone can't, or wouldn't want to, press the down key to go to the next line?

Copy link
Member

@the-j0k3r the-j0k3r Mar 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like I said, running commands in a sequence is faster than having to move down the cursor manually to next line. To me not only is faster but more efficient. Adding intermediary steps adds time if the steps are more than one.

We can make that behavior moving down automatically optional if you prefer to move down the cursor yourself and have that set as a default behavior.

Ide be OK with that.

IDK how you can say

Ctrl + Shift + i + Down Arrow to insert and move to the next line works better.

left hand ctrl + shift + i
right hand on arrow

= my left wrist hurts

left hand ctrl + shift

right hand i and down arrow is awkward to me.

Also sausage fingers none of that helps

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it doesnt, I never said it did

#23 (comment)

So you think the toggle should be on whether to insert or run the selection like terminus?

I still think more commands and different keymaps (Ctrl + Alt + r and Ctrl + Alt + i) would be more useful.

the length of this discussion and the impassable impasse is disproportionate to the importance of the feature.

That may be true, but the discussion will be much easier to have now than in the future when someone wants to change the default.

Copy link

@Kabouik Kabouik Mar 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about a toggle in the settings that enables "auto-move down" commands and in fact just assigns the two extra commands to new default keybindings instead of replacing the no-move down behavior? Best of both worlds. Of course, that's more keybindings, but only for those who want it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about a toggle in the settings that enables "auto-move down" commands

We already discussed the toggle.#23 (comment) and besides see #23 (comment)

Copy link

@Kabouik Kabouik Mar 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The toggle discussed so far was to enable the move-down commands instead of no-move-down-commands. My suggestion was to have all commands in there (and in the command palette) already, but only map the move-down ones to keybindings if they are toggled on in the options. This way:

  • users who don't need them don't enable them;
  • users who need them but don't want to edit a keybinding file (which is admittedly a bit cumbersome in Atom) can just see and set everything they need from x-terminal settings, no risks of missing the feature and the toggle setting would be compatible with profiles
  • users who need move-down commands but do not want to replace the no-move-down ones can have them all bound to keybinds, concurrently

This would mean @UziTech would still need to code a toggle, which he may think is overkill, but otherwise I believe that would clear all the limitations discussed above. I agree it is a bit complicated compared to having just 4 commands and 4 keybindings already mapped (which I think is the most parsimonious, but won't satisfy those who like the toggle approach).

[Edit] Forget it, it doesn't save the clutter of extra keybinds if one enables the move-down commands, which beats the point of a toggle. This would only facilitate mapping the keys since it would be from the settings and not from the keybinds file, I don't think it's worth it.

Copy link
Member

@the-j0k3r the-j0k3r Mar 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

equally solved by the toggle alone, the extra anything is just ott

also I proposed what youre proposing in #23 (comment)

If you want add the toggle and the hidden knobs I'de be fine with that too (however unlikely that seems to me). The opposite is true for me with just the hidden knobs implementation I dont find this way alone sufficiently simple.

So we just going in circles with the discussion

If its a matter of implementing a all cases scenario you dont need extra keymaps necessarily, you just need a robust implementation that accounts for all possible scenarios without complicating setup on the frontend while making it available to default profile and any created profiles.

You could have instead of a toggle a dropdown menu with options

Title: Enable Cursor to move down automatically when you:
option 1 Execute x-terminal:run-selected-text
option 2 Execute x-terminal:insert-selected-text
option 3 On a per line basis (x-terminal:cursor-move-to-next-line) <- this option is really not needed see option 4)
option 4 Never (This means manually only via cursor keys)

I feel that design is simple enough on frontend for all users or all expertise level and is no more demanding on the backend implementation. I dont think we even need option 3 at all never really implies manually and thats all its need to account for most edge cases)

Its a possible design, and note that usability is important, meaning how any user can configure this without special knowledge of the underpinnings of the feature, nothing should be available as hidden knobs to regular users, unless you have a settings design that has two levels standard user/advanced user so you have advanced options with hidden knobs of sorts visible when you enable advanced user mode.

I dont think we need settings levels though, we just need a simple way to present the options to users that account for the majority of use cases for that given feature(it could be any feature like this one or any other). It needs to be K.I.S.S. is all Im saying

So far the hidden knobs of sorts are incompatible with how any user can configure this without special knowledge of the underpinnings of the feature

https://www.nngroup.com/articles/usability-101-introduction-to-usability/

Usability is defined by 5 quality components:

Learnability: How easy is it for users to accomplish basic tasks the first time they encounter the design?
Efficiency: Once users have learned the design, how quickly can they perform tasks?
Memorability: When users return to the design after a period of not using it, how easily can they reestablish proficiency?
Errors: How many errors do users make, how severe are these errors, and how easily can they recover from the errors?
Satisfaction: How pleasant is it to use the design?

Based on this I disagree with any proposal that makes this process convoluted by design. Not only this isnt going to be a everyday feature but any options should be implemented following basic usability requirements not just suited to any particular feature or option.

@the-j0k3r
Copy link
Member

keymaps added 61cef6c

@Kabouik
Copy link

Kabouik commented Mar 9, 2020

I agree with @the-j0k3r, this could be kept simple by sending multiple selections as just one chunk of text with selections ordered from top to bottom. It would be necessary to support sending multi-line selections (but single selections) anyway. I suppose this requires properly recognizing ends of lines so that individual commands in one chunk are all run one after the other.

Multi selections or multi cursor positons could be handled the same way as chunks of texts, i.e., corresponding text is executed one selection after another, but I'm not sure what to do when a code line is partially selected without its end of line (should the terminal assume it needs to emulate an end of line, or should it assume the partial selection prepends the next partial selection?).

In any case, I think ignoring multiple selections/cursors (only first line/cursor is considered) would be fine as well, at least for a start.

I think it's fine to keep the hidden terminal as the default one if (1) there is no other terminal, or (2) it gained focus before. I don't really see a use case for sending commands from the same edited file to another terminal without focusing it before (but I might be missing some).

Shouldn't keybinds be based on r instead of i, for run? I think most other terminals use variations of either r or return.

Thanks working on this feature so quickly!

@the-j0k3r
Copy link
Member

the-j0k3r commented Mar 9, 2020

Shouldn't keybinds be based on r instead of i, for run? I think most other terminals use variations of either r or return.

Sure if there's no conflict with Atom, but tbh if you pick on that then you will pick on the existing keybinds for terminal opening.

Also here I used what terminus has set, because Im used to them and there's no conflicts with Atom.

I dont mind it, its not that straightforward to find non conflicting keybinds.

@Kabouik
Copy link

Kabouik commented Mar 9, 2020

I don't mind either, I was just asking but I'm fine with x-terminal defining my Atom standard and habits (which it sure will once this PR is merged)!

Copy link
Member

@the-j0k3r the-j0k3r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make optional the behavior of moving cursor down like terminus but without inserting eol chars

@the-j0k3r
Copy link
Member

@Kabouik changed the keybinds 4441a9d makes sense and no conflicts with Atom

@Kabouik
Copy link

Kabouik commented Mar 9, 2020

Not being a developer, I'm embarrassed to say I'm having trouble to test this PR, as I am not comfortable enough with the git workflow. Here's what I did:

apm uninstall x-terminal
git clone https://github.com/bus-stop/x-terminal.git ~/.config/atom-x-terminal
cd ~/.config/atom-x-terminal
git fetch origin pull/23/head:23
git checkout insert-selected-text
git pull
npm install
apm rebuild

Is that correct? How do I apm install from the local folder now?

@UziTech
Copy link
Member Author

UziTech commented Mar 9, 2020

@Kabouik you can run apm link to install a package from local folder.

@the-j0k3r
Copy link
Member

Inside the local folder run apm link the (re)open Atom

@the-j0k3r

This comment has been minimized.

@UziTech
Copy link
Member Author

UziTech commented Mar 9, 2020

The only issue I think we still need to resolve is how to indicate the active terminal.

I have been playing around with a few different ways to do it without associating a terminal with an editor and I think just keeping a list of most recently focused terminals should work.

When a terminal becomes focused it will move to the top of the list and when running/inserting text x-terminal will look down the list for the last focused terminal that is visible.

Should we indicate which terminal is active by an asterisk (*) in the title or something like that?

@Kabouik
Copy link

Kabouik commented Mar 10, 2020

Should we indicate which terminal is active by an asterisk (*) in the title or something like that?

Sounds pretty good to me, clear and minimal. Perhaps it wouldn't be completely straightforward at first for users who never heard about the feature, but it should be self-explanatory relatively quickly when using multiple instances of x-terminal and seeing that commands always end up in the marked terminal.

@UziTech
Copy link
Member Author

UziTech commented Mar 13, 2020

I added the activeIndex to keep track of the terminals active order so if a terminal gets hidden the last active terminal that is still visible will become the current active terminal.

I also added the asterisk to the title of the active terminal.

The selected text will only run if a terminal is visible.

Do we want to allow running commands in a terminal where the dock is visible but the terminal is not the active item in the dock?

@the-j0k3r
Copy link
Member

the-j0k3r commented Mar 13, 2020

Hi there, good day ;) Ive rebased this on master to be able to test this ontop.

Do we want to allow running commands in a terminal where the dock is visible but the terminal is not the active item in the dock?

Here is where I mutter about lack of support for mapping terminals to folder or file like the infamous terminus.

If x-terminal supported such file or folder mapping to terminal then the associated terminal with that file or folder (you can only configure either map terminal to file or folder not both) then I would say the answer to your question is yes, youde associate the terminal mapped to that file or folder and run whatever using is running in said terminal.

Else IDK, what consequences there are of running the commands to just any terminal that happens to be there focused or not, you could have terminals open for other projects, I cant say Ive tried, Im not sure what consequences there would be if youre running potentially destructive commands in a terminal thats just happened to be the next one in line to be active. Maybe I'm over-complicating.

Security concerns.

Without making sure the commands are not going to be run in a terminal that you could have used to login somewhere and have entered credentials and is somewhere in there that this could be a security risk also since you could potentially be running commands from an unsecured or unrelated file into that terminal without realizing it.

If that happens ouch and if it can be exploited double ouch.

other concerns

This has still not been addressed, its a reply to an earlier question of yours.

#23 (comment)

@@ -253,6 +253,7 @@ class XTerminalSingleton {
if (cursor) {
const line = editor.lineTextForBufferRow(cursor.row)
selectedText = line
editor.moveDown(1)
Copy link
Member

@the-j0k3r the-j0k3r Mar 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see youve restored the cursor auto move down, while for me its good, I still think the better design is to make that behavior optionally available.

run selected text is ok for moving down, but just insert text could possibly be not desired or could be desired depending on scenario.

#23 (comment) if you read that and down for previous discussion on this

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya I figured we could pickup that discussion after figuring every thing else out. For now we can assume it will work the same way as other terminals.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@UziTech
Copy link
Member Author

UziTech commented Mar 13, 2020

Security concerns

The user is the one choosing which commands to run and which terminal to run them in so there is nothing we can do to prevent them from making bad choices. That is one reason I want to find a way to make sure the user knows which terminal the commands will be run in and still allow them to have multiple terminals open.

If you are worried about a third party package automatically running commands. This PR doesn't do anything a third party package couldn't monkey patch so it is no less secure with this PR than without it.

@the-j0k3r
Copy link
Member

The user is the one choosing which commands to run and which terminal to run them in so there is nothing we can do to prevent them from making bad choices. That is one reason I want to find a way to make sure the user knows which terminal the commands will be run in and still allow them to have multiple terminals open.

Yes but you have to ensure the terminal that commands are going to run belong to the associtaed file or folder, else you can just push the commands into the wrong terminal.

If you are worried about a third party package automatically running commands. This PR doesn't do anything a third party package couldn't monkey patch so it is no less secure with this PR than without it.

I dont care or worry what 3rd party can do here, The system as I understand does not support mapping to file or folder so you are assuming that the terminals open are safe and while we cant stop people from making bad choices we can ensure that we arent going to cause some issues by running commands in wrong terminal.

@UziTech
Copy link
Member Author

UziTech commented Mar 13, 2020

If x-terminal supported such file or folder mapping to terminal then the associated terminal with that file or folder (you can only configure either map terminal to file or folder not both) then I would say the answer to your question is yes, youde associate the terminal mapped to that file or folder and run whatever using is running in said terminal.

I don't think this would solve our problem because x-terminal is meant to allow users to have multiple terminals open in different dock and panes.

While terminus has support for opening in docks, it is not made for that purpose (and doesn't work very well when it is).

I was just testing out terminus to see how it's insert-text works when multiple terminals are open in different docks and it looks like it just goes by whichever terminal was last focused even when that dock is closed and even when terminals are mapped to a file (probably a bug). This is why we have these discussions now because some people have probably come to rely on that bug and it will be much harder to fix it now than have the lengthy discussion to find it before the feature is implemented.

This feature needs to work well in the following situations:

  • One terminal in an open dock as the active item (probably most common)

    • Obviously that terminal should be the active terminal
    • image
  • One terminal in a closed dock as active item

    • No active terminals or that one active?
    • image
  • One terminal in an open dock as non-active item

    • No active terminals or that one active?
    • image
  • Multiple terminals open in one open dock same pane one as active item

    • Active item should be active terminal
    • image
  • Multiple terminals open in one open dock different panes both as active items

    • last focused?
    • image
  • Multiple terminals open in multiple open docks as the active item

    • last focused?
    • image
  • Multiple terminals open one in dock one in center both active items

    • last focused?
    • image
  • One terminal open in the center in a new tab active item

    • Obviously that terminal should be the active terminal
    • image
  • One terminal open in the center in a new tab non-active item

    • No active terminals or that one active?
    • image
  • Multiple terminals open in the center in new tabs non-active items

    • No active terminals or last focused?
    • image
  • Multiple terminals open in the center in different panes all active items

    • last focused?
    • image

@UziTech
Copy link
Member Author

UziTech commented Mar 13, 2020

we cant stop people from making bad choices we can ensure that we arent going to cause some issues by running commands in wrong terminal.

How can it be the wrong terminal if they are the one who is choosing which terminal to run the commands?

We just have to make sure they know which terminal they are choosing to run the commands in, hence the asterisk (or some other method of showing them which terminal is active).

@UziTech
Copy link
Member Author

UziTech commented Mar 13, 2020

I was looking for a way to tell if a terminal has a running command in it but I don't think there is a way to do that.

@UziTech UziTech marked this pull request as ready for review May 15, 2020 07:20
@UziTech UziTech removed the W.I.P. 🚧 In progress and not ready for merging label May 15, 2020
@UziTech
Copy link
Member Author

UziTech commented May 15, 2020

@Kabouik @the-j0k3r I think this is complete. If you guys approve I will merge this.

@the-j0k3r
Copy link
Member

@Kabouik @the-j0k3r I think this is complete. If you guys approve I will merge this.

Theres no solution to decide if the cursor will move down by user pref or not.

@UziTech
Copy link
Member Author

UziTech commented May 15, 2020

The cursor does move down. I figured we should make it similar to other terminals to start out with and if someone wants a setting or more commands we can add them after this is merged.

@Kabouik
Copy link

Kabouik commented May 18, 2020

I just tested it, sorry for the delay. Seems pretty good to me. I like that you opted for moving down as default behaviour. Thanks a lot for your hard work on this feature.

Have you noticed that if you select a line and then use insert, it will actually run it? I have no problem with it, but I don't know if that is intentional since it is redundant with the run command. Likewise, if you select a line and then use run, it will run it but will also add two empty lines in the terminal, which is not the case when the line is not selected.

@UziTech
Copy link
Member Author

UziTech commented May 18, 2020

@Kabouik what os and shell are you using? I just tested it out on Windows in Powershell and it is working as it should. Can you share the file you are running the commands from?

git-bash

@Kabouik
Copy link

Kabouik commented May 18, 2020

This is Linux (Solus distribution) and I experienced it using a R script in a R console, but I tried with bash as you did and I get the same behaviour.

It turns out there is no issue if I carefully select the text in the script using the cursor, but if I select the full line with triple click, it will catch the EOL too and send an extra return. Unfortunately it's not in the video below, but at least you'll see what happens with lines selected using triple click:

  1. run with no prior line selection,
  2. then run on selected lines,
  3. then insert with no prior line selection and manual return in the terminal to confirm (note that I messed up the keybinding on the second line, so nothing happened, sorry),
  4. finally, insert on selected lines.

x-terminal_crop

I'm sorry about the quality, but see the different output when executing the last line of the script (with no other lines below it), probably because the EOL is different.

@UziTech
Copy link
Member Author

UziTech commented May 19, 2020

Should the selection be trimmed?

@UziTech
Copy link
Member Author

UziTech commented May 19, 2020

or maybe just \r and \n removed from the end of the selection?

@Kabouik
Copy link

Kabouik commented May 19, 2020

I guess it would make more sense than having extra new line when using run or having insert doing the same as run. What do you think? One could argue that the EOL is part of the line too, but having inconsistent behaviours depending on how the line was selected is confusing. I admit I don't remember how it is dealt with in other terminals.

@the-j0k3r
Copy link
Member

I guess it would make more sense than having extra new line when using run or having insert doing the same as run. What do you think? One could argue that the EOL is part of the line too, but having inconsistent behaviours depending on how the line was selected is confusing. I admit I don't remember how it is dealt with in other terminals.

Im not sure how making run the same as insert solves anything, they both do separate things.

@Kabouik
Copy link

Kabouik commented May 19, 2020

But I didn't suggest making run the same as insert. The issue is the EOL is caught when triple-clicking on a line to select it. Consequently, when doing that on any line but the last one in the script, insert actually behave like run because it will insert Return too, and run will insert an extra Return. run and insert should still do distinct things if \n and \r are not included in lines selected using triple click.

@the-j0k3r
Copy link
Member

the-j0k3r commented May 19, 2020

triple clicking issues aside, you dont need to triple click, you shouldn't even have to select, just placing the cursor on the desired line should be enough to run or insert that line.

@UziTech
Copy link
Member Author

UziTech commented May 19, 2020

When the user selects "selection" than insert will insert "selection" into the terminal and run will insert "selection\n". run essentially just adds a new line to the end of the selection to tell the terminal to run it.

The problem:

When the user selects "selection\n" than insert will insert "selection\n" into the terminal and run will insert "selection\n\n".

Possible solutions:

  1. leave it as is, since the user might want to extra \n's
  2. trim \r and \n from the end of the selection, because there is no reason for that to be in the selection

@Kabouik
Copy link

Kabouik commented May 19, 2020

triple clicking issues aside, you dont need to triple click, you shouldn't even have to select, just placing the cursor on the desired line should be enough to run or insert that line.

You're right, I don't see any actual use for full line selection. Partial selections are useful though, as detailed by UziTech just above. The issue might arise though for multiline selections, and those have a use.

I originally preferred solution (2) because I believe anyone explicitly needing extra \n would add them to their script in the first place, not count on the EOL when selecting a line. But then I'm afraid trimming would be pretty bad with multiline selection, no?

@UziTech
Copy link
Member Author

UziTech commented May 19, 2020

I don't think we should remove newlines in the middle of the selection, just at the very end. For example insert with "line1\nline2\n" selected would insert "line1\nline2".

@Kabouik
Copy link

Kabouik commented May 19, 2020

That would work then!

@UziTech
Copy link
Member Author

UziTech commented May 23, 2020

@the-j0k3r does this look good to merge?

@the-j0k3r
Copy link
Member

I guess.

@UziTech UziTech changed the title Insert/run selected text feat: Add commands to insert/run selected text May 24, 2020
@UziTech UziTech merged commit c10301a into master May 24, 2020
@UziTech UziTech deleted the insert-selected-text branch May 24, 2020 02:22
@github-actions
Copy link

🎉 This PR is included in version 8.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions bot added the released 📮 Release has been made label May 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement ⚙️ Improvements or feature being added released 📮 Release has been made
Development

Successfully merging this pull request may close these issues.

[FR] Send current line to terminal and move cursor to next line
3 participants