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

Draw Lines via command #180

Merged
merged 13 commits into from
Sep 11, 2023
Merged

Draw Lines via command #180

merged 13 commits into from
Sep 11, 2023

Conversation

VanillaViking
Copy link
Contributor

Hello, thanks for making and maintaining gromit-mpx.
This PR aims to implement the feature requested in #161

The original feature request also suggested opacity customization, however by the looks of it, opacity seems to be application-wide rather than individual lines? I was unsure how to implement it here. Would appreciate some guidance in that matter.

I would appreciate if you could review and provide some feedback on this code 😄

@bk138
Copy link
Owner

bk138 commented Sep 6, 2023

Thanks! I'll have a look ASAP!

And yes, opacity is per-screen, not per line; safe to leave this out.

@bk138 bk138 self-assigned this Sep 6, 2023
Copy link
Owner

@bk138 bk138 left a comment

Choose a reason for hiding this comment

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

Surprisingly small PR, good work so far! Only minor change requests.

src/main.h Outdated Show resolved Hide resolved
src/main.c Show resolved Hide resolved
src/callbacks.c Show resolved Hide resolved
src/callbacks.c Show resolved Hide resolved
src/main.c Show resolved Hide resolved
@bk138 bk138 linked an issue Sep 6, 2023 that may be closed by this pull request
@VanillaViking
Copy link
Contributor Author

VanillaViking commented Sep 9, 2023

Thanks for the review!

I mainly use Xorg, but I briefly tested this out on wayland using Hyprland and it seems to work as expected.

As for the failsafe checks, it seems that atoi attempts to convert any given value to an int, or returns 0 if it can't. This would take care of most type issues, so I added checks to ensure that the coords fell within the screen area. Let me know if this is okay.

Maybe it's a good idea to include example usage of the command, in the README?

Everything else should be fixed 👍

@VanillaViking VanillaViking requested a review from bk138 September 9, 2023 02:07
@bk138
Copy link
Owner

bk138 commented Sep 10, 2023

Code-wise looks good to me, I also tested under Wayland and it draws lines, but only red ones. I'm probably missing out on the correct color format so I guess adding an example to the README is indeed a good idea. Thanks for the good work so far!

README.md Outdated Show resolved Hide resolved
@bk138
Copy link
Owner

bk138 commented Sep 11, 2023

Thanks for the quick reply and the contribution!

@bk138 bk138 merged commit d30f9b8 into bk138:master Sep 11, 2023
@bk138 bk138 self-requested a review September 29, 2023 10:08
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.

Draw lines via command
2 participants