search

hackerb9 / lsix

Like "ls", but for images. Shows thumbnails in terminal using sixel graphics.
GNU General Public License v3.0
3.97k stars 129 forks source link

Improve compatibility with ImageMagick 7 #73

Closed alessandroberna closed 1 month ago

alessandroberna commented 1 month ago

The convert command has been deprecated starting from ImageMagick 7. Attempting to use the script results in something like this: image

This patch checks for the ImageMagick version and uses the new command if the major version is greater or equal to 7. After applying it, the warnings are no more: image

Fixes #71 Fixes #77

ibehnam commented 1 month ago

This solves the problem I'm having on Mac.

thomasmerz commented 1 month ago

Is this is a duplicate of PR #74 but without any shellchecking? 🤔

alessandroberna commented 1 month ago

Is this is a duplicate of PR #74 but without any shellchecking? 🤔

PR #74 doesn't check for the installed imagemagick version and just replaces convert with magick. I haven't had the time to test it out but it should break compatibility with earlier imagemagick versions.

thomasmerz commented 1 month ago

Well, there are now three PRs dealing with issue #71 plus shellcheck (has been fixed in PR #74 but not in #73 and there's my PR #75 that has only the shellcheck-fix plus a GitHub Action for automatic shellcheck'ing).

@hackerb9 should decide which one of #73 and #74 suits better and then I will fix my #75 with shellcheck stuff… 🤔 Does everybody involved agree? @sammcj @alessandroberna

alessandroberna commented 1 month ago

I believe that each issue should be addressed in a different PR to make it easier for the maintainer(s) to review the changes. Of course I have no objections to the shellcheck issue or the improved comparison logic. It goes without saying that the repo owner always has the final say on every issue; I'm okay with your proposal, but there isn't much to agree to on my end.

hackerb9 commented 1 month ago

I like that this patch fixes only one thing at a time, but I'm wondering if montage is also going to be deprecated by ImageMagick 7. It may make more sense to do something like:

magick=$(which magick)
...
$magick convert ...  sixel:-
$magick montage ... sixel:-

That way the error message on non-sixel terminals can give the correct command for people to test their terminals:

echo "$magick convert   foo.jpg  --geometry 800x480  sixel:-"
hackerb9 commented 1 month ago

I believe the latest release fixes this issue, but please re-open if I am mistaken. Thanks!