-
Notifications
You must be signed in to change notification settings - Fork 47
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: add shadow color for inactive windows #230
feat: add shadow color for inactive windows #230
Conversation
Tested this out locally. can't find any bugs and works amazing! Thanks! can't wait for somebody to approve it |
6e7933b
to
371cf55
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I'll let @WillPower3309 merge this just in case he has any final thoughts :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, one nit
if (memcmp(config->shadow_color, config->shadow_inactive_color, sizeof(config->shadow_color)) == 0) { | ||
color_to_rgba(config->shadow_inactive_color, color); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would result in shadow_inactive_color being set twice (once here and then again in the shadow_inactive_color command file), so this can be omitted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is there so if you only set the shadow_color (and not the shadow_inactive_color) both of them are set to your custom one like right now, so that the changes are backward compatible. It does mean that it is set twice but the alternative is to make everyone change the configs to also add shadow_inactive_color even if they want shadows to be uniform. I'm ok changing it either way though, so please tell me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also if shadow_inactive_color is already set that is ignored, that's what the memcmp is for, so you can define them in any order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah got it, misinterpreted the comparison
This repo is pretty complex, great work as a first contributor! |
Feel free to ping me if you're on the SwayFX discord server for the contributor role |
Thank you! ^^ |
Solves #228.
This pull request adds a configuration option for changing the inactive windows' shadow color,
shadow_inactive_color
. It follows the same syntax asshadow_color
. If it is not defined, it takes the default value ofshadow_color
, there is no change for existing configurations.This is my first time contributing to this project and one of my first pull requests, so please if there is anything I have to do differently or change tell me. (Also thank you, I have been using this as my main desktop and couldn't be happier!)
2023-10-18.22-26-03.mp4