-
Notifications
You must be signed in to change notification settings - Fork 14
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Thanks!
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.