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 OS version checking to QSRequirements #1629

Merged
merged 1 commit into from Oct 4, 2013
Merged

add OS version checking to QSRequirements #1629

merged 1 commit into from Oct 4, 2013

Conversation

skurfer
Copy link
Member

@skurfer skurfer commented Oct 3, 2013

Pretty small and self-explanitory.

I mainly did this so the new File Tagging plug-in can require 10.9.

I've tested this under 10.9, by the way. It works if you specify either "10.9.0" or "10.9" as the required OS.

Once merged, I'll update the plug-in dev reference.

@skurfer
Copy link
Member Author

@skurfer skurfer commented Oct 3, 2013

Maybe we should also do something on the web server side to make sure minSystemVersion gets populated with this info? And having looked at that, maybe I should allow support for both a min and max OS?

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Oct 4, 2013

Maybe we should also do something on the web server side to make sure minSystemVersion gets populated with this info? And having looked at that, maybe I should allow support for both a min and max OS?

Seems to make sense to me. I guess we don't really want users of 10.7/8 won't seeing the File Tagging plugin.
For v1.1 I fixed the code for setting the minSystemVersion for Quicksilver itself server-side, and since it's treated as a plugin by the server I think it should be easy to do the same for plugins, if it's not already done.

(plugin updater code isn't in GitHub for security reasons)

@skurfer
Copy link
Member Author

@skurfer skurfer commented Oct 4, 2013

I've added a max check as well. The only trick with that is if you were to say "10.8.6" is the max supported version and Apple puts out 10.8.7, it'll quit working, though that probably wasn't what you meant.

If you meant to say "10.6.X", we can work around that by putting something like "10.6.20", which is unlikely to be reached. Ugly, but it'll be so rare that I think we can let it stand. I'll make a note of all this in the dev reference.

@tiennou
Copy link
Member

@tiennou tiennou commented Oct 4, 2013

I've added a max check as well. The only trick with that is if you were to say "10.8.6" is the max supported version and Apple puts out 10.8.7, it'll quit working, though that probably wasn't what you meant.

Make the test "this this the first unsupported version" then. That'll be cleaner. Mathematically it's a [minVersion...maxVersion[ set.

Reply to this email directly or view it on GitHub.

@skurfer
Copy link
Member Author

@skurfer skurfer commented Oct 4, 2013

Make the test "this this the first unsupported version" then.

Will do. Then I'm going to squash the 3 commits and force-push.

`osRequired` is the minimum version needed for the plug-in
`osUnsupported` is the version at which a plug-in will stop working
@skurfer
Copy link
Member Author

@skurfer skurfer commented Oct 4, 2013

Done. Have a look.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Oct 4, 2013

Looks good. A few small things:

It might be work #defining the osSupported and unsupported strings. Maybe one day we can have the defines all in one handy header file, and it'll be easy to export to the plugin dev ref :)

Those files are in the QSCore framework. Do we need to use a different NSLocalizedString call (NSLocalizedStringFromTableInBundle) @tiennou ?

I'm going to bed now, so feel free to do what you think is right and merge then release.

@skurfer
Copy link
Member Author

@skurfer skurfer commented Oct 4, 2013

I'm pretty sure the localization function is correct there. The other is only needed in plug-ins. (My belief is strengthened by scrolling up and seeing several existing calls to NSLocalizedString() in that method.)

Not opposed to the #define idea, but doesn't seem worth it right now.

skurfer added a commit that referenced this issue Oct 4, 2013
add OS version checking to QSRequirements
@skurfer skurfer merged commit e3c01cf into master Oct 4, 2013
@skurfer skurfer deleted the osreq branch Oct 4, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants