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

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Account Configuration, defect)

Description

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

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

[R.K.Aa.]

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

Comment 2

[R.K.Aa.]

*** Bug 137209 has been marked as a duplicate of this bug. ***

Comment 3

[[e-mail]]

Attachment: Proposed patch

Comment 4

[racham]

Attachment: patch, v1.1 - scrolls to the account selected in the process of making it visible

Comment 5

[[e-mail]]

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

Comment 6

[Jan Varga [:janv]]

I prefer ensureRowIsVisible()

Comment 7

[Jan Varga [:janv]]

Comment on attachment 84625 [details] [diff] [review]
Proposed patch

r=varga

Comment 8

[racham]

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

Comment 9

[racham]

Adding Jennifer to the cc list.

Comment 10

[[e-mail]]

Attachment: 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.

Comment 11

[jglick]

>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

[racham]

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.

Comment 13

[[e-mail]]

Attachment: Fix local folders

Thanks for pointing out the hole in my logic, but this is the correct fix for
it.

Comment 14

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

reviewing and testing neil's patch now...

Comment 15

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

Comment on attachment 94328 [details] [diff] [review]
Fix local folders

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

sr=sspitzer

Comment 16

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

landed.  thanks for the fix, neil.