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

Fallbacks #936

Merged
merged 13 commits into from Jun 25, 2012
Merged

Fallbacks #936

merged 13 commits into from Jun 25, 2012

Conversation

pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jun 14, 2012

These changes make the situations when an interface or string ranker doesn't load more graceful:

For both string ranker and interface loading they:

  • Display a notification to the user saying so
  • Switch to the default

I have made 'Bezel' the default fallback for the interface. It was previously Primer.
A few other bits and bobs have creeped in

pjrobertson added 4 commits Jun 14, 2012
…'t be loaded

Notify the user with a message popup
When copying e.g. an image, no 'dataForType:NSStringPboardType' exists, meaning originally, the dictionary would be created with nil objects (the objects array would start with 'nil') and 13 keys.
@skurfer
Copy link
Member

@skurfer skurfer commented Jun 14, 2012

For both string ranker and interface loading they:

There’s only one string ranker and it works with 64-bit, but something like this would have come in handy when I made that change recently, so it’s probably good to leave it in. :-)

I have made 'Bezel' the default fallback for the interface. It was previously Primer.

I don’t know about this. Of course my personal preference is for wider interfaces that show more text, but that aside, it’s called “Primer” for a reason. I think it’s a bit easier to deal with for new users (labels, buttons, hints, etc.).

A few other bits and bobs have creeped in

Like the pasteboard thing?

@skurfer
Copy link
Member

@skurfer skurfer commented Jun 14, 2012

The interface changes, but the one shown in the prefs doesn't. Was that intentional so it will go back when their chosen interface is updated? Fine with me.

So I guess this works by checking for the interface in kQSCommandInterfaceControllers? And it does work, but why? All indications are that the interfaces load (and you can still see them in the prefs). If we can figure out how to detect load failure, we can show the plug-ins as disabled.

What almost works is to throw an exception in -[QSRegistry handleRegistration:] if instance is nil. That causes it to show up gray and on the Disabled list, and the status will be set to whatever message you include in the exception (with no additional code changes). Nice, right? But for some reason, instance is valid for the majority of 32-bit plug-ins.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jun 14, 2012

There’s only one string ranker and it works with 64-bit, but something
like this would have come in handy when I made that change recently, so
it’s probably good to leave it in. :-)

That's why I did it, thinking back to our conversation a few weeks back :)

I don’t know about this. Of course my personal preference is for wider
interfaces that show more text, but that aside, it’s called “Primer” for a
reason. I think it’s a bit easier to deal with for new users (labels,
buttons, hints, etc.).

Good point, I'll change it back to Primer. I agree it is much easier to
understand.

Like the pasteboard thing?

Yeah, that and just using a consistent kQSBundleID everywhere

The interface changes, but the one shown in the prefs doesn't. Was that
intentional so it will go back when their chosen interface is updated? Fine
with me.

That was intentional. I don't want to change users' settings just because
an interface didn't load for some strange reason. The notification would
display on each launch, so that's good enough.
I did half - implement a warning triangle in the prefs for when the
interface is 'wrong' but got lost with KVO so stopped :)

So I guess this works by checking for the interface in
kQSCommandInterfaceControllers? And it does work, but why? All
indications are that the interfaces load (and you can still see them in the
prefs). If we can figure out how to detect load failure, we can show the
plug-ins as disabled.

Yeah, this is the 3rd thing we need to fix. I knew that you'd been looking
into it so didn't include it here.

On 14 June 2012 16:20, Rob McBroom <
reply@reply.github.com

wrote:

The interface changes, but the one shown in the prefs doesn't. Was that
intentional so it will go back when their chosen interface is updated? Fine
with me.

So I guess this works by checking for the interface in
kQSCommandInterfaceControllers? And it does work, but why? All
indications are that the interfaces load (and you can still see them in the
prefs). If we can figure out how to detect load failure, we can show the
plug-ins as disabled.

What almost works is to throw an exception in -[QSRegistry handleRegistration:] if instance is nil. That causes it to show up gray
and on the Disabled list, and the status will be set to whatever message
you include in the exception (with no additional code changes). Nice,
right? But for some reason, instance is valid for the majority of 32-bit
plug-ins.


Reply to this email directly or view it on GitHub:
#936 (comment)

@skurfer
Copy link
Member

@skurfer skurfer commented Jun 14, 2012

I knew that you'd been looking into it so didn't include it here.

I’ve stopped, so feel free. :-) I can revisit it, but I’m making good progress on the Spotlight plug-in, so I plan to keep at that for now.

@skurfer
Copy link
Member

@skurfer skurfer commented Jun 22, 2012

OK, I think I’ve got a way to detect plug-ins without x86_64. Put something like this at the top of QSPlugIn(Registry) _registerPlugIn:

NSRunningApplication *Quicksilver = [[NSRunningApplication runningApplicationsWithBundleIdentifier:kQSBundleID] objectAtIndex:0];
NSNumber *myArch = [NSNumber numberWithInteger:[Quicksilver executableArchitecture]];
if (![[bundle executableArchitectures] containsObject:myArch]) {
    [NSException raise:@"QSWrongPluginArchitecture" format:@"Current architecture unsupported"];
}

That prevents non-working interfaces from showing up as choices in Appearance, causes all 32-bit plug-ins to get listed under the Disabled section, and the message from the exception shows as the status in the plug-in’s info panel.

Some outstanding issues:

  • The first two lines obviously only need to be run once per launch. Maybe that could be set as kQSCurrentArchitecture in QSDefines.h or something.
  • Speaking of constants, kQSBundleID only exists in this branch, so this change won’t function unless it comes along with (or after) this pull.
  • I keep getting notifications about Chat Support, E-mail Support, etc. being installed. I assume because they failed to load, but other plug-ins require them. The logic for checking requirements appears to be “If not loaded, install”. It should probably be changed to “If not installed, install, else if not enabled, enable”. In addition, much of this could be avoided if QSPlugInManager checkForUnmetDependencies would ignore dependencies for plug-ins that aren’t loaded.

Not saying all of this is your problem, but it seems to fit with the overall goals of this pull request and it depends on some of the changes you’ve already made here.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jun 24, 2012

Do you really think it's worth having the NSException? It massively clutters up the console.
Also - this fails for the Extra Scripts plugin, which doesn't have an executable file.

P.S.

NSRunningApplication *Quicksilver = [NSRunningApplication currentApplication];

works fine for getting the QS running app instance

@skurfer
Copy link
Member

@skurfer skurfer commented Jun 25, 2012

Do you really think it's worth having the NSException? It massively clutters up the console.

Well, the call to _registerPlugIn is wrapped in a try/catch already, and the message gets set as the plug-in’s status. So this seemed like the easiest way to get what we want without making too many changes. Of course there are other ways to do it. (Would it log all those messages for a Release build, or just in Debug?)

Also - this fails for the Extra Scripts plugin, which doesn't have an executable file.

True, so if there is no executable, I guess we have to assume everything’s fine.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jun 25, 2012

The exceptions seem to just be for Debug builds, which is fine.

I've added another commit with your changes (modified slightly)

Give a useful error message in the plugins preferences and make them greyed out.
@skurfer
Copy link
Member

@skurfer skurfer commented Jun 25, 2012

I've added another commit with your changes (modified slightly)

OK, looks good, but do you think myArch should be a constant so it doesn’t have to be processed for every plug-in?

Are you going to address the unnecessary installation of dependencies and the missing executable stuff, too? If you don’t have time, I can do it after we merge this.

@skurfer
Copy link
Member

@skurfer skurfer commented Jun 25, 2012

Merging this in spite of known issues, as I intend to address them today.

skurfer added a commit that referenced this issue Jun 25, 2012
@skurfer skurfer merged commit 7fbbcb2 into quicksilver:release Jun 25, 2012
@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jun 25, 2012

Cool, thanks

On 25 June 2012 20:12, Rob McBroom <
reply@reply.github.com

wrote:

Merging this in spite of known issues, as I intend to address them today.


Reply to this email directly or view it on GitHub:
#936 (comment)

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