Closed Bug 513456 Opened 12 years ago Closed 12 years ago

Double click and Del do not trigger Edit/Delete in Customize Message Views window

Categories

(SeaMonkey :: MailNews: Message Display, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.0

People

(Reporter: InvisibleSmiley, Assigned: InvisibleSmiley)

Details

(Keywords: fixed-seamonkey2.0)

Attachments

(2 files, 6 obsolete files)

The Customize Message Views window lists the available views as tree rows, selectable and clickable. Still the only way to trigger Edit is by using the Edit button (or its access key). Double clicking a tree row should do the same.

Note: the same applies to TB and the windows look very similar, yet the code (mailViewList.xul, mailViewList.js) is not shared (anymore, I guess).

BTW: I actually think Enter/Return should also trigger Edit but it's already bound to OK so let's focus on double click here.
Del for Delete doesn't work either, extending scope of this bug.
Summary: Double click does not trigger Edit in Customize Message Views window → Double click and Del do not trigger Edit/Delete in Customize Message Views window
Attached patch proposed patch (obsolete) — Splinter Review
Checked that the following works correctly:
- double click on item invokes Edit on it
- double click on empty space does nothing
- Del with selected item invokes Delete on it
- Del without selection does nothing
- New/Edit/Delete buttons still work as before

Note: No multiple selections possible by design.
Assignee: nobody → jh
Status: NEW → ASSIGNED
Attachment #397653 - Flags: review?(mnyromyr)
Comment on attachment 397653 [details] [diff] [review]
proposed patch

Looks basically good, just some nits.
But while you're touching many/most of the stuff anyway, let's do a bit of cleanup as well...

>diff --git a/suite/mailnews/mailViewList.js b/suite/mailnews/mailViewList.js
>@@ -46,25 +47,32 @@ var gMailListView;
> var gListBox; 
> var gEditButton;
> var gDeleteButton;

The defaultController should go here, have a 'g' prefix and a better name, e.g. gMailViewListController.

> function mailViewListOnLoad()

Capitalize all (just seven) old functions and, of course, your new ones.
<https://wiki.mozilla.org/index.php?title=SeaMonkey:MailNews:CodingStyle#Capitalize_function_names>
(Add 'a' prefixes where necessary.)

>   gMailListView = Components.classes["@mozilla.org/messenger/mailviewlist;1"].getService(Components.interfaces.nsIMsgMailViewList);; 

Please wrap.

> function refreshListView(aSelectedMailView)
...
>   var numItems = gMailListView.mailViewCount;
>   var mailView; 

Move mailView as a let variable into the loop (and kill the trailing space before the round brace).

Cleanup the wrapping in onDeleteMailView().

>@@ -126,18 +134,74 @@ function onEditMailView()

How is
  if (selectedIndex >= 0)
supposed to be false?
Use let in subscopes.

>     var args = {mailView: selMailView, onOkCallback: refreshListView};
> 

Drop the empty lines before/after args.

>     window.openDialog('chrome://messenger/content/mailViewSetup.xul', "", 'centerscreen,modal,resizable,titlebar,chrome', args);

One line per argument, please.

> function updateButtons()
> {
>   var selectedIndex = gListBox.selectedIndex;
>   // "edit" and "delete" only enabled when one filter selected
>   gEditButton.disabled = selectedIndex < 0;
>   gDeleteButton.disabled = selectedIndex < 0;

No need to calculate the same boolean twice.

>+  },
>+  isCommandEnabled: function(aCommand)

Separate all members by an empty line.

>+  onCommandUpdate: function()
>+  {
>+    var cmds = ["cmd_new", "cmd_edit", "cmd_delete"];
>+    for (let command in cmds)
>+      goUpdateCommand(cmds[command]);

Too complicated:
  for each (let command in ["cmd_new", "cmd_edit", "cmd_delete"])
    goUpdateCommand(command);

>+};
>\ No newline at end of file

Make sure that there is...

>diff --git a/suite/mailnews/mailViewList.xul b/suite/mailnews/mailViewList.xul
> <dialog id="mailViewListDialog"
>         xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>         onload="mailViewListOnLoad();"
>+        onunload="mailViewListOnUnload();"
>         windowtype="mailnews:mailviewlist"
>         title="&mailViewListTitle.label;"
>         width="400" height="340"
>         buttons="accept"

The "OK" button doesn't make much sense in this dialog, even more so its grabbing of the Enter command.
The button should be titled "Close" (or whatever that title is for the current platform) and hitting Enter should work like cmd_edit. It may be best to use a cancel button here anyway. 

>+  <commandset id="mailViewCommands">
>+    <command id="cmd_new" oncommand="goDoCommand('cmd_new');"/>
>+    <command id="cmd_edit" oncommand="goDoCommand('cmd_edit');"/>
>+    <command id="cmd_delete" oncommand="goDoCommand('cmd_delete');"/>
>+  </commandset>

Permission to align the oncommand attributes for enhanced beauty bonus. ;-)
Attachment #397653 - Flags: review?(mnyromyr) → review-
Attached patch patch v2 (obsolete) — Splinter Review
(In reply to comment #3)
> >@@ -126,18 +134,74 @@ function onEditMailView()
> 
> How is
>   if (selectedIndex >= 0)
> supposed to be false?

Well, only if no item is selected I guess (which is the case initially). The check is in OnDelete and OnEdit but it doesn't hurt, maybe it even prevents someone calling it foolishly (extension authors?); I kept it.

> The "OK" button doesn't make much sense in this dialog, even more so its
> grabbing of the Enter command.
> The button should be titled "Close" (or whatever that title is for the current
> platform) and hitting Enter should work like cmd_edit. It may be best to use a
> cancel button here anyway. 

Yeah, so we get Enter/Return for Edit, nice. :-) It was a bit strange to find that helpMessengerOverlay.xul needs to be adapted, too and that the default dialog accept handler (which closes the window, even if not defaultButton is set) needs to be overridden.

Note: I moved the key press handling to JS like in the filter dialog. I think it's cleaner that way and has the benefit of not having to include utilityOverlay.xul just to get key_delete.
Attachment #397653 - Attachment is obsolete: true
Attachment #399250 - Flags: superreview?(neil)
Attachment #399250 - Flags: review?(mnyromyr)
Attachment #399250 - Flags: review?(mnyromyr) → review-
Comment on attachment 399250 [details] [diff] [review]
patch v2

>diff --git a/suite/common/helpMessengerOverlay.xul b/suite/common/helpMessengerOverlay.xul
>   <dialog id="mailViewListDialog"
>-          buttons="accept,help"
>+          buttons="cancel,help"
>           ondialoghelp="return openHelp('message-views-using');"/>

I'm still not quite happy with that. Cancel is of course less intrusive than OK, but it's still wrong here - nothing gets cancelled. At the time of the last review, I imagined something like the Close button in the Filter Log dialog (Tools -> Message Filters -> Filter Log). But apart from that we don't have many of these (<http://mxr.mozilla.org/comm-central/search?string=%22close%22&find=suite>), in fact they're depracated and we shouldn't introduce new ones.

This leaves us with dropping the button entirely - which results in a rather odd dialog layout, because the help button is still there. Hence the help button should be "freed" from the dialog and put below the other buttons, but of course "keep" it's icon. (The Message Filter dialog, too, has a custom Help button, although it lacks the icon - I'll file a bug on that.)
+-----------------+
|List      New    |
|...       Edit   |
|........  Delete |
|....             |
|.                |
|....      Help   |
+-----------------+
The dialog - like other of its "sort" - will close on Escape or clicking the X (etc.) provided by the Window manager.

>diff --git a/suite/mailnews/mailViewList.js b/suite/mailnews/mailViewList.js
>+var gMailViewListController;

The implementation should go here as well, of course... ;-)

>+  for (let index = gListBox.getRowCount(); index > 0; index--)
>     gListBox.removeChild(gListBox.getItemAtIndex(index - 1));
> 
>   var numItems = gMailListView.mailViewCount;
>-  var mailView; 
>   for (index = 0; index < numItems; index++)

Either too much let or too few: ;-)

Warning: assignment to undeclared variable index
Source File: chrome://messenger/content/mailViewList.js
Line: 80
The custom Help button in the Message Filter dialog is not a very good example, the Help button definition should still come from helpMessengerOverlay.xul.
I filed bugs 515227, 515228 and 515230 on the Help icon issues.
Attached patch patch v3 (obsolete) — Splinter Review
Attachment #399250 - Attachment is obsolete: true
Attachment #399327 - Flags: superreview?(neil)
Attachment #399327 - Flags: review?(mnyromyr)
Attachment #399250 - Flags: superreview?(neil)
I haven't tested this, but by reading http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/themes/pinstripe/global/dialog.css I suspect it will regress mac...
(In reply to comment #10)
> I haven't tested this, but by reading
> http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/themes/pinstripe/global/dialog.css
> I suspect it will regress mac...

Probably. Can you please apply the patch and check if setting class="dialog-button" dlgtype="help" on the button (probably best in helpMessengerOverlay.xul) works?
Comment on attachment 399327 [details] [diff] [review]
patch v3

>-  <dialog id="mailViewListDialog"
>-          buttons="accept,help"
>-          ondialoghelp="return openHelp('message-views-using');"/>
>+  <button id="mailViewHelpButton"
>+          icon="help"
>+          oncommand="return openHelp('message-views-using');"/>
> 
>   <dialog id="mailViewSetupDialog"
>           buttons="accept,cancel,help"
>           ondialoghelp="return openHelp('message-views-create-new');"/>
Hmm, these files are forked, so we don't need to overlay them anymore, do we?

(Full list of forked files:
msgSelectOffline.xul
subscribe.xul
mailViewList.xul
mailViewSetup.xul)
(In reply to comment #12)
> Hmm, these files are forked, so we don't need to overlay them anymore, do we?

Maybe there's no need to overlay but isn't the idea to have the definitions ("bindings") in one place (cf. comment 6 and the bugs listed in comment 7)?
(In reply to comment #11)

> Can you please apply the patch and check if setting
> class="dialog-button" dlgtype="help" on the button (probably best in
> helpMessengerOverlay.xul) works?

Well, since you've choosed to not display any dialog-buttons it will automatically be hidden by the binding.
fwiw, I can atm only think of one solution: copy the relevant css (all rules for dlgtype="help") from toolkit/themes/pinstripe/global/dialog.css to themes/classic/mac/communicator/button.css.
It'd be far easier to
- say buttons="help"
- include chrome://global/skin/dialog.css in mailViewList.xul
- move the button to its new location on dialog startup (one line of code!)
Comment on attachment 399327 [details] [diff] [review]
patch v3

Almost there. The help button needs some special treatment to look right on Mac, but this actually makes the code and layout easier...

>diff --git a/suite/common/helpMessengerOverlay.xul b/suite/common/helpMessengerOverlay.xul
>-  <dialog id="mailViewListDialog"
>-          buttons="accept,help"
>-          ondialoghelp="return openHelp('message-views-using');"/>

Keep this, just remove the accept button.

>diff --git a/suite/mailnews/mailViewList.xul b/suite/mailnews/mailViewList.xul

We need dialog.css as well:
<?xml-stylesheet href="chrome://global/skin/dialog.css" type="text/css"?>

> <dialog id="mailViewListDialog"
>         xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>-        onload="mailViewListOnLoad();"
>+        onload="MailViewListOnLoad();"
>+        onunload="MailViewListOnUnload();"
>+        ondialogaccept="return false;"
>         windowtype="mailnews:mailviewlist"
>         title="&mailViewListTitle.label;"
>         width="400" height="340"
>-        buttons="accept"
>+        buttons=","

Still correct, since the help button will come from the overlay.

>       <vbox>

This needs an id, eg. buttonColumn.

>+        <spacer flex="1"/>
>+        <button id="mailViewHelpButton"
>+                label="&helpButton.label;"
>+                accesskey="&helpButton.accesskey;"/>

Remove the button, but keep the spacer. We'll move the button here in the startup code.

>diff --git a/suite/mailnews/mailViewList.js b/suite/mailnews/mailViewList.js
>+function MailViewListOnLoad()
...
>   gListBox = document.getElementById('mailViewList');
>+  

Trailing whitespace. Actually, just remove trailing whitespace in mailViewList.* globally. ;-)

Also, add code here to move the help button from the dialog footer to its wanted location, eg.:

var helpButton = document.getAnonymousElementByAttribute(document.documentElement, "dlgtype", "help");
document.getElementById("buttonColumn").appendChild(helpButton);
Attachment #399327 - Flags: review?(mnyromyr) → review-
Attached patch patch v4 (obsolete) — Splinter Review
Attachment #399327 - Attachment is obsolete: true
Attachment #399833 - Flags: superreview?(neil)
Attachment #399833 - Flags: review?(mnyromyr)
Attachment #399327 - Flags: superreview?(neil)
Comment on attachment 399833 [details] [diff] [review]
patch v4

>diff --git a/suite/mailnews/mailViewList.js b/suite/mailnews/mailViewList.js
> var gEditButton;
> var gDeleteButton;
>+var gHelpButton;

No use to make it global, Edit and Delete are only global because they're used in several places (and hence New isn't globally available either).

>+<!-- needed for moved Help button on Mac -->
>+<?xml-stylesheet href="chrome://global/skin/dialog.css" type="text/css"?>

Maybe better:
<!-- Mac needs dialog.css to correctly style the moved Help button -->


r=me with that
Attachment #399833 - Flags: review?(mnyromyr) → review+
Attached patch patch v4a (obsolete) — Splinter Review
Attachment #399833 - Attachment is obsolete: true
Attachment #399853 - Flags: superreview?(neil)
Attachment #399853 - Flags: review+
Attachment #399833 - Flags: superreview?(neil)
Comment on attachment 399853 [details] [diff] [review]
patch v4a

>+  // move help button below the others, separated by the existing spacer
>+  let helpButton = document.getAnonymousElementByAttribute(document.documentElement,
>+                                                           'dlgtype', 'help');
>+  document.getElementById('buttonCol').appendChild(helpButton);
This is really nasty :-( But since the file is forked, we can kill the overlay and add an explicit button to the XUL. (dialog.xml supports explicit <button dlgtype> elements as well as its anonymous buttons.)

>+function OnMailViewSelect(aEvent)
> {
>+  gMailViewListController.onCommandUpdate();
>+  UpdateButtons();
By my reading of the code, onCommandUpdate calls goUpdateCommand which sets/clears the disabled attribute on the command nodes, which sets/clears the disabled attribute on the buttons, thus making UpdateButtons unnecessary. (Don't forget to call onCommandUpdate on load though!)

>+    case KeyEvent.DOM_VK_DELETE:
Hmm, pressing Delete doesn't trigger cmd_delete? Perhaps not. But you might want to include backspace too for the benefit of Macbook users.

>+<!-- Mac needs dialog.css to correctly style the moved Help button -->
File a bug? (Point out dialog.xml's support for explicit <button dlgtype>.)

> <dialog id="mailViewListDialog"
Dialog? or window? How do we close it? I guess ESC works.

>+      <listbox id="mailViewList"
[At some point this might become a tree, depending on whether listboxes keep their "interesting" lack of XBL on newly created listitems behaviour.]
(In reply to comment #21)
> >+  document.getElementById('buttonCol').appendChild(helpButton);
> This is really nasty :-( But since the file is forked, we can kill the overlay
> and add an explicit button to the XUL.

Could we kill the entire helpMessengerOverlay.xul? (Guess not.)
If not, the overlay nicely bundles all help handler knowledge in one place, which might be a plus. I'm a bit torn undecided here...

> (dialog.xml supports explicit <button> dlgtype> elements as well as its
> anonymous buttons.)

Oh, nice, I didn't know that. 

<button id="helpButton"
        dlgtype="help"
        class="dialog-button"/>

does indeed work as expected on Mac (instead of the onload code).

> >+    case KeyEvent.DOM_VK_DELETE:
> Hmm, pressing Delete doesn't trigger cmd_delete? Perhaps not. But you might
> want to include backspace too for the benefit of Macbook users.

Hmpf, I forgot about key_delete2 (and I should know!). :-((
Then I think overlaying the keys would be best in terms of define-once-use-everywhere.

> >+<!-- Mac needs dialog.css to correctly style the moved Help button -->
> File a bug? (Point out dialog.xml's support for explicit <button dlgtype>.)

I assume toolkit peers will say "include dialog.css, then", but maybe worth a try. Maybe even a Mac only bug, since it seems to work without under Linux and Windows.

> > <dialog id="mailViewListDialog"
> Dialog? or window? How do we close it? I guess ESC works.

Dialog. Escape works.
(In reply to comment #21)
> >+function OnMailViewSelect(aEvent)
> > {
> >+  gMailViewListController.onCommandUpdate();
> >+  UpdateButtons();
> By my reading of the code, onCommandUpdate calls goUpdateCommand which
> sets/clears the disabled attribute on the command nodes, which sets/clears the
> disabled attribute on the buttons, thus making UpdateButtons unnecessary.
> (Don't forget to call onCommandUpdate on load though!)

I managed to remove UpdateButtons but only after discovering that the onCommandUpdate change proposed in comment 3 was wrong (FOR IN iterates over the array indices 0-2 instead of the array values so I changed it back). I also couldn't call onCommandUpdate in MailViewListOnLoad because the controller somehow hasn't been initialized yet (controller in globalOverlay.js goUpdateCommand is null) so I added disabled="true" to the cmd_edit and cmd_delete commands in mailViewList.xul (which is also a more obvious place IMO).

> >+    case KeyEvent.DOM_VK_DELETE:
> Hmm, pressing Delete doesn't trigger cmd_delete? Perhaps not. But you might
> want to include backspace too for the benefit of Macbook users.

Added locally.
Attached patch patch v5 (obsolete) — Splinter Review
Added help button and keys (+ platform overlay) to mailViewList.xul, removed keypress handler (+ call), kept the help overlay. Let's finally wrap this up, shall we?
Attachment #399853 - Attachment is obsolete: true
Attachment #400139 - Flags: superreview?(neil)
Attachment #400139 - Flags: review?(mnyromyr)
Attachment #399853 - Flags: superreview?(neil)
Comment on attachment 400139 [details] [diff] [review]
patch v5

(In reply to comment #23)
> the onCommandUpdate change proposed in comment 3 was wrong (FOR IN iterates
> over the array indices 0-2 instead of the array values so I changed it back).

Erm, you didn't implement what I proposed, you forgot the "each" keyword, which of course doesn't work as intended. ;-)

r=me with for each used on checkin.
Attachment #400139 - Flags: review?(mnyromyr) → review+
(In reply to comment #22)
> Could we kill the entire helpMessengerOverlay.xul? (Guess not.)
We definitely need it for shared windows (which is why it was created).

> If not, the overlay nicely bundles all help handler knowledge in one place,
> which might be a plus.
You could still use the overlay just to add the button. (You might need to give the container an id so that you have somewhere to attach the button to.) You then have to decide where to put the CSS include though, it looks odd adding the CSS to the customise window, but if you add it to the overlay you affect all the windows that it gets overlaid on to, which is inefficient.

> > >+<!-- Mac needs dialog.css to correctly style the moved Help button -->
> > File a bug? (Point out dialog.xml's support for explicit <button dlgtype>.)
> I assume toolkit peers will say "include dialog.css, then", but maybe worth a
> try. Maybe even a Mac only bug, since it seems to work without under Linux and
> Windows.
(Should be easy to fix by moving the style rules from dialog.css to button.css)
Comment on attachment 400139 [details] [diff] [review]
patch v5

I'm still not keen on the the use of this overlay, although while I'm here I'd like to suggest the alternative of a second overlay used to add arbitrary help buttons to mail windows.
Attachment #400139 - Flags: superreview?(neil) → superreview+
Attached patch patch v5aSplinter Review
Patch with nit from comment 25 addressed to ease checkin.

Basically a simple change (adds support for double click, Enter, Return, Delete and Backspace, the latter only on Mac) with the overhead of cleanup as requested through review. The only other notable change is the Help button which moved from the standard position to below the other buttons, see screen shot.
Attachment #400139 - Attachment is obsolete: true
Attachment #401947 - Flags: superreview+
Attachment #401947 - Flags: review+
Attachment #401947 - Flags: approval-seamonkey2.0?
Attachment #401947 - Flags: approval-seamonkey2.0? → approval-seamonkey2.0+
Keywords: checkin-needed
http://hg.mozilla.org/comm-central/rev/d736bb6bc996
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.0
You need to log in before you can comment on or make changes to this bug.