196135 - Double clicking an address book (with mailing lists expands/collapses the twisty) should not open AB rename/properties: Add Expand/Collapse default action to context menu

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[Cavin Song]

Thi is spun off bug 17230.

Double clicking on a PAB with mailing lists opens the properties (rename)
dialog, and expands (or collapses) the twisty. event.preventBubble() doesn't
seem to fix it.

Comment 1

[[e-mail]]

That's because it's too late. The folder pane captures the click events on the
tree, thus getting to them before the tree does.

Comment 2

[Ninoschka Baca]

Trunk build 2003-03-06: Mac 10.1.5
I see the same behavior with any address book that has a  mailing list. 

Expected results: After double clicking onto an address book (that includes a
list), the dialog to rename the address book should open with out
expanding/collapsing the twisty, right?

Comment 3

[(not reading, please use [e-mail] instead)]

mass re-assign.

Comment 4

[Stefan Borggraefe]

I think comment #2 is wrong. In other trees in mozilla, e.g. the bookmarks tree
or the folder tree double-clicking never opens a rename/properties dialog.
Instead the subtree is collapsed or expanded. The Addressbook tree should expose
the same behaviour.

Comment 5

[Wayne Mery (:wsmwk)]

TB and suite

Comment 6

[Wayne Mery (:wsmwk)]

morphing from 
 prevents access to rename/properties (which is no longer correct anyway)
to
 should not open AB rename/properties

Comment 7

[Thomas D. (:thomas8)]

Unbelievable. Wonder how such irratiting, useless, non-standard behaviour ever made it into the product, and then last that long. Or maybe folks are just not using the Address Book because it's stale anyway...

Comment 8

[Thomas D. (:thomas8)]

So this is what needs to be done here (for ux-consistency with most other navigation trees in the world, e.g. Windows Explorer):

- Add an Expand/Collapse menuitem to context menu of directory tree items
- Probably implement a corresponding command, cmd_toggleOpen
- make that command the default action on double-click of ABs with Mailing lists, LDAP Dirs (for double-click on ABs without mailing lists, do nothing; on mailing lists, we currently open "Edit My Mailing List" dialogue which is somewhat unorthodox, but convenient - why not).
- For the proper way of doing dynamic menuitem labels, there's a recent example in my bug 1310442.

Comment 9

[Thomas D. (:thomas8)]

Attachment: Patch V.1 - DirTree double click for properties only on mailing lists

So let's just fix this; it can only get better.
Comment 8 is nice and perfect but that can wait for another day.

After this patch, double-click will no longer open AB properties.
For the sake of convenience, we still allow double-click on mailing lists in directory tree to edit their properties (which is logically expanding them...).

ui-r=me.

Aceman, this should be a quick review as the changes are minimal and obvious.
If bug 1319409 gets the green light, I'll adapt this accordingly.
We can land it nevertheless after your review. I'm in the mood of landing things! ;)

Comment 10

[Thomas D. (:thomas8)]

(In reply to Thomas D. (needinfo?me) from comment #9)
> Created attachment 8813181 [details] [diff] [review]
> Patch V.1 - DirTree double click for properties only on mailing lists
> 
> So let's just fix this; it can only get better.
> Comment 8 is nice and perfect but that can wait for another day.
> 
> After this patch, double-click will no longer open AB properties.

...but only expand or collapse "All Address Books" or ABs with mailing lists. Consistent with that, do nothing for double clicks on ABs without mailing lists. Editing AB or LDAP properties is far too rare a scenario to be the default action.

Comment 11

[:aceman]

Comment on attachment 8813181 [details] [diff] [review]
Patch V.1 - DirTree double click for properties only on mailing lists

Review of attachment 8813181 [details] [diff] [review]:
-----------------------------------------------------------------

This works for me.

My hg client choked on the ü in your name and failed to apply the patch, because the character is not encoded in utf8. I don't know if that is the same problem as when pushing patches to the server (that the char displays badly), that Jorg was pointing out. You can try changing the name to utf8 but not the commit comment (which is in English anyway), if you say TortoiseHG has problems with it.

::: mail/components/addrbook/content/abCommon.js
@@ +528,4 @@
>      return;
>    }
>  
> +  // Default action for double click is expand/collapse which ships with the tree.

Right above this line there is this code:
  var row = gDirTree.treeBoxObject.getRowAt(event.clientX, event.clientY);
  if (row == -1 || row > gDirTree.view.rowCount-1) {
    // double clicking on a non valid row should not open the dir properties dialog
    return;
  }

It seems the comment about properties dialog no longer applies as we never open properties now. Please fix it.

Also please set your hg program to put more lines of context into the patch. Usual value is 8 lines from both sides of lines changed in the patch.

@@ +701,4 @@
>    }
>  }
>  
> +function GetSelectedDirObj()

Please fix bug 1319409 first, or use the current badly named functions instead of this new code. Then you will fix this spot in bug 1319409 too.

Comment 12

[Jorg K (CEST = GMT+2)]

Looks like the patch is ANSI/windows-1252/ISO-8859-1 encoded. Like this it could be pushed from Windows.

Comment 13

[:aceman]

OK, I'm fine with that if you converge on that working solution. I can manually replace the character in my local tests.

Comment 14

[Richard Marti (:Paenglab)]

Comment on attachment 8813181 [details] [diff] [review]
Patch V.1 - DirTree double click for properties only on mailing lists

Looks good.

Not related to your patch, it appears also without it. This error appears when closing with the twisty:

Tue Nov 22 2016 19:19:25
Warning: ReferenceError: reference to undefined property "_parent"
Source file: chrome://messenger/content/jsTreeView.js
Line: 59

Comment 15

[:aceman]

(In reply to Richard Marti (:Paenglab) from comment #14)
> Tue Nov 22 2016 19:19:25
> Warning: ReferenceError: reference to undefined property "_parent"
> Source file: chrome://messenger/content/jsTreeView.js
> Line: 59

I also sometimes saw this. But it is not consistent and the patch does not seem to make it worse.
I see these errors in the AB occasionally but never can reproduce them reliably and there is no bug filed yet. Do you have STRs?

Comment 16

[Richard Marti (:Paenglab)]

STR:
- open AB window
- in AB tree all treeitems needs to be expanded
- click directly on the twisty on the left of the "All Address Books" treeitem to close the tree.
The error appears.

It appears only the first time after opening the AB window and only by closing the item on clicking on the twisty.

Comment 17

[Jorg K (CEST = GMT+2)]

Looks like this issue got moved to bug 1319550. Can you please address the issues from comment #11. I'm happy to land this before bug 1319409. As far as I can see Thomas has got a few bugs boiling: This one here, bug 1319409 and bug 1308776, so it would be good to get moving ;-)

Comment 18

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #11)
> Comment on attachment 8813181 [details] [diff] [review]
> It seems the comment about properties dialog no longer applies as we never
> open properties now. Please fix it.

Done. Patch in next comment.

> Also please set your hg program to put more lines of context into the patch.
> Usual value is 8 lines from both sides of lines changed in the patch.

Done, thanks for assisting me.
 
> > +function GetSelectedDirObj()
> 
> Please fix bug 1319409 first

Done. Waiting for suite review, but we won't wait long.

(In reply to Jorg K (GMT+1) from comment #17)
> Looks like this issue got moved to bug 1319550. Can you please address the
> issues from comment #11. I'm happy to land this before bug 1319409. As far
> as I can see Thomas has got a few bugs boiling: This one here, bug 1319409
> and bug 1308776, so it would be good to get moving ;-)

Indeed. Let's land bug 1319409 first, as Aceman suggested. Thank you very much.
There are more bugs than mentioned by you, with current patches of mine in this area...

Comment 19

[Thomas D. (:thomas8)]

Attachment: Patch V.1.1 DirTree double click for properties only on mailing lists

Per my last comment, this patch addresses Aceman's comment 11 (as für ü in patch author's name, let's just try... Can't we find out from the existing good landings how they succeeded t get it right?).

This patch must be applied on top of my patch in Bug 1319409, Attachment 8814669 [details] [diff].

Philip, can you please review the Suite part. This is a one-liner so it should be a matter of minutes. As Suite has the same code, I assume they experience the same problem, and the same solution should work. Please test.

Comment 20

[:aceman]

Comment on attachment 8814699 [details] [diff] [review]
Patch V.1.1 DirTree double click for properties only on mailing lists

Review of attachment 8814699 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/addrbook/content/abCommon.js
@@ +502,5 @@
> +  // Default action for double click is expand/collapse which ships with the tree.
> +  // For convenience, allow double click to edit the properties of mailing
> +  // lists in directory tree.
> +  if (gDirTree && gDirTree.view.selection && gDirTree.view.selection.count == 1
> +      && getSelectedDirectory().isMailList) {

I just find it strange that we check gDirTree.view.selection and then inside getSelectedDirectory() we get the selected directory via gDirTree.currentIndex, not view.selection. Hopefully those attributes are properly bound in the backend. But this is not your fault, it happens at multiple places.

Comment 21

[Jorg K (CEST = GMT+2)]

Attachment: Patch V.1.1 fixed nit.

Nits: && goes to the end of the line, "double-click" has a hyphen.

Comment 22

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/4dd65a2f806bb2daca6ef9d2620f534c1b064994

Landed with the correct "ü" in Düllmann; on Windows the patch needs to be ANSI encoded, on Linux and Mac it needs to be UTF-8 encoded. Be careful with other non-ASCII characters like ellipsis ("…")!

This patch has a suite/ part which was landed without SM review and approval. The suite/ change is the exact port of change made in mail/ (abCommon.js) which has been reviewed by two Thunderbird peers. I decided to port this minor change (effectively one line) to suite/ as well, although there was no need (unlike bug 1319409).

If you SM people are not happy with this, please let me know.

Comment 23

[Jorg K (CEST = GMT+2)]

Comment on attachment 8816448 [details] [diff] [review]
Patch V.1.1 fixed nit.

Depends on bug 1319409 which didn't get uplift approval.

Comment 24

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+1) from comment #23)
> Comment on attachment 8816448 [details] [diff] [review]
> Patch V.1.1 fixed nit.
> 
> Depends on bug 1319409 which didn't get uplift approval.

Ah well. It's quite exaggerated to say it depends when we are talking about a single function call which need replacement to land this without the other patch.

Comment 25

[Jorg K (CEST = GMT+2)]

Aurora (TB 52):
https://hg.mozilla.org/releases/comm-aurora/rev/a7025eae3ee4ac9bee37c5d9c4deb2ea7e3a5a2c

Comment 26

[Jorg K (CEST = GMT+2)]

Comment on attachment 8816448 [details] [diff] [review]
Patch V.1.1 fixed nit.

If changed my mind here since I uplifted bug 1319409.
See bug 1319409 comment #60 for details.