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

0009814: add server console arrow command history #274

Merged
merged 12 commits into from Aug 29, 2018

Conversation

Projects
7 participants
@patrikjuvonen
Collaborator

patrikjuvonen commented Jul 28, 2018

Mantis Bug Tracker issue
9814

Summary

  • Add server console arrow history (up and down keys)
  • Does not save identical commands in a row (if you type refreshall 100 times in a row you will only get it once when you navigate)
  • CTRL+C combination behavior changes

CTRL+C key combination behavior change

  • Currently CTRL+C will shut down the server whenever key combination is pressed.
  • Now CTRL+C will shut down the server only if the input is empty. If input is not empty, it will output the current input as a new line and clear the input buffer.

Implement command history

  • Pressing the arrow up key will select the previous entry from the executed commands history. If currently selected history entry is at the start of the list, it will not do anything.
  • Pressing the arrow down key will select the next entry from the executed commands history. If currently selected history entry is at the end of the list, it will output whatever was left to the input buffer before selecting a command history entry.
  • Writing something in a command history entry will save the change to that entry.

Feel free to comment if there's anything else that I missed or something new that comes into mind.

patrikjuvonen added some commits Jul 28, 2018

@botder botder added the new feature label Jul 29, 2018

@qaisjp qaisjp added the enhancement label Jul 29, 2018

@qaisjp qaisjp added this to the 1.5.6 milestone Jul 31, 2018

@qaisjp

This comment has been minimized.

Show comment
Hide comment
@qaisjp

qaisjp Jul 31, 2018

Member

I've only tested on Windows. Code looks fine!

I would not expect the down arrow to wipe the command input (even though this is a quick and easy way to clear your input). Not sure if this should be changed.

A bug:

  • open server
  • make some space by holding enter
  • type "test1" and press enter
  • press up key (shows "test1")
  • press up again (shows empty line now)
  • type "hello" and press enter
  • press up key as many times as you like and discover you can't find "hello"

An additional feature that would be nice (but not required) is to be able to come back to your incomplete text. Example gif below (I'm sorry you can't see the arrow keys I'm pressing):

delete

Member

qaisjp commented Jul 31, 2018

I've only tested on Windows. Code looks fine!

I would not expect the down arrow to wipe the command input (even though this is a quick and easy way to clear your input). Not sure if this should be changed.

A bug:

  • open server
  • make some space by holding enter
  • type "test1" and press enter
  • press up key (shows "test1")
  • press up again (shows empty line now)
  • type "hello" and press enter
  • press up key as many times as you like and discover you can't find "hello"

An additional feature that would be nice (but not required) is to be able to come back to your incomplete text. Example gif below (I'm sorry you can't see the arrow keys I'm pressing):

delete

@patrikjuvonen

This comment has been minimized.

Show comment
Hide comment
@patrikjuvonen

patrikjuvonen Jul 31, 2018

Collaborator

Thank you for your review.

Yes, I understand what you mean. When I worked on this, I liked the fact that you can clear your input easily. However, I did also think about the feature you pointed out in the end. This could just be changed to work so that it won't clear the line but just continue where you left off. But in that case we should probably cancel the ability to wrap around the history and just stop in the end to make it clear where the history starts and ends.

I will look into the bug.

Collaborator

patrikjuvonen commented Jul 31, 2018

Thank you for your review.

Yes, I understand what you mean. When I worked on this, I liked the fact that you can clear your input easily. However, I did also think about the feature you pointed out in the end. This could just be changed to work so that it won't clear the line but just continue where you left off. But in that case we should probably cancel the ability to wrap around the history and just stop in the end to make it clear where the history starts and ends.

I will look into the bug.

@qaisjp qaisjp added this to In progress in release/v1.5.6 via automation Jul 31, 2018

@Citizen01

This comment has been minimized.

Show comment
Hide comment
@Citizen01

Citizen01 Jul 31, 2018

I'm use to hit Ctrl+C to clear/cancel what I am actually writing in consoles. I can clearly see me doing that and so ... closing the server too. Can you prevent Ctrl+C to shutdown the server if the current command input isn't empty ?

Anyone else doing that too ? If I'm the only one, I guess we can disregard what I just said :)

Citizen01 commented Jul 31, 2018

I'm use to hit Ctrl+C to clear/cancel what I am actually writing in consoles. I can clearly see me doing that and so ... closing the server too. Can you prevent Ctrl+C to shutdown the server if the current command input isn't empty ?

Anyone else doing that too ? If I'm the only one, I guess we can disregard what I just said :)

@patrikjuvonen

This comment has been minimized.

Show comment
Hide comment
@patrikjuvonen

patrikjuvonen Jul 31, 2018

Collaborator

In many command-line interface environments, control-C is used to abort the current task and regain user control.

I do like the idea of aborting a command when typing. Like you said, we can't do that right now, and it bugs me off to revert everything I've written instead of just easily aborting it.

I'll look into that as well.

Collaborator

patrikjuvonen commented Jul 31, 2018

In many command-line interface environments, control-C is used to abort the current task and regain user control.

I do like the idea of aborting a command when typing. Like you said, we can't do that right now, and it bugs me off to revert everything I've written instead of just easily aborting it.

I'll look into that as well.

@qaisjp

This comment has been minimized.

Show comment
Hide comment
@qaisjp

qaisjp Aug 1, 2018

Member

@Citizen01 I use ctrl+c too and I agree it would be a feature that is nice to have (but not required)

gif for the sake of gif

Member

qaisjp commented Aug 1, 2018

@Citizen01 I use ctrl+c too and I agree it would be a feature that is nice to have (but not required)

gif for the sake of gif

@ccw808

This comment has been minimized.

Show comment
Hide comment
@ccw808

ccw808 Aug 1, 2018

Member

What about removing the Ctrl-C shutdown shortcut?
Or make it Shift-Ctrl-C

Member

ccw808 commented Aug 1, 2018

What about removing the Ctrl-C shutdown shortcut?
Or make it Shift-Ctrl-C

@4O4

This comment has been minimized.

Show comment
Hide comment
@4O4

4O4 Aug 1, 2018

Contributor
Contributor

4O4 commented Aug 1, 2018

@ccw808

This comment has been minimized.

Show comment
Hide comment
@ccw808

ccw808 Aug 1, 2018

Member

In Windows at least, programs don't close when using Ctrl-C

Member

ccw808 commented Aug 1, 2018

In Windows at least, programs don't close when using Ctrl-C

@ccw808 ccw808 closed this Aug 1, 2018

@patrikjuvonen

This comment was marked as resolved.

Show comment
Hide comment
@patrikjuvonen

patrikjuvonen Aug 1, 2018

Collaborator

@ccw808 Did you close this PR accidentally? =)

Collaborator

patrikjuvonen commented Aug 1, 2018

@ccw808 Did you close this PR accidentally? =)

@ccw808 ccw808 reopened this Aug 1, 2018

@patrikjuvonen patrikjuvonen changed the title from 0009814: add server console arrow command history to WIP 0009814: add server console arrow command history Aug 1, 2018

@4O4

This comment has been minimized.

Show comment
Hide comment
@4O4

4O4 Aug 1, 2018

Contributor

In Windows at least, programs don't close when using Ctrl-C

This is not true.

  1. It's not really a matter of the OS, but the terminal or shell you're using. There are dozens of them available on Windows for a long time already, and now there are even more of them since the WSL came out.
  2. Even in powershell and cmd.exe jobs can be stopped with CTRL-C (implementation is a little bit different of course like everything on Windows).
Contributor

4O4 commented Aug 1, 2018

In Windows at least, programs don't close when using Ctrl-C

This is not true.

  1. It's not really a matter of the OS, but the terminal or shell you're using. There are dozens of them available on Windows for a long time already, and now there are even more of them since the WSL came out.
  2. Even in powershell and cmd.exe jobs can be stopped with CTRL-C (implementation is a little bit different of course like everything on Windows).
@4O4

This comment has been minimized.

Show comment
Hide comment
@4O4

4O4 Aug 1, 2018

Contributor

Or make it Shift-Ctrl-C

Sorry, I haven't seen this before because it was added later and I was responding to e-mail directly :)

This shortcut would be really awkward. but still better than no alternative at all.

Can you prevent Ctrl+C to shutdown the server if the current command input isn't empty ?

Somehow I also missed the second part of this sentence - sorry, I was answering half-asleep 😄 If this will be implemented this way then it would just take the double CTRL-C combo to send SIGINT (like in emacs). In this case this is totally fine by me :)

Contributor

4O4 commented Aug 1, 2018

Or make it Shift-Ctrl-C

Sorry, I haven't seen this before because it was added later and I was responding to e-mail directly :)

This shortcut would be really awkward. but still better than no alternative at all.

Can you prevent Ctrl+C to shutdown the server if the current command input isn't empty ?

Somehow I also missed the second part of this sentence - sorry, I was answering half-asleep 😄 If this will be implemented this way then it would just take the double CTRL-C combo to send SIGINT (like in emacs). In this case this is totally fine by me :)

@qaisjp

This comment has been minimized.

Show comment
Hide comment
@qaisjp

qaisjp Aug 1, 2018

Member
Member

qaisjp commented Aug 1, 2018

@patrikjuvonen

This comment has been minimized.

Show comment
Hide comment
@patrikjuvonen

patrikjuvonen Aug 1, 2018

Collaborator

This is how I am going to proceed (just to summarize this discussion):

CTRL+C key combination behavior change

  • Currently CTRL+C will shut down the server whenever key combination is pressed.
  • Now CTRL+C will shut down the server only if the input is empty. If input is not empty, it will output the current input as a new line and clear the input buffer.

Implement command history

  • Pressing the arrow up key will select the previous entry from the executed commands history. If currently selected history entry is at the start of the list, it will not do anything.
  • Pressing the arrow down key will select the next entry from the executed commands history. If currently selected history entry is at the end of the list, it will output whatever was left to the input buffer before selecting a command history entry.

Feel free to comment if there's anything else that I missed or something new that comes into mind.

Collaborator

patrikjuvonen commented Aug 1, 2018

This is how I am going to proceed (just to summarize this discussion):

CTRL+C key combination behavior change

  • Currently CTRL+C will shut down the server whenever key combination is pressed.
  • Now CTRL+C will shut down the server only if the input is empty. If input is not empty, it will output the current input as a new line and clear the input buffer.

Implement command history

  • Pressing the arrow up key will select the previous entry from the executed commands history. If currently selected history entry is at the start of the list, it will not do anything.
  • Pressing the arrow down key will select the next entry from the executed commands history. If currently selected history entry is at the end of the list, it will output whatever was left to the input buffer before selecting a command history entry.

Feel free to comment if there's anything else that I missed or something new that comes into mind.

@qaisjp

This comment has been minimized.

Show comment
Hide comment
@qaisjp

qaisjp Aug 1, 2018

Member

Sounds good to me 👍

Member

qaisjp commented Aug 1, 2018

Sounds good to me 👍

@Pirulax

This comment has been minimized.

Show comment
Hide comment
@Pirulax

Pirulax Aug 1, 2018

Contributor

I dont know actually.
What if I failed somehow to copy something?
Or what if I'm not sure that I have something on my clipboard?
Maybe using shift+c to shutdown the server(or completely removing binded shutdown) would be a better idea.

Contributor

Pirulax commented Aug 1, 2018

I dont know actually.
What if I failed somehow to copy something?
Or what if I'm not sure that I have something on my clipboard?
Maybe using shift+c to shutdown the server(or completely removing binded shutdown) would be a better idea.

@patrikjuvonen

This comment has been minimized.

Show comment
Hide comment
@patrikjuvonen

patrikjuvonen Aug 1, 2018

Collaborator

@Pirulax I didn't know you could copy or paste something in the server console?

Collaborator

patrikjuvonen commented Aug 1, 2018

@Pirulax I didn't know you could copy or paste something in the server console?

@Pirulax

This comment has been minimized.

Show comment
Hide comment
@Pirulax

Pirulax Aug 1, 2018

Contributor

Ohh, forget me 😂
I'm tired a little bit (as usual)

Contributor

Pirulax commented Aug 1, 2018

Ohh, forget me 😂
I'm tired a little bit (as usual)

@4O4

This comment has been minimized.

Show comment
Hide comment
@4O4

4O4 Aug 1, 2018

Contributor
Contributor

4O4 commented Aug 1, 2018

patrikjuvonen added some commits Aug 3, 2018

@patrikjuvonen patrikjuvonen changed the title from WIP 0009814: add server console arrow command history to 0009814: add server console arrow command history Aug 3, 2018

@patrikjuvonen patrikjuvonen requested a review from qaisjp Aug 3, 2018

@qaisjp

This comment has been minimized.

Show comment
Hide comment
@qaisjp

qaisjp Aug 10, 2018

Member

Should it be possible to edit old entries?

gif:

delete

Reproduce using:

  1. type "first", press enter
  2. type "second", press enter
  3. press up twice ("first" should be in the input)
  4. press "y" ("firsty" should be the input)
  5. press down key ("second" should be in the input)
  6. press up key (input now says "firsty", not "first")

Note that after stage four if you instead press enter, "first" will be in the history unchanged, and "firsty" will be pushed as a new history item (this is expected and normal behaviour)

Member

qaisjp commented Aug 10, 2018

Should it be possible to edit old entries?

gif:

delete

Reproduce using:

  1. type "first", press enter
  2. type "second", press enter
  3. press up twice ("first" should be in the input)
  4. press "y" ("firsty" should be the input)
  5. press down key ("second" should be in the input)
  6. press up key (input now says "firsty", not "first")

Note that after stage four if you instead press enter, "first" will be in the history unchanged, and "firsty" will be pushed as a new history item (this is expected and normal behaviour)

@patrikjuvonen

This comment has been minimized.

Show comment
Hide comment
@patrikjuvonen

patrikjuvonen Aug 10, 2018

Collaborator

That will require a small change in structure but I agree. Editing old entries is possible at least in Bash (will keep the edit there even if you navigate history), but it will clear changes when aborted or a new entry has been added. Perhaps we can implement it the same way.

Collaborator

patrikjuvonen commented Aug 10, 2018

That will require a small change in structure but I agree. Editing old entries is possible at least in Bash (will keep the edit there even if you navigate history), but it will clear changes when aborted or a new entry has been added. Perhaps we can implement it the same way.

@Citizen01

This comment has been minimized.

Show comment
Hide comment
@Citizen01

Citizen01 Aug 10, 2018

I don't see any good reason to be able to edit and save a command entry in the history. What is the use case ?

I think we should not save the modification made to a command so, using your example:

  1. type "first", press enter
  2. type "second", press enter
  3. press up twice ("first" should be in the input)
  4. press "y" ("firsty" should be the input)
  5. press down key ("second" should be in the input)
  6. press up key ("first" should be in the input)

Also:

  1. type "first", press enter
  2. type "second", press enter
  3. press up twice ("first" should be in the input)
  4. press "y" ("firsty" should be the input), press enter
  5. press up key ("firsty" should be in the input)
  6. press up key ("second" should be in the input)
  7. press up key ("first" should be in the input)

Citizen01 commented Aug 10, 2018

I don't see any good reason to be able to edit and save a command entry in the history. What is the use case ?

I think we should not save the modification made to a command so, using your example:

  1. type "first", press enter
  2. type "second", press enter
  3. press up twice ("first" should be in the input)
  4. press "y" ("firsty" should be the input)
  5. press down key ("second" should be in the input)
  6. press up key ("first" should be in the input)

Also:

  1. type "first", press enter
  2. type "second", press enter
  3. press up twice ("first" should be in the input)
  4. press "y" ("firsty" should be the input), press enter
  5. press up key ("firsty" should be in the input)
  6. press up key ("second" should be in the input)
  7. press up key ("first" should be in the input)
@patrikjuvonen

This comment has been minimized.

Show comment
Hide comment
@patrikjuvonen

patrikjuvonen Aug 11, 2018

Collaborator

I believe your second example is already the case.

I am not sure about the first one. That's not how Bash shell works for example but we can remove the save if that is what people believe is the way to go. I don't see any negative sides to it considering my previous comment:

Editing old entries is possible at least in Bash (will keep the edit there even if you navigate history), but it will clear changes when aborted or a new entry has been added. Perhaps we can implement it the same way.

Collaborator

patrikjuvonen commented Aug 11, 2018

I believe your second example is already the case.

I am not sure about the first one. That's not how Bash shell works for example but we can remove the save if that is what people believe is the way to go. I don't see any negative sides to it considering my previous comment:

Editing old entries is possible at least in Bash (will keep the edit there even if you navigate history), but it will clear changes when aborted or a new entry has been added. Perhaps we can implement it the same way.

@qaisjp

This comment has been minimized.

Show comment
Hide comment
@qaisjp

qaisjp Aug 11, 2018

Member
Member

qaisjp commented Aug 11, 2018

@Citizen01

This comment has been minimized.

Show comment
Hide comment
@Citizen01

Citizen01 Aug 11, 2018

I believe your second example is already the case.

Hmmm the gif was not showing that as 'first' got totally replaced by 'firsty' in the history after submitting 'third'. But if that is what you were about to implement then it's fine.

For my first example, just forget it, I never moved into the history while editing a command, but I can clearly see good scenarios where it can be usefull to keep the change until enter/clear like qaisjp said.

So now I agree with you guys.

Citizen01 commented Aug 11, 2018

I believe your second example is already the case.

Hmmm the gif was not showing that as 'first' got totally replaced by 'firsty' in the history after submitting 'third'. But if that is what you were about to implement then it's fine.

For my first example, just forget it, I never moved into the history while editing a command, but I can clearly see good scenarios where it can be usefull to keep the change until enter/clear like qaisjp said.

So now I agree with you guys.

@patrikjuvonen patrikjuvonen changed the title from 0009814: add server console arrow command history to WIP: 0009814: add server console arrow command history Aug 24, 2018

qaisjp and others added some commits Aug 29, 2018

@patrikjuvonen

This comment has been minimized.

Show comment
Hide comment
@patrikjuvonen

patrikjuvonen Aug 29, 2018

Collaborator

Finally had a tiny bit of time to finish the last bit! Let me know if there's anything @qaisjp

Collaborator

patrikjuvonen commented Aug 29, 2018

Finally had a tiny bit of time to finish the last bit! Let me know if there's anything @qaisjp

@patrikjuvonen patrikjuvonen changed the title from WIP: 0009814: add server console arrow command history to 0009814: add server console arrow command history Aug 29, 2018

release/v1.5.6 automation moved this from In progress to Ready Aug 29, 2018

@qaisjp

qaisjp approved these changes Aug 29, 2018

Excellent work. Thank you!

@qaisjp qaisjp merged commit bf2cf53 into multitheftauto:master Aug 29, 2018

3 checks passed

WIP ready for review
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

release/v1.5.6 automation moved this from Ready to Done Aug 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment