Closed Bug 196135 Opened 17 years ago Closed 3 years ago

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

Categories

(MailNews Core :: Address Book, defect, minor)

defect
Not set
minor

Tracking

(thunderbird52 fixed, thunderbird53 fixed)

RESOLVED FIXED
Thunderbird 53.0
Tracking Status
thunderbird52 --- fixed
thunderbird53 --- fixed

People

(Reporter: cavin, Assigned: bugzilla2007)

References

Details

(Keywords: polish)

Attachments

(1 file, 2 obsolete files)

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.
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.
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?
mass re-assign.
Assignee: racham → sspitzer
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.
Blocks: 156960
Product: Browser → Seamonkey
Assignee: sspitzer → mail
Assignee: mail → nobody
Component: Address Book → MailNews: Address Book
OS: Windows 2000 → All
Product: Mozilla Application Suite → Core
QA Contact: nbaca → addressbook
Hardware: PC → All
TB and suite
Keywords: polish
Summary: Double clicking on a PAB with mailing lists expands/collapses the twisty → Double clicking an address book with mailing lists expands/collapses the twisty, prevents access to rename/properties
morphing from 
 prevents access to rename/properties (which is no longer correct anyway)
to
 should not open AB rename/properties
Severity: normal → minor
Summary: Double clicking an address book with mailing lists expands/collapses the twisty, prevents access to rename/properties → Double clicking an address book (with mailing lists expands/collapses the twisty) should not open AB rename/properties
Product: Core → MailNews Core
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...
Blocks: 1319040
Blocks: 1319052
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.
Summary: Double clicking an address book (with mailing lists expands/collapses the twisty) should not open AB rename/properties → 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
Whiteboard: [l10n impact]
Blocks: 1319409
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! ;)
Assignee: nobody → bugzilla2007
Status: NEW → ASSIGNED
Attachment #8813181 - Flags: ui-review+
Attachment #8813181 - Flags: review?(acelists)
(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 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.
Attachment #8813181 - Flags: ui-review?(richard.marti)
Attachment #8813181 - Flags: ui-review+
Attachment #8813181 - Flags: review?(acelists)
Attachment #8813181 - Flags: feedback+
Looks like the patch is ANSI/windows-1252/ISO-8859-1 encoded. Like this it could be pushed from Windows.
OK, I'm fine with that if you converge on that working solution. I can manually replace the character in my local tests.
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
Attachment #8813181 - Flags: ui-review?(richard.marti) → ui-review+
Blocks: 1319493
(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?
Whiteboard: [l10n impact]
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.
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 ;-)
Blocks: 1320475
(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...
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.
Attachment #8813181 - Attachment is obsolete: true
Attachment #8814699 - Flags: ui-review+
Attachment #8814699 - Flags: review?(philip.chee)
Attachment #8814699 - Flags: review?(acelists)
Attachment #8814699 - Attachment description: 196135_ABDirTreeDoubleClick.new1.patch → Patch V.1.1 DirTree double click for properties only on mailing lists
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.
Attachment #8814699 - Flags: review?(acelists) → review+
No longer blocks: 1319040, 1319052, 1319409, 1319493, 1320475
Depends on: 1319409
Nits: && goes to the end of the line, "double-click" has a hyphen.
Attachment #8814699 - Attachment is obsolete: true
Attachment #8814699 - Flags: review?(philip.chee)
Attachment #8816448 - Flags: review+
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.
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 53.0
Blocks: 1322032
Comment on attachment 8816448 [details] [diff] [review]
Patch V.1.1 fixed nit.

Depends on bug 1319409 which didn't get uplift approval.
Attachment #8816448 - Flags: approval-comm-aurora? → approval-comm-aurora-
(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 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.
Attachment #8816448 - Flags: approval-comm-aurora- → approval-comm-aurora+
Blocks: 1331229
No longer blocks: 1331229
No longer blocks: 1322032
You need to log in before you can comment on or make changes to this bug.