search

eduvpn / apple

app for iOS and macOS
Other
62 stars 18 forks source link

Roll back a few changes applied to Mac target, they were not needed #308

Closed jeroenleenarts closed 4 years ago

jeroenleenarts commented 4 years ago

How to adjust the name in the Menu bar?

github-actions[bot] commented 4 years ago

Make sure to keep CHANGES.md up to date!

roop commented 4 years ago

Looks good. Renaming the files is a good idea. I think the name in the menu bar name and the name in the About window should be controlled by CFBundleDisplayName.

roop commented 4 years ago

Are you seeing that in the menu bar, the app name is still "LetsConnect"?

roop commented 4 years ago

Ah, I think I understand you idea now: Not use APP_DISPLAY_NAME or CFBundleDisplayName in macOS files. I think that's better than what I had in mind, but then you should remove the CFBundleDisplayName entry in the macOS Info.plist.

johankool commented 4 years ago

This was working fine already on Mac.

jeroenleenarts commented 4 years ago

@johankool What't your suggestion? Apply this change type only to the iOS parts? I've done this change to make the iOS and Mac targets more alike.

Right now on Master the Mac app has a quote and a space in a file path due to the Let's Connect! naming. Perfect when displaying to the users, but concerning on disk. It brings all kinds of issues when we are doing path related work...

roop commented 4 years ago

@jeroenleenarts Why not keep it as it was on macOS and apply the change only for iOS? The renaming can be a separate PR as it has nothing to do with #301.

jeroenleenarts commented 4 years ago

This was working fine already on Mac.

@johankool Also I tried building master as Let's Connect Mac app before the change and it is not working for me.

cp: /Users/jeroenleenarts/code/eduvpn/EduVPN-macOS/Config/Assets-Let's Connect!.xcassets: No such file or directory
Command PhaseScriptExecution failed with a nonzero exit code
johankool commented 4 years ago

That's because you spelled the name wrong. You should use a different, curly quote.

jeroenleenarts commented 4 years ago

Wow, that's pretty non-obvious.

jeroenleenarts commented 4 years ago

@johankool @roop Alright, what I did.

Rolled back the Mac part. I don't feel too good about the single quote. But if it works, it works. :)

So please re-review this pull.