when viewing settings for an account, scroll to that account in the account manager tree.

RESOLVED FIXED

Status

RESOLVED FIXED
17 years ago
14 years ago

People

(Reporter: sspitzer, Assigned: neil)

Tracking

({polish})

Trunk
x86
Windows 2000
polish

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

when viewing settings for an account, scroll to that account in the account 
manager tree.

launch mozilla to a profile with several (10) mailnews accounts.
click on the last one, and view account settings.

when the account manager comes up, we should be scrolling down in the account 
tree so that the selected account is visible.

Comment 1

17 years ago
*** Bug 136984 has been marked as a duplicate of this bug. ***

Comment 2

17 years ago
*** Bug 137209 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 3

17 years ago
Created attachment 84625 [details] [diff] [review]
Proposed patch
(Assignee)

Updated

17 years ago
Keywords: patch, polish, review, ui

Comment 4

16 years ago
Created attachment 91874 [details] [diff] [review]
patch, v1.1 - scrolls to the account selected in the process of making it visible
Attachment #84625 - Attachment is obsolete: true
(Assignee)

Comment 5

16 years ago
racham, do you really want to scroll the selected account to the top?

Comment 6

16 years ago
I prefer ensureRowIsVisible()

Comment 7

16 years ago
Comment on attachment 84625 [details] [diff] [review]
Proposed patch

r=varga
Attachment #84625 - Attachment is obsolete: false
Attachment #84625 - Flags: review+

Comment 8

16 years ago
I have noticed that scrolling process has a issue where the visual indication is
completely missing if the number of the items are less than the view can hold
and if we select the item at the bottom (eg. Local Folders) and use scrollTo(),
it does scroll to the item of our choice, but from that UI it is hard to figure
out that there exists few more accounts on the top. In this light, I agree with
you that ensureRowVisible() is a better choice.

I thought it will be useful to have all the sub items/categories of the accounts
displayed (which scrollTo does but for the above exception and
ensureRowIsVisible does not) as the intention of the user (most likely) would be
to access one of the sub items. So, I still think the right solution will be to
find out the number of children the selected item has and add that to the index
used in ensureRowIsVisible(). I have noticed that there is an API to tell us the
row count, but that is for the whole tree and our interest is the number of
children (children rows or the equivalent) of the selected cell/tree item.

Bhuvan

Updated

16 years ago
Attachment #91874 - Attachment is obsolete: true

Comment 9

16 years ago
Adding Jennifer to the cc list.
(Assignee)

Comment 10

16 years ago
Created attachment 92295 [details] [diff] [review]
Patch to ensure account is visible

This patch obtains the last child of the server (if it has one) and makes that
visible, rather than just the selected child.
Attachment #84625 - Attachment is obsolete: true

Comment 11

16 years ago
>launch mozilla to a profile with several (10) mailnews accounts.
>click on the last one, and view account settings.
>when the account manager comes up, we should be scrolling down in the account 
>tree so that the selected account is visible.

Sounds good. 

Comment 12

16 years ago
Patch looks good. However, it still fails to scroll to Local Folders item
properly when Local Folders is not in the view. You may have to replace the
following 

+  var lastItem = selectedServer.lastChild;
+  if (lastItem)
+    lastItem = lastItem.lastChild;
+  if (lastItem)
+    index = accounttree.contentView.getIndexOfItem(lastItem);

with 

+  var lastItem = selectedServer.lastChild;
+  if (lastItem) {
+    lastItem = lastItem.lastChild;
+  }
+  if (lastItem) {
+    var lastItemIndex = accounttree.contentView.getIndexOfItem(lastItem);
+    if (lastItemIndex != -1)
+      index = lastItemIndex;
+  }

to fix that problem.

Also, I think it will be better if you can move all

if (foo)
  bar;

to 

if (foo) {
  bar;
}

notation in order to make the code less error prone going forward.

Thanks for your contribution.
(Assignee)

Comment 13

16 years ago
Created attachment 94328 [details] [diff] [review]
Fix local folders

Thanks for pointing out the hole in my logic, but this is the correct fix for
it.
Attachment #92295 - Attachment is obsolete: true
reviewing and testing neil's patch now...
Assignee: racham → neil
Comment on attachment 94328 [details] [diff] [review]
Fix local folders

tested it, works fine (for local folders and non-local folders.)

sr=sspitzer
Attachment #94328 - Flags: superreview+
landed.  thanks for the fix, neil.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.