-
Notifications
You must be signed in to change notification settings - Fork 9
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
Conversation
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? |
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.
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? |
src/x-terminal.js
Outdated
@@ -251,7 +251,6 @@ class XTerminalSingleton { | |||
if (cursor) { | |||
const line = editor.lineTextForBufferRow(cursor.row) | |||
selectedText = line | |||
editor.moveDown(1) |
There was a problem hiding this comment.
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.
with is better
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
keymaps added 61cef6c |
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 Thanks working on this feature so quickly! |
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. |
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)! |
There was a problem hiding this 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
61cef6c
to
4441a9d
Compare
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:
Is that correct? How do I |
@Kabouik you can run |
Inside the local folder run |
This comment has been minimized.
This comment has been minimized.
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 ( |
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. |
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? |
Hi there, good day ;) Ive rebased this on master to be able to test this ontop.
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 concernsThis has still not been addressed, its a reply to an earlier question of yours. |
8bd980e
to
7bf5dbd
Compare
@@ -253,6 +253,7 @@ class XTerminalSingleton { | |||
if (cursor) { | |||
const line = editor.lineTextForBufferRow(cursor.row) | |||
selectedText = line | |||
editor.moveDown(1) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its here #23 (comment) and #23 (comment)
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. |
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.
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. |
I don't think this would solve our problem because 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 This feature needs to work well in the following situations:
|
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). |
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. |
75f11f8
to
9ba6809
Compare
@Kabouik @the-j0k3r I think this is complete. If you guys approve I will merge this. |
f90435d
to
2e62f04
Compare
Theres no solution to decide if the cursor will move down by user pref or not. |
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. |
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 |
@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? |
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
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. |
Should the selection be trimmed? |
or maybe just |
I guess it would make more sense than having extra new line when using |
Im not sure how making run the same as insert solves anything, they both do separate things. |
But I didn't suggest making |
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. |
When the user selects "selection" than The problem: When the user selects "selection\n" than Possible solutions:
|
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 |
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". |
That would work then! |
@the-j0k3r does this look good to merge? |
I guess. |
🎉 This PR is included in version 8.2.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
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: