Skip to content

Remove spaces from input of HTML color in color picker - #79345

Closed
guemax wants to merge 1 commit into
godotengine:masterfrom
guemax:remove-spaces-from-input-of-html-color-in-color-picker
Closed

Remove spaces from input of HTML color in color picker#79345
guemax wants to merge 1 commit into
godotengine:masterfrom
guemax:remove-spaces-from-input-of-html-color-in-color-picker

Conversation

@guemax

@guemax guemax commented Jul 11, 2023

Copy link
Copy Markdown
Contributor

The color picker is now much more liberal about the user input, and simply removes the spaces before passing the string for further processing.
This is especially useful when copy-pasting HTML color codes from color palettes, as there is always the chance of accidentally copying some spaces as well. Having to remove them manually is quite tedious and not as intuitive as it could be.

Accepting valid HTML color codes with spaces is also the default behavior of programs with similar color picker functionality such as Affinity Designer, Blender, or Gimp.

@guemax
guemax requested a review from a team as a code owner July 11, 2023 15:55
@AThousandShips

Copy link
Copy Markdown
Member

I'd say the commit description is a bit verbose, should just contain the information about what it does, not arguments in favour, they should be in the PR description

Comment thread scene/gui/color_picker.cpp Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think the comment is uneeded, pretty self explanatory

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I tried to explain why I changed that line, but after reading through my comment again... Yep, I must admit that this is quite a repetition.

@guemax

guemax commented Jul 11, 2023

Copy link
Copy Markdown
Contributor Author

I attempted to adress the why rather than the what in my commit body (since the what can usually be read from the code changes).
Is that better?

Passing the unstripped HTML color code with leading and/or trailing
spaces to Color::from_string resulted in the current color not being
updated because internal functions of Color rejected the input
surrounded by spaces as invalid.

The color picker is now much more liberal about the user input, and
simply removes the spaces before passing the string for further
processing.

Fixes #79338

@AThousandShips

Copy link
Copy Markdown
Member

I would say the title says it all

@guemax

guemax commented Jul 11, 2023

Copy link
Copy Markdown
Contributor Author

I've updated my commit according to your suggestions. (I might be more of a book author rather than a short and precise commit message author. 😉 That was indeed a lot of duplication!)

@guemax guemax closed this by deleting the head repository Jul 21, 2023
@Calinou

Calinou commented Jul 21, 2023

Copy link
Copy Markdown
Member

@guemax Did you mean to close this pull request?

The PR itself seems good to me, I think it's in a mergeable state. It needs to be reopened first, but since you removed your fork repository, you'll need to create a new pull request.

@AThousandShips

Copy link
Copy Markdown
Member

@guemax

guemax commented Jul 22, 2023

Copy link
Copy Markdown
Contributor Author

Oh, I was simply deleting some old repositories, and have somehow managed to delete my fork of Godot as well. That was a dumb idea! Thanks for pointing this out. 👍

(As @AThousandShips commented, I opened a new pull request)

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow HTML color codes surrounded by spaces in the color picker

3 participants