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

enhance Quicksilver version checking #1835

Merged
merged 2 commits into from May 13, 2014
Merged

enhance Quicksilver version checking #1835

merged 2 commits into from May 13, 2014

Conversation

skurfer
Copy link
Member

@skurfer skurfer commented May 6, 2014

implement minHostVersion and maxHostVersion

As discussed on the list.

I’ve already put a version of the Cube interface out that advertises its lack of support beyond 1.1.3. It’s a little messy, since the update system prevents you from seeing that update if you’re already on 1.2.0. But those people have surely discovered by now that the Cube crashes QS. People still on 1.1.3 should get the update.

We need to make a small tweak on the remote side for the update system. When it’s returning a list of plug-ins that support the current version, it will still list the plug-in if maxHostVersion is the current version + 1. So maxHostVersion is acting like “minimum unsupported version” instead of “maximum supported version”. I think. Basically, I had to add 1 to what was in the database (16391 → 16392) to get the right behavior. :-)

implement `minHostVersion` and `maxHostVersion`
@skurfer skurfer added this to the 1.2.0 milestone May 6, 2014
@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented May 8, 2014

Oh dammit, I just accidentally deleted your comment, but I copied it in the meantime...

@skurfer said earlier:

I changed line 56 of plugin.class.php from maxHostVersion > \"$value\" to maxHostVersion >= \"$value\". Let me know if you think that’s wrong.

Thinking about it, that seems right. By the same logic, shouldn't maxSystemVersion (3 lines after) be the same as well then?

I didn't see the need for us to include this in v1.2, but I guess you're going along the lines that the Cube interface might one day be fixed, so we shouldn't obsolete it (although, it it is fixed we can just change the plugin ID)
How about that?

@skurfer
Copy link
Member Author

@skurfer skurfer commented May 8, 2014

Well, I was thinking Cube should remain available for download, but I guess obsoleting it won’t prevent that in older builds. Seems a bit harsh, though. 😃

By the same logic, shouldn't maxSystemVersion (3 lines after) be the same as well then?

I think that’s a little different. In that case, the setting the “user” (plug-in developer) sees is osUnsupported, which means “this or higher is not supported”. With the Quicksilver build we’re saying “supported up to this number”.

Yes, that’s inconsistent, but I think there was a reason. Maybe because we control QS releases, but not OS releases? I’d have to look back at when I added the OS checks, but no time right now.

pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented on 03af562 May 9, 2014

Choose a reason for hiding this comment

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

Does this mean we should now be documenting the use of minHostVersion instead of version (Obsolete that one?)

Also, they should probably go in some header file as #define with a little comment explaining what they do... maybe? Maybe even a doxygen style comment? :)

skurfer
Copy link
Member Author

@skurfer skurfer commented on 03af562 May 9, 2014

Choose a reason for hiding this comment

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

Does this mean we should now be documenting the use of minHostVersion instead of version (Obsolete that one?)

The plug-in documentation has mentioned both for a long time (without me realizing one was never implemented). I see no reason not to keep supporting both.

Also, they should probably go in some header file as #define with a little comment explaining what they do... maybe? Maybe even a doxygen style comment? :)

Ugh. Probably. :-)

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented May 9, 2014

Looks good, just for my small (and no doubt annoying!) comment

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented May 9, 2014

I said:

Does this mean we should now be documenting the use of minHostVersion instead of version (Obsolete that one?)

Apologies, I've just looked at the dev ref and you've done this. Maybe the reference to version should be mentioned as obsolete (as long as the server deals with minHostVersion)
NOTE: I've just noticed you say version and minSystemVersion. That should be minHostVersion (and maxHostVersion)

I think that’s a little different. In that case, the setting the “user” (plug-in developer) sees is osUnsupported, which means “this or higher is not supported”. With the Quicksilver build we’re saying “supported up to this number”.

Aaah OK I see, in that case you're right :)

Edit: I can confirm the server first checks the requirements dict for minHostVersion, then for version if that's missing.

@skurfer
Copy link
Member Author

@skurfer skurfer commented May 9, 2014

I think I remember why OS is different. Say 10.7.0 is released when 10.6.5 is the latest. We find that something is broken on 10.7.0. We can’t just say “support up to 10.6.5” because then 10.6.6 - 10.6.8 come along and cause the plug-in to be disabled, when it really doesn’t need to be. So instead we say “don’t support 10.7.0 or later”, which is what we actually mean.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented May 10, 2014

Ugh. Probably. :-)

I wasn't sure if that meant you were going to add it to this pull or not? :P

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented May 10, 2014

I've updated the plugin dev ref: 85767be

@skurfer
Copy link
Member Author

@skurfer skurfer commented May 10, 2014

I wasn't sure if that meant you were going to add it to this pull or not?

Yeah, I plan to. Thought it would be done shortly after the comment, or I would have been more clear. Sorry.

I've updated the plugin dev ref

Thanks!

Also - document all keys in a doxygen style way
@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented May 13, 2014

Isn't documenting something always the best way to figure it out?

Well I've gone and documented/#defined the keys. Man that's a mess. I have no idea why the frameworks dict requires another resource dict inside it, and why there's no version key for the framework etc. etc., but I've documented the whole bang.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented May 13, 2014

So now it just needs you to merge before the next release ;-)

skurfer added a commit that referenced this issue May 13, 2014
enhance Quicksilver version checking
@skurfer skurfer merged commit f2b6c70 into master May 13, 2014
1 check passed
@skurfer skurfer deleted the versionCheck branch May 13, 2014
@skurfer
Copy link
Member Author

@skurfer skurfer commented May 13, 2014

Thanks. Sorry for the delay. Work has been ridiculous lately.

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

2 participants