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

Improve compatibility with ImageMagick 7 #73

Closed
wants to merge 1 commit into from

Conversation

alessandroberna
Copy link
Contributor

@alessandroberna alessandroberna commented Jun 6, 2024

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
Copy link

ibehnam commented Jun 6, 2024

This solves the problem I'm having on Mac.

@thomasmerz
Copy link

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

@alessandroberna
Copy link
Contributor Author

alessandroberna commented Jun 7, 2024

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
Copy link

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
Copy link
Contributor Author

alessandroberna commented Jun 7, 2024

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
Copy link
Owner

hackerb9 commented Jun 10, 2024

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



# convert has been deprecated with ImageMagick 7
if [ "$(identify -version | head -n 1 | awk '{print $3}' | cut -d'.' -f1)" -ge 7 ]; then
Copy link
Owner

@hackerb9 hackerb9 Jun 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that you were thorough, but parsing version numbers like this is notoriously brittle. Also, while I make pipelines like that at the command line all the time, in a shell script I'd prefer to avoid forking quite so many processes. For example: $(identify -version | awk 'NR==1 { print $3 >= 7 }'.

Another problem is that for many years ImageMagick 7 had all the same executables as IM6 before deprecating convert. Will identify be deprecated someday? All we know for sure is that IM6 does not have magick and IM7 does.

@hackerb9
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants