resurrecting-open-source-projects / scrot

SCReenshOT - command line screen capture utility
Other
495 stars 49 forks source link

scrot doesn't handle mutually exclusive options #264

Closed guijan closed 1 year ago

guijan commented 1 year ago

scrot -us, for instance, would tell scrot to select the current window, and also to interactively select a rectangle. Those 2 options are mutually exclusive. However, scrot silently proceeds to do whichever comes first in this if-else chain: https://github.com/resurrecting-open-source-projects/scrot/blob/8d0b22e4b2d82af06901ff6da1862c1fa5d76e26/src/scrot.c#L113-L133

The solution in this case is pretty easy, there should be only one variable that controls the screenshot mode, then all the options that control the screenshot mode set it to a unique value and error if it's already set to a different value. If we do it right, it's probably possible to do all of that in a single code block with multiple case labels pointing to it inside the getopt_long() switch statement.

It's still necessary to check which other options are mutually exclusive, I haven't looked deeply into it.

guijan commented 1 year ago

Just noticed --select and --autoselect should also be mutually exclusive.

Also, i'd redesign --autoselect to be a suboption of --select:

scrot --select auto=1440,1080,240,0

Something should probably be done about this too: https://github.com/resurrecting-open-source-projects/scrot/blob/6e2b5221bc25bb073552a2b0707ee99803f1a44f/src/scrot.c#L923-L927 Even if the code is right, the variables need to be named to reflect that, and the comment needs to be removed.

N-R-K commented 1 year ago

Also, i'd redesign --autoselect to be a suboption of --select:

--autoselect, I presume, is mostly used by scripts (e.g for selecting with slop). I wouldn't want to break working scripts for cosmetic purposes.

guijan commented 1 year ago

The future is more important than the past, I don't want a situation like the C programming language in which toupper() still uses int to represent a character because over 40 years ago C couldn't have types smaller than a register as function parameters. We can have the new version coexist with the old for a year, 2 years, maybe more, just enough time for the new version to be widely available and for people to change to it.

scrot seems to have added a new flag every time any functionality was added, even if it's related to other functionality. We could even write our own getsubopt() which can also match incomplete suboption names as long as they're unique, that way the invocation wouldn't be much longer:

scrot -s a=1440,1080,240,0

Although commas are the getsubopt() separator, so the syntax would have to change, and this is a good thing. The syntax could be:

scrot -s a=1440x1080+240+0

Another thing is that the autoselect syntax wasn't thought out very well. This new syntax (ripped off of ImageMagick) is a lot more powerful.

This could mean "remove 240 pixels from the left":

scrot -s a=+240

We could even make it default to having the selection rectangle in the middle of the screen for even more power. If you're playing a 4:3 video on a 1920x1080 screen, and the default is to put the selection area in the middle of the screen, this would screenshot the video without the black bars on the sides:

scrot -s a=1440

People could still put the selection rectangle in the top left by specifying the top left's coordinates:

scrot -s a=600+0+0

And we can top it off by interpreting negative offsets as coming from the right and bottom:

scrot -s a=1440-0

To screenshot a selection box that is 1440 pixels wide with the same height as the screen, in the right side of the screen.