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

Updated platform-specific handle documentation #961

Merged
merged 1 commit into from Nov 20, 2015

Conversation

Projects
None yet
6 participants
@mantognini
Member

mantognini commented Sep 6, 2015

This improves the documentation about platform-specific window handle types.

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Sep 8, 2015

Contributor

I'm not an OSX programmer, so I don't know if this is self-explanatory, but maybe you should note what is returned in what case.

Contributor

Foaly commented Sep 8, 2015

I'm not an OSX programmer, so I don't know if this is self-explanatory, but maybe you should note what is returned in what case.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Sep 8, 2015

Member

Yup. Maybe the whole case differentiation can be made more readable by using a list, where each item explains one platform. I was the one who did not do that in the beginning 😬

Member

Bromeon commented Sep 8, 2015

Yup. Maybe the whole case differentiation can be made more readable by using a list, where each item explains one platform. I was the one who did not do that in the beginning 😬

@mantognini mantognini self-assigned this Sep 9, 2015

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Sep 9, 2015

Member

You're both right, I'll rework on that PR soon.

(Initially I wanted to dedicate a full page of the doxygen documentation to OS specific limitations but actually that would just add links everywhere without making it really visible. And it takes more time to do.)

Member

mantognini commented Sep 9, 2015

You're both right, I'll rework on that PR soon.

(Initially I wanted to dedicate a full page of the doxygen documentation to OS specific limitations but actually that would just add links everywhere without making it really visible. And it takes more time to do.)

@mantognini mantognini added this to the 2.3.3 milestone Sep 13, 2015

@eXpl0it3r eXpl0it3r modified the milestones: 2.4, 2.3.3 Sep 16, 2015

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Sep 25, 2015

Member

I was thinking, instead of duplicating this information in multiple place, why not have sf::WindowHandle as a symbol in the doc?

If you don't want to generate the doc to see how it looks like, here are the relevant screenshots.

screenshot 2015-09-25 17 44 21

And the detailed documentation:

screenshot 2015-10-14 21 14 01

To do that, we need to trick Doxygen a bit as you can see here and there

So here are my questions:

  • do you like it or would you do it differently?
  • regarding Android, there was a discussion about the handle a few days/weeks ago. Should I update anything there or keep it defined as a ANativeWindow in the doc as well?
Member

mantognini commented Sep 25, 2015

I was thinking, instead of duplicating this information in multiple place, why not have sf::WindowHandle as a symbol in the doc?

If you don't want to generate the doc to see how it looks like, here are the relevant screenshots.

screenshot 2015-09-25 17 44 21

And the detailed documentation:

screenshot 2015-10-14 21 14 01

To do that, we need to trick Doxygen a bit as you can see here and there

So here are my questions:

  • do you like it or would you do it differently?
  • regarding Android, there was a discussion about the handle a few days/weeks ago. Should I update anything there or keep it defined as a ANativeWindow in the doc as well?

@mantognini mantognini changed the title from Updated platform-specific handle documentation for OS X to Updated platform-specific handle documentation Sep 25, 2015

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Oct 10, 2015

Member

@SFML/sfml Here's a cute little hamster to attract your attention on this PR. 🐹

Member

mantognini commented Oct 10, 2015

@SFML/sfml Here's a cute little hamster to attract your attention on this PR. 🐹

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Oct 10, 2015

Member

For code symbols, you should use \p or \c (typewriter font), not \a (italics).

And are you sure that the now enabled markdown support doesn't break other parts of the documentation? Maybe you could run a diff through the generated files...

Concerning #elif defined(SFML_DOXYGEN), that's a nice way of doing it. I solved this problem a bit differently in Aurora, see Config.hpp and doxyfile.txt. But your way is perfectly fine if we have so few occurrences. What I would change however is the identifier "platform-specific" to something like "{platform-specific}", so it's clear that it's not an actual identifier (it is already because of the dash, but it may not be for future ones).

I could also benefit from SFML_DOXYGEN in my shader uniform PR, so that

typedef priv::Matrix<4, 4> Mat4;

is documented as

typedef {implementation-defined} Mat4;
Member

Bromeon commented Oct 10, 2015

For code symbols, you should use \p or \c (typewriter font), not \a (italics).

And are you sure that the now enabled markdown support doesn't break other parts of the documentation? Maybe you could run a diff through the generated files...

Concerning #elif defined(SFML_DOXYGEN), that's a nice way of doing it. I solved this problem a bit differently in Aurora, see Config.hpp and doxyfile.txt. But your way is perfectly fine if we have so few occurrences. What I would change however is the identifier "platform-specific" to something like "{platform-specific}", so it's clear that it's not an actual identifier (it is already because of the dash, but it may not be for future ones).

I could also benefit from SFML_DOXYGEN in my shader uniform PR, so that

typedef priv::Matrix<4, 4> Mat4;

is documented as

typedef {implementation-defined} Mat4;
@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Oct 10, 2015

Member

For code symbols, you should use \p or \c (typewriter font), not \a (italics).

Fixed.

And are you sure that the now enabled markdown support doesn't break other parts of the documentation?

Actually it fixes a few things. Such as simple *bold* to list even. Nothing broken to report.

What I would change however is the identifier "platform-specific" to something like "{platform-specific}"

Yep, I agree but Doxygen doesn't handle this: it simply says:

WindowHandle.hpp:78: warning: documented symbol `sf::WindowHandle' was not declared or defined.

Member

mantognini commented Oct 10, 2015

For code symbols, you should use \p or \c (typewriter font), not \a (italics).

Fixed.

And are you sure that the now enabled markdown support doesn't break other parts of the documentation?

Actually it fixes a few things. Such as simple *bold* to list even. Nothing broken to report.

What I would change however is the identifier "platform-specific" to something like "{platform-specific}"

Yep, I agree but Doxygen doesn't handle this: it simply says:

WindowHandle.hpp:78: warning: documented symbol `sf::WindowHandle' was not declared or defined.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Oct 10, 2015

Member

Yep, I agree but Doxygen doesn't handle this: it simply says:

WindowHandle.hpp:78: warning: documented symbol `sf::WindowHandle' was not declared or defined.

That's annoying. It does accept - within identifiers, but not {}... quite arbitrary. Does it work with normal parentheses (platform-specific), square brackets [platform-specific] or angle brackets <platform-specific>?

Member

Bromeon commented Oct 10, 2015

Yep, I agree but Doxygen doesn't handle this: it simply says:

WindowHandle.hpp:78: warning: documented symbol `sf::WindowHandle' was not declared or defined.

That's annoying. It does accept - within identifiers, but not {}... quite arbitrary. Does it work with normal parentheses (platform-specific), square brackets [platform-specific] or angle brackets <platform-specific>?

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Oct 10, 2015

Member

Does it work with normal parentheses (platform-specific) or square brackets [platform-specific]?

Nope. :-|

Member

mantognini commented Oct 10, 2015

Does it work with normal parentheses (platform-specific) or square brackets [platform-specific]?

Nope. :-|

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Oct 10, 2015

Member

Too bad.

Little anectode: I had this very problem more than half a decade ago. After getting no meaningful answers from the Doxygen community, I just implemented what I wanted myself and submitted a patch to Dimitri. The patch extends the \hideinitializer command to typedefs, which allows to omit a typedef's referred-type for the sake of hiding implementation details from the API. Unfortunately, the feature has not been integrated the last time I checked... maybe you could check again if \hideinitializer works meanwhile.

If somebody were interested in taking another contribution attempt to Doxygen, I'd gladly search for that patch, but at the moment I don't have the time to do it myself.

Member

Bromeon commented Oct 10, 2015

Too bad.

Little anectode: I had this very problem more than half a decade ago. After getting no meaningful answers from the Doxygen community, I just implemented what I wanted myself and submitted a patch to Dimitri. The patch extends the \hideinitializer command to typedefs, which allows to omit a typedef's referred-type for the sake of hiding implementation details from the API. Unfortunately, the feature has not been integrated the last time I checked... maybe you could check again if \hideinitializer works meanwhile.

If somebody were interested in taking another contribution attempt to Doxygen, I'd gladly search for that patch, but at the moment I don't have the time to do it myself.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Oct 11, 2015

Member

I couldn't make \hideinitializer work with typename.. But I think it's already good as it is; at least for the time being.

What about

regarding Android, there was a discussion about the handle a few days/weeks ago. Should I update anything there or keep it defined as a ANativeWindow in the doc as well?

?

Member

mantognini commented Oct 11, 2015

I couldn't make \hideinitializer work with typename.. But I think it's already good as it is; at least for the time being.

What about

regarding Android, there was a discussion about the handle a few days/weeks ago. Should I update anything there or keep it defined as a ANativeWindow in the doc as well?

?

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini
Member

mantognini commented Oct 14, 2015

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Oct 14, 2015

Member

Leave ANativeWindow for now, this needs updating, but only once the actual code is updated as well (otherwise it would be wrong and misleading).

Member

MarioLiebisch commented Oct 14, 2015

Leave ANativeWindow for now, this needs updating, but only once the actual code is updated as well (otherwise it would be wrong and misleading).

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Oct 14, 2015

Member

Ok, it's updated. Let me know if there's anything else I should change. The screenshots above are up-to-date.

Member

mantognini commented Oct 14, 2015

Ok, it's updated. Let me know if there's anything else I should change. The screenshots above are up-to-date.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Oct 14, 2015

Member

Minor issues:

  • Max OS X
  • either NSWindow* or NSView* disguised as void* -- the precedence is not clear, I would add a comma:
    either NSWindow* or NSView*, disguised as void*
  • I would add parentheses to function calls for consistency: sf::Window::getNativeHandle() will return the handle...

I don't think this needs another review cycle with screenshot and everything. I'm fine with merging after those corrections 😀

Member

Bromeon commented Oct 14, 2015

Minor issues:

  • Max OS X
  • either NSWindow* or NSView* disguised as void* -- the precedence is not clear, I would add a comma:
    either NSWindow* or NSView*, disguised as void*
  • I would add parentheses to function calls for consistency: sf::Window::getNativeHandle() will return the handle...

I don't think this needs another review cycle with screenshot and everything. I'm fine with merging after those corrections 😀

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Oct 14, 2015

Member

Fixed. ;-)

Member

mantognini commented Oct 14, 2015

Fixed. ;-)

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Oct 17, 2015

Member

Android specific implementation/updates are now in branch feature/android-handles.

Haven't created a PR yet as I assume this one should be merged first. Not sure how GitHub reacts to that. :)

Also the example updates still need some work (comments + better variable names; possibly moving the code in its own inline/include file as well).

Member

MarioLiebisch commented Oct 17, 2015

Android specific implementation/updates are now in branch feature/android-handles.

Haven't created a PR yet as I assume this one should be merged first. Not sure how GitHub reacts to that. :)

Also the example updates still need some work (comments + better variable names; possibly moving the code in its own inline/include file as well).

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Oct 19, 2015

Member

Before this gets too cold, has anyone any additional suggestion/comment?

Member

mantognini commented Oct 19, 2015

Before this gets too cold, has anyone any additional suggestion/comment?

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Oct 19, 2015

Member

I'd say it looks good :)

👍

Member

Bromeon commented Oct 19, 2015

I'd say it looks good :)

👍

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Oct 21, 2015

Member

Change of plan, scrap the Android changes I've described above. Android just returns ANativeWindow. Leave it like that. I'll have to expose the activity in a different way (since you might need it isolated from a window instance).

Member

MarioLiebisch commented Oct 21, 2015

Change of plan, scrap the Android changes I've described above. Android just returns ANativeWindow. Leave it like that. I'll have to expose the activity in a different way (since you might need it isolated from a window instance).

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Oct 22, 2015

Member

@MarioLiebisch So the current changes are okay?

Member

eXpl0it3r commented Oct 22, 2015

@MarioLiebisch So the current changes are okay?

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Oct 22, 2015

Member

Checks in with me. 👍

Member

binary1248 commented Oct 22, 2015

Checks in with me. 👍

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Oct 22, 2015

Member

@eXpl0it3r Almost, actually it returns an ANativeWindow* I think.

Member

MarioLiebisch commented Oct 22, 2015

@eXpl0it3r Almost, actually it returns an ANativeWindow* I think.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Oct 22, 2015

Member

Let me know when you're sure and I'll update it. ;)

Member

mantognini commented Oct 22, 2015

Let me know when you're sure and I'll update it. ;)

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Oct 22, 2015

Member

It does, since ANativeWindowis a struct.

Member

MarioLiebisch commented Oct 22, 2015

It does, since ANativeWindowis a struct.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Oct 22, 2015

Member

Try to be a bit more specific with your comments. How should we know what exactly you mean with "It does"? It does be a pointer? It does be a value? It does need to change? 😉

Member

eXpl0it3r commented Oct 22, 2015

Try to be a bit more specific with your comments. How should we know what exactly you mean with "It does"? It does be a pointer? It does be a value? It does need to change? 😉

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Oct 25, 2015

Member

So is this ready?

Member

eXpl0it3r commented Oct 25, 2015

So is this ready?

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Oct 25, 2015

Member

I don't know; awaiting for @MarioLiebisch to give a more precise answer.

Member

mantognini commented Oct 25, 2015

I don't know; awaiting for @MarioLiebisch to give a more precise answer.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Oct 25, 2015

Member

ANativeWindow is a struct, the function returns a pointer, i.e. ANativeWindow*.

Member

MarioLiebisch commented Oct 25, 2015

ANativeWindow is a struct, the function returns a pointer, i.e. ANativeWindow*.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Oct 26, 2015

Member

I didn't update the screenshot above but now it should be alright for Android too. =]

Member

mantognini commented Oct 26, 2015

I didn't update the screenshot above but now it should be alright for Android too. =]

@mantognini mantognini added s:accepted and removed s:undecided labels Nov 6, 2015

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Nov 6, 2015

Member

Rebased and ready for merge.

Member

mantognini commented Nov 6, 2015

Rebased and ready for merge.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Nov 6, 2015

Member

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

Member

eXpl0it3r commented Nov 6, 2015

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

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Nov 10, 2015

Member

Just a quick screenshot before I merge it:

WindowHandle

Member

eXpl0it3r commented Nov 10, 2015

Just a quick screenshot before I merge it:

WindowHandle

@eXpl0it3r eXpl0it3r merged commit 0df97b4 into master Nov 20, 2015

16 checks passed

debian-gcc-64 Build #37 done.
Details
freebsd-gcc-64 Build #37 done.
Details
osx-clang-universal Build #33 done.
Details
static-analysis Build #37 done.
Details
windows-gcc-471-tdm-32 Build #39 done.
Details
windows-gcc-471-tdm-64 Build #39 done.
Details
windows-gcc-481-tdm-32 Build #39 done.
Details
windows-gcc-481-tdm-64 Build #39 done.
Details
windows-gcc-520-mingw-32 Build #37 done.
Details
windows-gcc-520-mingw-64 Build #39 done.
Details
windows-vc11-32 Build #38 done.
Details
windows-vc11-64 Build #39 done.
Details
windows-vc12-32 Build #38 done.
Details
windows-vc12-64 Build #37 done.
Details
windows-vc14-32 Build #37 done.
Details
windows-vc14-64 Build #39 done.
Details

@eXpl0it3r eXpl0it3r deleted the bugfix/doc_osx_handle branch Nov 20, 2015

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