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

feat: init img convert & compress for gallery view #139

Merged
merged 3 commits into from
Aug 14, 2024

Conversation

kunfang98927
Copy link
Contributor

@kunfang98927 kunfang98927 commented Aug 12, 2024

Please note that now before running python manage.py import_instruments, you should first run python manage.py download_imgs.

There are a few changes in this pull request:

  • Add pillow dependency for image conversion and compression (by running poetry add pillow);
  • Create a new Django management commands in download_imgs.py. By running python manage.py download_imgs, it will: (1) download all original instrument images (one image for each instrument) from wikimedia urls into vim-static volume; (2) convert original images to .png format and store them on UMIL server; (3) compress original images and store the smaller version as thumbnails on UMIL server;
  • Modify import_instruments.py command: replace the old image urls with current image paths;
  • Add thumbnail field in Instrument model;
  • Create additional migration file migrations/0002_instrument_thumbnail.py;
  • In gallery view HTMLs, replace the old image url with current thumbnail url with static template tag;
  • Add __init__.py under VIM/apps directory to fix pylint error: No name 'apps' in module 'VIM' Pylint(E0611:no-name-in-module);
  • Adjust importing order in all management commands;

Resolves: #138

- Add `pillow` dependency for image conversion and compression (by running `poetry add pillow`);
- Create a new Django management commands in `download_imgs.py`. By running `python manage.py download_imgs`, it will: (1) download all original instrument images (one image for each instrument) from wikimedia urls; (2) convert original images to .png format and store them on UMIL server; (3) compress original images and store the smaller version as thumbnails on UMIL server;
- Modify `import_instruments.py` command: replace the old image urls with current image path;
- Add `thumbnail` field in `Instrument` model;
- Update the `migrations/0001_initial.py` automatically by running `python manage.py makemigrations`;
- In gallery view HTMLs, replace the old image url with current thumbnail url;

Refs: #138
@kunfang98927 kunfang98927 requested a review from dchiller August 12, 2024 17:00
- Add `__init__.py` under `VIM/apps` directory to fix pylint error: `No name 'apps' in module 'VIM' Pylint(E0611:no-name-in-module)`;
- Adjust importing order in all management commands;
- In `download_imgs.py`: (1) remove class `ImageDownloader` and place all methods into `Command` class; (2) change constants into upper case; (3) add `timeout` for `requests.get`; (4) change general `Exception` into specific exception types such as `requests.RequestException` and `IOError`; (5) change all `print()` into `self.stdout.write` and `self.stderr.write`;
- In `import_instruments.py`, change relative image path into absolute path;
- Recover migrations file; create an additional file for thumbnail field;

Refs: #138
@kunfang98927 kunfang98927 requested a review from dchiller August 13, 2024 18:52
@dchiller
Copy link
Contributor

Ok, @kunfang98927... the changes look great, but there's one more thing I realized as I was running this locally about the location of the image files. Sorry I didn't catch it earlier.

  1. When you are downloading the images and saving the original and thumbnail to the filesystem, you are doing so within the bind mount ./web-app/django:/virtual-instrument-museum/vim-app (see line 11 of docker-compose.yml). This means that you'll notice that when you run download_imgs, the images show up outside the container within the web-app/django/ directory (eg. in a place recognized by git). We don't want the images committed, so that path could be gitignored in some way. On the other hand, I wonder if we really want the images to exist in the bind mount at all. You could save them somewhere within the vim-static volume (/virtual-instrument-museum/static within the app container) and then they wouldn't appear in your working directory but would be saved across container builds. Let me know if that doesn't make sense.
  2. With the Django static files app, rather than store the path to the image prepended with settings.STATIC_URL, the canonical approach is to use the static template tag. You can see this in use in the same img tag in the template that you modify in the onerror parameter: <img src="{{ instrument.thumbnail.url }}" class="card-img-top rounded p-2" alt="instrument thumbnail" onerror="this.onerror=null;this.src='{% static "instruments/images/no-image.svg" %}';" />. The benefit is that we then aren't hardcoding any internal links (one change to settings and we could serve all our images from a different location). If you save the image url without settings.STATIC_URL prepended, and use the static template tag, this should work. (My comment in the earlier review suggested the opposite of this 🤦 )

@kunfang98927
Copy link
Contributor Author

Thank you for the comments.

  1. When you are downloading the images and saving the original and thumbnail to the filesystem, you are doing so within the bind mount ./web-app/django:/virtual-instrument-museum/vim-app (see line 11 of docker-compose.yml). This means that you'll notice that when you run download_imgs, the images show up outside the container within the web-app/django/ directory (eg. in a place recognized by git). We don't want the images committed, so that path could be gitignored in some way. On the other hand, I wonder if we really want the images to exist in the bind mount at all. You could save them somewhere within the vim-static volume (/virtual-instrument-museum/static within the app container) and then they wouldn't appear in your working directory but would be saved across container builds. Let me know if that doesn't make sense.

Yes I think downloading to vim-static volume makes sense. So for the download_imgs.py, I'll change the OUTPUT_DIR to

OUTPUT_DIR = os.path.join(
    settings.STATIC_ROOT, "instruments", "images", "instrument_imgs"
)

2. With the Django static files app, rather than store the path to the image prepended with settings.STATIC_URL, the canonical approach is to use the static template tag. You can see this in use in the same img tag in the template that you modify in the onerror parameter: <img src="{{ instrument.thumbnail.url }}" class="card-img-top rounded p-2" alt="instrument thumbnail" onerror="this.onerror=null;this.src='{% static "instruments/images/no-image.svg" %}';" />. The benefit is that we then aren't hardcoding any internal links (one change to settings and we could serve all our images from a different location). If you save the image url without settings.STATIC_URL prepended, and use the static template tag, this should work. (My comment in the earlier review suggested the opposite of this 🤦 )

Sure. I'll replace the thumbnail url in template with the one using static template tag:

<img src="{% static instrument.thumbnail.url %}" ...

- change image output dir in `download_imgs.py` command to downloading images into static volume;
- change image url in `import_instruments.py` command to store relative img urls into database;
- add `static` template tag in gallery view templates;

Refs: #138
@kunfang98927 kunfang98927 merged commit b145416 into develop Aug 14, 2024
3 checks passed
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.

In gallery view, only thumbnails for instruments should be displayed
2 participants