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

[Win32] Fixed Unicode inconsistency #635

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@MarioLiebisch
Member

MarioLiebisch commented Jun 15, 2014

For Unicode builds this change is more cosmetic (as discussed in #633), but it should fix SFML's window class being registered as "S" rather than "SFML_Window" for non-Unicode builds.

[Win32] Fixed Unicode inconsistency
For Unicode builds this change is more cosmetic, but it should fix
SFML's window class being registered as "S" rather than "SFML_Window"
for non-Unicode builds.

@Bromeon Bromeon added p:windows and removed bug labels Jun 15, 2014

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Jun 15, 2014

Member

Well, the UNICODE macro is enforced in the CMake files, so there can't be non-Unicode builds.

Member

LaurentGomila commented Jun 15, 2014

Well, the UNICODE macro is enforced in the CMake files, so there can't be non-Unicode builds.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Jun 15, 2014

Member

Someone could still change it in the VS project file and screw everything up until the project files are rebuilt. :)

Member

MarioLiebisch commented Jun 15, 2014

Someone could still change it in the VS project file and screw everything up until the project files are rebuilt. :)

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Jun 15, 2014

Member

Someone can change anything in the generated project and screw up everything. This is not a valid point :p

Member

LaurentGomila commented Jun 15, 2014

Someone can change anything in the generated project and screw up everything. This is not a valid point :p

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Jun 15, 2014

Member

Well okay, then let's just settle with the used names being inconsistent so far. ;)

Member

MarioLiebisch commented Jun 15, 2014

Well okay, then let's just settle with the used names being inconsistent so far. ;)

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Jun 15, 2014

Member

That's not what I meant. If things can be "better" then let's do it. Did you change all the Win32 calls, or only those that have a direct or indirect relation to text handling?

Member

LaurentGomila commented Jun 15, 2014

That's not what I meant. If things can be "better" then let's do it. Did you change all the Win32 calls, or only those that have a direct or indirect relation to text handling?

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Jun 15, 2014

Member

I meant the reason for the change - didn't want to drop or revert this patch. I've updated all macroed calls in the Window implementation, even those not directly related to text input/output (unless I missed any). I didn't touch the other instances though (like the GL context dummy window).

Member

MarioLiebisch commented Jun 15, 2014

I meant the reason for the change - didn't want to drop or revert this patch. I've updated all macroed calls in the Window implementation, even those not directly related to text input/output (unless I missed any). I didn't touch the other instances though (like the GL context dummy window).

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Jun 15, 2014

Member

Ok, so it looks good to me.

Member

LaurentGomila commented Jun 15, 2014

Ok, so it looks good to me.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Jun 20, 2014

Member

I'm confused by the discussions over the three different issues.

In #633:

To me the implementation is ok, and deprecating Win 9x specific code is planned some day.

But cd68d66 says:

Removed support for Windows 9x (required deprecated functions)

My current assumption is, given @LaurentGomila's comment, that this PR can be merged, right?

Member

eXpl0it3r commented Jun 20, 2014

I'm confused by the discussions over the three different issues.

In #633:

To me the implementation is ok, and deprecating Win 9x specific code is planned some day.

But cd68d66 says:

Removed support for Windows 9x (required deprecated functions)

My current assumption is, given @LaurentGomila's comment, that this PR can be merged, right?

@eXpl0it3r eXpl0it3r self-assigned this Jun 20, 2014

@eXpl0it3r eXpl0it3r added this to the 2.2 milestone Jun 20, 2014

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Jun 20, 2014

Member

Yes, right.

Member

LaurentGomila commented Jun 20, 2014

Yes, right.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Jun 20, 2014

Member

The linked commit removed the explicit ANSI code paths and forced CMake into creating projects setup to use Unicode (i.e. Windows ME and newer). This pull request will remove the remaining "implicit" switches that could still be triggered by undefining UNICODE, basically forcing Unicode builds now (non-Unicode never worked for some parts of SFML).

Member

MarioLiebisch commented Jun 20, 2014

The linked commit removed the explicit ANSI code paths and forced CMake into creating projects setup to use Unicode (i.e. Windows ME and newer). This pull request will remove the remaining "implicit" switches that could still be triggered by undefining UNICODE, basically forcing Unicode builds now (non-Unicode never worked for some parts of SFML).

@eXpl0it3r eXpl0it3r added the bug label Jun 20, 2014

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Jun 20, 2014

Member

Thanks for clearing things up. Unless there are any concerns, I'll be merging this shortly.

Member

eXpl0it3r commented Jun 20, 2014

Thanks for clearing things up. Unless there are any concerns, I'll be merging this shortly.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Jun 20, 2014

Member

Merged in 121c3b2

Member

eXpl0it3r commented Jun 20, 2014

Merged in 121c3b2

@eXpl0it3r eXpl0it3r closed this Jun 20, 2014

@MarioLiebisch MarioLiebisch deleted the MarioLiebisch:win32-unicode branch Jul 5, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment