825596 - Support some actions of WM_APPCOMMAND such as New, Open, Close, Save, Find, Help, SendMail, ReplyToMail and ForwardMail ("Close" Multimedia Key on Windows closes browser, not current tab)

Status: NEW

(SeaMonkey :: OS Integration, defect)

Description

[Philip Chee]

From Bug 672193 Comment 8:

> Next, we should need to add new command atoms for New, Open, Close, Find, Help, 
> Print, Send Mail, Forward Mail and Reply To Mail.
> 
> Firefox should do nothing for Forward Mail and Reply To Mail. So, nsWindow 
> should check the result of the command event. And command event handler should 
> consume the events if it handles the events.
> 
> New and Close cause opening or closing tab on other browsers (IE10, Opera and 
> Chrome). So, we should handle the manipulation target is current tab.

Comment 1

[Philip Chee]

See https://hg.mozilla.org/mozilla-central/rev/bac88e283278 for details.

Comment 2

[Philip Chee]

> See https://hg.mozilla.org/mozilla-central/rev/bac88e283278 for details.

1. We have BrowserOpenTab()
2. We have BrowserCloseTabOrWindow()
3. for gFindBar.onFindCommand()
   We can use BrowserFind() which calls findInPage(getFindInstData()) which calls findbar.onFindCommand().
4. openHelpLink('firefox-help')
   We can open our in-application help via toolkit function openHelp();
   See http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/help/content/contextHelp.js#12
5. We have BrowserOpenFileWindow()
6. We have PrintUtils.print()
7. saveDocument(window.content.document) This is a toolkit function so we have that.
8.
> +  case "SendMail":
> +    MailIntegration.sendLinkForWindow(window.content);
For this we can use function sendPage(aDocument) (from mailNavigatorOverlay.js)

We might also want to extend HandleAppCommandEvent() in:
1. msgMail3PaneWindow.js
2. messageWindow.js.

Comment 4

[Abhirav Kariya]

hi! I'm new here , Please assign this bug to me!

Comment 5

[Mike Hoye [:mhoye]]

Hi, Abhirav, and welcome aboard. We'll typically assign a bug to a new contributor as soon as they attach a preliminary patch to the bug. If you have any questions getting set, please join us on IRC in the #introduction channel, and good luck!

Comment 6

[Abhirav Kariya]

sir, what is your nick on the IRC , I couldn't find u in the user list . Please help me get started as this is my 1st attempt, thank you!

Comment 7

[Philip Chee]

Hi!

In addition to #introduction you should also use #SeaMonkey for SeaMonkey specific developer questions.

We'll assign the bug to you when you've started work on a patch.

Comment 8

[Kousik Satish]

Hi!

I am new here... Where can I find the full code... Is there any repository???

Comment 9

[Philip Chee]

(In reply to Kousik Satish from comment #8)
> Hi!
> 
> I am new here... Where can I find the full code... Is there any repository???

Please read this first:
https://developer.mozilla.org/en-US/docs/Simple_SeaMonkey_build

SeaMonkey code resides in http://hg.mozilla.org/comm-central/ and also makes use of code from https://hg.mozilla.org/mozilla-central/

Comment 10

[Kousik Satish]

(In reply to Philip Chee from comment #9)
 
> Please read this first:
> https://developer.mozilla.org/en-US/docs/Simple_SeaMonkey_build
> 
> SeaMonkey code resides in http://hg.mozilla.org/comm-central/ and also makes
> use of code from https://hg.mozilla.org/mozilla-central/

Thank you very much...! I will check it out...!

Comment 11

[Ravi Shankar [:waffles]]

Hey Philip, I think I can work on this bug. I've already worked on a few bugs and so we can directly jump into the bug itself. So, what are we doing here? Can you get me started?

Comment 12

[Philip Chee]

First read comment 0 comment 1 and comment 2 and then for some background Firefox/Core bug 825596.

This is the Firefox version of HandleAppCommandEvent()
http://mxr.mozilla.org/comm-central/source/mozilla/browser/base/content/browser.js?rev=197f69574d75#1764
And here is SeaMonkey's:
http://mxr.mozilla.org/comm-central/source/suite/browser/navigator.js?rev=83106028dd34#401

What we want is to extend the SeaMonkey version to match Firefox. The names of the Firefox functions are not always the same as SeaMonkey so you might need to do some adjustments.

Comment 13

[Ravi Shankar [:waffles]]

Attachment: appevents.patch

It's just my initial patch (and I haven't done much - only the switch cases). I have a few questions. Are there any arguments to the `openHelp()` function call? Because, Firefox has passed a topic, and I don't know the topic name for Sea Monkey. And, that `aDocument` we pass into `sendPage()`, does it work for `msgMail3PaneWindow.js` and `messageWindow.js`? Apart from these, should I add any comments regarding those functions?

Comment 14

[Ravi Shankar [:waffles]]

Oh, and I forgot something. The changeset you showed in comment #1 has revisions made to "/mozilla/widget/windows/nsWindow.cpp" and "/mozilla/dom/base/nsGkAtomList.h". While the header file already has the necessary changes, the CPP file doesn't. Am I supposed to revise it?

Comment 15

[Philip Chee]

Hi! Sorry for the slow response. Anything under /mozilla/ is code in mozilla-central which we should not touch in *this* bug. This does mean that we are limited at the moment to the "atoms" in those files. But the current list should be sufficient for this bug at least.

Comment 16

[Ravi Shankar [:waffles]]

No problem Philip. I do understand the reason for your slow response. But, you haven't reviewed my patch yet :)

Comment 17

[Philip Chee]

Comment on attachment 8612486 [details] [diff] [review]
appevents.patch

> +++ b/suite/browser/navigator.js

> +    case "Help":
> +      openHelp();

function openHelp(topic, contentPack)
http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/help/content/help.js?rev=58fe9dc85fa4#65
The default topic is "welcome". So we just need to specify the contentPack. In our case this is:
"chrome://communicator/locale/help/suitehelp.rdf"
openHelp("", "chrome://communicator/locale/help/suitehelp.rdf")

> +    case "Print":
> +      PrintUtils.print();
Not your fault but we are switching to the new Print API
See Bug 1146454
https://hg.mozilla.org/mozilla-central/rev/dfd2a128d19c
and Bug 1195863
https://bug1195863.bmoattachments.org/attachment.cgi?id=8649372
You'll need something like this:
PrintUtils.printWindow(gBrowser.selectedBrowser.outerWindowID,
                       gBrowser.selectedBrowser);

> +    case "Save":
> +      saveDocument(window.content.document);
This will work but we can use the new API introduced in Bug 1141337
         saveBrowser(gBrowser.selectedBrowser)

> +++ b/suite/mailnews/messageWindow.js

I think some of these aren't appropriate.

>      case "Home": 
> +      goDoCommand('cmd_goStartPage');
Please remove this.

> +    case "New":
> +      BrowserOpenTab();
Please remove this.

> +    case "Close":
> +      BrowserCloseTabOrWindow();
Looking at File->Close we should be using
goDoCommand("cmd_close");

> +    case "Find":
> +      BrowserFind();
Use goDoCommand("cmd_find");

> +    case "Help":
> +      openHelp();
openHelp("mail", "chrome://communicator/locale/help/suitehelp.rdf");

> +      break;
> +    case "Open":
> +      BrowserOpenFileWindow();
(1) This is wrong.
(2) Also BrowserOpenFileWindow() isn't available in this window.
(3) File->Open calls MsgOpenFromFile() so use this instead.

> +    case "Print":
> +      PrintUtils.print();
Please remove this.

> +    case "Save":
> +      saveDocument(window.content.document);
> +      break;
Please remove this.

> +    case "SendMail":
> +      sendPage(aDocument);
Please remove this.


> +++ b/suite/mailnews/msgMail3PaneWindow.js

I think some of these aren't appropriate.

> +    case "New":
> +      BrowserOpenTab();
File->New in this window has several more or less equal options.
Please remove this.

> +    case "Close":
> +      BrowserCloseTabOrWindow();
Looking at File->Close we should be using
goDoCommand("cmd_close");

> +    case "Find":
> +      BrowserFind();
Please remove this. Find in message depends on whether a message is open and focused or not.

> +    case "Help":
> +      openHelp();
openHelp("mail", "chrome://communicator/locale/help/suitehelp.rdf");

> +    case "Open":
> +      BrowserOpenFileWindow();
(1) This is wrong.
(2) Also BrowserOpenFileWindow() isn't available in this window.
(3) File->Open calls MsgOpenFromFile() so use this instead.

> +    case "Print":
> +      PrintUtils.print();
(1) There should be a break;
(2) Please remove this item.
(3) Printing a message depends on whether a message is open and focused or not.

> +    case "Save":
> +      saveDocument(window.content.document);
Please remove this.

> +    case "SendMail":
> +      sendPage(aDocument);
Please remove this.

Comment 18

[Ravi Shankar [:waffles]]

Meh, I can't even recall whether it's me who really did this! (quite embarrassing indeed) :P
Anyways, I'll finish it off by this weekend :)