Converting outer gaps into side-specific gaps, also allowing outer/horizontal/vertical (squashed) #249
Conversation
cameronleger
force-pushed
the
feature-54-all-squash
branch
4 times, most recently
from
2ee8960
to
5f91513
Compare
There was a problem hiding this comment.
The changes look fine now, thank you! Also thanks for squashing the commits. The last minor thing would be to actually rephrase the commit message to something useful (a oneliner is enough). You don't need to open new PRs, you can just force push onto an existing one. This should do the trick, assuming you're on the branch of your PR:
git commit --amend -m"Implement separately configurable outer gaps on each edge"
git push --force
Drop me a message when you did that and we can finally merge this. I appreciate your patience :-)
cameronleger
force-pushed
the
feature-54-all-squash
branch
from
5f91513
to
6acb4a2
Compare
|
No problem! I've force pushed this branch multiple times with success. In past cases GH wouldn't even let me force push to some complex branches, so I assumed it wouldn't work here; I should've tried! |
src/commands.c
Outdated
| } else if (!strcmp(type, "outer")) { | ||
| CMD_GAPS(outer, inner); | ||
| CMD_GAPS(inner); | ||
| } else if (!strcmp(type, "top") || !strcmp(type, "vertical") || !strcmp(type, "outer")) { |
There was a problem hiding this comment.
Actually I'm just stumbling across this… this logic here isn't correct anymore, is it? When using "vertical", only the "top" one will be set. Since it's else-ifs, the bottom one will never be set. This mistake is on me due to my review comment, sorry. We need to split these cases up entirely into a single strcmp per conditional and call CMD_GAPS multiple times for the combined ones.
There was a problem hiding this comment.
Ahh, good catch. I only briefly looked at that code and took your suggestion as is. I made sure it built, but I didn't restart my entire X Session to test it 😶 Let me do that now.
There was a problem hiding this comment.
I just pushed that change. Since the additional CMD_GAPS are now in the same scope, I pulled out the declaration of current_value and reset.
There was a problem hiding this comment.
I think it'd be better if we instead used the generally recommend pattern for macros:
#define … \
do { \
// … \
} while (0) \
That way we also don't have any scoping issues and this is how most i3 macros work as well.
There was a problem hiding this comment.
OK, I put them back and made that change.
There was a problem hiding this comment.
Thanks again for the patience. I think we are finally done now. Waiting for Travis to come back OK, then I'll merge it.
cameronleger
force-pushed
the
feature-54-all-squash
branch
from
6acb4a2
to
2d908cd
Compare
cameronleger
force-pushed
the
feature-54-all-squash
branch
from
2d908cd
to
1cdf198
Compare
|
We've done it. :-) |
Single-commit version of #211