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

add option for height offset fix #67 #68

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

add option for height offset fix #67 #68

wants to merge 2 commits into from

Conversation

acxz
Copy link

@acxz acxz commented Feb 27, 2022

in vertical scaling this accounts for used spaced in the terminal such as a prompt

This is a bug fix that resolves the display of scaling to the max terminal height. In reality the prompt get shown immediately after the image is displayed in the buffer, resulting in a view of a cutoff image. Adding an offset (optional) allows for users to change the effective max terminal height which their image is scaled against to render the complete image in their currently open terminal window.

This also fixes jarring updates from gifs. As if the displayed gif is larger than the display buffer, the gif is not played out smoothly in the terminal. Adding this offset allows scaling of the gif to fit the displayed buffer (this includes the user shell prompt) to ensure the smooth playback of the gif.

This can be treated as a feature request, however it primarily solves a vertical scaling display bug.

fixes #67

this issue could also be fixed by #69

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:

The PR fulfills these requirements:

  • 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

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)

Other information:
An alternative to catimg, chafa properly resolves this issue and scales according with user prompts. For a workaround and temporary solution, users can use chafa.

@posva
Copy link
Owner

posva commented Feb 28, 2022

maybe we could add something like this but I still think you should just pass the -H and compute the height outside of catimg

@acxz
Copy link
Author

acxz commented Feb 28, 2022

While it is totally possible to do so, one reason I really like catimg is that it resizes to my current terminal width. My prompt will always display after catimg and it will always push up the image/gif. It could be a bit cumbersome for users to manually specify a new height with -H every time they resize their terminal. With an offset approach users can still use the built-in automatic resize feature and add whatever offset their prompt is.

@posva
Copy link
Owner

posva commented Feb 28, 2022

I'm still not sure this logic belongs inside catimg:

# times 2 because of high resolution displaying
catimg -H $(( 2 * $(tput lines) - 2)) image.png

If we add an option like this it should default to 1 as most prompts are 1 height but work with other prompts too. It should also have a long name option only. Let's give this more time.

@acxz
Copy link
Author

acxz commented Feb 28, 2022

default to 1

That does make sense, added

It should also have a long name option only.

tbh, I was actually thinking about that but didn't add it since there was 1) no previous option as a long name and 2) didn't want to add any more processing than needed.

in vertical scaling this accounts for used spaced in the terminal such as a prompt
most prompts are size 1
@acxz
Copy link
Author

acxz commented Mar 3, 2022

@posva any updates on this?

catimg -H $(( 2 * $(tput lines) - 2)) image.png

What we are suggesting is not exactly this.

Imagine a case where I don't know if my picture is limited by my terminal width or my terminal height. If I specify an argument to -H then it will resize my image to -H. However, if I do not specify an argument than catimg will decided whether to resize based on my terminal height or my terminal width, whichever is appropriate.

Here is a showcase of what I mean:
First example is with -H with an arg (-H $(( 2 * $(tput lines) -2 ))), second case is only with -H
with-arg-H

With only -H
no-arg-H

@acxz
Copy link
Author

acxz commented Mar 3, 2022

a long name option only.

What would you suggest? Do you think --max-height-offset is a good name?

@acxz
Copy link
Author

acxz commented Mar 16, 2022

@posva can you provide closure to this PR?

Repository owner deleted a comment from acxz Apr 5, 2022
@posva
Copy link
Owner

posva commented Apr 5, 2022

I will take a look in a few weeks once I have the time

@acxz
Copy link
Author

acxz commented Jul 1, 2022

Closing due to inactivity.

@acxz acxz closed this Jul 1, 2022
@posva posva reopened this Jul 2, 2022
@posva
Copy link
Owner

posva commented Jul 2, 2022

I still haven't had the time but this is an interesting PR

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.

Images do not properly scale vertically
2 participants