search

bakkeby / patches

Collection of patches for dwm, st and dmenu
286 stars 30 forks source link

fullscreen-compilation: remove magic numbers #40

Open N-R-K opened 3 years ago

N-R-K commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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