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

OS X improvement: warnings + bugfix + refactoring, the lot! #1042

Merged
merged 7 commits into from Apr 14, 2016

Conversation

Projects
None yet
4 participants
@mantognini
Member

mantognini commented Jan 18, 2016

This PR fixes the warnings detected by static analyser regarding OS X, as well as clang's warning for regular build. That is, it replaces deprecated setColor for sf::Text, fixes a bit the documentation in one place, removes #warning that are anyway useless, replaces some deprecated Cocoa feature with newer ones (10.7+) and uses static_cast instead of C-cast where appropriate.

It also improves desktop/fullscreen modes detection for 10.8+ as explained in this comment and how they are handled when creating a fullscreen view.

Furthermore, it importantly refactors SFOpenGLView and splits this monolithic implementation into several files to make it manageable by human being. This way I can implement #614 correctly (already did, actually!)

In addition to all that, it fixes a bug related to fullscreen mode: when resizing a fullscreen window, the fullscreen mode was completely broken.

Finally, in order to anticipates some refactoring for #827: NSImage can now be easily created from raw pixel data.

NB: the branch is still named osx_warning but its content is significantly more important than just fixing warning. I didn't rename it as I don't know how GitHub will react...

@mantognini mantognini self-assigned this Jan 18, 2016

@mantognini mantognini added this to the 2.4 milestone Jan 18, 2016

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Jan 26, 2016

Member

Actually... there's a bug in there. 😩 I'm currently working on some refactoring (to grab the cursor maybe more elegantly) so I'll update this PR when the refactoring is done and the bug fixed.

Member

mantognini commented Jan 26, 2016

Actually... there's a bug in there. 😩 I'm currently working on some refactoring (to grab the cursor maybe more elegantly) so I'll update this PR when the refactoring is done and the bug fixed.

@louis-langholtz

This comment has been minimized.

Show comment
Hide comment
@louis-langholtz

louis-langholtz Jan 26, 2016

Contributor

improves desktop size detection for 10.8+ as explained in this comment.

Does the iOS code need a similar change perhaps?

I've noticed with the latest master branch code (that I've built for iOS) that on orientation change the resize event is fired and its data shows width and height values that are twice what sf::RenderWindow::getSize() returned (at least right after the sf::RenderWindow construction returns). It's like the initial getSize() call returned values in points and the resize event has values in pixels and where there are two pixels per point.

It would be preferable for these width/height values to be consistently in the same units and for those units to always be in pixels.

Contributor

louis-langholtz commented Jan 26, 2016

improves desktop size detection for 10.8+ as explained in this comment.

Does the iOS code need a similar change perhaps?

I've noticed with the latest master branch code (that I've built for iOS) that on orientation change the resize event is fired and its data shows width and height values that are twice what sf::RenderWindow::getSize() returned (at least right after the sf::RenderWindow construction returns). It's like the initial getSize() call returned values in points and the resize event has values in pixels and where there are two pixels per point.

It would be preferable for these width/height values to be consistently in the same units and for those units to always be in pixels.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Jan 26, 2016

Member

Does the iOS code need a similar change perhaps?

The related code is heavily different so I don't think so. But there might be some inconsistency with backing factor somewhere -- this thing is a nightmare at least on OS X for SFML.

Member

mantognini commented Jan 26, 2016

Does the iOS code need a similar change perhaps?

The related code is heavily different so I don't think so. But there might be some inconsistency with backing factor somewhere -- this thing is a nightmare at least on OS X for SFML.

@mantognini mantognini changed the title from OS X improvement: warnings + scaling to OS X improvement: warnings + bugfix + refactoring, the lot! Jan 26, 2016

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Jan 26, 2016

Member

I just updated the description of this PR as it now includes a few more thing. Testing/feedback welcome!

Member

mantognini commented Jan 26, 2016

I just updated the description of this PR as it now includes a few more thing. Testing/feedback welcome!

@mantognini mantognini referenced this pull request Jan 26, 2016

Merged

Feature/grab mouse #614

3 of 5 tasks complete
@louis-langholtz

This comment has been minimized.

Show comment
Hide comment
@louis-langholtz

louis-langholtz Jan 26, 2016

Contributor

Opened an issue for the problem I’d mentioned. Also wrote a fix and made a pull request for those changes: Bug: iOS orientation change handling re-scales window size by backingScaleFactor #1049. Hope this helps at least on the iOS side of things then.

On Jan 26, 2016, at 1:12 PM, Marco Antognini notifications@github.com wrote:

Does the iOS code need a similar change perhaps?

The related code is heavily different so I don't think so. But there might be some inconsistency with backing factor somewhere -- this thing is a nightmare at least on OS X for SFML.


Reply to this email directly or view it on GitHub #1042 (comment).

Contributor

louis-langholtz commented Jan 26, 2016

Opened an issue for the problem I’d mentioned. Also wrote a fix and made a pull request for those changes: Bug: iOS orientation change handling re-scales window size by backingScaleFactor #1049. Hope this helps at least on the iOS side of things then.

On Jan 26, 2016, at 1:12 PM, Marco Antognini notifications@github.com wrote:

Does the iOS code need a similar change perhaps?

The related code is heavily different so I don't think so. But there might be some inconsistency with backing factor somewhere -- this thing is a nightmare at least on OS X for SFML.


Reply to this email directly or view it on GitHub #1042 (comment).

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Feb 18, 2016

Member

The OpenGL example and Glsl.inl fixes are not really OS X specific, but I guess that doesn't really matter. 😉

Would be great if some other OS X users could test this.

Member

eXpl0it3r commented Feb 18, 2016

The OpenGL example and Glsl.inl fixes are not really OS X specific, but I guess that doesn't really matter. 😉

Would be great if some other OS X users could test this.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Feb 18, 2016

Member

The OpenGL example and Glsl.inl fixes are not really OS X specific

No, absolutely not. Even more reasons to check this PR! :-)

(I've put it in here because it calms down BuildBot.)

Member

mantognini commented Feb 18, 2016

The OpenGL example and Glsl.inl fixes are not really OS X specific

No, absolutely not. Even more reasons to check this PR! :-)

(I've put it in here because it calms down BuildBot.)

/// \param X Component of the 4D vector
/// \param Y Component of the 4D vector
/// \param Z Component of the 4D vector
/// \param W Component of the 4D vector

This comment has been minimized.

@Bromeon

Bromeon Feb 18, 2016

Member

Why this change? It doesn't add information, just makes the documentation more verbose. Now the parameter descriptions are even identical for all four parameters.

@Bromeon

Bromeon Feb 18, 2016

Member

Why this change? It doesn't add information, just makes the documentation more verbose. Now the parameter descriptions are even identical for all four parameters.

This comment has been minimized.

@eXpl0it3r

eXpl0it3r Feb 18, 2016

Member

Does Doxygen allow to document multiple parameter in one line?

@eXpl0it3r

eXpl0it3r Feb 18, 2016

Member

Does Doxygen allow to document multiple parameter in one line?

This comment has been minimized.

@Bromeon

Bromeon Feb 18, 2016

Member

Yes, the official documentation even uses a x,y,z example, so using a single \param here seems pretty appropriate ;)

@Bromeon

Bromeon Feb 18, 2016

Member

Yes, the official documentation even uses a x,y,z example, so using a single \param here seems pretty appropriate ;)

This comment has been minimized.

@mantognini

mantognini Feb 18, 2016

Member

Interestingly clang doesn't like it and emit a warning.

@mantognini

mantognini Feb 18, 2016

Member

Interestingly clang doesn't like it and emit a warning.

This comment has been minimized.

@Bromeon

Bromeon Feb 18, 2016

Member

clang bugreport! 😛

@Bromeon

Bromeon Feb 18, 2016

Member

clang bugreport! 😛

This comment has been minimized.

@Bromeon

Bromeon Feb 18, 2016

Member

As a side note, I'm personally a big fan of keeping things concise, including documentation. It's amazing how people write stuff like

/**
 * Checks if the object is stored.
 * 
 * @param object Object that is checked.
 * @return true if the object is stored, false otherwise.
 */
boolean containsObject(Object object) { ... }

where the method name alone would contain 100% of the documentation's information, just to fit in some questionable document-everything-strictly policy. The only thing it does is becoming outdated at the next refactoring. 😉

SFML contains quite a few such places, even if not that extreme. It's not something we have to adapt now that the documentation is already written, but I would not force currently concise documentation to the same level of verbosity, either.

@Bromeon

Bromeon Feb 18, 2016

Member

As a side note, I'm personally a big fan of keeping things concise, including documentation. It's amazing how people write stuff like

/**
 * Checks if the object is stored.
 * 
 * @param object Object that is checked.
 * @return true if the object is stored, false otherwise.
 */
boolean containsObject(Object object) { ... }

where the method name alone would contain 100% of the documentation's information, just to fit in some questionable document-everything-strictly policy. The only thing it does is becoming outdated at the next refactoring. 😉

SFML contains quite a few such places, even if not that extreme. It's not something we have to adapt now that the documentation is already written, but I would not force currently concise documentation to the same level of verbosity, either.

This comment has been minimized.

@mantognini

mantognini Feb 19, 2016

Member

clang bugreport!

Technically, I would need to go through Apple's bug report system as they modify clang for their needs... But you're right -- and I totally agree on the readability/conciseness part. However, last time I used it, it took them two years to answer my bug report. In the meantime, let's have a happy BuildBot report, shall we? 😄

@mantognini

mantognini Feb 19, 2016

Member

clang bugreport!

Technically, I would need to go through Apple's bug report system as they modify clang for their needs... But you're right -- and I totally agree on the readability/conciseness part. However, last time I used it, it took them two years to answer my bug report. In the meantime, let's have a happy BuildBot report, shall we? 😄

#import <SFML/Window/OSX/SFOpenGLView.h>
#import <SFML/Window/OSX/SFOpenGLView+mouse_priv.h>

This comment has been minimized.

@Bromeon

Bromeon Feb 18, 2016

Member

Is that a common convention for Objective C (I have no idea...)? Otherwise, I'd probably stick to SFML's existing one, i.e. SFOpenGLViewMousePriv.h or something. But if the + makes a crucial part of the semantics, we can also just leave it.

@Bromeon

Bromeon Feb 18, 2016

Member

Is that a common convention for Objective C (I have no idea...)? Otherwise, I'd probably stick to SFML's existing one, i.e. SFOpenGLViewMousePriv.h or something. But if the + makes a crucial part of the semantics, we can also just leave it.

This comment has been minimized.

@mantognini

mantognini Feb 18, 2016

Member

Yep, that's a common naming convention in Obj-C.

@mantognini

mantognini Feb 18, 2016

Member

Yep, that's a common naming convention in Obj-C.

This comment has been minimized.

@Bromeon

Bromeon Feb 18, 2016

Member

Ok, all clear then! 👍

@Bromeon

Bromeon Feb 18, 2016

Member

Ok, all clear then! 👍

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Mar 1, 2016

Member

So will you adjust the docs?
Do you think you'll find more testers or can I add this to the merge list?

Member

eXpl0it3r commented Mar 1, 2016

So will you adjust the docs?
Do you think you'll find more testers or can I add this to the merge list?

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Mar 1, 2016

Member

So will you adjust the docs?

I'd leave the commit as-is to get a clean build bot feedback. When the missing feature from clang is added, we can always change it back.

Do you think you'll find more testers or can I add this to the merge list?

We have two choice here: a) assume I do my job right and merge it, or b) put pressure on the community until testers show up. Besides me, who would be in favor of b) -- I cannot stress enough how much I'd love to see more good contributions for OS X from the community.

Member

mantognini commented Mar 1, 2016

So will you adjust the docs?

I'd leave the commit as-is to get a clean build bot feedback. When the missing feature from clang is added, we can always change it back.

Do you think you'll find more testers or can I add this to the merge list?

We have two choice here: a) assume I do my job right and merge it, or b) put pressure on the community until testers show up. Besides me, who would be in favor of b) -- I cannot stress enough how much I'd love to see more good contributions for OS X from the community.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Mar 29, 2016

Member

Since we've still not found more testers, do you thin we should go ahead and merge it?

Currently there seems to be merge conflicts, so a rebase would be nice.

Member

eXpl0it3r commented Mar 29, 2016

Since we've still not found more testers, do you thin we should go ahead and merge it?

Currently there seems to be merge conflicts, so a rebase would be nice.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Mar 29, 2016

Member

I could update this and include a "fix" for #1006 (i.e. dropping support for 32 bits), then we could indeed merge it so that #614 & #827 can move on as well.

Member

mantognini commented Mar 29, 2016

I could update this and include a "fix" for #1006 (i.e. dropping support for 32 bits), then we could indeed merge it so that #614 & #827 can move on as well.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Mar 29, 2016

Member

👍

Member

eXpl0it3r commented Mar 29, 2016

👍

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Mar 31, 2016

Member

Rebased and updated (dropping 32-bit support)

Member

mantognini commented Mar 31, 2016

Rebased and updated (dropping 32-bit support)

@mantognini mantognini added s:accepted and removed s:undecided labels Mar 31, 2016

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Apr 8, 2016

Member

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

Member

eXpl0it3r commented Apr 8, 2016

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

mantognini added some commits Jan 26, 2016

Refactoring NSImage creation from raw pixels
(in prevision for custom cursors)
Remove support for 32-bit on OS X
NOTE: external libraries are not updated in this commit but don't expect update to contain 32-bit symbols from now on.

@eXpl0it3r eXpl0it3r merged commit 845c684 into master Apr 14, 2016

16 checks passed

debian-gcc-64 Build #111 done.
Details
freebsd-gcc-64 Build #111 done.
Details
osx-clang-universal Build #115 done.
Details
static-analysis Build #111 done.
Details
windows-gcc-471-tdm-32 Build #114 done.
Details
windows-gcc-471-tdm-64 Build #114 done.
Details
windows-gcc-481-tdm-32 Build #114 done.
Details
windows-gcc-481-tdm-64 Build #114 done.
Details
windows-gcc-520-mingw-32 Build #112 done.
Details
windows-gcc-520-mingw-64 Build #114 done.
Details
windows-vc11-32 Build #113 done.
Details
windows-vc11-64 Build #114 done.
Details
windows-vc12-32 Build #113 done.
Details
windows-vc12-64 Build #112 done.
Details
windows-vc14-32 Build #112 done.
Details
windows-vc14-64 Build #114 done.
Details

@eXpl0it3r eXpl0it3r deleted the bugfix/osx_warnings branch Apr 14, 2016

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