catimg icon indicating copy to clipboard operation
catimg copied to clipboard

Simplify width/height logic

Open drybalka opened this issue 2 years ago • 1 comments

Currently the logic behind -w and -H flags is not straightforward. Here are a few problems:

  1. The 2 flags cannot be used together and throw an error
  2. The units of -w and -H depend on the resolution. For example, passing -w 80 returns an image either 80 or 40 columns wide, depending on the resolution.
  3. Passing some value with -w returns a smaller image if the original itself has fewer pixels, i.e., there is no scaling up. Therefore one cannot treat -w flag as "return an image of this size" anyway.
  4. Some black magic was involved when -H flag was passed. I tried to analyze the code, but got lost in all the if statements. As one example, for some reason the resulting image can depend on whether -H 0 is passed or not.

I propose to solve some of the problems above by simplifying the logic a bit. First of all, define the units of -w as columns and units of -H as lines. Second, treat -w and -H as maximally allowed width and height of the resulting image and use the terminal measurements if they are omitted. In other words, the resulting image should always fit into a box specified by -w and -H. This also allows one to use the 2 flags together without any errors.

What kind of change does this PR introduce? (check at least one)

  • [ ] Bugfix
  • [x] Feature
  • [ ] Code style update
  • [x] Refactor
  • [ ] Build-related changes
  • [ ] Other, please describe:

Does this PR introduce a breaking change? (check one)

  • [x] Yes
  • [ ] No

If yes, please describe the impact and migration path for existing applications: This PR intends to simplify the behavior of the program and technically some changes are breaking.

  • First of all, passing a width 80 with resolution 2 now produces an image 80 columns wide, which is twice as large than before. The mitigation is simply halving the value for -w.
  • Second, passing a single width flag now will not disregard the terminal height and vice versa. In order to allow overflowing simply pass in addition the height flag with a large enough number (or alternatively pass -H -1 as this will overflow the unsigned int and produce a giant positive number).
  • Some other complex behaviors are now simplified (like passing -H 0), but I could not find a rational explanation for them to provide a migration path.

The PR fulfills these requirements:

  • [NA] When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)
  • [NA] All tests are passing
  • [NA] New/updated tests are included

I did some testing in my own terminal. As the changes do not touch the actual img_resize() logic and only manipulate the width/height values this should be enough.

If adding a new feature, the PR's description includes:

  • [X] A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it) I hope my reason was convincing enough even without opening a PR =)

drybalka avatar Mar 25 '22 09:03 drybalka

Nice! I will have to take a proper look in a few weeks

posva avatar Apr 06 '22 16:04 posva