73865 - mail/news folder pane should use the new outliner

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, defect, P1)

Description

[Cathleen]

need to convert mail/news folder panes to use new outliner

Comment 1

[Jan Varga [:janv]]

Attachment: initial work

Comment 2

[Jan Varga [:janv]]

Attachment: add missing addref in nsXULOutlinerBuilder.cpp

Comment 3

[Jan Varga [:janv]]

Attachment: folder context menu, eliminate using of GetFolderOutlinerBody() and more

Comment 4

[Chris Waterson]

r=waterson on ``04/07/01 10:57 add missing addref in nsXULOutlinerBuilder.cpp''

Comment 5

[Jan Varga [:janv]]

Attachment: added method getIndexOfResource into nsIXULTemplateBuilder

Comment 6

[Jan Varga [:janv]]

Attachment: using getIndexOfResource in messenger

Comment 7

[Jan Varga [:janv]]

Still to do:

SortFolderPane()
FillInFolderTooltip()
messengerdnd.js rewrite
PerformExpandForAllOpenServers()
GetSelectedFolder()
FolderPaneOnClick()
FolderPaneDoubleClick()
MsgEmptyTrash()
MsgCompactFolder()

and maybe more

Comment 8

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

thanks for working on this, jan.

A road block might be dnd. 

can you drop on a outliner yet?

Comment 9

[Chris Waterson]

jan: in your 04/08/01 00:15 patch, why does getIndexOfResource() return
|iter.GetRowIndex() + 1|? Shouldn't it return |iter.GetRowIndex()|?

Comment 10

[Jan Varga [:janv]]

Yes, I already changed that in my tree.

But there is another problem.
This text is from mail I sent you:

I added GetIndexOfResource method to nsXULOutlinerBuilder.cpp

NS_IMETHODIMP
nsXULOutlinerBuilder::GetIndexOfResource(nsIRDFResource* aResource,
PRInt32* aResult)
{
    nsOutlinerRows::iterator iter = mRows.Find(mConflictSet, aResource);
    if (iter == mRows.Last())
        *aResult = -1;
    else
        *aResult = iter.GetRowIndex();
    return NS_OK;
}
 
I need this method for converting folder pane to use RDF outliner.
It works well until some container is opened.
When this happens, nsOutlinerRows::First() returns second row not first.
nsOutlinerRows::First() is used in Find method as start point.

But with this hack it works ok:

nsOutlinerRows::iterator
nsOutlinerRows::First()
{
    iterator result;
    Subtree* current = &mRoot;
//    while (current && current->Count()) {
    if (current) {
        result.Push(current, 0);
        current = GetSubtreeFor(current, 0);
    }
    result.SetRowIndex(0);
    return result;
}

I don't know this code too much and I think you could help me.
Thanks.

Comment 11

[Jan Varga [:janv]]

Attachment: newest patch for RDF outliner

Comment 12

[Blake Ross]

Seth, Ben's been talking about drop feedback...

Comment 13

[Chris Waterson]

r=waterson on the last patch. Nice work! (And thanks for wiping my chin on 
nsOutlinerRows::First().)

Comment 14

[Håkan Waara]

Just to let you all know: Seth promised to make a branch where this can be
worked on.  I've talked to Jan and I will also help out with this once we branch.

Comment 15

[Ben Goodger (use ben at mozilla dot org for email)]

sr=[e-mail] for the outliner builder patch. 

Comment 16

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

I didn't get a chance to start a branch.

I'll start one tomorrow, when I get to my linux box.

thanks for being patient.

Comment 17

[Chris Waterson]

Feel free to land the changes in content/xul/templates/src on the trunk.

Comment 18

[Jan Varga [:janv]]

I already did that, after ben's super review.

Comment 19

[Jan Varga [:janv]]

I just filed new bug 75391 (TODO list for folder pane outliner).

Comment 20

[Jan Varga [:janv]]

Branch tag is FOLDER_OUTLINER_20010410_BRANCH.

Comment 21

[Jan Varga [:janv]]

Branch build crashes on windows.
We should create new one.

Comment 22

[Jan Varga [:janv]]

Attachment: cvs rdiff -u -r FOLDER_OUTLINER_20010410_BASE -r FOLDER_OUTLINER_20010410_BRANCH SeaMonkeyAll

Comment 23

[Håkan Waara]

I sugges that you re-branch from the _trunk_ (not from your tree, as before) and
then re-checkin the stuff that needs to be checked in.

It's not _that_ much, anyway.  Let me know if I can help you...

Comment 24

[Jan Varga [:janv]]

New branch name is FOLDER_OUTLINER_20010417_BRANCH

Comment 25

[Håkan Waara]

Taking.

Comment 26

[Jan Varga [:janv]]

New branch name is FOLDER_OUTLINER_20010531_BRANCH.
It's ready for testing. Everything should be converted.

Comment 27

[Håkan Waara]

Yes, this is great news! Jan merged with the 05-31 tip and converted a lot! But
we still have some polishing to do.

Would be nice if anyone on the Netscape side would be willing to test (and maybe
do some performance measurements on) our branch once it's even more ready for
prime-time. Any takers?

Comment 28

[Jan Varga [:janv]]

Attachment: diff in content

Comment 29

[Jan Varga [:janv]]

Attachment: diff in layout

Comment 30

[Jan Varga [:janv]]

Attachment: diff in mailnews

Comment 31

[Jan Varga [:janv]]

Attachment: diff in themes

Comment 32

[Håkan Waara]

Note that the patches are not yet ready for review. This is for testing purposes.

Comment 33

[Håkan Waara]

see bug 85088 for a regression our branch will cause.

Comment 34

[Håkan Waara]

Leaf: The branch tag is FOLDER_OUTLINER_20010531, and we changed files in
themes/, mailnews/ and layout/.

Comment 35

[Daniel (Leaf) Nunes]

cc'ing lpham, as she'll be doing test builds this week.

Comment 36

[Jan Varga [:janv]]

don't forget content

Comment 37

[lpham]

Did you tag the whole mozilla tree?   Another word, if I pull from 
FOLDER_OUTLINER_20010531, then it should include all the fixes.  Please confirm 
before I start the test build.

Loan

Comment 38

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

the next two steps are code review and thorough testing.

for testing, we need to make sure to run through all the appropriate functional 
tests.  see http://www.mozilla.org/quality/mailnews/tests/index.html

running through all those tests will help to lower the risks.

for review, we'll need some cycles from a whole bunch of people (since the 
changes cover content, layout, themes and mailnews.)

as for estimating when reviews can be completed, I don't know.
personally, this fix isn't top on my list of priorities, since the folder pane 
currently works and I've got a lot on my plate for 0.9.2

but, we do want this fixed (as all multi-columned trees will eventually be 
switching over to outliner).

following blizzard's guidelines (see 
http://www.mozillazine.org/articles/article1938.html), this is too risky with 
not enough reward for 0.9.2

a good next step is to get some branch builds so those that have time, can do 
some testing.

Comment 39

[Håkan Waara]

(This is the second mid-air I got!)

The code we have now is very stable, not one known regression except for bug
85088. It works nicely and solid.

Would be nice if some NS people could help out with testing the experimental
builds once loan has made them?

loan: I'm pretty certain it contains all fixes, and yes, I think Jan branched
the whole tree (even though we didn't change anything else than the folders I
mentioned in my last comment).

Comment 40

[Jan Varga [:janv]]

it is FOLDER_OUTLINER_20010531_BRANCH

Comment 41

[Jan Varga [:janv]]

err, sorry for spam.
I tagged all tree and it includes all fixes.

Comment 42

[Joe Hewitt (gone)]

sr=hewitt for the CSS changes

Comment 43

[scottputterman]

Håkan what Seth said is correct, in my opinion.  We want this fix, but I think
this is too late in 0.9.2 to consider landing this. As for help testing it, I
think there will be individuals who will be interested in looking at builds (I'm
interested in trying it out), but in the near future Netscape mail QA isn't
going to be able to help out with this which was why Seth was suggesting looking
at the functional tests on mozilla.

Comment 44

[Håkan Waara]

All the stuff will bitrot till 0.9.3! :(

Comment 45

[lpham]

The build failed because mozilla/security/makefile.win didn't have the branch 
name; therefore makefile.win didn't get pull.  I created 
FOLDER_OUTLINER_20010531_BRANCH for mozilla/security/makefile.win and continue 
the build.

Loan

Comment 46

[lpham]

The build completed and it is on
ftp://ftp.mozilla.org/pub/mozilla/nightly/2001-06-11-06-bug_73865/

Loan

Comment 47

[andreww]

r = andreww for the folderpane.css changes (which constitutes all the 
themes changes needed, I'm told)

Comment 48

[Jan Varga [:janv]]

Attachment: complete patch for tip

Comment 49

[Jan Varga [:janv]]

Changes to popups are already r=pink (bug 82597).

Comment 50

[Håkan Waara]

We hope we can get the necessary reviews for 0.9.3 since it won't make it till
0.9.2.

Comment 51

[[e-mail] (gone - use [e-mail] instead)]

My apologies for running this build for performance so late in the game, here's 
what I found:

Methodology here: 
http://www.mozilla.org/mailnews/win_performance_results.html#method

2001-06-08 Win32 build:

Folder Loading Speed 2.78 1.97 2.53

2001-06-11-12 Win32 Outliner build:

Folder Loading Speed 2.46 2.06 2.23 2.19

This is on a MS-Windows: 98 Second Edition, 266 MHz Pentium II, 128M RAM 
machine. 

Comment 52

[Håkan Waara]

We cut the branch on 05-31, so stephend is so kind that he'll re-run the tests
on a 05-31 build. Thanks stephend!

Comment 53

[[e-mail] (gone - use [e-mail] instead)]

Those numbers will have to do, I can't get a 5-31 build, the earliest I can get 
is a 6-10 build.

ftp://ftp.mozilla.org/pub/mozilla/nightly/

Comment 54

[Håkan Waara]

Bienvenu told me to remind him after the NS-branch is released. We're aiming to
get this into 0.9.3

Comment 55

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

sorry for the delay, and thanks for being patient.

on monday (7-16), reviewing / testing and landing this work onto the trunk will
be my top priority (unless there is a smoketest blocker.)
I'd like to take this bug so that it's officially on my radar.

any objections?

Comment 56

[Håkan Waara]

I'm cool with that.

I'll be around on AIM for questions and general assistance.

Comment 57

[Jan Varga [:janv]]

Attachment: latest patch for the tip

Comment 58

[Suresh]

One thing I've noticed in the new folder pane is that the gray dotted lines are 
displayed from the sever icon to all subfolder icons. Is this the desired 
behaviour?  (fyi: This is not the case in the current one)

Comment 59

[timeless]

it's an improvement that's 4xp-win32. hewitt already sr='d for themes so i'd 
assume he's happy w/ it.

Comment 60

[Håkan Waara]

Suresh: Depends on who you ask. If you ask me, one of the authors -
sure it's the desired behaviour. I think it looks very cool and it's 4xp.

Comment 61

[Mark Anderson]

There are two changes in the latest diff that are not going to work.  See bug 
88332.  messenger.xul and mail3PaneWindowVertLayout.xul both contain an 
<outliner> tag inside of a <vbox> in the patch.  Unless someone can fix 88332 by 
Monday, you'll have to change those <vbox> tags to <box orient="vertical"> to 
get correct alignment for now.

Comment 62

[Jan Varga [:janv]]

I've been using mozilla with this patch for a long time and I don't see any
problems with <outliner> inside <vbox>

Comment 63

[Mark Anderson]

The orientation of whatever is inside the <vbox> is correct, then?  The problem 
I found was fairly consistent; any <vbox> that contained an <outliner> somehow 
ended up with horizontal orientation rather than vertical (that would be implied 
by the <vbox>).

Comment 64

[Mark Anderson]

Ok.  Disregard my comments.  88332 is invalid.  I thought it existed because of 
a weird overlay in security that expressed orientation of boxes. :)

Comment 65

[Jan Varga [:janv]]

Attachment: the most recent patch (Mon Jul 16 11:36:18 UTC 2001)

Comment 66

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

I'm starting with the last patch attached by Jan.

jan / hwaara, 

while I apply / review / test the mailnews changes, can you find the 
appropriate people to review the content, layout and xpfe changes?

Index: content/xul/content/src/nsXULPopupListener.cpp
Index: content/xul/templates/public/nsIXULTemplateBuilder.idl
Index: content/xul/templates/src/nsXULOutlinerBuilder.cpp
Index: layout/xul/base/public/nsIPopupSetBoxObject.idl
Index: layout/xul/base/public/nsIPopupSetFrame.h
Index: layout/xul/base/src/nsPopupSetBoxObject.cpp
Index: layout/xul/base/src/nsPopupSetFrame.cpp
Index: layout/xul/base/src/nsPopupSetFrame.h
Index: layout/xul/base/src/outliner/src/nsOutlinerBodyFrame.cpp
Index: xpfe/global/resources/content/bindings/popup.xml

Comment 67

[Marc Attinasi]

I'll review the content/XUL and layout/XUL files, but you should try to get
Hyatt to sr the changes there, he's your XUL-dude.

Comment 68

[Marc Attinasi]

The layout and content changes look fine to me. But please, what is the constant
'21' in the xulPopupListener?

+    yAdjust += 21;

it seems arbitrary, and possibly dependent on something that can change, like
resolution, font size, etc.

Also, the tabbing seems off (4 spaces instead of 2?) in nsXULOutlinerBuilder.cpp
 - is that just local convention?

r=attinasi

Comment 69

[Håkan Waara]

I think pinkerton reviewed that code earlier, and also noted the weird constant
but when he got an explanation, it was rational.  Jan?

Comment 70

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

pinkerton has already reviewed the popup stuff.

http://bugzilla.mozilla.org/show_bug.cgi?id=82597

the constant (21) is mentioned there.

pink wrote:

"we really need to explain the origin of that constant, but that can be left 
for me to do."

Comment 71

[Jan Varga [:janv]]

The constant 21 is only moved:

-        self->LaunchPopup ( self->mMouseClientX, self->mMouseClientY+21);
+        self->LaunchPopup ( self->mMouseClientX, self->mMouseClientY )


+  else if ( popupType == eXULPopupType_tooltip ) {
     type.AssignWithConversion("tooltip");
+    yDelta += 21;
+  }

functionality is the same

Comment 72

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

jan,  I'm getting the following assertion when I try to rename the last folder 
on my pop account:

###!!! ASSERTION: couldn't find row: 'iter != mRows.Last()', file C:\builds\mozi
lla\content\xul\templates\src\nsXULOutlinerBuilder.cpp, line 913

Comment 73

[Mike Pinkerton (not reading bugmail)]

that is apparantly an offset that I added long ago to make tooltips show up below 
the mouse cursor instead of on top of it. 

Comment 74

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

jan / hwaara:  do you see this problem:

start up messenger with a lot of accounts (so the folderpane has plenty of 
folders / newsgroups.) 

I've got a pop account, imap, local folders, and some news accounts.

all the accounts are expanded on startup.

after start up, scroll down to the bottom of the folder pane and select 
something near the bottom.

it ends up loading / selecting a folder near the top.

Comment 75

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

"after start up, scroll down to the bottom of the folder pane and select 
something near the bottom."

select a news server near the bottom.  selecting a newsgroup seems to work fine.

Comment 76

[Håkan Waara]

Seth, I cannot reproduce your problem. But I found another:

sometimes when I click GetMsg, and it receives the messages, the folder that
receives the messages doesn't update it's look to be bold and contain the # of
new messages in its name.

Instead the folder is just plain blank, with no info that there are new messages
in it.

Comment 77

[Jan Varga [:janv]]

Sorry guys, but I can't reproduce described problems with my opt build on linux.
Now building debug and will look again.

Comment 78

[Håkan Waara]

Hah, a simple restart of the debug build made it; everything works fine now, and
I  still can't reproduce Seth's problem. So no problem here.

Comment 79

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

jan / hwaara, try the alternate 3 pane to reproduce the problem I reported 
on "2001-07-16"

with the standard 3 pane, I can't reproduce the problem.

Comment 80

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

also, load a folder first and then scroll down and click on a server.

I'm debugging this problem now.

Comment 81

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

something is calling ChangeFolderByURI() twice.

In ChangeFolderByURI uri = news://[e-mail] sortType = 22
In ChangeFolderByURI uri = mailbox://nobody@Local%20Folders/Trash sortType = 18

still debugging...

Comment 82

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

FolderPaneSelectionChange()is being called twice.

I think the second one is happening when the outliner is resized.

the outliner gets resized when you click on a server because selecting a server 
loads account central, and loading account central hides the message pane, and 
hiding the message pane in the vert. 3 pane (unlike messenger.xul) changes the 
size of the folder pane.

Comment 83

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

nsOutlinerSelect::Select() is getting called twice when I click on a folder, 
both from outliner.xuml

the first time is on the mousedown handler, the second time on click handler,

nsOutlinerSelection::FireOnSelectHandler() line 686
nsOutlinerSelection::Select(nsOutlinerSelection * const 0x0598f670, int 26) 
line 356
XPTC_InvokeByIndex(nsISupports * 0x0598f670, unsigned int 7, unsigned int 1, 
nsXPTCVariant * 0x0012d158) line 139
XPCWrappedNative::CallMethod(XPCCallContext & {...}, XPCWrappedNative::CallMode 
CALL_METHOD) line 1881 + 42 bytes
XPC_WN_CallMethod(JSContext * 0x0511ed30, JSObject * 0x045df878, unsigned int 
1, long * 0x04611e14, long * 0x0012d38c) line 1252 + 11 bytes
js_Invoke(JSContext * 0x0511ed30, unsigned int 1, unsigned int 0) line 807 + 23 
bytes
js_Interpret(JSContext * 0x0511ed30, long * 0x0012e12c) line 2701 + 15 bytes
js_Invoke(JSContext * 0x0511ed30, unsigned int 1, unsigned int 2) line 824 + 13 
bytes
js_InternalInvoke(JSContext * 0x0511ed30, JSObject * 0x04401660, long 73587768, 
unsigned int 0, unsigned int 1, long * 0x0012e304, long * 0x0012e254) line 896 
+ 20 bytes
JS_CallFunctionValue(JSContext * 0x0511ed30, JSObject * 0x04401660, long 
73587768, unsigned int 1, long * 0x0012e304, long * 0x0012e254) line 3320 + 31 
bytes
nsJSContext::CallEventHandler(nsJSContext * const 0x0511ec00, void * 
0x04401660, void * 0x0462dc38, unsigned int 1, void * 0x0012e304, int * 
0x0012e300, int 0) line 942 + 33 bytes
nsJSEventListener::HandleEvent(nsJSEventListener * const 0x0630edb0, 
nsIDOMEvent * 0x0630eb54) line 139 + 57 bytes
nsXBLPrototypeHandler::ExecuteHandler(nsXBLPrototypeHandler * const 0x05948030, 
nsIDOMEventReceiver * 0x05684178, nsIDOMEvent * 0x0630eb54) line 432
DoMouse(nsIAtom * 0x03230e30, nsIXBLPrototypeHandler * 0x05948030, nsIDOMEvent 
* 0x0630eb54, nsIDOMEventReceiver * 0x05684178) line 102
nsXBLMouseHandler::MouseClick(nsXBLMouseHandler * const 0x05949ca0, nsIDOMEvent 
* 0x0630eb54) line 117 + 40 bytes
nsEventListenerManager::HandleEvent(nsEventListenerManager * const 0x042bbc80, 
nsIPresContext * 0x0512a160, nsEvent * 0x0012f2dc, nsIDOMEvent * * 0x0012f1e8, 
nsIDOMEventTarget * 0x05684178, unsigned int 7, nsEventStatus * 0x0012f648) 
line 1260 + 41 bytes
nsXULElement::HandleDOMEvent(nsXULElement * const 0x05684170, nsIPresContext * 
0x0512a160, nsEvent * 0x0012f2dc, nsIDOMEvent * * 0x0012f1e8, unsigned int 1, 
nsEventStatus * 0x0012f648) line 3633
PresShell::HandleEventInternal(nsEvent * 0x0012f2dc, nsIView * 0x00000000, 
unsigned int 1, nsEventStatus * 0x0012f648) line 5631 + 47 bytes
PresShell::HandleEventWithTarget(PresShell * const 0x05136120, nsEvent * 
0x0012f2dc, nsIFrame * 0x0446fd8c, nsIContent * 0x05684170, unsigned int 1, 
nsEventStatus * 0x0012f648) line 5604 + 22 bytes
nsEventStateManager::CheckForAndDispatchClick(nsEventStateManager * const 
0x051ea800, nsIPresContext * 0x0512a160, nsMouseEvent * 0x0012f754, 
nsEventStatus * 0x0012f648) line 2456 + 61 bytes
nsEventStateManager::PostHandleEvent(nsEventStateManager * const 0x051ea808, 
nsIPresContext * 0x0512a160, nsEvent * 0x0012f754, nsIFrame * 0x0446fd8c, 
nsEventStatus * 0x0012f648, nsIView * 0x05949970) line 1541 + 28 bytes
PresShell::HandleEventInternal(nsEvent * 0x0012f754, nsIView * 0x05949970, 
unsigned int 1, nsEventStatus * 0x0012f648) line 5651 + 43 bytes
PresShell::HandleEvent(PresShell * const 0x05136124, nsIView * 0x05949970, 
nsGUIEvent * 0x0012f754, nsEventStatus * 0x0012f648, int 0, int & 1) line 5558 
+ 25 bytes
nsView::HandleEvent(nsView * const 0x05949970, nsGUIEvent * 0x0012f754, 
unsigned int 8, nsEventStatus * 0x0012f648, int 0, int & 1) line 377
nsView::HandleEvent(nsView * const 0x05131f90, nsGUIEvent * 0x0012f754, 
unsigned int 28, nsEventStatus * 0x0012f648, int 1, int & 1) line 350
nsViewManager::DispatchEvent(nsViewManager * const 0x05132030, nsGUIEvent * 
0x0012f754, nsEventStatus * 0x0012f648) line 2057
HandleEvent(nsGUIEvent * 0x0012f754) line 68
nsWindow::DispatchEvent(nsWindow * const 0x05949834, nsGUIEvent * 0x0012f754, 
nsEventStatus & nsEventStatus_eIgnore) line 721 + 10 bytes
nsWindow::DispatchWindowEvent(nsGUIEvent * 0x0012f754) line 742
nsWindow::DispatchMouseEvent(unsigned int 301, nsPoint * 0x00000000) line 4241 
+ 21 bytes
ChildWindow::DispatchMouseEvent(unsigned int 301, nsPoint * 0x00000000) line 
4490
nsWindow::ProcessMessage(unsigned int 514, unsigned int 0, long 23068748, long 
* 0x0012fb94) line 3204 + 24 bytes
nsWindow::WindowProc(HWND__ * 0x00120350, unsigned int 514, unsigned int 0, 
long 23068748) line 989 + 27 bytes
USER32! 77e13eb0()
USER32! 77e1401a()
USER32! 77e192da()
nsAppShellService::Run(nsAppShellService * const 0x01120720) line 424
main1(int 3, char * * 0x00485f20, nsISupports * 0x00000000) line 1174 + 32 bytes
main(int 3, char * * 0x00485f20) line 1478 + 37 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 77e87903()

Comment 84

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

my instinct is that it is a bug in outliner.xml

we are firing select() twice, and that is not good.

even in the normal case, we are firing it twice.

the only reason we aren't loading the folder twice (in normal cases) is because 
we've got mailnews js to prevent that.

Comment 85

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

well, I've got a patch to outliner.xml but it doesn't fix the problem I found.

it does prevent us from calling select() twice in the normal case.

perhaps we'll also need to suppress select() when it happens during a resize / 
reflow?

Comment 86

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

Attachment: supplimental patch for outliner.xml

Comment 87

[Håkan Waara]

r=hwaara on the supplimental patch. Thanks for finding and fixing this Seth

Comment 88

[Jan Varga [:janv]]

Attachment: patch for outliner (should fix problem described by seth)

Comment 89

[Jan Varga [:janv]]

Attachment: next try

Comment 90

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

the last patch fixes the problem I was seeing, but I'm nervous about side 
effects.

coupled with my fix for outliner.xml, we're only firing the onselect handler 
once.

jan, can you run your idea by hyatt?

I'll attach an updated fix, the complete fix that I'm running in my local tree, 
and then I'll continue on with my testing and reviewing.

Comment 91

[Jan Varga [:janv]]

Explanation of the problem:
select() method, which fires SelectionChanged() is called from "mousedown" and
"click" event handler. Both event handlers call getCellAt() to get index of
corresponding row. We're recomputing mInnerBox (used by getCellAt) and
mPageCount in Reflow() method. But mousedown is fired before reflow and click
after reflow. This way we get two diffrent indexes. The second one is incorrect

Solution:
Don't change mInnerBox and mPageCount in Reflow() method. Just get it locally

hyatt, can you comment what you think ?

Comment 92

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

I think there is a problem with jan's last patch.

It think it is having side effects on the thread pane.

see next screen shot.

Comment 93

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

Attachment: screen shot

Comment 94

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

another problem that I've been having is every so often when I start up, 
account central doesn't load.

it doesn't happen all the time.

###!!! ASSERTION: morkBool_kFalse: '0', file c:\builds\mozilla\db\mork\src\morkC
onfig.cpp, line 78
###!!! ASSERTION: morkBool_kFalse: '0', file c:\builds\mozilla\db\mork\src\morkC
onfig.cpp, line 78
###!!! ASSERTION: morkBool_kFalse: '0', file c:\builds\mozilla\db\mork\src\morkC
onfig.cpp, line 78
JavaScript error:
 line 20: uncaught exception: [Exception... "Component returned failure code: 0x
80004003 (NS_ERROR_INVALID_POINTER) [nsIMsgFolder.getMsgDatabase]"  nsresult: "0
x80004003 (NS_ERROR_INVALID_POINTER)"  location: "JS frame :: chrome://messenger
/content/commandglue.js :: FolderPaneSelectionChange :: line 661"  data: no]

this might be win32 only.  

see related bug
http://bugzilla.mozilla.org/show_bug.cgi?id=88949

(that's how to reproduce the problem consistently on the trunk)

Comment 95

[Jan Varga [:janv]]

Attachment: fixed problem pointed out on the screenshot

Comment 96

[Jan Varga [:janv]]

We were scrolling to negative index in the PositionChange() method and that
caused blank area in outliner.
This was not visible before because there was another positive ScrollToRow() in
Reflow() method.

Comment 97

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

I'll try out the latest patch from Jan tonight or tomorrow.

anyone with a mac want to try out this build:

ftp://ftp.mozilla.org/pub/mozilla/nightly/2001-07-16-17-bug_73865

it's the outliner branch build (not the trunk) on the mac.  (thanks to leaf for 
spinning it up.)

I've been working on win2k, jan's been doing linux, but we need to give it a go 
on the mac, too.

Comment 98

[[e-mail] (gone - use [e-mail] instead)]

Drag and drop on that Mac build highlights the target folder as gold, and the
name of the folder is highlighted in black.  Known issue?  This is with Modern3.

Comment 99

[Suresh]

stephend, is your problem in Mac very similar to
http://bugzilla.mozilla.org/show_bug.cgi?id=90274 ?

Comment 100

[[e-mail] (gone - use [e-mail] instead)]

No, it doesn't sound like that bug.  When I drag a message over to a folder drop
target, the folder name becomes white.  (white text on a black background.)

Comment 101

[Mike Pinkerton (not reading bugmail)]

given that nobody was using the outliner folder drop feedback when i wrote it, 
it's possible there are some coloring bugs with it. those are probably ok and we 
can clean them up later, no?

Comment 102

[[e-mail] (gone - use [e-mail] instead)]

Pink: agreed.  My comment wasn't of urgency, just an observation.  Pretty good, 
since that's all I can find wrong with the outliner code so far (other than the 
known issues).

Comment 103

[Daniel (Leaf) Nunes]

the branch doesn't build on linux for me, dies in security/nss because 
../coreconf/Makefile doesn't exist.

So, i'm giving up on the branch, and attempting to make a test build using the 
patch on linux.

Comment 104

[Jan Varga [:janv]]

leaf, can you apply "fixed problem pointed out on the screenshot" patch
after applying main patch ?

Comment 105

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

jan's last patch appears to have fixed the problem I was seeing.

I'll resume my testing and reviewing.

Comment 106

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

I got side tracked with other reviews (and some 0.9.2 branch bugs).

I'll get back to this tomorrow.

Comment 107

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

as I mentioned a while back, I frequently get the problem where account central 
doesn't load.

related to bugs #81705 and #88949, except I see it very frequently with the 
outliner patch.  I'll investigating it more now.

###!!! ASSERTION: morkBool_kFalse: '0', file c:\builds\mozilla\db\mork\src\morkC
onfig.cpp, line 78
###!!! ASSERTION: morkBool_kFalse: '0', file c:\builds\mozilla\db\mork\src\morkC
onfig.cpp, line 78
###!!! ASSERTION: morkBool_kFalse: '0', file c:\builds\mozilla\db\mork\src\morkC
onfig.cpp, line 78
###!!! ASSERTION: morkBool_kFalse: '0', file c:\builds\mozilla\db\mork\src\morkC
onfig.cpp, line 78
###!!! ASSERTION: morkBool_kFalse: '0', file c:\builds\mozilla\db\mork\src\morkC
onfig.cpp, line 78
###!!! ASSERTION: morkBool_kFalse: '0', file c:\builds\mozilla\db\mork\src\morkC
onfig.cpp, line 78
JavaScript error:
 line 20: uncaught exception: [Exception... "Component returned failure code: 0x
80004003 (NS_ERROR_INVALID_POINTER) [nsIMsgFolder.getMsgDatabase]"  nsresult: "0
x80004003 (NS_ERROR_INVALID_POINTER)"  location: "JS frame :: chrome://messenger
/content/commandglue.js :: FolderPaneSelectionChange :: line 661"  data: no]

JavaScript error:
 line 0: uncaught exception: [Exception... "Component returned failure code: 0x8
0070057 (NS_ERROR_ILLEGAL_VALUE) [nsIXULOutlinerBuilder.getResourceAtIndex]"  ns
result: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: chrome://m
essenger/content/msgMail3PaneWindow.js :: GetFolderResource :: line 1007"  data:
 no]

Comment 108

[Jan Varga [:janv]]

I can't reproduce it on linux, but I suspect that method
folderOutliner.outlinerBoxObject.selection.getRangeAt(0, startIndex, endIndex)at
649 line of commandglue.js
returns -1
it should not because
folderOutliner.outlinerBoxObject.selection.count == 1

Comment 109

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

I think the general problem is bug #88949, and I've got a fix for it.

I'll land it on the trunk and then continue on with my testing and review for 
this bug.

I'll investigate jan's theory about why it might be happening with the outliner 
in cases where it doesn't happen on the trun.

Comment 110

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

ok, even with my fix for 88949, I still get a similar problem.

I'll attach a screen shot.

I no longer get any asserts, but nothing is selected in the folder pane.  

it might be what jan was talking about.  I'll investigate.

Comment 111

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

Attachment: screen shot of the start up and nothing is selected problem I see intermittantly

Comment 112

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

When I get into that state, FolderPaneSelectionChange() isn't even being called.

still looking...

Comment 113

[Jan Varga [:janv]]

maybe problem is in loadStartFolder(initialUri)

Comment 114

[scottputterman]

moving to 0.9.4

Comment 115

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

ok, we are bailing out early here:

in msgMail3PaneWindow.js, in loadStartFolder(), we return early because

var inboxFolder = rootMsgFolder.getFoldersWithFlag(0x1000, 1, outNumFolders);
if(!inboxFolder) return;

that fails intermittantly.  I'm still investigating.

Comment 116

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

update:

1)  I think the intermittant problem with getFoldersWithFlag
 failing is because we haven't gotten the subfolders into rdf yet.
I'll test that theory out.

2)  I talked to hyatt, and we need to revisit the solution the previous problem
(with the alternate 3 pane).  Instead of our last two patches (mine to
outliner.xml and jan's to nsOutlinerBodyFrame.cpp), we have to examine
outliner.xml.  hyatt doesn't think that we should be calling select() twice.
he said he'd take (and talk to pink) about why we're doing that.

Comment 117

[Jan Varga [:janv]]

Interesting, I took look at the tree folder pane. mousedown event is fired
before vertical resizing of the folder pane and click is fired after (same
behavior as with outliner). But when I click on a server near the bottom of the
folder pane (the folder pane gets resized, top folder is moved down) and click
event is not fired. Therefore we didn't see this problem before.

Comment 118

[Jan Varga [:janv]]

I don't think we can fix it only in outliner.xml
Both handlers (mousedown and click) calls getCellAt() at the beginning.
getCellAt() returns row, column and object. But without patched
nsOutlinerBodyFrame.cpp we get two differenct row indexes.

Comment 119

[fenella]

Per Seth, this bug will fix bug 89346.

Comment 120

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

talking to hyatt, the last two fixes are probably bad.  (mine to outliner.xml
is, and possibly jan's as well.)

the real problem is covered in bug #92366

Comment 121

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

ok, I've got a fix for the problem 
(http://bugzilla.mozilla.org/showattachment.cgi?attach_id=43413)
I was seeing in my local tree.

I'll attach a patch for it.

I'll resume my reviewing and testing.

Comment 122

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

Attachment: additional patch

Comment 123

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

again, sorry for the delay.  I've been busy chasing crashers.

in my local tree I've got an additional fix:  in modern/folderPane.css, I 
needed to reorder the news rules so that news server would have the right icons.
the current patch makes it so all news servers and folders have the same icon, 
the icon for a news folder.

classic/mac/folderPane.css and classic/win/folderPane.css are ok.

I'll work on getting an update patch, one that is merged to the trunk (there 
have been changes underneath us) and one without the two bad patches that were 
intended to work around #92366 [the select() handler being called twice 
problem.]

Comment 124

[Jan Varga [:janv]]

It seems that we have saved some memory:

Trunk build after mail window appeared
VmSize:    39084 kB
VmLck:         0 kB
VmRSS:     23808 kB
VmData:    19232 kB
VmStk:        80 kB
VmExe:        56 kB
VmLib:     18112 kB

Trunk build after scrolling of folder pane
VmSize:    39192 kB
VmLck:         0 kB
VmRSS:     23880 kB
VmData:    19340 kB
VmStk:        80 kB
VmExe:        56 kB
VmLib:     18112 kB

Trunk with patch after mail window appeared
VmSize:    38012 kB
VmLck:         0 kB
VmRSS:     23624 kB
VmData:    18160 kB
VmStk:        80 kB
VmExe:        56 kB
VmLib:     18112 kB

Trunk with patch after scrolling
VmSize:    38040 kB
VmLck:         0 kB
VmRSS:     23696 kB
VmData:    18188 kB
VmStk:        80 kB
VmExe:        56 kB
VmLib:     18112 kB

Comment 125

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

ok, the mozilla/mailnews changes look good to me.

while we wait for some sr= action from hyatt (and a fix for 92336) I'll 
continue testing.

right now, I'm busy testing the following areas:

dnd
context menus
double click to open new 3 pane
check send unsent messages context crap
check download flagged / download selected
noselect folders
unsubscribe to newsgroups
empty trash while trash is selected
folder compaction

I'll attach a complete patch, updated to the tip.

Comment 126

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

also, while we wait from some hyatt sr=, I'll investigate the other places 
where folderPane.css was being used, and see if they need to be fixed.

http://lxr.mozilla.org/seamonkey/search?string=folderPane.css

one known place is the select folder for offline dialog, which I'm working on 
right now.

Comment 127

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

Attachment: update to date patch

Comment 128

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

whoops, that patch has some addition cruft that is not part of the patch.  new 
patch coming up.

Comment 129

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

openSaveAttachment.xul is being removed.  (see bug #92960)

the only real problem is with msgSelectOffline.xul, which is covered in bug 
#85088

I'm going to work on fix that in my local tree now, so it can land at the same 
time as this fix.

Comment 130

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

Attachment: up to date patch, includes fix for #92960, but not #92366

Comment 131

[Jan Varga [:janv]]

the last patch looks good

Comment 132

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

Attachment: update patch, includes start of fix for #85088

Comment 133

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

Attachment: updated diff, more work for #85088

Comment 134

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

adding #89702 because as the thread pane, folder pane and select offline dialog 
switch to the folder pane, it is really annoying to have double click expand or 
collapse the thread or folder.

Comment 135

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

Attachment: latest patch with #85088, POP over SSL and some dead code removal thrown in.

Comment 136

[David Hyatt]

Ok, here are my comments.

(1) I don't like the hidden column header stuff in the folder pane outliner. 
Seems like this should be addressed, especially if you ever have plans of
returning to 4.x's three column behavior.

(2) I have issues with all the popup code and want to understand it better
before allowing API changes to createPopup.  This means I can't approve the
changes to xpfe (popup.xml) or to any of the popup code in layout and content.

Everything else in the last sspitzer patch, though, is sr=hyatt.
 

Comment 137

[Jan Varga [:janv]]

I've just commented popup changes in bug 82597.

Comment 138

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

ok, here's what I'll go do:

> (1) I don't like the hidden column header stuff in the folder pane outliner. 
> Seems like this should be addressed, especially if you ever have plans of
> returning to 4.x's three column behavior.

agreed.  I'll go fix that and fix #54171 along the way.  (adding unread / count 
back to the folder pane.)

I got some hints from hyatt how to fix #89702, I'll go work on that too.

hyatt thanks for the reviews and comments.

Comment 139

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

talking to hyatt, we've got a new plan.

he's doesn't want to take #82597, so I'm going to take those changes out of my
tree and come up with a new patch.  this means we won't have tooltips in the
outliner, but I'm ok with that (see below).  

note: hewitt is going to work on titletips for the outliner.

I'm going to work on #54171 (adding unread and total columns back to the folder
pane), and #89702 and finish up #85088.

when that's done (assuming I don't find any more problems), we'll just need 
#92366 (from hyatt) before we can land.

I'll be working on my three bugs on monday.

once I finish my 3 bugs, I'll probably spin up a new branch for QA to test.
It's going to be a monster landing, so we'll need to be cautious.

Comment 140

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

Attachment: screen shot of folder pane with unread / total counts

Comment 141

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

Attachment: update patch, excludes fix for #82597

Comment 142

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

that new patch is the old patch, plus some more stuff, like unread and total 
columns in the folder pane.  (that patch excludes the fix for #82597, so no 
tooltips.)

here's my todo list:

1) can't hide columns in folder pane
2) unread / total in folder pane should be hidden by default
3) bug, don't show "Foldername (Unread Count)" if unread column is not hidden.
4) fix #92366 (hyatt)

Comment 143

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

> 1) can't hide columns in folder pane

fix in hand.

on to 2,3.

Comment 144

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

> 2) unread / total in folder pane should be hidden by default

fix in hand.  I needed to specify defaults in localStore.rdf.  (these defaults 
will only be for new profiles, not exisiting ones.)

new patch on the way.

Comment 145

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

Attachment: updated diff.

Comment 146

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

Attachment: updated patch.

Comment 147

[Jan Varga [:janv]]

Attachment: 3) fixed

Comment 148

[Jan Varga [:janv]]

There are some nits...

We should find nicer names for these:
http://home.netscape.com/NC-rdf#FolderTreeName
http://home.netscape.com/NC-rdf#FolderTreeSimpleName
FolderTreeSimpleName is FolderTreeName without unread count

Comment 149

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

jan's fix for #3 works like a charm.

I'll attach some screen shots of the folder pane in the four different modes.

about your nit:  after this lands, we should rename to this:

FolderTreeName -> FolderTreeNameWithUnread
FolderTreeSimpleName -> FolderTreeName

that's going to be a lot of search and replace, so I'll log a bug to do it 
later.

jan's going to create a new branch so we can land this patch and get builds to 
QA.

Comment 150

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

Attachment: 4 screen shots of folder pane

Comment 151

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

Attachment: updated patch

Comment 152

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

that patch includes two additional fixes.

1)get context menus to work in the folder pane (merge problem)
2)clicking on the name column in the folder pane should not sort the list of
folders.

Comment 153

[Jan Varga [:janv]]

new branch name: FOLDER_PANE_20010807_BRANCH

Comment 154

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

Attachment: updated patch, non-essential cruft has landed and fixed secure news icon problem in folderpane

Comment 155

[Jan Varga [:janv]]

Attachment: assertions fixed (patch for branch)

Comment 156

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

Attachment: latest patch

Comment 157

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

that latest patch includes fixes for this bug, all the blocker bugs: 
80844,85376,89702,92366

and several of the bugs that are blocked by this bug:
54171,85088,89346,93963,94052,93011,78768

the only known problems are: 

1) the outliner allows for multiple selection in the folder pane.  fixing that 
is blocked by #94652, which can happen after this lands.

2) no titletips in outliner.  see #32157

I'm going to get the folder pane branch up to date, ask lpham to spin new bits, 
and then get some QA eyeballs on the branch.

Comment 158

[Blake Ross]

There needs to be more space between the icons and the text for each row in the
outliner. Compare the screenshot to the current folderpane.  This is also a
problem with the threadpane, but not with say history (which also uses outliner).

Comment 159

[David Hyatt]

This can be done with padding rules in CSS.

Comment 160

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

I've fixed the padding problem by adding "padding-right: 2px;" to the rules for 
the subject column (in the thread pane) and the folder name column (in the 
folder pane.)

I'll attach a new patch.

Comment 161

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

Attachment: better padding in folder pane / thread pane

Comment 162

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

Attachment: updated patch, with padding rules fixes.

Comment 163

[Henrik Gemal]

just tried the speciel build. Here are my comments:
- Size, Lines, Unread and Total should be rightaligned

- Sometimes when switching between folders I get:
Error: uncaught exception: [Exception... "Component returned failure code: 
0x80004002 (NS_NOINTERFACE) [nsISupports.QueryInterface]"  nsresult: "0x80004002 
(NS_NOINTERFACE)"  location: "JS frame :: 
chrome://messenger/content/commandglue.js :: ChangeFolderByURI :: line 157"  
data: no]
Source File: 
Line: 23

Comment 164

[Håkan Waara]

This is a problem I found immediately:

I found that the Unread/Total columns way too large by default. You can probably
make them considerably narrower (fixed="true"?) without cropping the column-name.

Comment 165

[Håkan Waara]

Another problem:

The history sidebar's outliner is behaving wierd. The "threadlines" are painted
over the twisties.

Comment 166

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

#94653 can't right align text in outliner cell
#94813 dotted lines in outliner draw over twisty

I'll go work on making the unread and total column smaller by default, and I'll
look into the JS error.

Comment 167

[[e-mail] (gone - use [e-mail] instead)]

In classic, when I have a folder selected, I see a the thread line highlighted
for that folder. IE, |- becomes highlighted along with, Inbox.  Sheela and I
found that renaming a folder leaves a box outline on the folder's name for a
short while (but auto-corrects itself).

Comment 168

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

I've landed.  I'll go spin up new bugs on all the known issues.

giving back to hwaara, I stole this bug to drive this into the tree.

Comment 169

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

fixed.

Comment 170

[Keyser Sose]

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

Comment 171

[Karen Huang]

In order to verifiy this fix, I did the funcational testing on IMAP folders as 
following: (Verified on all the platforms: 08-15-06-trunk WinNT build & 
Linux/Mac 08-15-08-trunk builds)

Open/View Folders 
Create Folders 
Expand/Collapse Folders 
Copy/Move(Drag & Drop)Msgs to Folders
Properties of Folders 
Rename Folders
Subscribe/Unsubscribe Folders

I also run some regression testing for IMAP folders & IMAP interop as following:
context menus
double click to open new 3 pane
check msgs from Local folders
send unsent messages from the local folder
noselect folders
empty trash while trash is selected
folder compaction
check IMAP interop folder panes of AOL, Webmail, UW, Cyrus & MS Exchange.

It looks good from above test scenarios for IMAP folders except known 
[folderpane]bugs. (search these bugs by the status whiteboard)

POP/Local folder testing from Sheela.
News testing from Stephen.

I am marking as verified for this bug.  
Thanks everyone for helping this bug's verification.

Comment 172

[lchiang]

great job on verifications!
stephen - are there new performance numbers for this new outliner?

Comment 173

[[e-mail] (gone - use [e-mail] instead)]

The only thing in my test matrix that involves the folder pane is folder loading
speed, and that only gained a negligible amount (due to us leaking less).  That
was a while ago (the 1st time a test build was made), so I'll be re-testing the
trunk the day before on Win32 and the new build to see the memory usage
(requested by Cathleen, and others).  Stay tuned for results Tuesday night or
Wednesday morning...