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

Windows linux translation #89

Merged
merged 16 commits into from
Feb 3, 2023

Conversation

Ryan-DowlingSoka
Copy link
Contributor

@Ryan-DowlingSoka Ryan-DowlingSoka commented Nov 1, 2022

This is my first submission in an attempt to update the Image Magick docs to help out windows users by doing auto conversion between linux cli syntax and windows.

It is heavily inspired by https://imagemagick.org/Usage/windows/ which contains a script to do just this.

I have not 1 to 1 implemented the script due to some complexities I'm not sure were fully covered, such as inline comments, but did an additional fix of ensuring spaces on either side of parens which is a requirement of dos.

Here is a rundown:

  • The logic is implemented in cli-translate.js it works by finding any <pre> tags with the "cli" class and then copies the contents 2 times.
  • The first copy is translated to the windows syntax, then the second copy is translated into the windows bat file syntax. (Double percentages for example is required for bat files, but not for the dos or powershell commandlines.)
  • The conversions are done with regex and replaces, similar in design to the script at Usage/windows.
  • Once both version are translated if the batch version is the same as the linux original version then I early out, but if it is different then they are added to the DOM and so is a combo-box button to the bottom right of the box.
  • When this button switches, all code boxes are swapped at the same time. I chose to do it this way since it seemed more likely that the general use case will be a user setting their desired cli language once for a page instead of wanting to see one command box one way and another a different way.
    • It would be possible to re-engineer this to be a box by box setting, but I think that is a worse user experience.
    • It would also be possible to store the language into the URL or into a cookie, whichever is preferred, but I personally don't think it is that necessary.
  • The javascript is minified and echo'd into the magick.js as a local addition.
  • No external libraries are used.
  • I added the cli tag to all matches of: <pre "*"><samp>magick
    • It is possible some were missed, especially any that were written a different way, perhaps in a <code> box.
    • Any interpreter and results implementations $ magick ... were missed, but this was to avoid mangling their return results.
    • Any linux bash files were missed, as I did not convert their headers and did not implement support for inline comments.

Here is a gif showing what the system looks like in practice.
imagemagic_docs_2 gif

I did spot check several image magick commands using both the dos commandline (windows 10) and .bat files

I had to fix the very first command which had an errant ") at the end.

If this goes well I would like to bring this logic (modified of course) to the usage examples, which will require a bit more effort but I think be very valuable.

@dlemstra
Copy link
Member

Thanks for your contribution. I like this idea but I haven't had time yet to properly test/review this. Can you resolve the conflicts in this pull request?

@Ryan-DowlingSoka
Copy link
Contributor Author

Sure thing. Done.

Copy link
Member

@dlemstra dlemstra left a comment

Choose a reason for hiding this comment

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

Thanks for making these changes. Left some review comments.

@Ryan-DowlingSoka
Copy link
Contributor Author

Made the suggested changes. I left two comments on changes, 1 where I'm not sure what you meant by the comment, and the other where I didn't think using a query selector would be beneficial, but am open to being convinced.

@Ryan-DowlingSoka
Copy link
Contributor Author

Updated to a query selector for the samp child.

@Ryan-DowlingSoka
Copy link
Contributor Author

Removed sourcemappingurl from the bootstrap min.js and remove eol echo into the magick js php.

@dlemstra
Copy link
Member

Looks like there are still some merging issue with this PR?

@Ryan-DowlingSoka
Copy link
Contributor Author

Interesting it says there arent any merge conflicts here. ill try to merge locally again tonight.

@dlemstra
Copy link
Member

Thanks, your local main branch might be older than you think it is?

@dlemstra
Copy link
Member

dlemstra commented Feb 2, 2023

Feel free to reach out by email if you need any help.

@Ryan-DowlingSoka
Copy link
Contributor Author

I hadn't had any time to look at this, but I don't see any conflicts:

image

@Ryan-DowlingSoka
Copy link
Contributor Author

I've updated and merged, without issue, and see no conflicts still. Not sure what to look at next.

@dlemstra
Copy link
Member

dlemstra commented Feb 2, 2023

My bad I was seeing this:

afbeelding

But of course this should be squashed instead.

Copy link
Member

@dlemstra dlemstra left a comment

Choose a reason for hiding this comment

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

Found some minor other things. Almost ready to merge.

Copy link
Member

@dlemstra dlemstra left a comment

Choose a reason for hiding this comment

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

Thanks for adding this!

@dlemstra dlemstra merged commit 80f7b21 into ImageMagick:main Feb 3, 2023
@dlemstra
Copy link
Member

dlemstra commented Feb 3, 2023

I made some tweaks to your script. I tried to put this in small commits so it will be easier for you to see what I did.

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

Successfully merging this pull request may close these issues.

2 participants