-
Notifications
You must be signed in to change notification settings - Fork 127
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
Conversation
This solves the problem I'm having on Mac. |
Is this is a duplicate of PR #74 but without any |
Well, there are now three PRs dealing with issue #71 plus @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 |
I believe that each issue should be addressed in a different PR to make it easier for the maintainer(s) to review the changes. |
I like that this patch fixes only one thing at a time, but I'm wondering if
That way the error message on non-sixel terminals can give the correct command for people to test their terminals:
|
|
||
|
||
# convert has been deprecated with ImageMagick 7 | ||
if [ "$(identify -version | head -n 1 | awk '{print $3}' | cut -d'.' -f1)" -ge 7 ]; then |
There was a problem hiding this comment.
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.
I believe the latest release fixes this issue, but please re-open if I am mistaken. Thanks! |
The convert command has been deprecated starting from ImageMagick 7.
Attempting to use the script results in something like this:
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:
Fixes #71
Fixes #77