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

kvui: add autocompleting new hint text input #3535

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Berserker66
Copy link
Member

What is this fixing or adding?

adds a autocompleting new hint text input

How was this tested?

locally, connected with TextClient, not connected with TextClient, and connected with SC2 Client.

If this makes graphical changes, please attach screenshots.

image

@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 Jun 14, 2024
@Ziktofel
Copy link
Collaborator

could the items get color depending on type (progression/useful/filler/trap)?

Copy link
Contributor

@EmilyV99 EmilyV99 left a comment

Choose a reason for hiding this comment

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

LGTM

@EmilyV99
Copy link
Contributor

could the items get color depending on type (progression/useful/filler/trap)?

Doubtful, since a single item name/id can have different flags in different locations, so that info isn't known without scouting it

kvui.py Outdated Show resolved Hide resolved
@MatthewMarinets
Copy link
Contributor

MatthewMarinets commented Jun 14, 2024

If you click off the hint textbox / lose focus, then the dropdown disappears (as expected). If it regains focus, however, the dropdown does not come back, forcing a user to potentially remove and re-add a character. This should probably be fixed.

Edit: also, the dropdown registers a selection with all mouse buttons, including middle and right mouse buttons. I'd expect only left mouse button to select. Losing focus of the text box with tab doesn't cause the dropdown to disappear, so I'm no longer sure what conditions exactly cause it to disappear.

Separate issue (though I'm fine to call this a stretch goal/leave for a later PR): I'd expect pressing up/down from the text box to focus one of the dropdown options to let you select one of them with pure keyboard, but this doesn't seem to be the case. The only way to interact with the dropdown is the mouse.

Would also be nice to have fuzzy search so I could search "marnie" to get "marine" or something, but that's even more of a stretch goal.

@MatthewMarinets
Copy link
Contributor

Text within dropdown options should probably be left-aligned rather than centered.

This can avoid visual distance between the input text and resulting options for very wide screens:
image

And it's probably more usable to see the start of a name rather than the middle for very narrow window sizes relative to item name:
image

kvui.py Outdated Show resolved Hide resolved
kvui.py Outdated Show resolved Hide resolved
@MatthewMarinets
Copy link
Contributor

Another issue: The text input loses focus after hitting enter, even if the hint failed (e.g. due to a typo). It looks like focus entirely vanishes, so there's no recovering it by spamming tab a bunch of times.

if len(value) >= self.min_chars:
self.dropdown.clear_widgets()
ctx: context_type = App.get_running_app().ctx
item_names = ctx.item_names._game_store[ctx.game].values()
Copy link
Contributor

Choose a reason for hiding this comment

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

This introduces a new mypy issue:

Invalid index type "str | None" for "dict[str, ChainMap[int, str]]"; expected type "str" [index]

Copy link
Member Author

Choose a reason for hiding this comment

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

This is technically purely a typing issue, as the underlying class is a defaultdict. Maybe @ThePhar for comment, as he made the typing.

@MatthewMarinets
Copy link
Contributor

After more testing, I've found some of my code suggestions have minor knock-on effects. Also, arrow-key navigation is rather a big change for code suggestions. I've tossed everything together into a PR into this branch (#3537). Merge at your leisure or pick pieces out if you don't like all of the decisions there.

@Berserker66
Copy link
Member Author

I did try the no focus lost before making this PR, and then deleted the line again as at least personally I did not like it. There is already not enough feedback to what's going on, but at least the focus loss means something at least tried to happen.

@Berserker66
Copy link
Member Author

Berserker66 commented Jun 16, 2024

Now looks like this
image
I avoid regex as much as possible, and I think color is too heavy handed here. (also means we can repurpose this element somewhere where we want colored entries).
I also avoided storing each item name twice, rejecting the suggestion above.

@MatthewMarinets
Copy link
Contributor

I did try the no focus lost before making this PR, and then deleted the line again as at least personally I did not like it. There is already not enough feedback to what's going on, but at least the focus loss means something at least tried to happen.

There's two parts there: Focus moving away (reasonable imo), and focus going totally undefined so no amount of tabbing can recover it (bug).

I had the command-line client open on the other monitor so I could get feedback from there when testing, but yeah without it the feedback is lacking. The normal pattern here is probably to just clear the text input on entry, though that could get annoying with typos (unless we also add command history to this, which is less useful than commands as there's little reason to hint the same thing repeatedly :/).

@Exempt-Medic Exempt-Medic added the is: enhancement Issues requesting new features or pull requests implementing new features. label Jun 19, 2024
Copy link
Member

@Exempt-Medic Exempt-Medic left a comment

Choose a reason for hiding this comment

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

Briefly looked at the code, but really just did functional testing. The scrolling is a little jittery/flashy, but the feature works great. Tested a few different games and all the items displayed and were filtered properly.

Copy link
Member

@black-sliver black-sliver left a comment

Choose a reason for hiding this comment

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

I don't like the look of this at all, but it's probably good enough until we switch to kivymd one day.

data/client.kv Outdated
size_hint_y: None
height: dp(30)
multiline: False
write_tab: False
Copy link
Member

Choose a reason for hiding this comment

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

Missing newline at end of file

kvui.py Show resolved Hide resolved
@Exempt-Medic Exempt-Medic added the waiting-on: author Issue/PR is waiting for feedback or changes from its author. label Nov 28, 2024
@Berserker66 Berserker66 removed the waiting-on: author Issue/PR is waiting for feedback or changes from its author. label Nov 28, 2024
kvui.py Outdated Show resolved Hide resolved
@ReverM
Copy link
Contributor

ReverM commented Nov 28, 2024

Does this discriminate between hint and hint_location? Furthermore, could the list maybe two entries wide so there's more on the screen at once? (the names seem to mostly be smaller than a full line width)
Maybe dynamic wrt to the window's size?

Co-authored-by: Silvris <[email protected]>
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: peer-review Issue/PR has not been reviewed by enough people yet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants