set initial file icons based only on type

[skurfer]

A small change that should fix #1431.

If you look at the comment above my change in setQuickIconForObject: (or if you just think about it), it should have been this way all along.

I wonder if it was in the past, because I remember iconForFile: was being called in both methods. I removed the call from loadIconForObject: at one point, but I've put it back with this commit.

[pjrobertson]

Seems sensible, and about right. Have you tested to see what the actual icons are like? (Put return YES at the top of loadIconForObject: so that the real icon is never loaded)

Since folders don't have extensions, this doesn't work. It may be better to use NSHFSTypeOfFile() and put a HFS type in iconForFileType (it accepts extensions or HFS file types. But then NSHFSTypeOfFile() might be slow as well. It might make sense to cache that value like you've done for most other interesting file flags.

...although I've just tested NSHFSTypeOfFile() and it always seems to return nil. Can't investigate too much more atm.

One other thing - can this be held off until #1488 is merged - your if(!theImage) block could be merged into my else {} block in that pull.

[skurfer]

Seems sensible, and about right. Have you tested to see what the actual icons are like? (Put return YES at the top of loadIconForObject: so that the real icon is never loaded)

No need for that. All you have to do is look at the contents for a catalog entry. I've always known that part of the UI wouldn't update, but it's especially noticeable (and annoying) now.

I thought it was because the prefs views aren't watching for QSObjectIconModified, but if you make the icon for something load in the main UI, the prefs seem to update just fine.

Now I think that panel just calls icon, which doesn't call loadIcon, and that's why they never update. I'm not sure how we want to address that (or if we should leave it alone).

Since folders don't have extensions, this doesn't work.

In practice, the icon always seems to be available, with the exception of the catalog prefs. But since folders are so common, I think it's worth adding a special case for them. Maybe a special case for volumes, too? Once we have that, I wonder if it's worth the effort to make the catalog prefs call loadIcon at all.

One other thing - can this be held off until #1488 is merged - your if(!theImage) block could be merged into my else {} block in that pull.

I'm running with them merged together and I think it makes sense as is. That is, your else block should only be triggered if all attempts to load the icon fail. My change is the last of those attempts, so it should happen before that test, right?

[skurfer]

New commit for folders, but all isn't perfect.

For example, launch QS and hold / to show the hard drive. The correct icon is loading, but for some reason, the QSObjectView isn't updating. The problem was probably introduced with my background loading changes, and isn't caused by anything here, but this is as good a place as any to fix it. I'm looking into it.

[pjrobertson]

In practice, the icon always seems to be available, with the exception of the catalog prefs. But since folders are so common, I think it's worth adding a special case for them

Hmm... I still think this should be done properly - we look at the actual file type, since now for anything without an extension we just get the generic icon file. Albeit not for very long, but annoying in the prefs as you say.
...ok so with a reliable internet connection I've been able to play a little more.

If you just do theImage = [[NSWorkspace sharedWorkspace] iconForFileType:[object singleFileType]]; instead then we get everything we wanted :)

... The problem was probably introduced with my background loading changes, and isn't caused by anything here, but this is as good a place as any to fix it. I'm looking into it.

OK, I'll let you look into it

I'm running with them merged together and I think it makes sense as is. That is, your else block should only be triggered if all attempts to load the icon fail. My change is the last of those attempts, so it should happen before that test, right?

My else block is to make sure 'static' icons aren't updated. I think that icons for a specific filetype can be considered 'static' (although they can change if the user changes the default app for a given filetype. E.g. change .txt files from TextEdit to TextMate and the icon will change), so perhaps they're not static.
I'm basically trying to reduce the number of (expensive) calls to the Quicklook API to get the icon, so I think making these icons permanent is OK.

Now I think that panel just calls icon, which doesn't call loadIcon, and that's why they never update. I'm not sure how we want to address that (or if we should leave it alone).

Yeah it should really show the real icon, I'll look into it

[skurfer]

If you just do theImage = [[NSWorkspace sharedWorkspace] iconForFileType:[object singleFileType]]; instead then we get everything we wanted :)

Not quite. What we wanted was to avoid ever asking the disk for more information about the file. If you follow singleFileType down to NSFileManager (BLTRExtensions), you'll see things that hit the disk. The things I've told it to check (extension and isFolder) use information that's cached when the object is created.

Now granted, singleFileType was created before @tiennou added all the file object metadata, so it could probably be rewritten to do something similar to what I've done in setQuickIcon… using only cached information.

... The problem was probably introduced with my background loading changes, and isn't caused by anything here, but this is as good a place as any to fix it. I'm looking into it.

OK, I'll let you look into it

It's because holding / calls setObject: instead of selectObject:, which never adds the observer. Changing those places to call selectObject: instead fixes it, but feels wrong. If setObject: is putting things in the first pane, their icons should be observed too. But I don't want to redo all the same code that's in selectObject:. So I'm still looking into the best solution.

My else block is to make sure 'static' icons aren't updated. I think that icons for a specific filetype can be considered 'static' (although they can change if the user changes the default app for a given filetype. E.g. change .txt files from TextEdit to TextMate and the icon will change), so perhaps they're not static.

Pretty rare and cured by a restart of QS. Probably not worth reloading every icon every time.

I'm basically trying to reduce the number of (expensive) calls to the Quicklook API to get the icon, so I think making these icons permanent is OK.

Well, wait. The way the code is now (in #1488), it will only retain the icon if there was no Quick Look preview. So we're still calling Quick Look for things with previews, which is the expensive part. (And I think we should, since the preview can change when contents change.) I thought you were only trying to retain failures, as if to say "there is no useful icon for this thing, so stop trying to load one".

In any case, I think we agree. If we find that a file didn't have a Quick Look preview, or the user has previews disabled, it makes sense to just retain the type-based icon. I'll see if #1488 can be merged, then rebase this on it.

[pjrobertson]

Now granted, singleFileType was created before @tiennou added all the file object metadata, so it could probably be rewritten to do something similar to what I've done in setQuickIcon… using only cached information.

Yeah, just [[self infoRecord] objectForKey:@"filetype"] I guess, maybe make a method for that to make it even simpler. Sure you're on it already :)

Pretty rare and cured by a restart of QS. Probably not worth reloading every icon every time.

Yeah, which is what I thought.

So we're still calling Quick Look for things with previews, which is the expensive part. (And I think we should, since the preview can change when contents change.)

Exactly. Since if you edit a text file its quicklook preview changes. Looks like you agree and have merged anyway.

In any case, I think we agree. If we find that a file didn't have a Quick Look preview, or the user has previews disabled, it makes sense to just retain the type-based icon. I'll see if #1488 can be merged, then rebase this on it.

Again, looks like you're on it :)

[skurfer]

One question, though. Do we want to retain the "real" icon from iconForFile:? It's not a Quick Look preview and is unlikely to change. Or do we only want to retain the ones that end up sticking with iconForFileType:?

[skurfer]

Force pushed. Let me know if you find any issues.

Depending on your answer to my last question, it might be done. The way it is now, the initial type-based icon is retained if nothing better is found (which I think is what you were suggesting). If you want to also retain specific icons (but not Quick Look previews), I'll need to move some things around.

[pjrobertson]

One question, though. Do we want to retain the "real" icon from iconForFile:? It's not a Quick Look preview and is unlikely to change. Or do we only want to retain the ones that end up sticking with iconForFileType:?

I'd still class iconForFile: as being static - as we explained it only changes if the user changes the default app for a given filetype (e.g. HTML files from Opera to Safari, and the icon changes). So I'd retain that icon as well.

[skurfer]

I'd still class iconForFile: as being static

I would too, but in practice after trying it I think it "wastes" memory, considering how fast it is to just load the thing over again. I haven't heard anyone complain about waiting on plain file icons. I think we'd hear people complain about the memory footprint though. (It's a 25% - 60% increase for me and I didn't even leave QS running that long. I imagine it would get worse over time.)

I've been testing quite a bit this morning. Here are some things I found.

• setQuickIconForObject: is almost never called. Most places in the UI call loadIcon before the call to icon happens. If loadIcon has finished by the time icon is called, the "final" icon is returned. Only if loadIcon is taking a while (common with Quick Look previews) will setQuickIconForObject: be reached. So trying to retain what it comes up with is mostly pointless.
• Many icons are the same, whether you use iconForFile: or iconForFileType:, but every icon is a unique object in memory. NSFileManager doesn't reuse them. So even if we did retain the type-based icon, we'd still be chewing up a lot of memory.

[pjrobertson]

Is there a reason why you haven't gone for what I suggested here. i.e. [[self infoRecord] objectForKey:@"filetype"] instead of using the file extension and making a special exception for folders?

RE 536be7a

I'd added this to one of my own branches, but I see you got there first ;-)
Is there a reason why you removed the line for the childresults setNeedsDisplayInRect? Normally resultsChild is nil so this line doesn't do anything anyway

I'm worried 4919bcd might break something somewhere else (I've battled with setObjectValue and selectObjectValue many a time and it always does something strange. I'll run with the changes.

I think we'd hear people complain about the memory footprint though. (It's a 25% - 60% increase for me and I didn't even leave QS running that long. I imagine it would get worse over time.)

I think 25-60% is a bit exaggerated ;-) After 10 minutes of testing I got 1.3% increase in memory usage (84.6MB as opposed to 83.5MB), but who knows if that's related to the icons. Probably is. I'll take your values with a pinch of salt and say 'OK' ;-)

[pjrobertson]

OK I see, [[self infoRecord] objectForKey:@"filetype"] isn't always set :/

I'd create a method like this for file objects:
-(NSString *)fileType { if ([[self infoRecord] objectForKey:@"filetype"] unsignedLongValue]) { return [self infoRecord objectForKey:@"filetype"]; } return [object isFolder] ? @"'fold'" : [object fileExtension]; }

This gives the advantage that some things are given nicer icons. Doesn't seem to work that often though. stupid Apple for not making it easier.

---

Aside:

FYI, we can now start using NSURL's getResourceValue:forKey:error: a bit more for getting interesting information. Works well for getting e.g. the UTI, and better than what we currently have (I've been playing with UTIs... pull request to come soon!)

e.g. - CSS files have no 'real' defined UTI. Use mdls to see that it's dyn.ah62d4rv4ge80g65x (at least on my system). But currently [fileObject fileUTI] gives dyn.age80g65x which isn't the same, and doesn't conform to the public.item UTI, which causes problems.

[skurfer]

Is there a reason why you removed the line for the childresults setNeedsDisplayInRect? Normally resultsChild is nil so this line doesn't do anything anyway

I think that's just the diff looking weird. All I did was wrap that in a block without changing it.

I'm worried 4919bcd might break something somewhere else

Me too. I looked at the places that call both methods, and I think the change is OK. I also haven't run into any problems when testing, but another set of eyeballs is always good.

I think 25-60% is a bit exaggerated ;-)

Maybe, but it was at least 25%. Try right-arrowing into ~/Documents and scrolling down through all the results, then dismiss the interface. In 1.0.0, there's a large drop in memory use when you dismiss the interface. If we retain the icons, it stays up.

Maybe it's more noticeable for me since I have Quick Look previews turned off for most types.

I'd create a method like this for file objects:
-(NSString *)fileType {

We could, but I feel like that would make things more confusing. When would you use that instead of singleFileType? Why not just alter singleFileType, or get rid of - [NSFileManager typeOfFile:], or both? And if it's not OK to explicitly check for folders (in setQuickIconForObject:) when it's a single file, why is it OK directly below for a multiple selection? Should that be reworked too?

I didn't want to address all that here, so I went with the (admittedly less elegant) one-line fix. 😃

[pjrobertson]

You always convince me in the end... ;-)

Me too. I looked at the places that call both methods, and I think the change is OK. I also haven't run into any problems when testing, but another set of eyeballs is always good.

Have you run into any issues? I must admit I haven't been running with these changes, but I will now for a few days, and see what I find.

Anyway, all the singleFileType business and horrible stuff there should be fixed in my nice new UTI branch :)
Life's so much easier when you can just call [fileObj fileUTI] (and it reliably works)!

[skurfer]

The only issue I've seen is that the occasional application will have a generic document icon until you clear it from the object view, then call it back up. I was seeing that prior to 4919bcd. It happens so rarely, I haven't been able to troubleshoot.