Skip to content

Use different command to convert images with ImageMagick - #1480

Merged
david-yz-liu merged 23 commits into
Courseography:masterfrom
ethanliu24:use-diff-conversion-command
Nov 25, 2024
Merged

Use different command to convert images with ImageMagick#1480
david-yz-liu merged 23 commits into
Courseography:masterfrom
ethanliu24:use-diff-conversion-command

Conversation

@ethanliu24

@ethanliu24 ethanliu24 commented Nov 18, 2024

Copy link
Copy Markdown
Contributor

Proposed Changes

(Describe your changes here. Also describe the motivation for your changes: what problem do they solve, or how do they improve the application or codebase? If this pull request fixes an open issue, use a keyword to link this pull request to the issue.)

In recent versions of ImageMagick, the convert command is deprecated, and it is recommended to use the magick command. This PR uses the recommended command to convert graphs to display in the export modal.
...

Screenshots of your changes (if applicable)

Type of Change

(Write an X or a brief description next to the type or types that best describe your changes.)

Type Applies?
🚨 Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change that adds functionality)
🐛 Bug fix (non-breaking change that fixes an issue)
🎨 User interface change (change to user interface; provide screenshots)
♻️ Refactoring (internal change to codebase, without changing functionality)
🚦 Test update (change that only adds or modifies tests)
📦 Dependency update (change that updates a dependency)
🔧 Internal (change that only affects developers or continuous integration) X

Checklist

(Complete each of the following items for your pull request. Indicate that you have completed an item by changing the [ ] into a [x] in the raw text, or by clicking on the checkbox in the rendered description on GitHub.)

Before opening your pull request:

  • I have performed a self-review of my changes.
    • Check that all changed files included in this pull request are intentional changes.
    • Check that all changes are relevant to the purpose of this pull request, as described above.
  • I have added tests for my changes, if applicable.
    • This is required for all bug fixes and new features.
  • I have updated the project documentation, if applicable.
    • This is required for new features.
  • If this is my first contribution, I have added myself to the list of contributors.
  • I have updated the project Changelog (this is required for all changes).

After opening your pull request:

  • I have verified that the CircleCI checks have passed.
  • I have requested a review from a project maintainer.

Questions and Comments

(Include any questions or comments you have regarding your changes.)

@david-yz-liu david-yz-liu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@ethanliu24 great work, and in particular the research for fixing the text-anchor value.

@david-yz-liu david-yz-liu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@ethanliu24 oops, there is one thing I forgot to flag for you. Currently there's a check on the server code that the convert executable is installed. This should also be changed to magick. Look at the Haskell setup files for the place I'm referring to.

@ethanliu24
ethanliu24 force-pushed the use-diff-conversion-command branch from 79ff283 to f3afc29 Compare November 19, 2024 23:04
@ethanliu24
ethanliu24 force-pushed the use-diff-conversion-command branch from 8633b1f to a77ed93 Compare November 20, 2024 01:19
@ethanliu24
ethanliu24 force-pushed the use-diff-conversion-command branch from 2d9f5ca to 6e27cc8 Compare November 20, 2024 01:43
Comment thread .circleci/config.yml Outdated

corepack enable
corepack prepare yarn@stable --activate

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

delete this line

Comment thread .circleci/config.yml Outdated
stack build hlint
yarn install

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

revert the changes on this line (GitHub is highlighting trailing whitespace here; I recommend configuring your text editor to automatically strip trailing whitespace)

Comment thread .circleci/config.yml
- run:
name: Install Package Dependencies
command: |
sudo apt-get update

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

keep this line (put the sudo apt-get install command immediately below it)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I meant the existing command that installed texlive-latex-base (just running sudo apt-get install without additional arguments doesn't do much)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm not too familiar with linux (yet), but is there a reason for installing latex first before imagemagick?

@david-yz-liu david-yz-liu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@ethanliu24 good work! LaTeX didn't need to be installed imagemagick (it could be installed afterwards), and my comments were about keeping the apt-get commands together, rather than separated.

@david-yz-liu
david-yz-liu merged commit 27ac1bb into Courseography:master Nov 25, 2024
@ethanliu24 ethanliu24 mentioned this pull request Nov 30, 2024
7 tasks
@akarki2005 akarki2005 mentioned this pull request Oct 19, 2025
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants