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

Unsupported plug-ins #968

Merged
merged 10 commits into from Jun 29, 2012
Merged

Unsupported plug-ins #968

merged 10 commits into from Jun 29, 2012

Conversation

skurfer
Copy link
Member

@skurfer skurfer commented Jun 29, 2012

A continuation of the work that began in #936.

Plug-ins that don’t provide the current architecture will appear disabled with a helpful status as before. In addition:

  • Plug-ins that are themselves supported, but can’t load due to a missing dependency will show as disabled with a helpful status
  • Missing dependencies will be installed (only if they aren’t already present on the system)
  • Dependencies for plug-ins that will never load are ignored

The original plan to ignore dependencies for plug-ins that didn’t load was a chicken-egg dead end. :-) A missing dependency prevents a plug-in from loading, which prevents dependencies for that plug-in from being checked, and without the dependencies it won’t load, etc. So I instead check to see if the plug-in can load.

Hopefully this is good to go and I can do another dev preview tomorrow.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jun 29, 2012

All looks good, except for the one small thing.

Great work powering through this and getting a pull request out!

I'll be travelling until about 6-7pm BST (1-2pm your time), at which point I'll be sure to try and merge this so you can get a new release out.

@skurfer
Copy link
Member Author

@skurfer skurfer commented Jun 29, 2012

I see you're deciding whether or not a plugin will load based solely on whether or not it meets the current arch.

Well, not really. I’m only deciding whether or not it should process dependencies. The comment/commit message probably could have been worded better. When I say “can’t load”, it’s technically correct, but I’m really only referring to one of many reasons why it might not load. And that happens to be the only reason we care about in this context.

I'm sure there will be other reasons why a plugin won't load (like a missing executable) but I don't expect this to be a problem really.

True, but the code to handle all that was already in place. Here’s the scenario I tested a few times:

  • Remove the E-mail support plug-in
  • Load the Mail plug-in which will work on it’s own
  • See that it recognizes the dependency and installs E-mail support
  • Relaunch and see that it doesn’t try to install e-mail support again

And you’ll then see both plug-ins correctly in the interface. They’ll show as disabled and the status will tell you why.

I see that I’ve broken Extra Scripts again, so I’ll fix that. There was also a warning that I don’t think applies any more and an isUniversal method that’s not used anywhere, so I think I’ll clean those up.

@skurfer
Copy link
Member Author

@skurfer skurfer commented Jun 29, 2012

New commits. Give it a look.

…instead of avoiding them in all cases, which would prevent updates from getting installed.
@skurfer
Copy link
Member Author

@skurfer skurfer commented Jun 29, 2012

OK, now it should be good.

pjrobertson added a commit that referenced this issue Jun 29, 2012
@pjrobertson pjrobertson merged commit 28f04ad into quicksilver:release Jun 29, 2012
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