CENTER_PATCH Fix - Githubissues

bakkeby / dmenu-flexipatch

A dmenu build with preprocessor directives to decide which patches to include during build time

MIT License

183 stars 78 forks source link

CENTER_PATCH Fix #28

Closed johnnymast closed 8 months ago

johnnymast commented 8 months ago

After i did enable the CENTER_PATCH feature i noticed the patch did not work. I have fixed it. It seems the old code did not activate the centering but disabled it.

johnnymast commented 8 months ago

I have noticed an issue with my patch. I am sorry for this because now centered is always activated.

bakkeby commented 8 months ago

You have the following configuration option: https://github.com/bakkeby/dmenu-flexipatch/blob/6bd3860e4b84c7e2701ca3ab86d9c11355c47ebc/config.def.h#L21

If that is set to 1 then dmenu will be centered by default, but you can set this to 0 to not have it centered by default.

The -c argument toggles this setting, i.e. if the config is 0 in the config then passing -c will make it centered, but if the config is 1 then passing -c will make dmenu appear at the top or bottom.

Maybe it is better to have 0 by default. It may be that it is set to 1 just to mimic what the original patch is doing.

I am personally not a fan of command line options that change behaviour depending on config like this.

johnnymast commented 8 months ago

Hi @bakkeby

You are absolutely right, that is the reason why i have commented on my own pull request. I will look into it because i discovered the problem my in my pull-request. The original patch had the variable named center it was named something like "centered" but not center. In your code you had "if -c" then center = !center (making it false(. I will test tomorrow is removing the exclamation mark is enough to fix the issue (i think that is the case).

bakkeby commented 8 months ago

Removing the exclamation mark would just result in the -c argument having no effect, because the value would always remain as it is set in the config.h file.

johnnymast commented 8 months ago

Hi @bakkeby,

Just woke up again and with my first coffee in hand i made the proposed changes.

Changes made