Fix launch of cocoa example on macOS

[Ceylo]

Description

The generated bundle application was missing the file "MainMenu.nib" that had been generated but not copied inside the bundle:
2017-12-29 16:39:10.146 cocoa[6044:84729] Unable to load nib file: MainMenu, exiting

I also simplified the CMake file, I didn't understand why the xib->nib compilation was done manually as it looks to work automagically when the xib is part of the sources for the target. @mantognini do you remember ? I tested both with CMake 3.10 and 2.8.12 and it worked fine both when running the app from Xcode and in the installed dir (/usr/local/share/SFML/examples/cocoa/cocoa.app)

GitHub issue link

None

Forum thread link

None

[mantognini]

I couldn't reproduce the bug you're referring to, but since your patch also seems to work and improves the cmake script, I'm approving this PR.

I think that 6 years ago gcc/clang frontends didn't support xib as input format, or something like that.

[Ceylo]

Ah, I think I understand why you did xib compilation manually... when using Xcode generator, Xcode will indeed automatically use ibtool. But with makefiles generator it may try to give it to clang and then fail! I didn’t check that. Clang isn’t supposed to know what to do with xib files.

[mantognini]

Yet, it seems to work on my machine but I'd appreciate if you double check. :)

[eXpl0it3r]

So is there anything out standing or can this be merged?

[Ceylo]

Need to check build with makefiles. But I won’t be able to do that before tomorrow.

[eXpl0it3r]

No hurry, just checking the status.

[Ceylo]

I confirm that with Xcode generator the xib is correctly converted into a nib then copied in the app. But with Makefile generator the xib is copied as is and thus the app fails to launch…

[Ceylo]

Updated the PR to have it work also with makefiles. Now it's as complicated as it originally was… maybe a bit better separated… and now uses sfml_add_example(). Actually the only real mistake in the original code was that MainMenu.nib was not part of the list "RESOURCES". This is what later triggers copying of the nib into the bundle app with the MACOSX_PACKAGE_LOCATION property.

[mantognini]

Thanks, this looks great. I can't test it right now but will come back to you if I encounter any problems.

[Ceylo]

Do I need to merge again master each time another PR is merged to master, then wait for CI to run again?

@mantognini do you know when you'll have time to check ?

[mantognini]

Usually we rebase things onto master and @eXpl0it3r takes care of a lot of things (such as this) when there's no merge conflicts. We however love when commits are squashed together. :)

So this seems to works fine for me. Just one minor thing: when installed, the resources seem to be copied twice. Do you know why?

[Ceylo]

Ah. Crap. Didn't pay attention to this :)
That's because sfml_add_example() installs all the sources it is given. And the resources need to be given among target sources to be copied in the bundle app, so they're given as sources to sfml_add_example() command. Hmmmm…

On the other side here having a resource dir copied makes no sense, as everything is put in the bundle app.

Going to fix this.

[Ceylo]

@mantognini do you prefer a INSTALL_RESOURCES_DIR option to sfml_add_example() command that would be needed in several examples, or a DONT_INSTALL_RESOURCES_DIR that would be needed only in cocoa example ?

Edit: I went the "positive" way and chose INSTALL_RESOURCES_DIR option. If it's ok then all is fine, if not I'll change :)

I notice I didn't answer about squashed commits: I didn't pay attention to this because I know that at the time of the PR merge you have the option to squash on the fly.

[mantognini]

The option suits me as well. @LaurentGomila, no objection?

Actually, we don't use the web interface to merge stuff into SFML master branch for technical reasons. So if you could squash everything that'd be very appreciated. :)

[eXpl0it3r]

For me it's generally good rule to always use positive terms, as code becomes harder to understand once you have double negations or when you have to set something to false in order to activate it, it becomes very intuitive.

[Ceylo]

Aaaaaaaaah!
Ok I squashed and used the holy forbidden forced push :(

I'm glad you both agree on positive logic :)

[eXpl0it3r]

Force push is quite normal when you want to update your branch. As long as you're not force pushing the master or any (larger) collaboration branches.

[LaurentGomila]

@LaurentGomila, no objection?

Since there's no way around adding a new argument, let's use this opportunity to get rid of the hardcoded "resources" path in sfml_add_example(), and give the name of the resource(s) folder(s) to install instead of a simple on/off flag. And of course the Cocoa example would not pass this argument.

[Ceylo]

Does not look really useful for now, and I doubt you'll use anything else than "resources" for the resources dir in near future. Done anyway.

[LaurentGomila]

That was just an idea, no need to rush for implementation.

But I think it is useful: imagine you write an example, something that you've never done for SFML before. Currently, nothing forces you, nor even gives you a small hint, that you have to put all your resources in a sub-folder named "resources".
With this modification, there's no risk of mistake, once you've had a look at the sfml_add_example documentation, or at the other examples.

However, maybe we can find something more flexible or useful; for example, listing everything that needs to be installed (including source code) under a INSTALL_FILES option, or whatever. Again, just throwing some ideas to improve stuff.

[Ceylo]

I understand that you want to improve the situation. My feeling right now is that this is going a bit too much out of scope. At least for the current PR. So I suggest we stop here if there nothing blocking, and see what’s highest priority for future. There are many things to improve in CMake files but this PR was just to fix a bug about being unable to launch an example :)

[mantognini]

Yes, we can keep this for a later PR if we have a need for such refactoring. The current changes are good as-is, IMO.

[LaurentGomila]

"Add this because it works and then we'll see later to make it cleaner and refactor" is not my philosophy -- at least with such changes, which are small enough and have a limited scope. Adding small pieces of unrelated stuff on top of each other is what leads to bloated code and API. You've already doubled the argument count of the sfml_add_example macro, just to make the Cocoa example work; I think we can come up with something better 👍

Yes, we can keep this for a later PR if we have a need for such refactoring.

There will never be a need for further refactoring in the near future, so this will stay as is for a long time. On the other hand, the current PR is an opportunity to refactor and make things right. Look at the hard-coded "resources" path in the macro: this is one of these examples of "we'll see later" that's been in the repo for too long. Let's not add even more mess on top of it.

And no, I'm not over-thinking this tiny modification, I just feel like we can end with a nicer integration of this Cocoa example. Don't be lazy guys.

This is just my opinion however, there are many other voices here. You can perfectly ignore me and go ahead 😸

[MarioLiebisch]

Maybe I'm missing something here, but why are the resources even copied? Shouldn't they be inside the bundled application? Isn't that the whole point (it's just a directory with meta data inside after all)?

[Ceylo]

Maybe I'm missing something here, but why are the resources even copied? Shouldn't they be inside the bundled application? Isn't that the whole point (it's just a directory with meta data inside after all)?

Actually this is what happens now, for Cocoa example the resources are only copied inside the bundle app. The screenshot @mantognini showed was before I fix this issue :)

"Add this because it works and then we'll see later to make it cleaner and refactor" is not my philosophy -- at least with such changes, which are small enough and have a limited scope. Adding small pieces of unrelated stuff on top of each other is what leads to bloated code and API. You've already doubled the argument count of the sfml_add_example macro, just to make the Cocoa example work; I think we can come up with something better 👍

This is where our opinions differ :) . To me it looks rather clean actually. There're indeed 2 mores arguments. One that's needed only for bundle applications, so in most examples the code remain as simple. And the other argument because you wanted things to be explicit :) You can't have "more explicit " and expect everything to remain exactly as simple.

To me the current solution is good for current needs of SFML examples. You can argue that we would like to install other things than a resources dir and adapt the API for that, but there's no such need at the moment. You can also imagine that we may want to install the resources dir at another location than the one chosen (and "hardcoded") by sfml_add_example(), but there's also no such need at the moment.

[Ceylo]

Where has the Windows builder gone? 😢

[eXpl0it3r]

We had a system freeze and today there was overall maintenance for the servers. I restarted the service again, hopefully it will kick in again.

[Ceylo]

I suppose you have seen these errors ? Just in case

[Failure instance: Traceback (failure with no frames): <class 'twisted.internet.error.ConnectionLost'>: Connection to the other side was lost in a non-clean fashion.
]

[JonnyPtn]

It's minor, but this example also needs the high resolution flag changed until we have a way of handling high dpi displays.

[Ceylo]

Why do you say so?
On my high dpi display it looks sharp and correct.

[JonnyPtn]

My mistake, This seems to be the only example without that problem ( I guess because SFML isn't opening the window?). Otherwise, the window is opened at half size, but I'll raise a separate issue.

[mantognini]

@JonnyPtn Have a look at https://github.com/SFML/SFML/blob/master/examples/cocoa/resources/Cocoa-Info.plist#L33-L34. It's the only example that uses .app format and this settings, that's probably why you're experiencing this.

On the issue itself: I think it's a bit more explicit now and it's actually fine. While I agree with you, @LaurentGomila, on your general point, I don't have a suggestion to improve the current script. Do you?

[Ceylo]

Oh finally everything was built successfully 😃

[Ceylo]

@mantognini @eXpl0it3r Is there anything preventing from merging now?

[mantognini]

For me, all is good.

[Ceylo]

Who's supposed to press the 'merge' button or whatever you do to merge?
Just wondering why it's still open if everything is ok.

[eXpl0it3r]

There were on going discussions so I wasn't sure whether everything was resolved. Plus I'm not sitting in front of my computer just waiting for PRs to get finished so it can be insta merged. 😉

[Ceylo]

Right, I'm not blaming anyone! I just don't know yet what's the process and how it's supposed to happen :)

[eXpl0it3r]

Merged in ce7ced5