Closed
Bug 485701
Opened 16 years ago
Closed 15 years ago
Add sort markers/arrows to Password Manager and keep sort direction when clearing filter
Categories
(Toolkit :: Password Manager, defect)
Toolkit
Password Manager
Tracking
()
RESOLVED
FIXED
mozilla1.9.3a1
Tracking | Status | |
---|---|---|
status1.9.2 | --- | beta1-fixed |
status1.9.1 | --- | wontfix |
People
(Reporter: InvisibleSmiley, Assigned: InvisibleSmiley)
References
Details
Attachments
(3 files, 2 obsolete files)
7.33 KB,
patch
|
dveditz
:
approval1.9.1.3-
|
Details | Diff | Splinter Review |
11.40 KB,
patch
|
Dolske
:
review+
|
Details | Diff | Splinter Review |
7.33 KB,
patch
|
Dolske
:
review+
dietrich
:
approval1.9.2+
|
Details | Diff | Splinter Review |
The Toolkit Password Manager (which is also used by SeaMonkey) lacks sort markers/arrows and the sorting is reset when clearing the filter (for no apparent reason). I'm attaching a patch for FF/Toolkit which will be a prerequisite for the SeaMonkey fix (which will be as simple as porting the sortDirection attribute changes). I'll file a new bug for the latter to allow this one to be closed independently from the other. Fixing SeaMonkey 2 would require this bug to be fixed on the 1.9.1 branch, that's why I'm requesting 1.9.1 approval at the same time.
I aligned the implementation with the changes I made to SeaMonkey's Cookie Manager; see http://mxr.mozilla.org/comm-central/source/suite/common/permissions/cookieViewer.* for reference. If you look closely you'll see that the Password Manager, unlike the Cookie Manager, doesn't get notified of changes made in the background (saving passwords or adding sites to the reject list). Implementing those observers might have an impact on sorting but I think this is out of scope here.
Attachment #369810 -
Flags: review?(mconnor)
Flags: wanted1.9.1?
Updated•16 years ago
|
Attachment #369810 -
Flags: review?(mconnor) → review?(dolske)
Comment 1•16 years ago
|
||
Comment on attachment 369810 [details] [diff] [review]
proposed patch
Punting to dolske, he's a better reviewer and he should have cycles.
Comment 2•16 years ago
|
||
Comment on attachment 369810 [details] [diff] [review]
proposed patch
>+ if (lastSignonSortAscending)
>+ sortedCol.setAttribute("sortDirection", "ascending");
>+ else
>+ sortedCol.setAttribute("sortDirection", "descending");
sortedCol.setAttribute("sortDirection", lastSignonSortAscending ?
"ascending" : "descending";
>+ // clear out the sortDirection attribute on the rest of the columns
>+ var currentCol = sortedCol.parentNode.firstChild;
>+ while (currentCol) {
>+ if (currentCol != sortedCol && currentCol.localName == "treecol")
>+ currentCol.removeAttribute("sortDirection");
>+ currentCol = currentCol.nextSibling;
>+ }
This can be smarter, because earlier in the function you know exactly what the last sorted column was (lastSignonSortColumn). Spin out the switch statement to something like getColumnByName(), call it early on with lastSignonSortColumn, and removeAttribute on it.
>- lastSignonSortColumn = "";
>- lastSignonSortAscending = false;
>+ lastSignonSortAscending = !lastSignonSortAscending;
> LoadSignons();
I don't understand why you're toggling lastSignonSortAscending.
In any case, move the set of lastSignonSortAscending into LoadSignons(). It resets the sort column there, so it should reset the sort order too.
>--- a/toolkit/components/passwordmgr/content/passwordManagerExceptions.js
>+++ b/toolkit/components/passwordmgr/content/passwordManagerExceptions.js
Similar comments for the related changes in this file. :)
Finally, I'd like to see a test for this. Ehsan already wrote one for filtering (see toolkit/components/passwordmgr/test/browser), you should be able to cut'n'paste a fair amount from it. Just add a few logins, synthesize clicking on columns, and verify the order is as expected. Should be fairly straightforward...
Attachment #369810 -
Flags: review?(dolske) → review-
Assignee | ||
Comment 3•16 years ago
|
||
(In reply to comment #2)
> >- lastSignonSortColumn = "";
> >- lastSignonSortAscending = false;
> >+ lastSignonSortAscending = !lastSignonSortAscending;
> > LoadSignons();
>
> I don't understand why you're toggling lastSignonSortAscending.
I added an inline comment. You'll see that this actually fixes a bug that was just not too visible until I introduced the sort markers: The sort direction is reset upon clearing the filter without this change.
> Finally, I'd like to see a test for this. Ehsan already wrote one for filtering
> (see toolkit/components/passwordmgr/test/browser), you should be able to
> cut'n'paste a fair amount from it. Just add a few logins, synthesize clicking
> on columns, and verify the order is as expected. Should be fairly
> straightforward...
Easier said then done... I'd first have to setup a FF build environment and then figure out how to start the test - make mochitest-browser-chrome, right?
Attachment #369810 -
Attachment is obsolete: true
Attachment #387978 -
Flags: review?(dolske)
Assignee | ||
Comment 4•15 years ago
|
||
Attachment #388328 -
Flags: review?(dolske)
Comment 5•15 years ago
|
||
Comment on attachment 387978 [details] [diff] [review]
patch v2 (1.9.1 branch)
>+ // The sort column didn't change. SortTree (called by
>+ // SignonColumnSort) assumes we want to toggle the sort
>+ // direction but here we don't so we have to trick it
>+ lastSignonSortAscending = !lastSignonSortAscending;
>+ SignonColumnSort(lastSignonSortColumn);
Ah. I think you need similar code + comment for LoadRejects().
Could you also update the patch to apply cleanly? I'd like to test it out briefly before marking r+.
Attachment #387978 -
Flags: review?(dolske)
Updated•15 years ago
|
Attachment #388328 -
Flags: review?(dolske)
Comment 6•15 years ago
|
||
Comment on attachment 388328 [details] [diff] [review]
test
>+ function setFilter(string) {
>+ filter.focus();
>+ filter.value = string;
>+ EventUtils.synthesizeKey("VK_RETURN", {}, win);
>+ setTimeout(runNextTest, 0);
>+ }
You'll need to pick up the fix for bug 507046 here.
>+ case 0:
>+ checkSortDirection(siteCol, true);
>+ // Toggle sort direction on Host column
>+ clickCol(siteCol);
>+ break;
Test looks good, but can you also have it check to see if the tree contents are in the correct order? [just the username column should be sufficient]
See toolkit/components/satchel/test/test_form_autocomplete.html for an example, specifically checkMenuEntries() and getMenuEntries(). That's poking at a <tree> too, so I bet that code will work with minimal tweaks.
r+ with these two things.
Comment 7•15 years ago
|
||
Sorry for the review delay, I promise to review the updates ASAP you post them. :/
Assignee | ||
Comment 8•15 years ago
|
||
Notes:
- followed bug 507046 comment 6
- checked content order against sorted column instead of (always) first one because it's safer and more comprehensible (e.g. if checking Site column while sorting by Username, we'd have to guess whether http://mozilla.com or http://mozilla.org were listed first since both have user ehsan)
Attachment #388328 -
Attachment is obsolete: true
Attachment #394591 -
Flags: review?(dolske)
Assignee | ||
Updated•15 years ago
|
Attachment #387978 -
Attachment description: patch v2 → patch v2 (1.9.1 branch)
Assignee | ||
Comment 9•15 years ago
|
||
I hope this applies cleanly for you.
(In reply to comment #5)
> (From update of attachment 387978 [details] [diff] [review])
> >+ // The sort column didn't change. SortTree (called by
> >+ // SignonColumnSort) assumes we want to toggle the sort
> >+ // direction but here we don't so we have to trick it
> >+ lastSignonSortAscending = !lastSignonSortAscending;
> >+ SignonColumnSort(lastSignonSortColumn);
>
> Ah. I think you need similar code + comment for LoadRejects().
I don't think so. The above is only needed because that code is triggered when you clear the search. For Exceptions, there is no search.
Attachment #394592 -
Flags: review?(dolske)
Updated•15 years ago
|
Attachment #394591 -
Flags: review?(dolske) → review+
Comment 10•15 years ago
|
||
Comment on attachment 394592 [details] [diff] [review]
patch v2 (trunk)
Looks and works fine! Thanks!
Attachment #394592 -
Flags: review?(dolske) → review+
Comment 11•15 years ago
|
||
(Please checkin the code + test as a single changeset.)
Assignee | ||
Updated•15 years ago
|
Comment 12•15 years ago
|
||
(I have this in my checkin queue, just waiting for a green tree)
Keywords: checkin-needed
Whiteboard: c-n: comment 11
Comment 13•15 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Assignee | ||
Updated•15 years ago
|
Attachment #394592 -
Flags: approval1.9.2?
Assignee | ||
Updated•15 years ago
|
Attachment #387978 -
Flags: approval1.9.1.4?
Attachment #387978 -
Flags: approval1.9.1.3?
Assignee | ||
Comment 14•15 years ago
|
||
This has been on trunk for a few days, asking for branch approvals (implicitly requesting the same for the test). Reasoning:
* relatively simple patch with test so low chance of regressing things
* not just visual polish but actually fixing a bug (sort order toggled upon clearing the filter)
* SM 2.0 can only pick this up (bug 485702) if it lands on 1.9.1
Comment 15•15 years ago
|
||
Comment on attachment 387978 [details] [diff] [review]
patch v2 (1.9.1 branch)
Not the kind of fix we have to take on the stable branch. 1.9.1 approval denied.
Attachment #387978 -
Flags: approval1.9.1.4?
Attachment #387978 -
Flags: approval1.9.1.3?
Attachment #387978 -
Flags: approval1.9.1.3-
Updated•15 years ago
|
status1.9.1:
--- → wontfix
Assignee | ||
Comment 16•15 years ago
|
||
(In reply to comment #15)
> (From update of attachment 387978 [details] [diff] [review])
> Not the kind of fix we have to take on the stable branch. 1.9.1 approval
> denied.
OK, can we talk about a timely 1.9.2 decision then? Once that branch becomes stable the chances for approval there become equally low.
Flags: wanted1.9.1?
Comment 17•15 years ago
|
||
There is already an approval1.9.2 flag on the patch.
Dietrich: Can you approve it?
Updated•15 years ago
|
Attachment #394592 -
Flags: approval1.9.2? → approval1.9.2+
Comment 18•15 years ago
|
||
Comment on attachment 394592 [details] [diff] [review]
patch v2 (trunk)
polish + bugfix + low footprint + tests = approval granted for 1.9.2.
Assignee | ||
Updated•15 years ago
|
Keywords: checkin-needed
Whiteboard: c-n: m-192
Comment 19•15 years ago
|
||
Assignee | ||
Updated•15 years ago
|
Flags: wanted1.9.2?
You need to log in
before you can comment on or make changes to this bug.
Description
•