405358 - Add QuickLook to download manager

Status: RESOLVED INCOMPLETE

(Camino Graveyard :: OS Integration, enhancement)

Description

[Samuel Sidler (old account; do not CC)]

More and more I find myself pushing |space| in the download manager to invoke QuickLook. The problem is... we don't support that. ;)

Filing as UNCO to get some comments before confirming this RFE.

Comment 1

[Stuart Morgan]

"Clients of Quick Look request thumbnails and previews for listed and selected documents, respectively, and receive images for display. These clients are currently restricted to certain Apple-developed system applications, particularly Finder, Spotlight, Time Machine, and FileSync."

That suggests to me that we may not be able to do this.

Comment 2

[Samuel Sidler (old account; do not CC)]

I noticed that Delicious Library had something like it in their alpha/beta. I was hoping Apple would either expose this or maybe a third party would implement something similar.

Thinking another way... Could we make whatever call the Finder makes? Is it a file system call?

Comment 3

[Jeff Dlouhy]

There is a command line utility shipping in 10.5 called 'qlmanage' that allows you to open a specific file with Quick Look. So we could possibly do something like 'qlmanage -p destPath' where destPath is the filepath for a specific item from the ProgressViewController.

Comment 4

[Jeff Dlouhy]

Attachment: Proof of Concept

This code was quickly hacked together to show that it is possible to Quick Look the selected item in the downloads window. However we do not get the fancy animation which would be optimal. This code should not be seen as final by any means ;).

Comment 5

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

We also get to beachball while the QL window is up ;)  

Once I was able to get in a situation where Camino was focused but the menu bar was the Finder's and would not switch back to Camino, but I couldn't repro it.

Anyway, I shoved this in a build for Sam to play with, so I guess it'll be my fault when he becomes insufferable.... ;)

Comment 6

[Samuel Sidler (old account; do not CC)]

I'm going to confirm this and assume we can figure it out later either on our own or by Apple providing some sort of an API, though it's obviously not imperative.

I tried the build Smokey gave me and two things stuck out for me that we'd need to fix before something like the proof of concept is landed:

  * The hanging Smokey mentions in comment 5. That is kind of scary to me, but
    it's also bad UE. We'd need to find out why it was doing that and fix it.
  * Pressing space ten times creates the QuickLook window ten times over, just
    delayed until the previous one is closed. That seems kind of bad given the
    delay before the window shows after pushing space.

Other than that, something like this would be good. I'm not too concerned about the animation, though that of course would be nice. That could be done in a follow up bug though.

One other question: On 10.4, what happens when a user pushes the space bar? I assume we send an error to the console? Or is that code just ignored on that platform?

Comment 7

[Jeff Dlouhy]

Attachment: QuickLook v1

This patch uses undocumented QuickLook APIs discussed at http://ciaranwal.sh/2007/12/07/quick-look-apis. I don't know what our stance on using private APIs is, so we might just have to wait if Apple ever gives this feature to developers.

QuickLook is invoked when the user selects a item(s) in the downloads window and hits the spacebar.

Video: http://drop.jeffd.org/QuickLookDL.mov

Comment 8

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

--> Jeff since he's writing patches here ;)

Comment 9

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 298537 [details] [diff] [review]
QuickLook v1

As we discussed last night, this doesn't static-build :(

Comment 10

[Jeff Dlouhy]

Attachment: QuickLook v3

I still cannot get this to build on Static. I get a 'ld: library not found for -lnecko' error message when compiling.

I did however clean up the UI a bit. Now the panel swoops out of the icon a la Finder and does not swoop out when it is not visible in the scroll view.

Comment 11

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #10)
> I still cannot get this to build on Static. I get a 'ld: library not found for
> -lnecko' error message when compiling.

I get a different error (a seemingly more-fixable one):

CompileC build/Camino.build/Deployment/CaminoStaticApp.build/Objects-normal/i386/ProgressDlgController.o /Users/smokey/Camino/dev/trunk/CaminoStaticTrunk/camino/src/download/ProgressDlgController.mm normal i386 objective-c++ com.apple.compilers.gcc.4_0
[...]
cc1objplus: warnings being treated as errors
/Users/smokey/Camino/dev/trunk/CaminoStaticTrunk/camino/src/download/ProgressDlgController.mm: In function ‘ProgressViewController* -[ProgressDlgController progressViewControllerWithURL:](ProgressDlgController*, objc_selector*, NSURL*)’:
/Users/smokey/Camino/dev/trunk/CaminoStaticTrunk/camino/src/download/ProgressDlgController.mm:490: warning: ‘progressItem’ may be used uninitialized in this function

Comment 12

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

The other thing I noticed is that, while in the Finder you can both arrow up and down in the list view (to change the previewed item) and also Esc to close the QL window, you can only close the QL window here.  

I'm sure the focus issues are quite nasty, but it is a departure.

Comment 13

[Jeff Dlouhy]

Attachment: QuickLook v4

> I'm sure the focus issues are quite nasty, but it is a departure.

I am able to get 'right' and 'left' working for scrolling through a group of quick looked items.

Changing selections also updates the Quick Look panel.

Escape is a little trickier and I don't have support for it. By default Cocoa binds Esc to 'cancelOperation:' in NSResponder. I don't know how to hook that up into our current responder chain or if it is worth it.

Comment 14

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

This feels better, but I still miss my Esc ;)  I think we'd have to support that, and when we get to that point, borrow murph's brain?

The other thing I noticed tonight is that we'll also happily try to ql a file we've marked as missing/lost, which seems just a little bit odd (OTOH, were we to do nothing, a user might wonder if ql works before realizing that the file is marked missing).  Luckily the Finder never has to worry about people qling missing files!

Comment 15

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: QuickLook v4a

This is just Jeff's v4 patch unbitrotted by me and made static-buildable by hendy.

Comment 16

[Jeff Dlouhy]

Attachment: QuickLook v5

(In reply to comment #14)
> This feels better, but I still miss my Esc ;)  I think we'd have to support
> that, and when we get to that point, borrow murph's brain?

Adding the view to the responder chain did the trick.

> The other thing I noticed tonight is that we'll also happily try to ql a file
> we've marked as missing/lost, which seems just a little bit odd (OTOH, were we
> to do nothing, a user might wonder if ql works before realizing that the file
> is marked missing).  Luckily the Finder never has to worry about people qling
> missing files!

If you try to select one item in the window that is missing, it will beep at you. If you select a bunch or try to use the arrow keys to select a missing file, it just does not try to view it (and no beep).

Comment 17

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

[11:51pm] sauron: jeff: maybe i'm just too exhausted, but i am unable to find anything to complain about with that patch
[11:51pm] jeff:  :)
[11:51pm] sauron: so i'm going to make sam a build and suggest you find a review victim

Comment 18

[Christopher Henderson]

Attachment: QL on a missing file

A few behaviour-related issues:

If I QL an item, then select the item above it and tap the spacebar a few times, the QL pane appears to come from the originally selected item. This persists across restarts, too.

Also, if I QL an item, then select a missing file, I would expect the QL pane to disappear. Having a preview of an unrelated file when a missing file is selected seems wrong to me.

Items are no longer updated when they are moved, and invoking QL on them leads to a strange pane (see attached screenshot). To get that item's preview back, you need to close the DL window, restore the file, then open the DL window again and invoke QL.

The following menu times are broken by this patch:
Close Window
Hide/Show Toolbar
Customize Toolbar…
Minimize
Zoom

Comment 19

[Sean Murphy]

Comment on attachment 381218 [details] [diff] [review]
QuickLook v5

Hey Jeff,  Looks very cool, this is a great addition!  Listed here are just a few things I noticed, nothing major.  I'll r- for now to take a look at the updated patch to see how that looks and also to ensure again that I didn't miss anything.

>	+  if(mQuickLookAvailable)
>	+    [[[QLPreviewPanel sharedPreviewPanel] windowController] setDelegate:self];

Space needed after if.

> +-(ProgressViewController*)progressViewControllerWithURL:(NSURL*)URL
> +{
> +  unsigned int numControllerItems = [mProgressViewControllers count];
> +  ProgressViewController* progressItem = nil;
> +
> +  for (unsigned int i = 0; i < numControllerItems; i++) {
> +    progressItem = [mProgressViewControllers objectAtIndex:i];
> +    NSDictionary* downloadAttrib = [progressItem downloadInfoDictionary];
> +    NSString* curPathString = [@"file://localhost" stringByAppendingString:[downloadAttrib valueForKey:@"destPath"]];

As you do later in the patch, instead of |curPathString| why not just create a NSURL with +[NSURL fileURLWithPath:] and then compare that directly to |URL| with isEqual:?

> +- (void)quickLookItems:(NSArray*)selectedItems
> +{
> +  if (mQuickLookAvailable && [selectedItems count]) {
> ...
> +  }
> +}

Instead of nesting the entire method in the check to ensure mQuickLookAvailable and that there are selected items, maybe just do this check and then an early return.  Also, just a personal preference, but I prefer to be explicit when checking that an array has zero items to do ([selectedItems count] > 0).  It just makes the purpose of the check even clearer.

if (QL conditions)
	return;

Also, I was wondering if the naming of this method is not as clear in the sense that it is also used to close QuickLook.  Maybe rename it to updateQuickLookWithItems: or performQuickLookWithItems:?                                

> +    // Closing Time
> +    if ([[QLPreviewPanel sharedPreviewPanel] isOpen]) {
> +      // Check to see if the selcted view is still available to animate to

Spelling error here, but more so, maybe make it more explicit what's being checked for - not to make sure the downloaded file is available (I thought at first this implied exists on disk), but rather that it is visible.

>> // Animation types: 0 = none; 1 = fade; 2 = zoom
...
>     if ([self viewVisibleInScrollView:[selectedController view]])
>       [[QLPreviewPanel sharedPreviewPanel] closeWithEffect:2]; // Fancy animation
>     else
>       [[QLPreviewPanel sharedPreviewPanel] closeWithEffect:1]; // Closes immediately

Effect 1 actually doesn't close immediately, it's the fade animation.
Even though they're not ours, I think it'd be beneficial to declare an enum for these effects. This eliminates the need for comments, and rather than seeing magic numbers the code can speak for itself.

>	+-(void)quickLookSelectionChanged

This method is called to update the QuickLook panel, but the name to me at first made it seem like it was a delegate method triggered by something in the QuickLook panel, such as flipping to another item.  Maybe this should be renamed to "updateQuickLookSelection" or something similar, to imply even clearer that it's called to update QuickLook after the download selection changes?

Additionally, if the selection change was by growing to include more items, we might want to have the QuickLook panel update to reflect that as well.  For instance, with multiple downloaded items, selecting the first one, showing QuickLook, then with QL open holding shift and pressing keyDown: a few times to select a couple more items.  (This could also work for shift and mouse clicking).  This probably isn't that common of an occurrence, but I figured I'd mention it anyway for sake of completeness in our support.

Speaking of mouse clicking, this patch only updates the QuickLook view when the selection changes via keyboard (key{Down,Up}), not when selected with the mouse.  Is this something we want to also handle?

Comment 20

[Nomis101]

Any news/progress on this? If this works for Camino, maybe we can port this to other Mozilla applications, would be nice. :-)

Comment 21

[Daniel]

I'm not sure if this approach is applicable to Snow Leopard or not. Because the patch is using a private framework mQuickLookAvailable = [[NSBundle bundleWithPath:@"/System/Library/PrivateFrameworks/QuickLookUI.framework"] load];

which is not available under SL.

Comment 22

[Jeff Dlouhy]

Snow Leopard did add support for using Quick Look inside your applications.

Here is some sample code they have posted on their site:
http://developer.apple.com/mac/library/samplecode/QuickLookDownloader/

Maybe I should finally finish this patch! :-P