Skip to content
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

Add CFBundleShortVersionString to macOS app plist files #5209

Merged
merged 1 commit into from May 3, 2023

Conversation

scribblemaniac
Copy link
Contributor

@scribblemaniac scribblemaniac commented Apr 28, 2023

Fixes #3794

Description of the Change
Adds the CFBundleShortVersionString property list key which is a recommended key to include with macOS apps. This is what is used to display the version in Spotlight, which is why the BOINC Manager is currently listed as having an "Unknown version" when searching for it.

Previously, SetVersion sort of tried to parse the plist files in clientgui/mac/templates, find the CFBundleVersion, and then replace the contents of the following tags. This doesn't work well for replacing multiple property values however. So I instead simply search for and replace any instance of %VERSION%, which is an approach I've seen used in other projects for populating the app plist file.

Alternate Designs
I considered keeping the approach more similar to the original code, but it would have been more complicated, would have enforced an unnecessary ordering on some elements (the CFBundleVersion would have to come before the CFBundleShortVersionString, or vice versa), and worst of all from what I can tell the old code would have broke if the tag name happened to be split between buffers. The approach I took does have the disadvantage of loading the entire template file in memory (twice), but considering how small these files are, it actually works out to about the same size as the buffer currently being used and should not be a concern.

** Side Note **
I was only able to test this fix on macOS 10.13. I can't say for 100% certain that Spotlight behaves the same on newer versions of macOS, but as long as the version is being correctly inserted into the plist files (which it is for me at least), it should at the very least not make anything worse.

Release Notes

Fix "Unknown version" listed when searching for BOINC apps on macOS's Spotlight

Also simplify SetVersion's FixInfoPlistFile(char*) so it just
replaces instances of %VERSION% in the plist template files.
@AenBleidd
Copy link
Member

@CharlieFenton, could you please review this?

@CharlieFenton
Copy link
Contributor

I will look at this within a few days, as I'm looking at a coupe of more urgent issues currently.

@AenBleidd AenBleidd added this to Backlog in The SCI Campaign via automation Apr 28, 2023
@AenBleidd AenBleidd moved this from Backlog to In Review in BOINC Client/Manager Apr 28, 2023
@AenBleidd AenBleidd moved this from Backlog to In Review in The SCI Campaign Apr 28, 2023
@AenBleidd AenBleidd linked an issue Apr 28, 2023 that may be closed by this pull request
BOINC Client/Manager automation moved this from In Review to Reviewed May 2, 2023
@AenBleidd
Copy link
Member

AenBleidd commented May 2, 2023

@CharlieFenton, should I proceed and merge it?

Copy link
Contributor

@CharlieFenton CharlieFenton left a comment

Choose a reason for hiding this comment

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

I have never used std::if stream or std::ofstream so I am not familiar enough with them to comment on the implementation of those constructs. But the approach seems reasonable. Assuming @scribblemaniac has tested this properly, I hav no problem with it.

However I have a request: Many of the changed lines only eliminate blank space and so are unnecessary. This tends to confuse GIT's blame / annotate feature, potentially making it more difficult to recall several years from now why code was changed or written. Please eliminate any changes that only affect blank space. If you editor is set to do that automatically, please turn that feature off.

@CharlieFenton
Copy link
Contributor

@AenBleidd I meant to add the above comment to my review but accidentally pressed "submit before I did that. I have now added that comment and would prefer that unnecessary changes to blank space be eliminated before merging.

@scribblemaniac Thank you for this submission.

@AenBleidd
Copy link
Member

@CharlieFenton, I don't completely agree: removing whitespaces in general is a good practice (probably we should make a separate PR and remove all whitespaces in the whole codebase in one commit?).
Also, it's possible to use git blame -w that will ignore whitespaces change.

If this is not a blocker for you, I'd merge this PR and talk separately with you and @davidpanderson privately about whitespaces.

@davidpanderson
Copy link
Contributor

I'm OK with merging this, but in the future we should do whitespace cleanup
in a separate PR. Lots of irrelevant changes makes the diff harder to read.

@AenBleidd
Copy link
Member

I'll handle than issue with whitespaces in a separate PR (probably today later).

@AenBleidd AenBleidd merged commit d98c451 into BOINC:master May 3, 2023
39 of 41 checks passed
BOINC Client/Manager automation moved this from Reviewed to Merged May 3, 2023
The SCI Campaign automation moved this from In Review to Done May 3, 2023
AenBleidd added a commit to AenBleidd/boinc that referenced this pull request May 5, 2023
Add CFBundleShortVersionString to macOS app plist files

Signed-off-by: Vitalii Koshura <lestat.de.lionkur@gmail.com>
AenBleidd added a commit to AenBleidd/boinc that referenced this pull request May 5, 2023
Add CFBundleShortVersionString to macOS app plist files

Signed-off-by: Vitalii Koshura <lestat.de.lionkur@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Fix BOINC version numbering on MacOS BOINC Manager macOS Spotlight search result says "Unknown Version"
4 participants