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

feat: adds hexcode below the cursor #95

Merged
merged 10 commits into from
Nov 28, 2024
Merged

feat: adds hexcode below the cursor #95

merged 10 commits into from
Nov 28, 2024

Conversation

bun137
Copy link
Contributor

@bun137 bun137 commented Nov 17, 2024

Adds the feature to show the hex code under the picker while picking the colour.

hyprpicker.mp4

hyprpicker with hexcode img1

hyprpicker with hexcode img2

@fufexan
Copy link
Member

fufexan commented Nov 17, 2024

Looks nice!

One thing I feel is missing is a background for the text. When you hover over white the text is not visible. A background with 0.5 alpha would make it more visible.

@danielwerg
Copy link

Nice! What @fufexan said and is there a reason you put hex code inside the ring? I don't think I have a preference either way but was just wondering.

@vaxerski
Copy link
Member

vaxerski commented Nov 17, 2024

2/10, should be right below the circle with a background. Right now it's barely readable and quite annoying

@bun137
Copy link
Contributor Author

bun137 commented Nov 17, 2024

Thank you! I've added background to the hex (made a simple rectangle behind the text).

I was messing around tryna figure out the best place to put the hex code. Putting it at the bottom of the screen wasn't so great. Plus I had read a comment saying some folks use it to compare colours so kinda made sense to put in the ring!

@bun137
Copy link
Contributor Author

bun137 commented Nov 17, 2024

2/10, should be right below the circle with a background. Right now it's barely readable and quite annoying

I'll make those changes.

@danielwerg
Copy link

Oh great, no issues functionality-wise anymore for me. For design I would suggest trying to round corners a little, right now it looks a bit bold.

I think this feature should be put behind a flag, I would want to use it 100% of the time but I don't think that going to be the case for most people.

Good point on the hex code position! The only time user won't be able to see is when cursor is at top right corner of the screen or anywhere on X axis at the bottom.

@vaxerski
Copy link
Member

I think this feature should be put behind a flag, I would want to use it 100% of the time but I don't think that going to be the case for most people.

Yeah, enabled by default though, with a flag to disable

@fufexan
Copy link
Member

fufexan commented Nov 17, 2024

image
Looks really nice

@fufexan
Copy link
Member

fufexan commented Nov 17, 2024

Good point on the hex code position! The only time user won't be able to see is when cursor is at top right corner of the screen or anywhere on X axis at the bottom.

Right, this could have a check for the bottom-right corner, and move the text above and left of the cursor.

@bun137
Copy link
Contributor Author

bun137 commented Nov 17, 2024

Thanks! Added the rounded corners to the rectangle, I'm working on having a check for the bottom so that I can move the text above the cursor.

@danielwerg
Copy link

I don't know C++ but I did some hard-coding as a PoW

- cairo_rectangle(PCAIRO, CLICKPOS.x, CLICKPOS.y + 20, 80, 30);

+ if (CLICKPOS.y > (PBUFFER->pixelSize.y - 50) && CLICKPOS.x >(PBUFFER->pixelSize.x - 100)) {
+   cairo_rectangle(PCAIRO, CLICKPOS.x - 80, CLICKPOS.y - 40, 80, 30);
+ } else if (CLICKPOS.y > (PBUFFER->pixelSize.y - 50)) {
+   cairo_rectangle(PCAIRO, CLICKPOS.x, CLICKPOS.y - 40, 80, 30);
+ } else if (CLICKPOS.x > (PBUFFER->pixelSize.x - 100)) {
+   cairo_rectangle(PCAIRO, CLICKPOS.x - 80, CLICKPOS.y + 20, 80, 30);
+ } else {
+   cairo_rectangle(PCAIRO, CLICKPOS.x, CLICKPOS.y + 20, 80, 30);
+ }


- cairo_move_to(PCAIRO, CLICKPOS.x, CLICKPOS.y + 40);

+ if (CLICKPOS.y > (PBUFFER->pixelSize.y - 50) && CLICKPOS.x > (PBUFFER->pixelSize.x - 100)) {
+   cairo_move_to(PCAIRO, CLICKPOS.x - 80, CLICKPOS.y - 20);
+ } else if (CLICKPOS.y > (PBUFFER->pixelSize.y - 50)) {
+   cairo_move_to(PCAIRO, CLICKPOS.x, CLICKPOS.y - 20);
+ } else if (CLICKPOS.x > (PBUFFER->pixelSize.x - 100)) {
+   cairo_rectangle(PCAIRO, CLICKPOS.x - 80, CLICKPOS.y + 40, 80, 30);
+ } else {
+   cairo_move_to(PCAIRO, CLICKPOS.x, CLICKPOS.y + 40);
+ }

@danielwerg
Copy link

Add some padding on left for the alpha box, otherwise # is touching the border.

@bun137
Copy link
Contributor Author

bun137 commented Nov 19, 2024

I added padding and tested the picker across various resolutions to ensure the hex code is visible everywhere. Also, I updated the hex code to render in uppercase.

@bun137
Copy link
Contributor Author

bun137 commented Nov 19, 2024

Here are some visuals!

1920x1080:

hyprpicker_fix.mp4

640x340:

hyprpicker_fix_640x340.mp4

3440x1440:

hyprpicker_fix_3440x1440.mp4

@danielwerg
Copy link

Yeah, uppercase hex is better for consistency since that the only format hyprpicker can output right now #55.

Looks perfect to me, need vaxry to review now.

@bun137
Copy link
Contributor Author

bun137 commented Nov 20, 2024

@vaxerski could you please review

Copy link
Member

@vaxerski vaxerski left a comment

Choose a reason for hiding this comment

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

rest ok

src/hyprpicker.cpp Outdated Show resolved Hide resolved
@danielwerg
Copy link

it still missing a flag to disable this feature, right?

@vaxerski
Copy link
Member

ah, right

@bun137
Copy link
Contributor Author

bun137 commented Nov 25, 2024

I've implemented the flag, its d to disable live preview of the hex code! Can you check it out?

src/main.cpp Outdated Show resolved Hide resolved
src/hyprpicker.cpp Outdated Show resolved Hide resolved
Copy link

@danielwerg danielwerg left a comment

Choose a reason for hiding this comment

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

DisableLive were not renamed internally only in help command, same goes for order of flags. Other than that lgtm.

@bun137
Copy link
Contributor Author

bun137 commented Nov 28, 2024

ah sheesh, my bad, will rename the flag

src/main.cpp Outdated Show resolved Hide resolved
Copy link
Member

@vaxerski vaxerski left a comment

Choose a reason for hiding this comment

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

lgtm, thanks!

@bun137
Copy link
Contributor Author

bun137 commented Nov 28, 2024

This is kinda my first PR to a very popular open source project. Been using Hyprland myself for the last two years and I'm glad I'm able to contribute a little to it! It makes me really happy :)

@vaxerski vaxerski merged commit d26cb2f into hyprwm:main Nov 28, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants