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: Color Picker GUI #3536

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

Conversation

Silvris
Copy link
Collaborator

@Silvris Silvris commented Jun 15, 2024

What is this fixing or adding?

Adds a GUI for editing user.kv, supports both regular JSONToKivy colors as well as the default white used for labels, with room for further expansion. Also supports creating and loading color presets that can be shared between users.

A change to client.kv for supporting coloring Selectable/HintLabel is also included, but not yet a part of Color Picker behavior (as I'd like to support more than just those two).

How was this tested?

Manually. It's a self-contained GUI, so no automated testing was added.

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 15, 2024
@remyjette remyjette added the is: enhancement Issues requesting new features or pull requests implementing new features. label Jun 15, 2024
@qwint
Copy link
Contributor

qwint commented Jul 27, 2024

some questions

  1. is it possible to have a way to save without closing?
  2. I know it's a "Text Color Picker" but it seems kinda silly to not be able to change background colors as well to match

some comments

  • the new command echo color is not covered here
  • I know the internals are remapping the color names to the alternative color values, but from a GUI perspective changing the color of "black" and "slateblue" is very confusing and takes a lot of trial and error to see the intent of each color group
  • you should probably import kvui before kivy so this doesn't happen to you on frozen https://discord.com/channels/731205301247803413/1261299992564469780
  • FileNotFoundError: [Errno 2] No such file or directory: '\\Archipelago-Branch\\data\\presets\\test.kv' seems saving a preset needs to handle when data/presets is missing

some praise

  • actual color picking UI feels well designed, was very easy to make my text client unreadable :p

did a light code review and nothing stood out, will probably review a bit more in depth later

Copy link
Contributor

@qwint qwint left a comment

Choose a reason for hiding this comment

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

realized i didn't actually technically request changes before and i'm at the very least doubling down on the issue of importing kivy before importing kvui, especially after opening #3823

but I also believe the rest of my previous comments need addressing in some way

  • the new command echo color is not covered here
  • I know the internals are remapping the color names to the alternative color values, but from a GUI perspective changing the color of "black" and "slateblue" is very confusing and takes a lot of trial and error to see the intent of each color group
  • you should probably import kvui before kivy so this doesn't happen to you on frozen https://discord.com/channels/731205301247803413/1261299992564469780
  • FileNotFoundError: [Errno 2] No such file or directory: '\\Archipelago-Branch\\data\\presets\\test.kv' seems saving a preset needs to handle when data/presets is missing

Copy link
Contributor

@qwint qwint left a comment

Choose a reason for hiding this comment

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

my outstanding questions (quoted below) are still unanswered, but I don't think they're showstoppers and the actual issues i found in my previous review are fully resolved

  1. is it possible to have a way to save without closing?
  2. I know it's a "Text Color Picker" but it seems kinda silly to not be able to change background colors as well to match

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.

3 participants