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

Core: CommonClient: command history and echo #3236

Merged

Conversation

MatthewMarinets
Copy link
Contributor

@MatthewMarinets MatthewMarinets commented Apr 30, 2024

What is this fixing or adding?

Client command history

  • When executing a command (ie enter a message starting with / or !) in the common client, the command gets saved to a history buffer.
  • Pressing "up" fills in the text input with the previous command, up to a maximum of 50 commands
  • Pressing "down" returns to newer commands
  • Pressing "down" from the last executed command returns to the empty command (ie clears the text prompt)
  • Pressing "down" when on the empty command doesn't clear history (ie this will only clear your text if you've already been scrolling through history)
  • Executing a new command adds to the history and resets the history index to the empty command

Client command echo

  • Executing a command (ie enter a message starting with / or !) in the common client will echo the command back to you
  • The printout is handled by .on_ui_command() on CommonClient, so game-specific clients can override this behaviour
  • I chose to colour these commands for some extra visual clarity, introduced orange to the base colours to represent commands.

Note commands entered in the GUI client don't appear echoed in the command-line client and vice versa.

How was this tested?

I opened the Starcraft 2 client with these changes and tested manually -- entering various commands, going through history, verifying echo manually. GUI tests unfortunately aren't really unit-testable most of the time :/

If this makes graphical changes, please attach screenshots.

image
image

ap_client_history_2

@github-actions github-actions bot added affects: core Issues/PRs that touch core and may need additional validation. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Apr 30, 2024
@MatthewMarinets MatthewMarinets changed the title Client command history and echo Core: CommonClient: Client command history and echo Apr 30, 2024
@MatthewMarinets MatthewMarinets changed the title Core: CommonClient: Client command history and echo Core: CommonClient: command history and echo Apr 30, 2024
Copy link
Contributor

@Salzkorn Salzkorn left a comment

Choose a reason for hiding this comment

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

LGTM, though I'll point out the two typing imports. Seems fine to keep the PR scope focused, but looks a little unclean.

kvui.py Outdated Show resolved Hide resolved
kvui.py Outdated Show resolved Hide resolved
@Ziktofel Ziktofel added the is: enhancement Issues requesting new features or pull requests implementing new features. label May 1, 2024
kvui.py Outdated Show resolved Hide resolved
kvui.py Outdated Show resolved Hide resolved
@beauxq
Copy link
Collaborator

beauxq commented May 2, 2024

I'm not sure about the decision to only keep commands.

I think my intuition would expect everything I type to be in the history.

I would be interested to hear what others think of that.

@MatthewMarinets
Copy link
Contributor Author

MatthewMarinets commented May 2, 2024

I'm not sure about the decision to only keep commands.

I think my intuition would expect everything I type to be in the history.

I would be interested to hear what others think of that.

My concern would be that recording chat messages would just promote spam. I don't really use the chat messages often in Archipelago, but in general with chat apps I don't see the need to send the same or very similar messages repeatedly in quick succession. In contrast, I've very often found the need to rerun / go back to / adjust a prior command, especially for commands that take arguments that must be spelled correctly.

With respect to echo, I also see no point in echoing that. If we're only echoing commands, we already have a precedent for commands and chat messages being treated differently by the client.

@Ziktofel Ziktofel added waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer. and removed waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels May 16, 2024
kvui.py Show resolved Hide resolved
kvui.py Outdated Show resolved Hide resolved
@MatthewMarinets MatthewMarinets force-pushed the mm/client_cmd_history_and_echo branch from 8cea0b2 to b900c78 Compare May 17, 2024 20:30
@MatthewMarinets MatthewMarinets force-pushed the mm/client_cmd_history_and_echo branch from b900c78 to 382cf02 Compare June 9, 2024 01:50
@MatthewMarinets
Copy link
Contributor Author

Rebased onto latest from main, did a quick sanity test. Everything still seems to work.
echo_history_demo

@NewSoupVi NewSoupVi merged commit 2198a70 into ArchipelagoMW:main Jun 9, 2024
17 checks passed
agilbert1412 pushed a commit to agilbert1412/Archipelago that referenced this pull request Jun 13, 2024
* client: Added command history access with up/down and command echo in common client

* client: Changed command echo colour to orange

* client: removed star import from typing

* client: updated code style to match style guideline

* client: adjusted ordering of calling parent constructor in command prompt input constructor

* client: Fixed issues identified by beauxq in PR; fixed some typing issues

* client: PR comments; replaced command history list with deque
jnschurig pushed a commit to Tranquilite0/Archipelago-SoulBlazer that referenced this pull request Jun 13, 2024
* client: Added command history access with up/down and command echo in common client

* client: Changed command echo colour to orange

* client: removed star import from typing

* client: updated code style to match style guideline

* client: adjusted ordering of calling parent constructor in command prompt input constructor

* client: Fixed issues identified by beauxq in PR; fixed some typing issues

* client: PR comments; replaced command history list with deque
@MatthewMarinets MatthewMarinets deleted the mm/client_cmd_history_and_echo branch June 16, 2024 20:07
sflavelle pushed a commit to sflavelle/Archipelago-tgc that referenced this pull request Jun 20, 2024
* client: Added command history access with up/down and command echo in common client

* client: Changed command echo colour to orange

* client: removed star import from typing

* client: updated code style to match style guideline

* client: adjusted ordering of calling parent constructor in command prompt input constructor

* client: Fixed issues identified by beauxq in PR; fixed some typing issues

* client: PR comments; replaced command history list with deque
qwint pushed a commit to qwint/Archipelago that referenced this pull request Jun 24, 2024
* client: Added command history access with up/down and command echo in common client

* client: Changed command echo colour to orange

* client: removed star import from typing

* client: updated code style to match style guideline

* client: adjusted ordering of calling parent constructor in command prompt input constructor

* client: Fixed issues identified by beauxq in PR; fixed some typing issues

* client: PR comments; replaced command history list with deque
AustinSumigray pushed a commit to AustinSumigray/Archipelago that referenced this pull request Jan 4, 2025
* client: Added command history access with up/down and command echo in common client

* client: Changed command echo colour to orange

* client: removed star import from typing

* client: updated code style to match style guideline

* client: adjusted ordering of calling parent constructor in command prompt input constructor

* client: Fixed issues identified by beauxq in PR; fixed some typing issues

* client: PR comments; replaced command history list with deque
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: core Issues/PRs that touch core and may need additional validation. is: enhancement Issues requesting new features or pull requests implementing new features. waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants