258219 - Launch option in context menu should be hidden and OnTrigger should call showinshell on certain platforms as it is for toolbar

Status: RESOLVED FIXED

(SeaMonkey :: Download & File Handling, defect)

Description

[Ian Neal]

The context menu in download manager shows the launch option even though it is
hidden on the toolbar for platforms that are not Win, OS/2 or Mac. Clicking on
this option generates an error message in the JS console.

Comment 1

[Ian Neal]

Attachment: Simple patch (v0.1) to hide launch in context menu on certain platforms

Comment 2

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

Comment on attachment 158072 [details] [diff] [review]
Simple patch (v0.1) to hide launch in context menu on certain platforms

maybe a broadcaster would be a better idea?

Comment 3

[Ian Neal]

Attachment: Hides context launch and alters behaviour of double-click/return on certain platforms

V0.2 of patch stops cmd_openfile from being called in context menu by hiding it
and on double-click/return by altering behaviour to call showinshell on
affected platforms

Comment 4

[[e-mail]]

Comment on attachment 158075 [details] [diff] [review]
Hides context launch and alters behaviour of double-click/return on certain platforms

> function onTrigger() {
>   if (downloadViewController.isCommandEnabled('cmd_properties'))
>     goDoCommand('cmd_properties');
>-  else if (downloadViewController.isCommandEnabled('cmd_openfile'))
>-    goDoCommand('cmd_openfile');
>+  else if ((navigator.platform.indexOf("Win") == -1) &&
>+           (navigator.platform.indexOf("OS/2") == -1) &&
>+           (navigator.platform.indexOf("Mac") == -1))
>+    if (downloadViewController.isCommandEnabled('cmd_showinshell'))
>+      goDoCommand('cmd_showinshell');
>+    else if (downloadViewController.isCommandEnabled('cmd_openfile'))
>+      goDoCommand('cmd_openfile');
> }
Your ifs are badly nested, this won't launch at all (on Win/Mac/OS/2).

I'm wondering whether a) isCommandEnabled('cmd_openfile') should be false on
linux etc, which would mean you could just append a test for cmd_showinshell
instead b) if we should have a global gCanLaunch instead of continually testing
for navigator.platform

Comment 5

[Ian Neal]

Comment on attachment 158075 [details] [diff] [review]
Hides context launch and alters behaviour of double-click/return on certain platforms

New patching coming soon

Comment 6

[Ian Neal]

Attachment: Revised patch (v0.3) along lines of Neil's suggestions

This patch:
a) returns false for isCommandEnabled('cmd_openfile') on non Windows, Mac and
OS/2 platforms
b) uses global gCannotLaunch so platform testing is only done once
c) is written at a more sensible time of the day

Not used a broadcaster as only changing to hidden not changing back and forth.

Tested on both Win32 and Linux and no unwanted side effects spotted.

I suppose someone should confirm that showinshell is the action wanted on
Linux, etc platforms for double click?

Comment 7

[[e-mail]]

Comment on attachment 158136 [details] [diff] [review]
Revised patch (v0.3) along lines of Neil's suggestions

Nits:

>+  gCannotLaunch = ((navigator.platform.indexOf("Win") == -1) &&
>+                   (navigator.platform.indexOf("OS/2") == -1) &&
>+                   (navigator.platform.indexOf("Mac") == -1));
navigator.platform exists at global scope, so you could initialize it when you
declare it.

>+  if (gCannotLaunch)
>   {
>     document.getElementById("btn_openfile").hidden = true;
>+    document.getElementById("downloadPaneContext-openfile").hidden = true;
>   }
You could just use
document.getElementById("downloadPaneContext-openfile").hidden = gCannotLaunch;

Comment 8

[Ian Neal]

Attachment: Another revised patch (v0.4) taking onboard sr's comments

Changes made as suggested.

Comment 9

[Ian Neal]

Comment on attachment 158321 [details] [diff] [review]
Another revised patch (v0.4) taking onboard sr's comments

Carrying forward Neil's sr+

Comment 10

[Ian Neal]

Checking in downloadmanager.js;
/cvsroot/mozilla/xpfe/components/download-manager/resources/downloadmanager.js,v
  <--  downloadmanager.js
new revision: 1.40; previous revision: 1.39
done

Comment 11

[henry.ye]

*** Bug 289862 has been marked as a duplicate of this bug. ***