himdel
commented
6 years ago Hey, first of all, wow, that's a huge amount of work, thanks! :)
However, while I'm certainly interested in big chunks of it, some of the changes don't quite match the original goals so, this may take some time to go through.
Would you be open to splitting the PR into multiple smaller changesets? (I'm definitely pro- rebase merges, but this is still a lot of even kinds of changes in one PR..) Or do you prefer me cherry-picking?
At a quick glance, these are my thoughts about it:
- move outputsxrandr.c -> outputs.c as per Makefile - nay, the idea is to have multiple outputs*.c backends, because I'm still hoping this can be made to work with libxrandr ; EDIT: added the right .o to makefile
- clean up Makefile - parts, the whole point of dropping autotools was to keep makefile simple, so while overridable prefix makes sense, I don't feel like I need to support renaming the binary or to override the default build rules (unless it actually breaks your build?)
- suppress build info by default - it runs 3 commands, nothing much to supress really
- add 'debug' target to Makefile - that's what
make CFLAGS=-ggdb3is for - move source to 'src' directory - not much point for 3 source files
- remove extern flags - sure, why not, looks like it may be technically wrong to use extern when including from the corresponding c file EDIT: done
- fix typos - definitely :) EDIT: done
- default no argument usage to 'help' - not sure, it's a change that makes sense, but I tend to
hsetrootsometimes to get rid of a visually disturbing wallpaper - refactor outputs info printing - replaces 1 debug printf with 31 extra lines - I don't think we need
-outputs, you can already run xrandr for that (EDIT: though, looking at the later changes, maybe you do) - collect output name info - makes sense, but not sure if we need
namewithout-outputs? - nest output info in struct - the same could be achieved with a trailing NULL, not sure which is better
- add 'outputs_by_name' to target specific outputs - this sounds useful, definitely useful enough to save the name :)
- add options struct for passing to 'load_image' - weird inconsistency where you still keep noutputs in options, when it's already in outputlist, also I don't think outputs are options, and I'd keep the original variable names for things that otherwise stay the same, but nice :)
- switch to bools where appropriate - definitely, except for the random single ret variable in
load_image, and thatsscanfactually does return meaningful int, not bool - restructure to support new '-output
' option - too big for one commit, and a lot of random renames which make the whole harder to decipher, could be a whole PR .. apart from that, I think I like it, but will have to read it properly - whitespace changes only - I'd say "yay consistency" but since most of the issues seem to come from commits in this PR, I'll say "yay fixup" ;)
- make 'blur' and 'sharpen' output specific - makes sense
- README.md, overhaul - sure, with minor fixes
- licencing info - LICENSE and AUTHORS sure, why not, but definitely none of that crap in source files please :)
Overwhelming preference for a rebase based 'merge' for this pull request to give the repository sufficient enough context for its final state, that and the fact that the additional 'Merge commit' entries in a repo are a pet hate of mine.
Of course, that's assuming you're at all interested!
The 'value-add' (beyond aesthetics, admin and refactoring) of this change set, is addressed in the final additional section of the README (see repo) namely the addressing of outputs by xrandr name, and the pushing 'manipulations' through by output ..albeit only for those I was interested in, the rest remain as 'global-only' options and are effectively untested
The latter
make 'blur' and 'sharpen' output specificcommit should be easy enough for anyone to follow should they want to switch any further options across from 'global only' to 'output specific'. However, perhaps further (todo) work could add either:-gbluror even a double hyphen--blur)-output allswitch--globaloption..to facilitate this switching between global and output specific option modes