emersion / kanshi

Dynamic display configuration (mirror)
https://wayland.emersion.fr/kanshi/
MIT License
655 stars 46 forks source link

Implement reload config command #107

Closed ericonr closed 3 years ago

ericonr commented 3 years ago

I rebased #52, solved conflicts, fixed all the issues that appeared, and addressed all of @emersion's pending comments in the original PR.

I'm not sure how I should group the commits together here, so would appreciate suggestions. Since this is the initial version, I left all my changes (besides the one mentioning varlink is optional in README) as additional commits; this way reviewers can hopefully have a clearer idea of what I changed. I can try and squash most of them together for a final version before pushing.

emersion commented 3 years ago

Thanks for working on this!

I'm not sure how I should group the commits together here, so would appreciate suggestions.

Hm, I've made a bunch of review comments, but I only now realized you have may already fixed some of these in future commits. Sorry about that, feel free to disregard outdated comments.

Squashing commits together would help with the review process, because I don't want to review the full changes all together, I prefer to do one bit at a time. In an ideal world I'd split the commits into (1) add event loop (2) add signal handlers (3) add varlink interface (4) add kanshictl utility. But this may be too much work, I'm fine with a split into less commits. I'd still prefer to have logical commits rather than incremental commits (see https://www.bitsnbites.eu/git-history-work-log-vs-recipe/).

emersion commented 3 years ago

Also, please retain attribution for the commits, but feel free to add Co-authored-by trailers to indicate when a commit has been worked on by multiple contributors.

ericonr commented 3 years ago

Luckily I had to do a similar refactor recently, so my git muscles were active already :)

It's done!

emersion commented 3 years ago

Overall this looks pretty good!

ericonr commented 3 years ago

I think it should be ready now, then :D