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

CommonClient: Add a hints tab #2392

Merged
merged 12 commits into from
Nov 7, 2023
Merged

CommonClient: Add a hints tab #2392

merged 12 commits into from
Nov 7, 2023

Conversation

Silvris
Copy link
Collaborator

@Silvris Silvris commented Oct 28, 2023

What is this fixing or adding?

Adds a tab to all CommonClient-inherited clients that displays any currently received hints.

How was this tested?

Manually, by use during several Archipelago sessions.

No automated testing was added.

If this makes graphical changes, please attach screenshots.

image

@ScootyPuffJr1 ScootyPuffJr1 added the is: enhancement Issues requesting new features or pull requests implementing new features. label Oct 28, 2023
CommonClient.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@Zunawe Zunawe left a comment

Choose a reason for hiding this comment

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

Messed around with it for a bit on BizHawkClient and everything seems fine. Nothing in the code really stands out to me as needing changing, but I didn't look too in depth and I'm not familiar with the UI code at all.

Where we're touching the line anyway, replacing single quote strings with double quote strings might be nice.

And a very minor suggestion for the column headers would be to remove the colons, since they already look appropriately like headers anyway.

@alwaysintreble
Copy link
Collaborator

And a very minor suggestion for the column headers would be to remove the colons, since they already look appropriately like headers anyway.

could the headers maybe be slightly higher point to differentiate them a bit? or a line under them like a traditional table. i think it's fine as is, but that would look a bit nicer.

kvui.py Outdated Show resolved Hide resolved
kvui.py Outdated Show resolved Hide resolved
kvui.py Outdated Show resolved Hide resolved
kvui.py Outdated Show resolved Hide resolved
kvui.py Outdated Show resolved Hide resolved
kvui.py Outdated Show resolved Hide resolved
kvui.py Outdated Show resolved Hide resolved
kvui.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@el-u el-u left a comment

Choose a reason for hiding this comment

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

  • The cells in a row should be vertically aligned such that the first lines of text of all cells are on the same height.
  • When hints are added the line height can become too small and the text overlaps.
  • The separator between rows is almost invisible to me (But I guess that's related to the style of CommonClient as a whole and not specific to the Hints tab.)
  • One thing I am conflicted about is the Command box being present on the Hints tab. A user might take this to mean that they have to input the !hint command while on this tab; but if they do, they might not notice the server responses such as item name suggestions, because those are printed to a different tab.

image

kvui.py Outdated
'color': "blue", 'text': hint['entrance']
if hint['entrance'] else "Vanilla"})},
'found': {'text': parser.handle_node({'type': "color", 'color': "green" if hint['found'] else "red",
'text': "Found" if hint['found'] else "Not Found"})}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
'text': "Found" if hint['found'] else "Not Found"})}
'text': "found" if hint['found'] else "not found"})}

(nitpick to be consistent with the network messages)

@Silvris
Copy link
Collaborator Author

Silvris commented Nov 3, 2023

* When hints are added the line height can become too small and the text overlaps.

This was an issue for resizing, definitely a bit mad that it's still showing up for some reason. Kivy will just forget to update when texture_size changes for some reason.

Copy link
Member

@ThePhar ThePhar left a comment

Choose a reason for hiding this comment

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

Looks fine to me if we can update the quote usage.

Has this been tested in CommonClient-derived clients that also add additional tabs like Starcraft II client to make sure it renders okay?

kvui.py Outdated Show resolved Hide resolved
kvui.py Outdated Show resolved Hide resolved
kvui.py Outdated Show resolved Hide resolved
kvui.py Show resolved Hide resolved
kvui.py Show resolved Hide resolved
@Silvris
Copy link
Collaborator Author

Silvris commented Nov 7, 2023

image
Updated visuals based on feedback from Discord

@Silvris
Copy link
Collaborator Author

Silvris commented Nov 7, 2023

Has this been tested in CommonClient-derived clients that also add additional tabs like Starcraft II client to make sure it renders okay?

I have not completely tested SC2 client (I do not have SC2 downloaded), but I did load it to ensure the hint tab was correctly placed and did not interfere with their additions. I'll need to confirm LADX, but I cannot confirm Wargroove as I do not own it.

@ThePhar ThePhar merged commit ced35c5 into ArchipelagoMW:main Nov 7, 2023
12 checks passed
@ThePhar
Copy link
Member

ThePhar commented Nov 7, 2023

Just checked LADX and Wargroove, they looked fine on my machine.

FlySniper pushed a commit to FlySniper/Archipelago that referenced this pull request Nov 14, 2023
Jouramie pushed a commit to Jouramie/Archipelago that referenced this pull request Feb 28, 2024
@Silvris Silvris deleted the hint_tab branch April 16, 2024 05:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is: enhancement Issues requesting new features or pull requests implementing new features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants