Skip to content

[SVCS-407] Adding support for .psd files to MFR #269

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.

Sign up for GitHub

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

Closed
AddisonSchiller wants to merge 1 commit into from
Closed

[SVCS-407] Adding support for .psd files to MFR #269

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion mfr/core/extension.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,17 @@

class BaseExporter(metaclass=abc.ABCMeta):

def __init__(self, source_file_path, output_file_path, format):
def __init__(self, ext, source_file_path, output_file_path, format):
Copy link
Contributor

@cslzchen cslzchen Oct 6, 2017

Choose a reason for hiding this comment

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

For naming, normalized_name may be better then ext since it is consistent (LINK). Or consider full word extension if no conflicting/shadowing issue. I understand it is confusing since the normalized_name is indeed the extension name (LINK).

Our code base is not documented well. It will be great if we can gradually add doc string for functions and methods (e.g. for the ones that you modified, or the ones that you had a hard time to understand) that have many arguments. Here is an example where my description is not accurate and please revise:

def __init__(self, extension, source_file_path, output_file_path, format):

    """Initialize the base exporter.
    
    :param extension: the name of the extension to be exported
    :param source_file_path: the path of the input file
    :param output_file_path: the path of the output file
    :param format: the format of the exported file (e.g. 1200*1200.jpg)
    """
    ...

Copy link
Contributor Author

@AddisonSchiller AddisonSchiller Oct 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

ext and self.ext was used as I saw it already being used elsewhere. Extension in MFR could refer to an actual extension library (eg the images extension, or Unoconv extension) So that is why it was avoided. normalized_name and ext are the same thing,
but the variable name doesnt make sense to me, so i did didn't use it.(dont remember why it is this way).

Copy link
Contributor

@cslzchen cslzchen Oct 6, 2017

Choose a reason for hiding this comment

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

Agreed. Let's keep using ext then and having the doc string should help for clarification.


"""Initialize the base exporter.

:param ext: the name of the extension to be exported
:param source_file_path: the path of the input file
:param output_file_path: the path of the output file
:param format: the format of the exported file (e.g. 1200*1200.jpg)
"""

self.ext = ext
self.source_file_path = source_file_path
self.output_file_path = output_file_path
self.format = format
Expand Down
2 changes: 1 addition & 1 deletion mfr/core/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ def make_exporter(name, source_file_path, output_file_path, format):
namespace='mfr.exporters',
name=normalized_name,
invoke_on_load=True,
invoke_args=(source_file_path, output_file_path, format),
invoke_args=(normalized_name, source_file_path, output_file_path, format),
).driver
except RuntimeError:
raise exceptions.MakeExporterError(
Expand Down
13 changes: 12 additions & 1 deletion mfr/extensions/image/export.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import os
import imghdr
import warnings

from PIL import Image
from psd_tools import PSDImage

from mfr.core import extension
from mfr.extensions.image import exceptions
Expand All @@ -23,7 +25,16 @@ def export(self):
'max_size_h': max_size[1],
})
try:
image = Image.open(self.source_file_path)
if self.ext in ['.psd']:
# silence warnings from psd-tools
Copy link
Contributor

@cslzchen cslzchen Oct 6, 2017

Choose a reason for hiding this comment

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

What are the warnings? Could this silence useful warnings as well?

Copy link
Contributor Author

@AddisonSchiller AddisonSchiller Oct 6, 2017

Choose a reason for hiding this comment

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

Warnings are custom thrown by the library. If there is an error, that still is caught. warnings look something like this:
/usr/local/lib/python3.5/site-packages/psd_tools/decoder/image_resources.py:138: UserWarning: ICC profile is not handled; colors could be incorrect. Please build PIL or Pillow with littlecms/littlecms2 support.

I remember doing some research into this when i originally did this PR, but Its been quite awhile and i can't remember much besides coming to the conclusion that it would be okay.
Warnings are silence so they dont spam the logs.

Copy link
Contributor

@cslzchen cslzchen Oct 6, 2017

Choose a reason for hiding this comment

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

I see. Warnings are fine since they are not exceptions. Your comment in the code also helps. We do want to catch exceptions thrown by psd-tools but maybe in another ticket so this is not a blocker.

# Warnings warn of outdated depedency that is a pain to install
# and about colors being possibly wrong
with warnings.catch_warnings():
warnings.simplefilter("ignore")
image = PSDImage.load(self.source_file_path).as_PIL()
else:
image = Image.open(self.source_file_path)

if max_size:
# resize the image to the w/h maximum specified
ratio = min(max_size[0] / image.size[0], max_size[1] / image.size[1])
Expand Down
1 change: 1 addition & 0 deletions requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ pydocx==0.7.0

# Image
Pillow==2.8.2
psd-tools==1.4

# IPython
ipython==5.2.2
Expand Down
2 changes: 2 additions & 0 deletions setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ def parse_requirements(requirements):
'.png = mfr.extensions.image:ImageExporter',
'.bmp = mfr.extensions.image:ImageExporter',
'.gif = mfr.extensions.image:ImageExporter',
'.psd = mfr.extensions.image:ImageExporter',
'.tif = mfr.extensions.image:ImageExporter',
'.tiff = mfr.extensions.image:ImageExporter',

Expand Down Expand Up @@ -620,6 +621,7 @@ def parse_requirements(requirements):
'.gif = mfr.extensions.image:ImageRenderer',
'.ico = mfr.extensions.image:ImageRenderer',
'.png = mfr.extensions.image:ImageRenderer',
'.psd = mfr.extensions.image:ImageRenderer',
'.tif = mfr.extensions.image:ImageRenderer',
'.tiff = mfr.extensions.image:ImageRenderer',

Expand Down