Closed Bug 154580 Opened 22 years ago Closed 29 days ago

Save URL of downloaded documents in comment (Mac filesystem metadata)

Categories

(Camino Graveyard :: Downloading, enhancement)

Product:
Camino Graveyard
Component:
Downloading
Platform:
PowerPC
macOS
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED INCOMPLETE

People

(Reporter: jcho, Unassigned)

References

(Depends on 1 open bug)

Details

Attachments

(5 obsolete files) 

Could we have the URLs of downloaded files included into the files so that it
would show up in the comments textbox in the "Show Info" pane, and under the
Comments column in list view.
Marking as enhancement.
Severity: normal → enhancement
Summary: Saving comments in downloaded web documents → [RFE] Save URL of downloaded documents in comment
See also bug 90918 on the Mozilla side.
As an FYI - the Mac OS X Finder doesn't use the same mechanism as Mac OS 9 and
earlier for file comments.  Currently the only way to set file comments for the
OS X Finder is via sending an AppleEvent.  Where that's documented I'm not sure.
I agree with this idea, when I used IE it would do this and it was really 
helpful at times.  Just implementing it wouldn't be easy...
Summary: [RFE] Save URL of downloaded documents in comment → Save URL of downloaded documents in comment
I'd like to see that option in Phoenix too. Do I have to write somewhere else too?

I already posted here for the same:
http://www.mozillazine.org/forums/viewtopic.php?t=4481


thanks
Information about the needed AppleEvents can be found here:
<http://developer.apple.com/samplecode/Sample_Code/Overview/MoreIsBetter/MoreFinderEvents.c.htm>
Search for MoreFESetComment.
-> me
Assignee: saari → pinkerton
Target Milestone: --- → Camino1.0
Also - it would be good to include into the files the date and time downloaded.
This is a good idea for Mozilla not just Camino and Macintosh.
Mike - 

Should we change the Platform/OS to All/All (and Product/Component to
Browser/Networking:File)?

It's been suggested here and requested elsewhere (bug 224675); otherwise, we
have a separate bug for each platform.

Also note bug 90918, which wants the same thing, but with access to the url via
a GUI in OSX.
It appears bug 125729 is a similar request for all platforms.  I'll leave it up
to you whether to dupe this one ...
Um, this is a Camino bug.  Camino is Macintosh/MacOS X only.
As I mentioned in bug 90918: 

Not be flippant, but is it possible to ask the author of the Safari plugin
DownloadComment http://www.ecamm.com/mac/free/ what calls/API he/she uses (if
that's the problem here, rather than time or priority) to get this to work?  Or
better yet, the author of iCab http://www.icab.de/ since iCab actually stores
the URLs in the comments 100% of the time and the Safari plugin barely manages 50%.
Note that there's code in the Camino source code (in ProgressViewController.mm)
to do this using AppleEvents. Its currently #if 0'ed out with a comment that we
need a better way to do this. However if it works (which I haven't checked) we
could just enable this code again as a stop-gap measure.
I ended up running debug builds of the Safari plugin I mentioned in comment 13
to see if I could help those folks figure out why URLs were not getting saved
sometimes.  My ex post facto conclusion was that I was dragging a series of
images to the desktop too quickly for the AppleScript code to keep up.  

Even if Camino's AE code has the same issue (I assume the AS code ultimately
calls the AE), it would only affect rapid dragging of images to the desktop;
saving files, downloads, etc., would all benefit from having their URL stored. 
Are there performance considerations at play in keeping the current Camino code
(if, as Bruce mentions, it really works), disabled?

I'd certainly be interested in playing with (=testing) a build that had the
existing Camino code enabled.
heck, if we have the code, let's give it a spin! if that is the only way to do
it, then I can't see a reason to keep it commented out.
For me the AppleScript code doesn't work: no comment is set and the console
tells me this:

 Get error when running AppleScript to set comments for
 '/Users/torben/Desktop/van_gogh_results.gif':

 Finder got an error: Can't set comment of file
 "MacintoshHD:Users:torben:Desktop:van_gogh_results.gif" to
 "http://www.google.com/logos/van_gogh_results.gif".

Change the code to return the error number gives this as -10006, which is 
 errOSACantAssign    -10006    An object cannot be set in a container.
This code works from Script Editor:

tell application "Finder"
	set comment of file (alias "Krak des
Chevaliers:Users:smokey:Desktop:geneva.html") to "testing"
end tell

My guess is the problem is that it needs to be told the pathname is an alias (I
first tried to write the code as "set comment of file "volume:path:file" to
"testing" and hit a similar error), but I don't know what's being fed into the
code....

(OTOH, in the corresponding Core bug 90918 the following URL was given for the
OS X File Manager stuff
http://developer.apple.com/documentation/Carbon/Reference/File_Manager/index.html#//apple_ref/c/func/PBDTSetCommentAsync
and the commented-out Camino code has a comment that Carbon stuff would be
preferable to AppleScript).

All I care is that storing comments works for saves, drags, and downloads--and
doesn't crash Camino in the process :-)
Strange, if you replace an existing file with your download the comment gets
set. I guess that Camino is too fast for the poor Finder and tries to set the
comment before Finder knows that the file exist. However, automatically
downloading every file twice is probably not a good idea :-) Camino's
Applescript code works for me using Script Editor.
I still need to download twice to set the comment using the (alias "...")-hack
from comment 18, however, the error is slightly different:

 Get error when running AppleScript to set comments for
 '/Users/torben/Desktop/van_gogh.gif':

 File MacintoshHD:Users:torben:Desktop:van_gogh.gif wasn't found.

For me it also takes ~1 second from the download is finished in the download
manager untill the file shows up on the desktop.
This is definitely a timing issue, the script to set the comment get run before
the file is created. Moving the script elsewhere in the code fixes this, I'm
just not sure where the best place is. 

I've tried putting it into the code for updating the download icon in
refreshDownloadInfo, this seems to try to set the comment approx. every second.
The good thing is that this makes sure the comment get set even for larger files
which may take a few seconds to be created. The bad thing is that it keeps
setting the comment every second even after the download is completed. This
doesn't stop until the file is removed from the download manager, just closing
the download manager window does not work.

Waiting until the download is finished is not perfect either. For larger files
(which may take hours to complete) it is nice to know the location earlier. I
will look around some more.
This works fairly well. The comment is set twice, first on the start of the
download (onStartDownload), this works for files that automagically starts to
download when you click on a link, but fails for downloads started through a
contextual menu :-(. Therefore the comment is set again when the download is
finished (onEndDownload), this always works.
What about files saved through Save As... (webpages, or images viewed alone) or
the Save Image... contextual menu, or images saved by dragging to the
desktop/folder; do they work properly as well?

It looks like every variation *except* dragging images is passed through the
download manager, so I'm guessing dragging images should be the only case that's
iffy.

It sounds like you've mostly fixed the timing issue with downloads proper, but I
recall for downloads, there's always a "temporary file" (foo.bar.part) created
when the download begins and then renamed/deleted when the download is finished
(it's most apparent for large downloads, but I assume it happens for all
downloads)....

(Thanks for working on this, BTW...this is a feature that has proved invaluable
in my research and has been a pain to go without when using Camino.)
Everything except drag & drop WFM, I might be possible to hook this up with the
drag & drop code too. 
This patch sends an Appleevent to set the comment, but adds a lot of code (~300
lines). Most of the code is from Apples MoreAppleEvents sample code
<http://developer.apple.com/samplecode/MoreAppleEvents/MoreAppleEvents.html>.
The code could probably be reduced and maybe moved somewhere else (any
suggestions where?).

For a short time solution (Camino 0.9), I suggest using the previous
AppleScript patch.
Does the latest (AppleEvent) patch handle drag-drop of images, or is that still
for "patch v2"? :-)

I've noticed a number of places recently (including Macworld) where the Safari
plugin is getting mentioned as the only game in town that does comments in
downloaded/saved/dragged files (though it's not technically true), so it's good
to have you working on this.... :D
Attached patch Cleaned up Apple Events patch (obsolete) — — Splinter Review 
This does the same as the last patch, but I've removed a lot of code that is
not need for our use.

Still no drag-and-drop support, need to find out where that code lives.
Attachment #179614 - Attachment is obsolete: true
Comment on attachment 179119 [details] [diff] [review]
Enables automatic saving of URL in comment

Asking for review for the AppleScript patch. Fairly simple change that
shouldn't break anything, AFAIK the worst that can happen is thay we don't set
the comment. I suggest this is used for 0.9, the Apple Event patch could do
with more testing.
Attachment #179119 - Flags: review?
Flags: camino0.9?
Can't we do all this with the Cocoa scripting apis?
(In reply to comment #29)
> Can't we do all this with the Cocoa scripting apis?

Isn't that what the AppleScript version does? It is probably be possible to use
the NSAppleEventDescriptor instead of NSAppleScript, but that would be more work
for no good reason. AIUI, the request for Carbon APIs was so this code could be
used with Mozilla too.
> What about [...] images saved by dragging to the desktop/folder; do they
> work properly as well?

Since Camino doesn't have its own drag&drop-code, I have attached a patch for
that in the mozilla bug 90918.

Tested together with the above Camino-patch, the URL gets saved as a Finder
comment for all file saves tested (files saved through Save As... (webpages, or
images viewed alone), the Download Link Target... contextual menu, the Save
Image... contextual menu, images saved by dragging to the desktop/folder, and
automagically saved files).
While we may be able to do this for 0.9, we would not block the release if this
wasn't done. That is what the flag is for, so denying the request.

I will try to look into this when I get time, but that probably won't be for at
least 2 weeks.
Flags: camino0.9? → camino0.9-
(In reply to comment #28)
>  I suggest this is used for 0.9, the Apple Event patch could do
> with more testing.

The AppleScript is much simpler and smaller than the alternative (see bug
90918), so I suggest we stick with it.
This uses the changes in bug 90918 to set the comment. I still think the
orginal AppleScript-solution is nicer, but that's not for me to decide...
Attachment #179691 - Attachment is obsolete: true
Comment on attachment 182940 [details] [diff] [review]
Alternative patch, works together with patch in bug 90918

Someone care to review this?
Attachment #182940 - Flags: review?
Torben: is the patch in bug 90918 ready for review, too?  

(Samuel: the two patches go hand in hand to cover all types of downloads, but
the trunk one's needed first.  I don't know when the trunk freeze is...)
> is the patch in bug 90918 ready for review, too?

Yes, I'm just not sure who to ask for review.

> the two patches go hand in hand to cover all types of downloads, but
> the trunk one's needed first.

Depends on which version is choosen, the AppleScript-version (attachment 179119 [details] [diff] [review])
is independet of bug 90918, but attachment 182940 [details] [diff] [review] need those changes.
Camino should save the URL as metadata on OS X v10.4
Safari 2.0 now sets kMDItemWhereFroms to the download URL, we should do the same. 
Comment on attachment 182940 [details] [diff] [review]
Alternative patch, works together with patch in bug 90918

Bug 90918 will move all this into back-end code. No Camino-specific actions
should be necessary except for some clean-up of unused code. I'll submit a
patch after bug 90918 is landed.
Attachment #182940 - Attachment is obsolete: true
Attachment #182940 - Flags: review?
Attachment #179119 - Attachment is obsolete: true
Attachment #179119 - Flags: review?
Depends on: 90918
(In reply to comment #38)
> Camino should save the URL as metadata on OS X v10.4
> Safari 2.0 now sets kMDItemWhereFroms to the download URL, we should do the
> same. 

I think this would be better as a separate bug. (I can't do anything with it
since I neither have Tiger nor could I find any real documentation on how to do
this from Apple.)
Since it seems unlikely that the fixes in bug 90918 would be approved for the
branch even if the reviewers agreed on what should be done, can we take Torben's
AppleScript version for 1.0 and convert to the Core method when/if it ever lands?

Torben, does your AppleScript version patch still apply?  (It doesn't handle
drag-drop, right? :-(  Am I remembering that correctly?)
> Torben, does your AppleScript version patch still apply?  

It should, but I'll doublecheck when I get home tonight before re-requesting
reviews.

> (It doesn't handle drag-drop, right? :-(  Am I remembering that correctly?)

No, only what goes through the download manager is handled.
Comment on attachment 179119 [details] [diff] [review]
Enables automatic saving of URL in comment

This still works (the line numbers are wrong but that shouldn't matter), should
probably be branch only (bug 90918 is the long-term soultion).
Attachment #179119 - Attachment is obsolete: false
Attachment #179119 - Flags: review?(pinkerton)
Comment on attachment 179119 [details] [diff] [review]
Enables automatic saving of URL in comment

sr=pink for branch only.

can someone r and test it to make sure it still works so we can get it in for RC1?
Attachment #179119 - Flags: superreview+
While this patch needed some tweaks to get it to work, (forget to declare |tryToSetFinderComments| in the Private declaration), it seemed to work sparatically. If the applescript event could not get to access the file, it dumped out an error/warning message in the console. 

I.E.:
Finder got an error: Can't set comment of file "Macintosh HD:Users:nickkreeger:Documents:Downloads:Firefox 1.0.7-1.dmg" to "http://mirror.switch.ch/ftp/mirror/mozilla/firefox/releases/1.0.7/mac/en-US/Firefox%201.0.7.dmg".

Jasper also concluded that he has found the Applescript access to the finder buggy.
ok, then lets put this off.
Target Milestone: Camino1.0 → Camino1.1
(In reply to comment #45)
> While this patch needed some tweaks to get it to work, (forget to declare
> |tryToSetFinderComments| in the Private declaration), it seemed to work
> sparatically. 

Camino will (try to) set the comment twice, once at the begining of the download (when the file might not exist yet) and again after the download is finnished (which in my testing always works), see comment 22 for more details.

> If the applescript event could not get to access the file, it
> dumped out an error/warning message in the console. 
> 
> I.E.:
> Finder got an error: Can't set comment of file "Macintosh
> HD:Users:nickkreeger:Documents:Downloads:Firefox 1.0.7-1.dmg" to
>"http://mirror.switch.ch/ftp/mirror/mozilla/firefox/releases/1.0.7/mac/en-US/Firefox%201.0.7.dmg".

This is what is supposed to happen, if we don't want this just remove the NSLog-statement.

> Jasper also concluded that he has found the Applescript access to the finder
> buggy.

??
Torben the question is, even when you try to set the comment twice it sometimes fails. Now we really want to know why that happens. I found that working with applescript and the finder that for some unknown reason the finder sometimes doesn't want to set the comment.

It would be really nice to see if anybody could use omniobjectmeter to see how safari does this. This would be a really nice thing to work. But we really should have something that always works.
(In reply to comment #48)
> Torben the question is, even when you try to set the comment twice it
> sometimes fails. Now we really want to know why that happens. I found
> that working with applescript and the finder that for some unknown reason
> the finder sometimes doesn't want to set the comment.

OK, I've never seen this so I've got no idea what's going on :-(

> It would be really nice to see if anybody could use omniobjectmeter to see
> how safari does this. This would be a really nice thing to work. But we
> really should have something that always works.

One thing that might work is to tell Finder to update the file before setting the comment, the AppleScript would be something like

"tell application "Finder" \
  update file \"%@\" \
  set comment of file \"%@\" to \"%@\" \
end tell",
hfsPath, hfsPath, mSourceURL

However, I won't be able to test this thoroughly for a couple of weeks, so I guess this won't make 1.0 anyway (and I really hope to have bug 90918 fixed before 1.1).
Attached patch Patch w/updated AppleScript (obsolete) — — Splinter Review 
The script now checks if the file exists before it try to set the comment. This may work better.
Attachment #179119 - Attachment is obsolete: true
Attachment #209730 - Flags: review?
Attachment #179119 - Flags: review?(mikepinkerton)
-> Torben

(Since he's working on this.)
Assignee: mikepinkerton → torben
Component: General → Downloading
QA Contact: winnie → downloading
Has anyone tried the patch to see if it works better?
It might be easier to just set the comment using Cocoa APIs instead of using the inline AppleScript.
Comment on attachment 209730 [details] [diff] [review]
Patch w/updated AppleScript

So, I just tried the in my trunk build, and it applies with offsets but, indeed, does not work :-(

I saved the Camino image from cb.o/start (via the context menu) and opt-clicked the link "website".  This showed up in the console:

spec=/error.jpg
WARNING: malformed url: no scheme: file /Users/smokey/Camino/dev/trunk/mozilla/netwerk/base/src/nsStandardURL.cpp, line 714
spec=/website
WARNING: malformed url: no scheme: file /Users/smokey/Camino/dev/trunk/mozilla/netwerk/base/src/nsStandardURL.cpp, line 714

I also noticed that with this patch applied, there's a noticeable delay before downloads appear in the download manager (the manager opens...wait...wait...download appears).

:-(  Any luck on getting biesi and dougt to agree on where they want the Core changes to go?
Attachment #209730 - Flags: review? → review-
Summary: Save URL of downloaded documents in comment → Save URL of downloaded documents in comment (Mac filesystem metadata)
Torben: Just wondering how you're coming with an updated patch for this addressing the issues Smokey saw. Not to keep you too busy or anything... ;)
Comment on attachment 209730 [details] [diff] [review]
Patch w/updated AppleScript

I've given up on the AppleScript solution. In addition to the issues Smokey saw it has unavoidable memory leaks :-( Unfortunately I haven't had time to argue a solution for bug 90918 either.
Attachment #209730 - Attachment is obsolete: true
Pushing off based on comment 55 :weeps:
Target Milestone: Camino1.1 → Camino1.2
*** Bug 357219 has been marked as a duplicate of this bug. ***
Target Milestone: Camino1.6 → ---
We could do this with MDItemSetAttribute(kMDItemFinderComment) à la bug 325873, undocumented but available in 10.4 and up.  That sets the Finder comment, which the Finder now calls a Spotlight comment.  But I don't know if it's really so important now, because bug 325873 sets the more specific kMDItemWhereFroms attribute on the same platforms we'd be able to set kMDItemFinderComment.  kMDItemWhereFroms is visible in the UI as "Where from" in the "More Info" pane of a Finder "Get Info" window.  The only difference is that a Finder/Spotlight comment is editable/selectable/copyable.
Is setting the comment this way any more permanent than setting kMDItemWhereFroms (which vanishes on move/copy)?

The two really big plusses of Finder comments for me are the selectable/copyable and the permanence, so when I want to find where I got this thing to see if there's a new version or to cite it as source (or whatever), the URL is still there, no matter if I've moved the file or copied it to a new disk....
Neither attribute disappears on move/copy on Leopard, so you or someone else is going to have to tell me.

In a pinch, you can test this out by taking the patch from bug 325873 and adding the following between the existing mdItemSetAttributeFunc and CFRelease:

  mdItemSetAttributeFunc(mdItem, kMDItemFinderComment, @"Finder/Spotlight comment text");

If you're on the branch and using the 10.3.9 SDK, you can't use kMDItemFinderComment like that.  Instead, pass @"kMDItemFinderComment".
Maybe it was a bug on 10.4 that got fixed?  

The impermanence was documented in the internets in various places, and Wevah and hennk reproduced it locally one day about a year ago while we were playing with this and bug 325873 on irc.
Well, if we can use this method to set both attributes on 10.4, and one disappears but the other doesn't, then that's a reasonable argument to set both.

If they're both treated equally, then we're back to the only benefit of the Finder/Spotlight comment being that it's editable and copyable.
Setting "?" so that we remember to make the call on this one way or the other.  See comment 58 et seq.
Flags: camino1.6b1?
Per the meeting, there was no objection to doing this as described in comment 58 et seq.
Target Milestone: --- → Camino1.6
In that case, let's do it.
Assignee: torben → mark
I spoke too soon.  We can set the metadata without any trouble, and Spotlight will see it just fine, but the Finder won't (on Leopard, at least).  Apparently, the Finder only consults the .DS_Store file for the comment, not the file's metadata.  When setting the comment, the Finder sets both.  Oh well.
Assignee: mark → nobody
Ha, this looks reliable, but it's 10.5-only:

http://developer.apple.com/samplecode/SBSetFinderComment/
Pushing out of 1.6, given comment 66.
Flags: camino1.6b3?
Target Milestone: Camino1.6 → ---
Status: NEW → RESOLVED
Closed: 29 days ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: