Closed
Bug 258219
Opened 20 years ago
Closed 20 years ago
Launch option in context menu should be hidden and OnTrigger should call showinshell on certain platforms as it is for toolbar
Categories
(SeaMonkey :: Download & File Handling, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: iannbugzilla, Assigned: iannbugzilla)
References
Details
Attachments
(1 file, 3 obsolete files)
3.19 KB,
patch
|
janv
:
review+
iannbugzilla
:
superreview+
|
Details | Diff | Splinter Review |
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.
Attachment #158072 -
Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #158072 -
Flags: review?(cbiesinger)
Comment 2•20 years ago
|
||
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?
Attachment #158072 -
Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #158072 -
Flags: review?(cbiesinger)
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
Attachment #158072 -
Attachment is obsolete: true
Summary: Launch option in context menu should be hidden on certain platforms as it is for toolbar → Launch option in context menu should be hidden and OnTrigger should call showinshell on certain platforms as it is for toolbar
Attachment #158075 -
Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #158075 -
Flags: review?(varga)
Comment 4•20 years ago
|
||
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
Attachment #158075 -
Flags: superreview?(neil.parkwaycc.co.uk) → superreview-
Comment on attachment 158075 [details] [diff] [review] Hides context launch and alters behaviour of double-click/return on certain platforms New patching coming soon
Attachment #158075 -
Attachment is obsolete: true
Attachment #158075 -
Flags: review?(varga)
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?
Attachment #158136 -
Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #158136 -
Flags: review?(cbiesinger)
Updated•20 years ago
|
Attachment #158136 -
Flags: review?(cbiesinger) → review?(varga)
Comment 7•20 years ago
|
||
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;
Attachment #158136 -
Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
Attachment #158136 -
Attachment is obsolete: true
Attachment #158136 -
Flags: review?(varga)
Comment on attachment 158321 [details] [diff] [review] Another revised patch (v0.4) taking onboard sr's comments Carrying forward Neil's sr+
Attachment #158321 -
Flags: superreview+
Attachment #158321 -
Flags: review?(varga)
Updated•20 years ago
|
Attachment #158321 -
Flags: review?(varga) → review+
Assignee | ||
Comment 10•20 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Updated•20 years ago
|
Product: Browser → Seamonkey
Comment 11•19 years ago
|
||
*** Bug 289862 has been marked as a duplicate of this bug. ***
You need to log in
before you can comment on or make changes to this bug.
Description
•