Closed Bug 462843 Opened 16 years ago Closed 16 years ago

Cannot change sort column and direction in contacts sidebar in compose window

Categories

(Thunderbird :: Message Compose Window, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b2

People

(Reporter: takeshi2, Assigned: takeshi2)

Details

Attachments

(1 file, 4 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; ja; rv:1.9.0.3) Gecko/2008092417 Firefox/3.0.3
Build Identifier: 

Sort order and direction in results list in contacts sidebar in compose window cannot be changed. I don't know since when this does not work. It does not work both on trunk and on 1.8.1 branch.

Reproducible: Always

Steps to Reproduce:
1. Click Write button to open a compose window.
2. View / Contacts Sidebar
3. Click GeneratedName and PrimaryEmail several times.
Actual Results:  
Nothing occurs.

Expected Results:  
Some action occurs - clicked column is selected or sort direction indicator is changed.
Copied from |AbResultsPaneOnClick()|:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/addrbook/resources/content/abResultsPane.js&rev=1.16&mark=37#37
(but double click proccessing is removed because it is proccessed in elsewhere.)

Fix for branch does not include patch to preserve sort column and direction in reopening cached compose window. This can be fixed by attachment 345600 [details] [diff] [review].
Fix for trunk includes the patch to preserve sort direction in opening another fresh compose window.
Attachment #346027 - Flags: review?(bugzilla)
Attachment #346027 - Flags: review?(bugzilla)
Takeshi, I think we do want this patch I just haven't had time to take a look at it - I've had other items taking up my time over the last week or so. If you request review again, I'll look at it as soon as I can (hopefully early next week).
Status: UNCONFIRMED → NEW
Ever confirmed: true
(In reply to comment #3)
Sorry for confusing.
I cancelled review because I had not understood the steps to propose patches.
Now I'll (change status to new and) assign this bug to me and request review again.
Thanks in advance for reviewing.
Assignee: nobody → takeshi2
Attachment #346027 - Flags: superreview?(bienvenu)
Attachment #346027 - Flags: review?(bugzilla)
Comment on attachment 346027 [details] [diff] [review]
Fix: add event listener (for trunk) 

thx for the patch...

+  if (t.localName == "treecol") {
+    var sortDirection;      
+    var currentDirection = t.getAttribute("sortDirection");
+
+    if (currentDirection == kDefaultDescending)
+        sortDirection = kDefaultAscending;
+      else 
+        sortDirection = kDefaultDescending;
+
+    SortAndUpdateIndicators(t.id, sortDirection);
+  }


This can be something like 

+  if (t.localName == "treecol")
+  sortDirection = t.getAttribute("sortDirection") == kDefaultAscending ? kDefaultAscending : kDefaultDescending;

maybe change the var name from t to target - it's a little more readable.

Also, that really long xul line should really be broken into shorter lines.

I'll let Mark comment on whether this is the right approach...
Attachment #346027 - Flags: superreview?(bienvenu) → superreview-
Attachment #346027 - Flags: review?(bugzilla)
Attached patch Fix for trunk v.2 (obsolete) — Splinter Review
Thanks for suggestion. Addressed David's comment.
Attachment #346027 - Attachment is obsolete: true
Attachment #348259 - Flags: review?(bugzilla)
Comment on attachment 348259 [details] [diff] [review]
Fix for trunk v.2 

Looking good, but I think I'd like to see contactsListDoubleClick and contactsListOnClick merged into one function, see for example AbResultsPaneOnClick in abResultsPane.js: http://mxr.mozilla.org/comm-central/source/mailnews/addrbook/resources/content/abResultsPane.js#263
Attachment #348259 - Flags: review?(bugzilla) → review-
Attached patch Fix for trunk v.3 (obsolete) — Splinter Review
Thanks for suggestion. Addressed Mark's comment #7.
Attachment #346024 - Attachment is obsolete: true
Attachment #348259 - Attachment is obsolete: true
Attachment #351820 - Flags: review?(bugzilla)
Attachment #351820 - Flags: review?(bugzilla) → review+
Comment on attachment 351820 [details] [diff] [review]
Fix for trunk v.3

Sorry for the delay in reviewing this. Thanks for doing the patch. Just some minor comments:

>+  var target = event.originalTarget;
>+  if (target.localName == "treecol") {
>+    var sortDirection = target.getAttribute("sortDirection") == kDefaultDescending ? kDefaultAscending : kDefaultDescending;      

Please can you format this like:

    var sortDirection = target.getAttribute("sortDirection") == kDefaultDescending ?
                        kDefaultAscending : kDefaultDescending;

and drop the blank space from the end of the line.

>+  else if (target.localName == "treechildren" && event.detail == 2) {
>+    var contactsTree = document.getElementById("abResultsTree");
>+    var row = contactsTree.treeBoxObject.getRowAt(event.clientX, event.clientY);
>+    if (row == -1 || row > contactsTree.view.rowCount-1)
>+    {
>+      // double clicking on a non valid row should not open the edit filter dialog
>+      return;
>+    }

You can drop the brackets { } here, as it only one line. Please add a blank line after the return, and update the comment as we're not opening the edit filter dialog either.

r=me with those fixed.
Oops. Thanks a lot. Fixed Mark's comment #9.
Attachment #351820 - Attachment is obsolete: true
Attachment #353980 - Flags: superreview?(bienvenu)
Attachment #353980 - Flags: review+
Attachment #353980 - Flags: superreview?(bienvenu) → superreview+
Comment on attachment 353980 [details] [diff] [review]
Fix for trunk v.4
[Checkin: Comment 12]

thx for the patch
Keywords: checkin-needed
Comment on attachment 353980 [details] [diff] [review]
Fix for trunk v.4
[Checkin: Comment 12]

http://hg.mozilla.org/comm-central/rev/845332763980
Attachment #353980 - Attachment description: Fix for trunk v.4 → Fix for trunk v.4 [Checkin: Comment 12]
Status: NEW → RESOLVED
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.0b2
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.