Skip to content

Conversation

@pazner
Copy link
Contributor

@pazner pazner commented Aug 10, 2021

Now that #174 was merged and GLVis behaves more like a standard Mac application (only once instance is running, single Dock icon vs. many icons, etc.), it might make sense to also package GLVis as a Mac application bundle ("GLVis.app") that can be double-clicked, added to the Dock, and so on.

This PR adds a makefile target (make app) that does that. It does require adding an icns file (~ 960 KB) to the repo.

image

@tzanio
Copy link
Member

tzanio commented Aug 10, 2021

This is so cool!!

@tzanio tzanio added this to the glvis-4.1 milestone Aug 10, 2021
@publixsubfan
Copy link
Contributor

Very neat @pazner! Had a few comments/suggestions:

  • Can we add a similar target to the CMake build system as well?
  • It might be worth considering generating both the SDL icon and the app bundle icon from a common image file
  • Do we need to add code-signing? IIRC there were some requirements on newer versions of macOS that required notarization for app bundles (although maybe this only is needed for distribution)

@pazner
Copy link
Contributor Author

pazner commented Aug 10, 2021

  • Can we add a similar target to the CMake build system as well?

Yes, I just added a CMake target. make app should work with CMake now as well.

  • It might be worth considering generating both the SDL icon and the app bundle icon from a common image file

I agree, I can generate the icns file from the SDL icon with ImageMagick and iconutil. If we had a PNG format icon instead of raw RGBA data, we could generate the icns using only iconutil (which comes standard with Mac OS).

mkdir -p GLVis.iconset
convert -size 1024x1024 -depth 8 logo.rgba GLVis.iconset/icon_512x512@2x.png
iconutil --convert icns GLVis.iconset
rm -r GLVis.iconset
  • Do we need to add code-signing? IIRC there were some requirements on newer versions of macOS that required notarization for app bundles (although maybe this only is needed for distribution)

I don't think you need code-signing to run an app that you compile yourself, only to distribute it. It would be good to test this though.

@pazner
Copy link
Contributor Author

pazner commented Aug 10, 2021

@kanye-quest, see the branch mac-app-icon for an example of creating the icns file directly from the RGBA data. Instead of ImageMagick, I used the Mac's AppKit APIs to convert to PNG. Since if we are building a Mac app bundle, it's probably OK to depend on having Mac SDKs. That being said, it might be more hassle than it's worth compared with just including the ~ 1 MB icon file.

@tzanio
Copy link
Member

tzanio commented Aug 11, 2021

If you don't want to see the .app extension, deselect "Show all filename extensions" in Finder > Preferences > Advanced

Is it possible to add something to the dialog for the "About GLVis" menu item? Currently it only shows the GLVis logo, it will be nice to at least link to the website.

@tzanio tzanio added the Mac label Aug 11, 2021
@pazner
Copy link
Contributor Author

pazner commented Aug 11, 2021

Is it possible to add something to the dialog for the "About GLVis" menu item? Currently it only shows the GLVis logo, it will be nice to at least link to the website.

Yep, the contents of the file Credits.rtf will be displayed in the "About" window:

Screen Shot 2021-08-10 at 10 06 48 PM

@tzanio
Copy link
Member

tzanio commented Aug 11, 2021

Yep, the contents of the file Credits.rtf will be displayed in the "About" window:

Thanks @pazner! I adjusted the text a bit -- feel free to edit further if you'd like.

Screen Shot 2021-08-11 at 8 50 24 AM

Copy link
Member

@tzanio tzanio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome, thanks for thinking about it and making it happen @pazner 🎉

  • Please mention in CHANGELOG

@tzanio
Copy link
Member

tzanio commented Aug 12, 2021

@kanye-quest and @tomstitt -- do you need more time to review this?

(totally fine if you do, just checking)

Copy link
Member

@tomstitt tomstitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just wanted to try it out! Very cool, nice work @pazner !

Copy link
Contributor

@publixsubfan publixsubfan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, I don't have a Mac to test this with for the time being, so I'm deferring to @tomstitt and @tzanio. But the changes look good to me!

@tzanio tzanio self-assigned this Aug 12, 2021
@tzanio tzanio merged commit dbd8397 into master Aug 12, 2021
@tzanio tzanio deleted the mac-app branch August 12, 2021 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants