Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify width/height logic #69

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

drybalka
Copy link

@drybalka drybalka commented Mar 25, 2022

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
  • Feature
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

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

  • 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:

  • 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 =)

@posva
Copy link
Owner

posva commented Apr 6, 2022

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

@acxz acxz mentioned this pull request Jul 1, 2022
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants