[BeOS] Allow Launch/Reveal and ShowLocation in Tracker

RESOLVED FIXED

Status

defect
RESOLVED FIXED
17 years ago
12 years ago

People

(Reporter: daniel_furrer, Assigned: sergei_d)

Tracking

({fixed1.8.1.8})

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

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.
cc'ing some beos developers
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
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 ;))
Posted patch Patch (obsolete) — Splinter Review
Allows to open BeOS native file explorer for downloaded file folder
changing component
Assignee: asa → sergei_d
Component: Browser-General → Download Manager
OS: other → BeOS
Posted patch Patch - fixed identation (obsolete) — Splinter Review
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.
Attachment #115994 - Attachment is obsolete: true
new mozilla/xpfe/components/download-manager/resources/beos/ folder in zip
needed to introduce BeOS-specific chrome for download manager
Posted file Patch for 3 files (obsolete) —
patches downloadmanager.js, Makefile.in and downloadmanager.properties. Needs
existence of beos folder from previous attachment
Attachment #115997 - Attachment is obsolete: true
PC-only
Hardware: All → PC
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment on attachment 116004 [details]
new mozilla/xpfe/components/download-manager/resources/beos/ folder in zip

? review
Attachment #116004 - Flags: review?(arougthopher)
Comment on attachment 116005 [details]
Patch for 3 files

please review
Attachment #116005 - Attachment is patch: false
Attachment #116005 - Flags: review+
Comment on attachment 116005 [details]
Patch for 3 files

requestee box don't allow me to put any name here, hoping review from
arougthopher@lizardland.net
Attachment #116005 - Attachment is patch: true
Attachment #116005 - Flags: review?
Comment on attachment 116005 [details]
Patch for 3 files

oops, I need to review it first
Attachment #116005 - Attachment is patch: false
Attachment #116005 - Flags: review+ → review?(arougthopher)
Attachment #116005 - Flags: review?
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.
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.
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.
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
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)
Thaks for notice. Met similar problem with URI instead Path when fixed last Net+
bookmark break.
Will look into.
something wrong with getting path from RDF in function getFileForItem(aElement) {}
it initializes localFile with URI intead path as result
Comment on attachment 116004 [details]
new mozilla/xpfe/components/download-manager/resources/beos/ folder in zip

This zip should not contain the CVS directory.
Attachment #116004 - Flags: review?(arougthopher) → review-
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
Attachment #116005 - Flags: review?(arougthopher) → review-
bug 206441 should probably fix the file: problem -> marking it as blocking this bug
Depends on: 206441
Depends on: 175881
Comment on attachment 116004 [details]
new mozilla/xpfe/components/download-manager/resources/beos/ folder in zip

need to check this again
Attachment #116004 - Flags: review- → review?
Comment on attachment 116005 [details]
Patch for 3 files

need to check this again
Attachment #116005 - Flags: review- → review?
No longer depends on: 206441
QA Contact: asa → timeless
Sergei, what is the status of this patch? it obviously hasn't been applied to
the 1.7a netserver build from bebits.com

Prog.
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
Was this bug ever resolved?
Who is svakharia@atsva.com and why does he have the access to remove all of our
addresses from the CC: field?
I don't know who svakharia@atsva.com 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.
Product: Browser → Seamonkey
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"
Attachment #116004 - Attachment is obsolete: true
Attachment #116004 - Flags: review?
Comment on attachment 116005 [details]
Patch for 3 files

new version will follow
Attachment #116005 - Attachment is obsolete: true
Attachment #116005 - Flags: review?
changing bug name.
Probably there is DUP somewhere
Summary: BeOS: "Show in Browser" should become "Show in Tracker" → [BeOS] Allow Launch/Reveal and ShowLocation in Tracker
Posted patch patch (obsolete) — Splinter Review
enables launch and "native show" for BeOS

review request
Attachment #205485 - Flags: review?(thesuckiestemail)
Comment on attachment 205485 [details] [diff] [review]
patch

r=thesuckiestemail@yahoo.se

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.
Attachment #205485 - Flags: review?(thesuckiestemail) → review+
Comment on attachment 205485 [details] [diff] [review]
patch

asking second(?) review
Attachment #205485 - Flags: superreview?(neil.parkwaycc.co.uk)
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 ;-)
Attachment #205485 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
>>          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
Posted patch patchSplinter Review
same as previous (so reviewed and superreviewed) but with change suggested by Neil:
line
file = file.QueryInterface(Components.interfaces.nsIFile);
removed
Attachment #205485 - Attachment is obsolete: true
landed in trunk by timeless
2005-12-10 21:37

closing bug
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
this very useful functionality is missing in current branch (1.8.1* ?)
needs backporting.
wondering if new bug is required to open
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 on attachment 205502 [details] [diff] [review]
patch

I don't see why you can't apply it to the branch as-is.
Attachment #205502 - Flags: approval-seamonkey1.1.5+
/cvsroot/mozilla/xpfe/components/download-manager/resources/downloadmanager.js,v  <--  downloadmanager.js
new revision: 1.41.20.1; previous revision: 1.41
done 
Keywords: fixed1.8.1.8
You need to log in before you can comment on or make changes to this bug.