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

When using Move to.../Copy to... set the 3rd pane object to the parent directory of the file to be moved/copied #665

Merged
merged 6 commits into from Jan 31, 2012

Conversation

pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jan 26, 2012

Does exactly what it says on the tin.

Short of a better place to start, I think the 'current' folder is typically quite near where you want to move things to. I found myself doing this a lot lately, so thought I'd implement it.

It's certainly better than a completely random folder

@skurfer
Copy link
Member

@skurfer skurfer commented Jan 27, 2012

By the way, I love this change. I always saw the weirdest folders pop up in the third pane.

@skurfer
Copy link
Member

@skurfer skurfer commented Jan 27, 2012

If I try to move something out of ~/Downloads, the parent folder in the third pane has a semi-transparent generic folder icon. I think this is because you’re creating the object from scratch every time. Something like this seems to fix it (and probably runs faster for things already in the catalog).

NSString *currentFolderPath = [[[[dObject splitObjects] lastObject] singleFilePath] stringByDeletingLastPathComponent];
QSObject *currentFolderObject = [QSObject objectWithIdentifier:currentFolderPath];
if (!currentFolderObject) {
    // if it wasn't in the catalog, create it from scratch
    currentFolderObject = [QSObject fileObjectWithPath:currentFolderPath];
}

Also, did you notice if you set a breakpoint in this method, it gets run twice? Completely unrelated to this pull request, but it sounds like a good opportunity for a speed-up somewhere.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jan 27, 2012

:) I like how you're improving on this!

I'll make the changes when I get home. (OK, I'm now home and making the changes. Completely forgot about this - work was too fun! :P)

Also, did you notice if you set a breakpoint in this method, it gets run twice? Completely unrelated to this pull request, but it sounds like a good opportunity for a speed-up somewhere.

I've seen that in hundreds and hundred of places with Quicksilver. If we can stop it, then Quicksilver will definitely be much, much faster!

This commit:

* Obtains the list of objects more cleanly (using mutableCopy)
* Ensures a new file object for the folder isn't created if it already exists
* Performs a more robust check for bundles (using NSWorkspace's `isFilePackageAtPath`)
* Creates one instance of NSFileManager as opposed to thousands (for each run of the for loop)

Thanks to @skurfer for tips
@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jan 27, 2012

Take a look and see what you think :)

P.S. - you may have just omitted it in your line note, but a mutableCopy retains the object so we need to release it when we're done :)

@skurfer
Copy link
Member

@skurfer skurfer commented Jan 27, 2012

I've seen that in hundreds and hundred of places with Quicksilver. If we can stop it, then Quicksilver will definitely be much, much faster!

Yeah, but usually something runs 3 times (once per pane, I assume) or some multiple of 3. Probably pretty tricky to find and fix those. But this is twice, which makes me think it’s probably just a silly mistake somewhere.

As for how the folder object gets created, I looked at fileObjectWithPath: and it’s already doing a check for existing objects so in theory, my fix should be redundant, but in practice, it seemed to fix the issue with the icon.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jan 27, 2012

my fix should be redundant, but in practice, it seemed to fix the issue with the icon.

Yeah looking at fileObjectWithPath: it looks completely redundant. I'll look into it.
I haven't noticed the 'running twice' problem you're talking about, but I'll look into it

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jan 27, 2012

FWIW: I still saw the 'semi' transparent problem with your fix, but it was pretty random. Removing your (redundant) check makes no difference for me.

And... the 'running twice' business is: The method is run however many times you type a letter.

e.g. if you type 'm' for 'Move To...' the method is run once.
If you type 'moveto' the method is run 6 times! Good catch, now to fix... :o

pjrobertson added 3 commits Jan 27, 2012
This is already done in `fileObjectWithPath
searchArray is an NSMutableArray, yet these methods refer to NSArrays.
Code has been checked for all occurrences of `setSearchArray` and they all set an NSMutableArray
@skurfer
Copy link
Member

@skurfer skurfer commented Jan 27, 2012

If you type 'moveto' the method is run 6 times! Good catch, now to fix

Ah, this could be a result of having the “Wait before searching” preferences set to 0.0. I’ll bet if I up that, the method will only have to run once.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jan 27, 2012

OK you have many, many commits to look at now! I thought this was going to be an easy one...

So the fix for the 'running twice' part was to check the actual objects as opposed to the ranked objects (which obviously aren't the same as you alter your search)

The commits also:

  • Reverts the check to see if the file object exists, as fileObjectWithPath: does this
  • Properly defined some setter/getters that were giving a warning. From what I can see, they're always NSMutableArrays
  • Moved the 'disabling catalog' part to run for debug builds only. This will mean we'd have to remove the section on 'Catalog' here. I think we'd gain more from users not reporting problems than we would from users not being able to debug a crash. Who's going to go through altering their .plists from 'YES' to 'NO' anyway?!

1 similar comment
@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jan 27, 2012

OK you have many, many commits to look at now! I thought this was going to be an easy one...

So the fix for the 'running twice' part was to check the actual objects as opposed to the ranked objects (which obviously aren't the same as you alter your search)

The commits also:

  • Reverts the check to see if the file object exists, as fileObjectWithPath: does this
  • Properly defined some setter/getters that were giving a warning. From what I can see, they're always NSMutableArrays
  • Moved the 'disabling catalog' part to run for debug builds only. This will mean we'd have to remove the section on 'Catalog' here. I think we'd gain more from users not reporting problems than we would from users not being able to debug a crash. Who's going to go through altering their .plists from 'YES' to 'NO' anyway?!

@skurfer
Copy link
Member

@skurfer skurfer commented Jan 27, 2012

Increasing the time before searching did fix the double call, but your fix is still necessary because as I type, the action isn’t changing and so there should be no need to re-validate. Good catch.

Properly defined some setter/getters that were giving a warning.

Maybe this is pre-mature optimization, but it doesn’t need to be mutable, right? There doesn’t seem to be an immutableCopy method though. :-)

@lovequicksilver
Copy link

@lovequicksilver lovequicksilver commented Jan 27, 2012

Great! I got 'Scripts archive iPhoto' in pane 3. On relaunch it became 'Public', and now it's 'Example materials and photos'…

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jan 28, 2012

Maybe this is pre-mature optimization, but it doesn’t need to be mutable, right? There doesn’t seem to be an immutableCopy method though. :-)

I guess not, but is it worth us changing it? I think it may be a bit premature considering the optimisation gains would be minimal, but the consequences if anything every does try to alter the array would be catastrophic.

@skurfer
Copy link
Member

@skurfer skurfer commented Jan 28, 2012

I guess not, but is it worth us changing it?

Nah, leave it.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jan 31, 2012

Is there anything more that needs to be done on this?

I know the changes here grew to a bit more than just the move to.../copy to... action, so I guess it might take a bit more time to test.

@skurfer
Copy link
Member

@skurfer skurfer commented Jan 31, 2012

Is there anything more that needs to be done on this?

No, I’ll probably merge this and a couple of your others this afternoon.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jan 31, 2012

Cool, thanks :)

On 31 January 2012 18:25, Rob McBroom <
reply@reply.github.com

wrote:

Is there anything more that needs to be done on this?

No, Ill probably merge this and a couple of your others this afternoon.


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

skurfer added a commit that referenced this issue Jan 31, 2012
When using Move to.../Copy to... set the 3rd pane object to the parent directory of the file to be moved/copied
@skurfer skurfer merged commit 780f465 into quicksilver:master Jan 31, 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

3 participants