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

Small bug fixes #494

Merged
merged 4 commits into from
Oct 13, 2011
Merged

Conversation

pjrobertson
Copy link
Member

Two things:

  • Make the eject action work with the comma trick.
    I've also updated to a 10.6 method for ejection (this method now ejects all volumes for the specified mounted drive). e.g. if you have a drive with 3 partitions and ask to eject one, it'll eject all 3 - not sure if this is unwanted or not.
  • Fix a bug I introduced when fixing triggers to work with objects that aren't saved in the catalog (found out about it with the 'show playing track' trigger. Basically, QS should attempt to guess the object from the string last (if all else fails)

Shouldn't have been working on these, but with no internet I can't really work on the plugin system :(

… saved in the catalog bug'

Basically, sniffing the object's string to create the object should be the last thing tried, or it messes up other methods of creating the object
Also made a few tidy ups. It was pretty messy before :(
Plus: Updated to a 10.6 method for ejecting. Previously, the unmountAndEjectDeviceAtPath method was failing almost every time and the Finder script was used instead.
This 10.6 method ejects all the mounted volumes from a single device.
@pjrobertson
Copy link
Member Author

Fixes #222

if (!object) {
// sniffs the string to create a new object
object = [QSObject objectWithString:directID];
object = [QSAction actionWithIdentifier:directID];
Copy link
Member

Choose a reason for hiding this comment

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

Should the direct object be set to a QSAction here? Seems odd, but maybe there’s a reason.

Copy link
Member Author

Choose a reason for hiding this comment

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

This was in Alcor's original code, but I took it out when I made the first
changes in this area. (Actually it may have been Etienne that put it in,
can't remember, don't have the internet capabilities atm to check - sorry!)
I took it out as we had no idea what it did.

This time around I left it in because I thought I shouldn't necessarily
change things just because I don't understand them.

On 5 October 2011 14:01, Rob McBroom <
reply@reply.github.com>wrote:

          if (!object) {
  •                 // sniffs the string to create a new object
    
  •                 object = [QSObject objectWithString:directID];
    
  •                 object = [QSAction actionWithIdentifier:directID];
    

Should the direct object be set to a QSAction here? Seems odd, but maybe
theres a reason.

Reply to this email directly or view it on GitHub:
https://github.com/quicksilver/Quicksilver/pull/494/files#r155754

Copy link
Member

Choose a reason for hiding this comment

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

I keep a copy of the B54 code (or at least it’s the oldest commit we have) for reference. There’s no mention of QSAction in that method. My guess is something got copied and pasted, but not changed to suit the new context. I say yank it.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK then I'm pretty sure Etienne put it in when he made the changes there
before me. I'll yank it :)

On 6 October 2011 19:34, Rob McBroom <
reply@reply.github.com>wrote:

          if (!object) {
  •                 // sniffs the string to create a new object
    
  •                 object = [QSObject objectWithString:directID];
    
  •                 object = [QSAction actionWithIdentifier:directID];
    

I keep a copy of the B54 code (or at least its the oldest commit we have)
for reference. Theres no mention of QSAction in that method. My guess is
something got copied and pasted, but not changed to suit the new context. I
say yank it.

Reply to this email directly or view it on GitHub:
https://github.com/quicksilver/Quicksilver/pull/494/files#r158319

@pjrobertson
Copy link
Member Author

Sorry, thought I'd already pushed the fix here, but it was on another branch.

Should be fine now :)

@skurfer
Copy link
Member

skurfer commented Oct 10, 2011

Some observations:

  1. The eject action shows up when I have multiple volumes selected, so the validation appears to be working
  2. Eject isn’t working for network volumes (but that’s true with or without these changes)
  3. The Show Playing Track trigger doesn’t work of me when I have this merged in. I’ll see if I can figure it out with the debugger, but if you know off the top of your head, feel free to chime in. :)

@skurfer
Copy link
Member

skurfer commented Oct 10, 2011

Wait, maybe Show Playing Track does work. Tried it again without anything else merged in. Still investigating.

@skurfer
Copy link
Member

skurfer commented Oct 10, 2011

Well, now I don’t know what to think, but long story short, this looks good. I’ll run with it for a bit before merging.

@pjrobertson
Copy link
Member Author

This definitely fixed the show playing track problem for me. You can see if
it's working by checking that the trigger has the iTunes icon.

Probably worth opening an issue for eject - network volumes.

On 10 October 2011 16:32, Rob McBroom <
reply@reply.github.com>wrote:

Well, now I dont know what to think, but long story short, this looks
good. Ill run with it for a bit before merging.

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

@skurfer
Copy link
Member

skurfer commented Oct 11, 2011

This definitely fixed the show playing track problem for me. You can see if it's working by checking that the trigger has the iTunes icon.

Yes, I see that. I had it merged with a bunch of other stuff when it wasn’t working, although I am now running with the same stuff and it’s fine. It seems that I have to remove /tmp/QS completely to accurately test it. Here’s what I found:

  • Show Playing Track doesn’t work with master
  • It works with this merged in
  • It works with everything but this merged in (of the things currently going into the mine branch)… so it’s double fixed? Maybe your triggers branch?

Anyway, you can see why I gave only the short version. :)

Probably worth opening an issue for eject - network volumes.

Yeah.

@pjrobertson
Copy link
Member Author

Well. I tried my triggers branch and it didn't fix the problem for me :/

I looked through the code and couldn't see anywhere where I may have fixed the problem. I think this is good to go :)

skurfer added a commit that referenced this pull request Oct 13, 2011
@skurfer skurfer merged commit d8b4bdc into quicksilver:master Oct 13, 2011
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