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

File object enhancements #1180

Merged
merged 42 commits into from Nov 13, 2012
Merged

File object enhancements #1180

merged 42 commits into from Nov 13, 2012

Conversation

skurfer
Copy link
Member

@skurfer skurfer commented Oct 17, 2012

This brings together one of @tiennou's branches with one of mine.

He made a number of improvements to the way file objects are dealt with, including caching frequently used attributes to avoid repeated disk access. Both of us have removed a lot of unused code. I've also rebased it against the current master, so it shouldn't conflict.

My changes were mostly about allowing more control over preview icons. See #1177 for details. The one difference from that is I've restored the old default behavior, so previews will be generated for everything. Users have the option to control previews via a hidden preference. To test, try something like

defaults write com.blacktree.Quicksilver QSFilePreviewTypes '(public.image, public.movie, public.audio, com.adobe.pdf)'

This also includes all the commits from #1119 except for 16cb992, as it sounds like that was unresolved.

skurfer and others added 23 commits Oct 15, 2012
This allows any future operations that depend on UTI to grab it from
memory instead of (potentially) hitting the disk again.
Other changes:
* Unused variables/statements removed
* Code specific to preference panes removed (It was never hit anyway)
* Attempts to `initWithContentsOfFile:` removed (It was never getting
hit - Quick Look takes care of it)
* Special case code from plug-ins is now tried first (and based on UTI
instead of extension)

close #1149
- Apply DRY on those that do the same thing. The specific check for 'fold' was dropped because it's a standard type.
- Replace some UTI constants.
I don't want to know what would happen in case the file wasn't on the same drive than ~/.Trash… Hopefully it looks it's only used by the update code when moving the old QS app to the Trash.
Conflicts:
	Quicksilver/Code-QuickStepCore/QSObject_FileHandling.h
Provide accessors for `-isPackage`, `-isAlias`, `-fileExtension`, and `-fileUTI`. Also changes `-isApplication` & `-isFolder` to use the cached LSItemInfoRecord.
You should never touch an NSError object before checking that an error was triggered, in case the callee decides to set the pointer to garbage.
This would force a restart in case a plugin declares one of those and gets loaded late.
This is a special case for pref panes that draws the pane's icon in the old-style preference pane template. Now that we have _real_ previewing for those, that's useless ;-).
Conflicts:
	Quicksilver/Code-QuickStepCore/QSObject_FileHandling.m
If we start assigning UTIs to all objects, this method could
potentially be expanded to evaluate everything (and moved to the main
QSObject code).
Users will have the ability to override this by setting a hidden default.
@skurfer skurfer mentioned this pull request Oct 17, 2012
Developers that aren't on 10.8 will be missing this macro.
@skurfer
Copy link
Member Author

@skurfer skurfer commented Oct 17, 2012

There's another potential area for optimization I noticed while resolving conflicts and looking through the changes. -[QSDirectoryParser objectsFromPath:depth:types:excludeTypes:descend:] looks at a lot of the things that are now cached (but it does so prior to creating the QSObject from the file, so the new LSItemInfoRecord cache can't be used). I wonder if we could move things around a bit to avoid reading the same information twice.

@tiennou
Copy link
Member

@tiennou tiennou commented Oct 17, 2012

Be aware that 70d34ac already changes the method. I don't know if it's what NSWorkspace uses, and I still haven't checked how it handles trashing files from network drives. In fact, I was keen to have that checked before, in order to cut the amount of code churn, but that's not really a problem.

@skurfer
Copy link
Member Author

@skurfer skurfer commented Oct 18, 2012

Be aware that 70d34ac already changes the method.

Yeah, but I didn't think anyone was disputing that change.

I don't know if it's what NSWorkspace uses, and I still haven't checked how it handles trashing files from network drives.

It makes the sound, but nothing happens. So that answers that. I guess we need another way.

skurfer added 2 commits Oct 19, 2012
This reverts commit 9c4c4d0.

Conflicts:
	Quicksilver/Code-QuickStepCore/QSObject_FileHandling.m
* only play the sound if at least one file was trashed successfully
* if any files couldn't be trashed, display a notification listing them
@skurfer
Copy link
Member Author

@skurfer skurfer commented Nov 6, 2012

OK, I've updated the Trash action. Take a look.

@skurfer
Copy link
Member Author

@skurfer skurfer commented Nov 9, 2012

Some small issues with this:

  1. The infoRecord method checks for a valid path before checking for cached results. This goes to disk on every cal, which is too expensive. I'm going to change it to check the cache first. If the file no longer exists, that should get picked up elsewhere.
  2. To the greatest extent possible, we want all the disk reads necessary to populate the infoRecord dict to take place during catalog scanning and not in real-time as someone uses the app. The dict isn't preserved across relaunches, but the file objects themselves are, which means they aren't rescanned and the infoRecord isn't repopulated until the user does something with the object in the UI. I'd like to see if we can cache the dict to disk with the objects.

skurfer added 4 commits Nov 9, 2012
This can be a bit slower than the old way if a lot of file objects don't have infoRecord cached, but it's not terrible (about 0.1s), and once the info is cached, it's about 10 times faster than the previous code.

This is a proof-of-concept. There are probably many other validation methods we could simplify.
Also, remove the now unused universalApps code.
close #1184
@skurfer
Copy link
Member Author

@skurfer skurfer commented Nov 9, 2012

OK, the commit to add all applications to “Open With…” is in. Let me know if there’s anything else.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Nov 10, 2012

There's something in here (or possible somewhere else between ß70 and here) that breaks right arrowing into text files.

I guess it's most likely here since this is where the fiddling with [QSObject loadChildrenForObject:] went on ;-)

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Nov 10, 2012

OK, here's the problem:
https://github.com/skurfer/Quicksilver/commit/c09deabb969b478582f0db7a3e98dcee55890d82

Caught you @tiennou ;-)

Seems like the return NO; shouldn't be there, otherwise the method never looks at the other handlers that could load children (like the text parser)


For @tiennou

And on the matter of that pull request: what if there's a method that say… returns 'NO' if there is an error, or if there's nothing to do or some other reason.

For example, take a look at: https://developer.apple.com/library/mac/#documentation/Cocoa/Reference/Foundation/Classes/NSFileManager_Class/Reference/Reference.html

This method returns NO if an error occurred OR "if a file, directory, or link already exists at path."
So in this case, how would you check to see if an error occurs? By checking if(error != nil) {} no?!

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Nov 10, 2012

Aaaand…. one more thing :)

I don't think that NSLog should be there at all. Try right arrowing into any file that shouldn't be right arrowed into (e.g. a .zip file or .jpg etc.) and watch your console.
Yucky yucky yuck!

Even if you right arrow into a text file, you still get that error log :(

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Nov 10, 2012

OK, last comment, I promise… :)

Just wrap lines 402 - 426 in an if ([object isFolder]) {} statement and we're all good :)

Should that method be isDirectory, since that's seems to be the convention, not isFolder?

@skurfer
Copy link
Member Author

@skurfer skurfer commented Nov 10, 2012

Just wrap lines 402 - 426 in an if ([object isFolder]) {} statement and we're all good :)

I can take care of that.

Should that method be isDirectory, since that's seems to be the convention, not isFolder?

For what it's worth, I had some weird problems when changing the validation around and it was because I was using isDirectory (thinking that was the name). There is a method by that name already, so we'd need need to be specific about the type of object in the loop, but I should have done that anyway.

So sounds like two votes for isDirectory. I'll fix that and the type assigned to the objects during validation.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Nov 10, 2012

So sounds like two votes for isDirectory. I'll fix that and the type
assigned to the objects during validation

Awesome, cheers :)

On 10 November 2012 14:05, Rob McBroom notifications@github.com wrote:

Just wrap lines 402 - 426 in an if ([object isFolder]) {} statement and
we're all good :)

I can take care of that.

Should that method be isDirectory, since that's seems to be the
convention, not isFolder?

For what it's worth, I had some weird problems when changing the
validation around and it was because I was using isDirectory (thinking
that was the name). There is a method by that name already, so we'd need
need to be specific about the type of object in the loop, but I should have
done that anyway.

So sounds like two votes for isDirectory. I'll fix that and the type
assigned to the objects during validation.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1180#issuecomment-10255444.

@skurfer
Copy link
Member Author

@skurfer skurfer commented Nov 10, 2012

Two new commits.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Nov 10, 2012

Cool. I'll run with it a little longer incase anything else crops up

@tiennou
Copy link
Member

@tiennou tiennou commented Nov 11, 2012

Sorry guys :-(. @skurfer your fix looks good.

Another quick thing : how do you feel about keeping -isFolder with the real "folder" meaning ? Something akin to return [self isDirectory] && !([self isPackage] || [self isApplication]. I'm sure that test is wrong but you see the point ;-).

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Nov 11, 2012

Another quick thing : how do you feel about keeping -isFolder with the real "folder" meaning ? Something akin to return [self isDirectory] && !([self isPackage] || [self isApplication]. I'm sure that test is wrong but you see the point ;-).

I think that's what isDirectory does now. Correct me if I'm wrong, but it
returns false if the file object is a package or application, yeah?

On 11 November 2012 13:17, Etienne Samson notifications@github.com wrote:

Sorry guys :-(. @skurfer https://github.com/skurfer your fix looks good.

Another quick thing : how do you feel about keeping -isFolder with the
real "folder" meaning ? Something akin to return [self isDirectory] &&
!([self isPackage] || [self isApplication]. I'm sure that test is wrong
but you see the point ;-).


Reply to this email directly or view it on GitHubhttps://github.com//pull/1180#issuecomment-10266574.

@skurfer
Copy link
Member Author

@skurfer skurfer commented Nov 11, 2012

I think that's what isDirectory does now. Correct me if I'm wrong, but it returns false if the file object is a package or application, yeah?

Worth testing to find out. You can see I'm looking at isPackage too when populating the third pane for the Move/Copy To action.

@tiennou
Copy link
Member

@tiennou tiennou commented Nov 11, 2012

Yeah, the old tests look way too fishy for me. Testing using ß70, it looks to me .bundle things (a.k.a packages) get a folder quick icon, just before the preview kicks in and override the default. We might want to have both -isDirectory and -isFolder, in case we need precision : the former means low-level multi-children thing, the latter means the thing you expect to open in a new Finder window ;-). But IIRC (I reimplemented File Objects in Ruby, remember ;-)), applications would return YES to -isDirectory calls kind because of this...

@skurfer
Copy link
Member Author

@skurfer skurfer commented Nov 12, 2012

I've confirmed that isDirectory returnsYES for applications and other packages (which is what I expected). If you want, I can add an isFolder method. I think it just needs to do what the Move/Copy To actions are doing now, which is

return ([self isDirectory] && ![self isPackage]);

Applications are packages, so no need to test isApplication.

Looking for directories that aren't packages is a pretty common task.
@pjrobertson pjrobertson merged commit bd5163a into quicksilver:master Nov 13, 2012
@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Nov 13, 2012

Fixed the conflict and pushed to master. Hopefully I got the merge conflict right ;-)

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Nov 13, 2012

Oh yeah - ß71 anyone? :)

@pjrobertson pjrobertson mentioned this pull request Nov 13, 2012
@skurfer
Copy link
Member Author

@skurfer skurfer commented Nov 13, 2012

Oh yeah - ß71 anyone? :)

Heh. I thought this is all we were waiting on, but now there's some tempting stuff out there. :-)

Probably take me all day just to add this one pull to the change log.

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