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

fullscreen-compilation: remove magic numbers #40

Open
N-R-K opened this issue Oct 7, 2021 · 3 comments
Open

fullscreen-compilation: remove magic numbers #40

N-R-K opened this issue Oct 7, 2021 · 3 comments

Comments

@N-R-K
Copy link

N-R-K commented Oct 7, 2021

While trying to edit some stuff I found myself not knowing what 2 or 3 meant. I think it's best to use enum instead; The names can probably be improved, but this is certainly better than what we currently have, magic numbers.

enum { FFSNone, FFSActive, FFSActual, FFSActualExit }; /* fakefullscreen */

I'd supply the fix myself, but I see that the patch has evolved a bit more compared to what I have on my build. Unfortunately the changes don't look interesting enough for me to bother porting them. Here's a diff on my build if you're interested: https://dpaste.com/82NLG5QT6

@bakkeby
Copy link
Owner

bakkeby commented Oct 7, 2021

Heh, I think of something else when I read FFS. Whether magic text FFSActual is more readable than magic number 2 I guess is debatable.

Style wise if you do it like this then arguably you should do it for c->isfullscreen as well?

Anyway I see the point and will take it into consideration if I end up refactoring this patch.

@N-R-K
Copy link
Author

N-R-K commented Oct 7, 2021

Heh, I think of something else when I read FFS.

Hence why the names can be improved :)

Style wise if you do it like this then arguably you should do it for c->isfullscreen as well?

Correct me if I'm wrong, but I don't think I saw anything but 0 and 1 being assigned to isfullscreen. 0 and 1 are pretty synonymous to false and true so I don't think it makes much sense to change them. At best I think one can use a bool and true/false for them instead. But that's upto the suckless guys, not really in scope for the fakefullscreen patch.

@N-R-K
Copy link
Author

N-R-K commented Oct 7, 2021

One more thing I just noticed (haven't tested it) is that c->isfullscreen = fullscreen; can probably be put inside this block.

if (fullscreen != c->isfullscreen) { // only send property change if necessary

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

No branches or pull requests

2 participants