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

Handle Unicode objects properly #656

Merged
merged 17 commits into from Dec 6, 2018
Merged

Handle Unicode objects properly #656

merged 17 commits into from Dec 6, 2018

Conversation

@dlon
Copy link
Member

@dlon dlon commented Dec 3, 2018

Unicode objects now work for Sound(), music.load(), .image.load_extended()/load(), image.save_extended()/save(), ftfont, and font.

Changes: SDL generally expects UTF-8 strings, so I switched to using that in a lot of places. fopen() in Windows cannot handle UTF-8, so I added workarounds for that.

I had to disable test_save_bad_filename() and test_load_bad_filename() for now. SDL simply strips out "bad" characters instead of returning an error.

Close #37
Close #645

Related PR: #649


artifacts:
- path: 'dist\*'
environment:

This comment has been minimized.

@illume

illume Dec 3, 2018
Member

Hrmm. What changed in this file? It seems all the lines were changed for some reason, but I can't see what the changes were.

This comment has been minimized.

@e1000

e1000 Dec 3, 2018

the file in master has \r\n at end of each line, maybe that?

This comment has been minimized.

@dlon

dlon Dec 3, 2018
Author Member

I only added PYTHONIOENCODING: "UTF-8". AppVeyor reroutes stdout, and it caused some issue. But I'm not sure if it's needed anymore.

This comment has been minimized.

@illume

illume Dec 3, 2018
Member

Ah, ok. I guess the editor (on windows) changed all the line endings.

#else /* !WIN32 */
static FILE*
pg_Fopen(const char *filename, const char *mode) {
return fopen(filename, mode);

This comment has been minimized.

@illume

illume Dec 3, 2018
Member

I wonder what we have to do about the GIL here? Does it need to be wrapped in Py_BEGIN_ALLOW_THREADS; ?

This comment has been minimized.

@dlon

dlon Dec 3, 2018
Author Member

If SDL is used for error handling, then the caller can safely use Py_BEGIN_ALLOW_THREADS. It shouldn't matter if the GIL is held by the thread, but it doesn't have to be held?
Maybe imageext should be rewritten to use pgRWopsFromObjectThreaded() or SDL_RWFromFile() at some point. Then pg_Fopen() won't be needed anymore.

@illume
Copy link
Member

@illume illume commented Dec 3, 2018

Very good :)

I left two questions for bits I didn't understand.

/*
Py_BEGIN_ALLOW_THREADS;
*/
Py_BEGIN_ALLOW_THREADS; /* safe as long as Python is never used */

This comment has been minimized.

@illume

illume Dec 3, 2018
Member

Quite some years ago (10yrs) jpeg saving multiple jpegs at once had problems in threads when releasing the GIL. I never investigated why. It could have been a libjpeg thing at the time... not sure.

This comment has been minimized.

@dlon

dlon Dec 3, 2018
Author Member

Holding the GIL blocks multiple threads from using SaveJPEG() simultaneously, so I guess libjpeg wasn't thread safe.

@dlon dlon force-pushed the unicode branch from 68bfb6b to a464bd7 Dec 3, 2018
@dlon dlon force-pushed the unicode branch from a464bd7 to af5677e Dec 4, 2018
@dlon
Copy link
Member Author

@dlon dlon commented Dec 6, 2018

I guess it can be merged now?

@dlon dlon merged commit 4467da1 into master Dec 6, 2018
4 checks passed
4 checks passed
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@dlon dlon deleted the unicode branch Dec 6, 2018
@dlon dlon mentioned this pull request Mar 23, 2019
4 tasks done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants