posva / catimg

🦦 Insanely fast image printing in your terminal
http://posva.net/shell/retro/bash/2013/05/27/catimg
MIT License
1.4k stars 57 forks source link

fix #30 Fit vertically/adjust to terminal height #50

Closed boretom closed 4 years ago

boretom commented 4 years ago

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

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

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

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

boretom commented 4 years ago

Just as I submitted the PR I realized that I haven't considered both -w and -H passed. I only ever used one or the other. So you may want to hold on looking at that PR.

posva commented 4 years ago

Thank you for this! Yeah, I think having both -w and -H could yield an error

boretom commented 4 years ago

Morning @posva, what behaviour would you prefer if both -w and -H are passed? I see the following options

  1. only use whatever parameter is passed first, ignore the second
  2. only use whatever parameter is passed last, ignore the one before
  3. abort with error message, "only either -w or -H parameter is allowed"
  4. allow both at the same time and resize width and height

Personally I would prefer number 3

posva commented 4 years ago

Aborting with an error message sounds good :+1:

boretom commented 4 years ago

Hi, I added the check and abort if both are passed.

Additionally I did reorganise the man page file a bit so the options are sorted alphabetically. Plus reorganised the synopsis a bit. If you don't like it I'll of course restore the old man page + add the -H option.

Related to the man page I got some questions:

boretom commented 4 years ago

I did test it on macOS 10.15.5, macOS 10.14.6, Linux Mint 19.3, Linux Mint 20, Raspberry Pi 3B+ with Debian Buster and FreeBSD 12.1.

boretom commented 4 years ago

Oh, and no hurry at all. I'm old enough to know that the priorities can vary greatly.

posva commented 4 years ago

Oh wow, you already tested it in so many devices. After the needed changes, it should be good for a merge!

boretom commented 4 years ago

Testing is a big word :) ... it compiles and works fine for me on the mentioned platforms/distros with different images and two animated gifs.

I haven't done any testing on Windows. It would be interesting because according to StackOverflow getting Y the way I did it (same as for X) gives the buffer height but not the terminal window height.

Btw: It also compiles on Haiku OS R1/Beta2 but the output in the BeOS Haiku OS terminal doesn't look right. But if I connect to the Haiku box using SSH from my macOS it looks correct. So it's the Haiku OS terminal which behaves badly. I'll drop a note to one of their mailing list but not this week.

Btw2: It compiles and works on Fedora 32 (all the OS except macOS & FreeBSD run on a Intel NUC) but you would have to move two variable definitions (color_map and yuv_color_map) out of sh_color.h to sh_color.c. Otherwise the linker complains about them being defined twice (I read one should define variables in header files for this reason). One of them - yuv_color_map - is defined again in sh_color.c so the one in sh_color.h can be deleted. If you like I can make that change too.

boretom commented 4 years ago

I made the changes as I mentioned in the comments + what I guessed does "put span around math operators" mean.

posva commented 4 years ago

I realized there is a floating point exception when providing a -H option that is too big. It should not exceed original height like it does with original width. I won't be able to take a look for some time, so please feel free to take a look at it and submit a PR!

boretom commented 4 years ago

I see, I'll check on it and great a PR. The same does not happen with -w? What number did you use to test (I tested with -1 and 100000) and on what OS?

boretom commented 4 years ago

And regarding upgrading homebrew. a) no hurry and b) brew edit catimg shows you the recipe for that package.

boretom commented 4 years ago

I pulled the merged master and also get an floating point exception. But not with the feature/add-height branch, strange. I'll investigate it this week.