662203 - Enhance openLocation so that it can be used by message compose

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Ian Neal]

Attachment: Enhance openLocation patch

openLocation and MsgAttachPage are very similar, so enhancing openLocation would enable it to be used by the message compose window to attach web pages.
This patch:
* Uses existing openLocation parameter to indicate if it is being opened from message compose window.
* Makes use of Services.jsm where possible.
* Makes use of GetStringPref from utilityOverlay.js.
* Hides bottom hbox when being used from message compose window.
* Uses individual global variables rather than a global array.
* Uses local rather than global variables where appropriate.
* Simplified useUBHistoryItem by passing label rather than element as the argument.
* Removed now unused MsgAttachPage.dtd file.
* Moved MsgAttachPage.xul/js into TB only part of jar.mn.

Comment 1

[[e-mail]]

I guess bug 332668 made this possible, but bug 248193 predates it.

(In reply to comment #0)
> * Moved MsgAttachPage.xul/js into TB only part of jar.mn.
Only suite actually MsgAttachPage.* ;-)

Comment 2

[Ian Neal]

Attachment: Enhance openLocation patch

Changes since previous version:
* Removed MsgAttachPage.xul/js completely.

Them being in mailnews/ rather than suite/ threw me.

Comment 3

[Philip Chee]

While you are at it Bug 641312 (Reorder menus to promote tabs more) missed the openLocation dialog.
See Firefox Bug 322736. Also see Firefox Bug 405376 ("Current Window" option in Open Web Location window should be named "Current Tab")

Comment 4

[Ian Neal]

Attachment: Openlocation with extra tabness patch

Changes since last version:
* Revised order of action list.
* Renamed current window to current tab.

Comment 5

[[e-mail]]

Comment on attachment 537571 [details] [diff] [review]
Openlocation with extra tabness patch

>+  var end = gBundle.getString(action ? "attachEnd" : "openEnd");
>+  var enter = gBundle.getFormattedString("enterLabel", [end]);
>+  document.getElementById("enterLabel").value = enter;
...
>+  gLastPref = action ? GetAttachLast() : GetLocationLast();
This is nonobvious bordering on unreadable. I also notice that action is a boolean in parameter and a string out parameter.

My first idea was the following scheme:
switch (action) {
  case "4": // or use const kAttach = "4";
    // fix up strings that the attach dialog wants, override globals etc.
    break;
  case "2": // or use const kEdit = "2";
    // change "current tab" into "existing navigator window" etc.
    // fall through
  default:
    gOpenAppList.value = action;
    // read the last url (if set!) etc.
}

But then I realised you could use some other scheme for distinguishing the actions, add selected="true" on the current tab menuitem to make it default, and override it in the editor case.

>+  if (gInput.value)
>+    gInput.select(); // XXX should probably be done automatically
[It might; dialog.xml may have changed since this was first written.]

>+  var opening = !params.action;
>+  if (opening) {
>+    params.url = gInput.value;
How does the url get set in the attach case?

Comment 6

[Ian Neal]

Attachment: Openlocation with switching

Changes since previous version:
* Use string for action and then use that to switch.
* Fix return for attach url and then make it an attachment.
* Use two non-formatted strings rather than one formatted and two non-formatted.

Comment 7

[[e-mail]]

Comment on attachment 537867 [details] [diff] [review]
Openlocation with switching

>+    case "2": // open web page from composer
>+      gOpenAppList.selectedItem = document.getElementById("editWindow");
I notice that this doesn't actually work. It didn't before either. However, it seems that we could fix this as follows:

>+      // fall through to default
1. Don't fall through here.

>+    default: // open web page
>+      enter = gBundle.getString("openEnterLabel");
>+
>+      gOpenAppList.value = Services.prefs.getIntPref("general.open_location.last_window_choice");
>+      gLastPref = "general.open_location.last_url";
2. Move these assignments to their declarations.
   Or, in the case of enter, you could add this code after the switch:
   if (!enter)
     enter = gBundle.getString("openEnterLabel");

Comment 8

[Ian Neal]

Attachment: Openlocation with switching and cancel fix [Checked in: Comment 11]

Changes since last version:
* Fixed issue with was action was preselected when opening from composer window.
* Fixes issue when clicking cancel from open location dialog (most noticeable from composer window).

Comment 9

[[e-mail]]

(In reply to comment #8)
> * Fixes issue when clicking cancel from open location dialog (most
> noticeable from composer window).
You were lucky the way my prefs were set up it had no effect ;-)

Comment 10

[[e-mail]]

Comment on attachment 543840 [details] [diff] [review]
Openlocation with switching and cancel fix [Checked in: Comment 11]

r=me with the following changes:

>-    var params = { browser: null, action: null, url: "" };
>+    var params = { browser: null, action: "2", url: "" };
We don't use params.browser any more do we? (Same goes for navigator.js)

>+  // Clear arguments action to prevent problems on cancel.
>+  window.arguments[0].action = null;
I'd prefer this set to "-1" as an "unusual" value to match gAction.

>+  if (gInput.value)
>+    gInput.select(); // XXX should probably be done automatically
Looks like this is true these days.
(I remember when it wasn't. Probably fixed in the mass <dialog> rewrite.)

>+    if (!gAction)
Please change this and navigator.js to use "0" instead for consistency.

>+            <menuitem value="3" id="newTab" label="&newTab.label;"/>
>             <menuitem value="1" id="newWindow" label="&newWindow.label;"/>
>-            <menuitem value="3" id="newTab" label="&newTab.label;"/>
[Ah, heh!]

>+<!-- Note: enter.accesskey should be present in both enter.label as defined
>+     above and attachEnterLabel as defined in openLocation.properties -->
Nit: not the correct style of an L10N note. (Same for the .properties file)

Comment 11

[Ian Neal]

Comment on attachment 543840 [details] [diff] [review]
Openlocation with switching and cancel fix [Checked in: Comment 11]

http://hg.mozilla.org/comm-central/rev/8c885c3fc9ac