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

For #45220: Tweaks support for non-ascii document names. #50

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

thebeeland
Copy link
Contributor

This attempts to resolve issues with publishing documents with paths that include non-ascii characters. The situation is different on Windows compared to OS X, so there is a small amount of OS-specific logic going on in one publish plugin, and in the adobe bridge.

In general, we return utf-8 strings, but there is now one situation on Windows where we're forced to decode to unicode before returning the document path.

Copy link
Contributor

@manneohrstrom manneohrstrom left a comment

Choose a reason for hiding this comment

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

i think we need to change it so that the return data is consistent, but no need for another CR pass.

#
# Note: This does not appear to be an issue on OS X.
if path and sys.platform.startswith("win") and not os.path.exists(path):
path = path.decode("utf-8")
Copy link
Contributor

Choose a reason for hiding this comment

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

We should add a :returns: to the docstring. But if i am reading this correctly, looks like the returns would look like:

returns a utf-8 encoded path to the document, UNLESS you are on 
windows and the path does not exist or contains non-ascii characters, 
in which case a unicode string is returned.

That doesn't seem like good encapsulation or consistency and seems like it can lead to subtle bugs.

As far as i understand, i think there are two ways to go, both approaches returning a consistent string from _document_path():

  • If there are several more complex path operations inside the photoshop hook logic, all requiring unicode paths on windows to function, we return all paths from _document_path() as unicode. Ideally for all OSes, but possibly for windows only if that makes sense.

  • if path operations are atomic and simple (like for example in the loader), we convert to unicode right before the operation (os.path.exists(path.decode("utf-8"))

I am happy either way as long as we keep the return values consistent and we explain the logic in some comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. Thanks!

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