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

ColorPalette: rearranged and added field for color set names #632

Merged
merged 1 commit into from
Oct 9, 2023

Conversation

dsizzle
Copy link
Collaborator

@dsizzle dsizzle commented Oct 6, 2023

Fixes #255

Incidentally, all the functionality was already there to support color set labels, so basically it was a layout exercise and then wiring up what was there plus a few minor bits like setting the default name.

lmk what you think about the layout.

- removed extra FillRect to lessen flicker when palette is resized.
- added key controls to palette - left/right/up/down to choose color,
page up/page down to change color set
- set focus on palette when window is activated
@humdingerb
Copy link
Member

humdingerb commented Oct 7, 2023

Very good. Comments:

  • The name "palette 1" should be translated and capitalized and renamed to "Color set 1".
  • It's a bit irritating that the palette name always is in selection/editing mode when switching sets.
  • Its text control always has focus. It'd be nicer if it didn't and the cursor keys changed the selected colour (maybe even with pageup/down switching colour sets).
  • There's quite some flickering going on when resizing the window.

@dsizzle
Copy link
Collaborator Author

dsizzle commented Oct 7, 2023

Fixed the focus and the name; will think about the arrow keys.

Yeah the flickering is really bad. I guess before you couldn't resize it so it wasn't a problem.

@dsizzle
Copy link
Collaborator Author

dsizzle commented Oct 8, 2023

I did a quick fix for the flicker; I think it's a lot better.

@humdingerb
Copy link
Member

Looks flicker-free to me. 👍
BTW, by default the Colors window has minimum dimensions. The palette swatches are a bit small, even the default 16 colors. Could the default window size made a bit wider and taller, so the swatches are 2x the current height?

@humdingerb
Copy link
Member

One more thing: The color set name shouldn't allowed to be empty. How about reverting to the default "Color set #" in that case?

@dsizzle
Copy link
Collaborator Author

dsizzle commented Oct 8, 2023

  • doubled the minimum height of the color container
  • if the user tries to enter an empty name, it reverts to the previous name (this was the simplest change and programmers are lazy)

@dsizzle
Copy link
Collaborator Author

dsizzle commented Oct 8, 2023

... and added keyboard control to the color container view - it needs to have focus but then left/right/up/down and page up/page down now work as requested.

@humdingerb
Copy link
Member

Nice!
Would it work to have the focus be automatically on the color container view when switching to the colors window? Maybe by calling MakeFocus(true) in the view's WindowActivated()?

@dsizzle
Copy link
Collaborator Author

dsizzle commented Oct 9, 2023

Would it work to have the focus be automatically on the color container view when switching to the colors window? Maybe by calling MakeFocus(true) in the view's WindowActivated()?

sheesh, so demanding. Your wish has been granted.

@humdingerb
Copy link
Member

Beautiful! Quick, merge before I have more brilliant ideas!

BTW, would that have worked too with using the color_container view's WindowActivated()? At least I interpret the BeBook's description as such:

Implemented by derived classes to take whatever steps are necessary when the BView's window becomes the active window, or when the window gives up that status. If active is true, the window has become active. If active is false, it no longer is the active window.

@dsizzle
Copy link
Collaborator Author

dsizzle commented Oct 9, 2023

Beautiful! Quick, merge before I have more brilliant ideas!

I appreciate all of your ideas and insight!

BTW, would that have worked too with using the color_container view's WindowActivated()?

Perhaps it might. I was just thinking that controlling the focus via the parent window makes more sense than controlling it from the container control. How does it resolve conflicts if two different controls on the same window try to get focus in their respective WindowActivated() methods?

@dsizzle dsizzle merged commit 3e450a6 into HaikuArchives:master Oct 9, 2023
@dsizzle dsizzle deleted the color-set-name branch October 9, 2023 19:12
@humdingerb
Copy link
Member

I appreciate all of your ideas and insight!

"All" of them? Let's say "some" or even "most" if we want to be generous... :)

...controlling the focus via the parent window makes more sense...

Agreed.

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

Successfully merging this pull request may close these issues.

Label color sets
2 participants