172785 - [BeOS] Allow Launch/Reveal and ShowLocation in Tracker

Status: RESOLVED FIXED

(SeaMonkey :: Download & File Handling, defect)

Description

[Daniel Furrer]

User-Agent:       Mozilla/5.0 (BeOS; U; BeOS BePC; en-US; rv:1.1b) Gecko/20020723
Build Identifier: Mozilla/5.0 (BeOS; U; BeOS BePC; en-US; rv:1.1b) Gecko/20020723

In the Downloadmanager there's a button called "Show in Browser". I think this
should become "Show in Tracker" (as in "Show in Explorer" on windows)


Reproducible: Always

Steps to Reproduce:
1. Tools > Download Manager

Actual Results:  
It shows the files location in Mozilla

Expected Results:  
It should show the folder which contains the file in a new tracker window.

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

cc'ing some beos developers

Comment 2

[Daniel Furrer]

Since nsLocalFile::Reveal() is already implemented for BeOS the only thing that
needs to be changed is 
http://lxr.mozilla.org/seamonkey/source/xpfe/components/download-manager/resources/downloadmanager.js

223         // on unix, open a browser window rooted at the parent
224         if ((navigator.platform.indexOf("Win") == -1) &&
225             (navigator.platform.indexOf("Mac") == -1) &&
226             (navigator.platform.indexOf("OS/2") == -1)) {

needs to become:

223         // on unix, open a browser window rooted at the parent
224         if ((navigator.platform.indexOf("Win") == -1) &&
225             (navigator.platform.indexOf("Mac") == -1) &&
___             (navigator.platform.indexOf("OS/2") == -1) &&
___             (navigator.platform.indexOf("BeOS") == -1)) {

and the text on the Button needs to be changed

Comment 3

[Daniel Furrer]

I just tested the changes in the .js-file I suggested in #2 and it is working as expected.(Well, nearly: A Tracker window with the directory containing the file is opened. It would of course be nice if the downloaded file would also be selected but I think I'll file a new bug on this as soon as this bug is fixed ;))

Comment 4

[Sergei Dolgov]

Attachment: Patch

Allows to open BeOS native file explorer for downloaded file folder

Comment 5

[Sergei Dolgov]

changing component

Comment 6

[Sergei Dolgov]

Attachment: Patch - fixed identation

There is still little "style" problem. Item in Download Manager menu should be
"Show in Tracker" and shortcut must be 'T', but it needs changes in makefile.in
and creating of BeOS subfolder in resource folder, as far as i understand.

Comment 7

[Sergei Dolgov]

Attachment: new mozilla/xpfe/components/download-manager/resources/beos/ folder in zip

new mozilla/xpfe/components/download-manager/resources/beos/ folder in zip
needed to introduce BeOS-specific chrome for download manager

Comment 8

[Sergei Dolgov]

Attachment: Patch for 3 files

patches downloadmanager.js, Makefile.in and downloadmanager.properties. Needs
existence of beos folder from previous attachment

Comment 9

[Sergei Dolgov]

PC-only

Comment 10

[Sergei Dolgov]

Comment on attachment 116004 [details]
new mozilla/xpfe/components/download-manager/resources/beos/ folder in zip

? review

Comment 11

[Paul]

Comment on attachment 116005 [details]
Patch for 3 files

please review

Comment 12

[Sergei Dolgov]

Comment on attachment 116005 [details]
Patch for 3 files

requestee box don't allow me to put any name here, hoping review from
[e-mail]

Comment 13

[Paul]

Comment on attachment 116005 [details]
Patch for 3 files

oops, I need to review it first

Comment 14

[Paul]

Comment on attachment 116005 [details]
Patch for 3 files

This,

>> ifneq (,$(filter BEOS,$(OS_ARCH)))

should be

>> ifneq (,$(filter BeOS,$(OS_ARCH)))


Also, could you provide unified diffs next time (cvs diff -u)?	They are much
easier to read and deal with.

Comment 15

[Paul]

This does not seem to work, since, the mPath.get() in nsLocalFileUnix returns
"file://.....", which, BPath does not seem to understand.

For this reason, a patch should also be applied to nsLocalFileUnix, so that
InitCheck is called on the BPath, before it is used.

Comment 16

[Sergei Dolgov]

Sorry, Paul, didn't understand your last comment...and in order to test how it
works - all Stripzillas now on bebits, including compact netserver version,
include this patch, and it works in download manager. 
But sure, there is one problem - sometimes you need "reselect" file/row again to
activate "Show in Tracker" control on toolbar.
Second imperfectnes is fact that it only opens Tracker in proper folder, but
don't select file there.

Comment 17

[Paul]

This does work, if, I download a file, select it in the download manager, and
choose "Show in Tracker".  But, if I choose a file in the download manager that
was downloaded previously, the selected file's location is prepended with a
"file:/" URL.  When this is passed to nsLocalFileUnix's reveal() method, it
cannot open Tracker, because BPath does not understand the "file:/" URL.

To illustrate this:
- Download a file to your machine to /boot/home
- Highlight the file in the download manager
- The status bar shows the file location as /boot/home/filename
- Restart mozilla
- Open the Download Manager from the Tools menu
- Highlight the file you downloaded previously
- The status bar shows the file location as file://boot/home/filename
- If you try to "Show in Tracker" this time, mozilla will crash

Comment 18

[Paul]

Also, before this is checked in, the contents.rdf should be updated with the
correct localeVersion, which, is currently 1.4a (though, I don't know how much
this matters, but, the latest unix/contents.rdf has 1.4a)

Comment 19

[Sergei Dolgov]

Thaks for notice. Met similar problem with URI instead Path when fixed last Net+
bookmark break.
Will look into.

Comment 20

[Sergei Dolgov]

something wrong with getting path from RDF in function getFileForItem(aElement) {}
it initializes localFile with URI intead path as result

Comment 21

[Paul]

Comment on attachment 116004 [details]
new mozilla/xpfe/components/download-manager/resources/beos/ folder in zip

This zip should not contain the CVS directory.

Comment 22

[Paul]

Comment on attachment 116005 [details]
Patch for 3 files

declining review based on comment 17 and comment 18.

Also, if you could post unified diffs, they are much easier to read (-u)
Thanks

Comment 23

[Ian Neal]

bug 206441 should probably fix the file: problem -> marking it as blocking this bug

Comment 24

[Paul]

Comment on attachment 116004 [details]
new mozilla/xpfe/components/download-manager/resources/beos/ folder in zip

need to check this again

Comment 25

[Paul]

Comment on attachment 116005 [details]
Patch for 3 files

need to check this again

Comment 26

[Prognathous]

Sergei, what is the status of this patch? it obviously hasn't been applied to
the 1.7a netserver build from bebits.com

Prog.

Comment 27

[Sergei Dolgov]

Last i remember, there was something missing in our solution here, so it didn't
work as expected.
Then Paul have set review request, but without reviewer target, so it's unclear,
who should review it. I will test those updated solution this week, and if it
works, i put review here, but it needs also superreview, IMHO, as changes happen
outside BeOS-port-only folders

Comment 28

[Disreali]

Was this bug ever resolved?

Comment 29

[Disreali]

Who is [e-mail] and why does he have the access to remove all of our
addresses from the CC: field?

Comment 30

[Prognathous]

I don't know who [e-mail] is, but you don't need any special
privileges to remove or add people to the cc list. Surprisingly, more often than
not this system works.

Prog.

Comment 31

[Sergei Dolgov]

Comment on attachment 116004 [details]
new mozilla/xpfe/components/download-manager/resources/beos/ folder in zip

no need for that, now DLManager has neutral item
"Show File Location"

Comment 32

[Sergei Dolgov]

Comment on attachment 116005 [details]
Patch for 3 files

new version will follow

Comment 33

[Sergei Dolgov]

changing bug name.
Probably there is DUP somewhere

Comment 34

[Sergei Dolgov]

Attachment: patch

enables launch and "native show" for BeOS

review request

Comment 35

[tqh]

Comment on attachment 205485 [details] [diff] [review]
patch

r=[e-mail]

Can't test it as I don't have Seamonkey, but I know the underlying code should work as I canged nsLocalFileUnix to work with BeOS.

Comment 36

[Sergei Dolgov]

Comment on attachment 205485 [details] [diff] [review]
patch

asking second(?) review

Comment 37

[[e-mail]]

Comment on attachment 205485 [details] [diff] [review]
patch

It really sucks that there's no way to tell ahead of time if launch won't work :-(

> var gCannotLaunch = ((navigator.platform.indexOf("Win") == -1) &&
>                      (navigator.platform.indexOf("OS/2") == -1) &&
>-                     (navigator.platform.indexOf("Mac") == -1));
>+                     (navigator.platform.indexOf("Mac") == -1) &&
>+                     (navigator.platform.indexOf("BeOS") == -1));
A regexp test would be nice, although not so important if you do the next suggestion.

>         if ((navigator.platform.indexOf("Win") == -1) &&
>             (navigator.platform.indexOf("Mac") == -1) &&
>-            (navigator.platform.indexOf("OS/2") == -1)) {
>+            (navigator.platform.indexOf("OS/2") == -1) &&
>+            (navigator.platform.indexOf("BeOS") == -1)) {
This looks familiar ;-) Perhaps this would work using if (gCannotLaunch) {

>           file = file.QueryInterface(Components.interfaces.nsIFile);
I don't think this line is necessary, so it would be nice if you could check and arrange to delete it ;-)

Comment 38

[Sergei Dolgov]

>>          file = file.QueryInterface(Components.interfaces.nsIFile);
>I don't think this line is necessary, so it would be nice if you could check
>and arrange to delete it ;-

Neil, I'm profanic in JS/XUL in general and in this given code aswell, also I lack permissions to checkin it myself, so only thing i can do here - submit patch with this line removed (under your responsibility) - to allow someone else to checkin this updated patch

Comment 39

[Sergei Dolgov]

Attachment: patch

same as previous (so reviewed and superreviewed) but with change suggested by Neil:
line
file = file.QueryInterface(Components.interfaces.nsIFile);
removed

Comment 40

[Sergei Dolgov]

landed in trunk by timeless
2005-12-10 21:37

closing bug

Comment 41

[Sergei Dolgov]

this very useful functionality is missing in current branch (1.8.1* ?)
needs backporting.
wondering if new bug is required to open

Comment 42

[Sergei Dolgov]

Current patch with neil's sugestion about removing "QueryInterface" line 
https://bugzilla.mozilla.org/attachment.cgi?id=205502
applies absolutely cleanly also to branch.
Somwehat all this is also related to Bug 265025

So I'm waiting for Neil's comments about best method to apply it to branch, as with that removed QueryInterface line it affects also other besides BeOS platforms

Comment 43

[[e-mail]]

Comment on attachment 205502 [details] [diff] [review]
patch

I don't see why you can't apply it to the branch as-is.

Comment 44

[Sergei Dolgov]

/cvsroot/mozilla/xpfe/components/download-manager/resources/downloadmanager.js,v  <--  downloadmanager.js
new revision: 1.41.20.1; previous revision: 1.41
done