545666 - Re-implement single message in a tab so that it doesn't reuse the browser/messagepane element

Status: RESOLVED DUPLICATE of bug 1729384

(Thunderbird :: Toolbars and Tabs, task)

Description

[Mark Banner (:standard8)]

For fixing bug 487386 (scroll positions etc getting lost), we really need to have each tab with its own browser element to display the message.

Starting with a single message in a tab seems the most obvious place as, in theory, it is the simplest of the message related tabs that we have.

There's various gecko fixes that we need to properly support messages in browser elements without id="messagepane", so I'll be marking those as blocking this bug.

Comment 1

[Jens Müller (:tessarakt)]

(In reply to comment #0)
> There's various gecko fixes that we need to properly support messages in
> browser elements without id="messagepane", so I'll be marking those as blocking
> this bug.

You still have this in mind?

Comment 2

[Thomas Schmid]

Attachment: extension defining a custom tabtype with separated messages panes

implements a custom tabtype where every tab has a separate browser and tree. You can load it via "Tools -> new folder tree view"

it just a quick hack to check if its feasible. It throws tons of errors and has still some issues when displaying messages. Hints would be great.

Converting this into a patch is a long way to go. It simplifies lots of wired code, but also means changing a breaking lots of things.

Before investing more time feedback would be perfect.

Comment 3

[Thomas Schmid]

Attachment: extension with separate messages panes for each tab

Works acceptable.

Compared to the previous xpi the following changed:
* unneeded methods are removed from folderDisplay and messageDisplay 
* Converted folderDisplay and messageDisplay into a Module. 
* Multimessage view works again.

Next steps would be:
* merging the messageHeaderSink into messageDisplay
* get tabmail related stuff working (titles etc...)
* Hookup Menu Item and context menu
* get non classic views working

Before investing more time, I relay need feedback.

Comment 4

[Mark Banner (:standard8)]

Comment on attachment 651152 [details]
extension with separate messages panes for each tab

Unfortunately, this doesn't give me enough of an indication as to what you are doing. The only new tab type doesn't actually appear to have its own panel (which would get the elements duplicated), or its own set of elements, so it is hard for me to tell what you are doing there. The other files are hard to review, as they aren't diffs to the original.

I'd also strongly recommend breaking this down into smaller steps, there's various issues that need to be fixed in the main code to make this more viable. I don't know if you've already seen it or not, but I did a post to tb-planning describing what I think those steps are:

http://groups.google.com/group/tb-planning/browse_thread/thread/4ef367cfbbcb19d6

Comment 5

[Thomas Schmid]

(In reply to Mark Banner (:standard8) from comment #4)
> Unfortunately, this doesn't give me enough of an indication as to what you
> are doing. The only new tab type doesn't actually appear to have its own
> panel (which would get the elements duplicated), or its own set of elements,
> so it is hard for me to tell what you are doing there. The other files are
> hard to review, as they aren't diffs to the original.

Sorry i don't want to be impolite, but such an answers really aggregates me. 
I've invested several hours to work through the code and building a demonstration and you did not even consider diffing these five files with the code repository? That can't be true or? Sounds to me like a bad joke.

But anyhow you find attached the diffs in the unified format.

> I'd also strongly recommend breaking this down into smaller steps, there's
> various issues that need to be fixed in the main code to make this more
> viable. I don't know if you've already seen it or not, but I did a post to
> tb-planning describing what I think those steps are:
> 
> http://groups.google.com/group/tb-planning/browse_thread/thread/
> 4ef367cfbbcb19d6

No I did not see this but the diff reflects only a tiny but important part of these changes you outlined.

It does the following:

At first it moves the xul for the treeview, folderview and message view into a separate file (view.xul).

Then it defines a new tabtype. Currently only one for a classic message pane is implemented. This tabtype loads the view.xul so that you end up with a separate messagepane for each folder. The tabtype is just a mockup.

As every tab has it's own exclusive message pane, you can get rid of all code in folderDisplay.js and messageDisplay.js 
that takes care about sharing tabs. 

But now the interesting part and the trouble begins, and it's time to apply some magic. Loading messages is the tricky part.

By default Thunderbird uses the messenger to loads all messages into a xul element named "messagepane" into the root window (!).
And that's the problem, the root window is one window above our view.xul, so we need a way to get around this. And the
root's windows docshell is hard coded into the c++ code.

One way would be to div deep into c++ code break the common code base with SeaMonkey and fix the all of 
these message pane references. 

The other way is to reuse the suppressMsgDisplay flag. I discovered this while browsing though the c++ code figuring out how much effort it would be fixing the messenger. This approach is efficient and a low hanging fruit. Thus I've attached
the demonstration.

So what's so special about this flag? It detaches the messenger from the dbView. Exactly what we want. If it's set it invokes folderDisplay.js's summarizeSelection() respectivly onSelectedMessagesChaned() method instead of loading a message.

Ok we now have a way to suppress loading messages into the root's element named messagepane and we are notified when a message needs to be loaded.

Sofar so good. The only part missing is loading messages. In order to deal with this we can use messengers DisplayMessage method. In contrast to messenger's loadUrl() (dbView relies on this) it does not use the roots hard coded docShell. You can pass a custom docschell to displayMessage(), in our case the docShell of the messagepane within our tab. 

But this works only for simple messages. Mulitmessages like the summary have their own mechanisms to load content, as the do not display messages. 

So I decided to implement MessageDisplayTypes, it's basically the same principle as with tabtypes. Every message type can can define his own messagepane and loading instructions. Currently there is one for simplemessages, 
multimessages(summaries), rss feeds and a default view which is used when no message type applies to this header.

What's the benefit of this? All messagedisplay related code is encapsulated within this messageDisplayType class. Which improves code readability dramatically. Eg. with feed, the current code is splatered around. You find fragments in mailWindow, MailWindowOverlay, msgMail3Pane and finally msgHdrViewOverlay.

Furthermore you can easily extend or overlay displaying messages with you own custom message display type.
Current extension, which overlay the messagepane just hide the messagepane. The message is always loaded in  backgound no way to suppress this. By using these message display types you can override and prevent displaying a message in the messagepane with ease.

So summarize. We manged it to hookup dbView to work with arbitrary docshells. Displaying messages is now easily extensible. 

As a side node messageDisplay.js and folderDisplay.js are now modules. Both where a total mess and relied on tons of calls to global methods defined in script files somewhere else. After stripping that down only few of these calls remained. But it still needs cleanup. For example the mark message as read timer should be integrated into folderDisplay.

 
That's what the demonstration shows...

What do you think about this approach? 

I agree it's still a long way to go. The biggest show stopper is solved. 

But especially the toolbar and menu related stuff is still a challenge.

Comment 6

[Thomas Schmid]

Attachment: Diff showing the changes compared to the respository

Comment 7

[Mark Banner (:standard8)]

I'm really sorry for not having got back to you earlier on this.

I think the idea of loading view.xul into the tab is certainly an interesting one. There's a few concerns I have there about ids and duplication of elements, but its potentially loading it in, than duplicating a set of elements like we do currently.

On the messagepane front, I think that is also interesting. My only concern would be if there's a performance impact. I certainly agree it is a good idea to de-couple the backend from the id of "messagepane", I think my original idea there was to be able to pass the message pane, to where ever, but if we can stream to a particular docshell, then that sounds better.

I think this is worth pursuing, maybe we could start off by getting some of the messagepane changes into core?

Also cc'ing more folks for comments.

Comment 8

[Joshua Cranmer [:jcranmer]]

(In reply to Mark Banner (:standard8) from comment #7)
> On the messagepane front, I think that is also interesting. My only concern
> would be if there's a performance impact. I certainly agree it is a good
> idea to de-couple the backend from the id of "messagepane", I think my
> original idea there was to be able to pass the message pane, to where ever,
> but if we can stream to a particular docshell, then that sounds better.

From the backend perspective, the protocol handlers use specific query arguments and the presence of an nsIMsgHeaderSink to key into message display. There's another layer of complexity (nsIMsgMessageService::DisplayMessage, IIRC) which causes the display to work properly if given an nsIDocShell as the consumer.

Beyond that, my grasp of the display pipeline is very tenuous, but I think nsIMessenger is the key last player, which appears to do the search for explicit "messagepane" elements. It might be possible to key it off of nsIDocShell instances instead of nsIDOMWindow. nsMessenger appears to only use the DOMWindow for file picker initialization and grabbing the docshell, so this seems very doable. Everything else is well outside my realm of expertise.

Comment 9

[Thomas Schmid]

In order to get the ball rolling and break down complexity I would start with converting messageDisplay and folderDisplay into a JavaScript module and use the chance to do some cleaning it up. This means moving initialization code into the modules as well as introducing a real constructor and getters/setters for both modules of these modules. Furthermore I would suggest to get rid of gMessageDisplay (it is used according to mxr in just 4 lines of code) and I suppose there are a lot more of such leftovers. 

I'll file two bugs on this, one on converting messageDisplay and one on folderDisplay. 

Is there a way to just build the jar files? Building the whole Thunderbird Project is very time consuming...

Comment 10

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Thomas Schmid from comment #9)
> Is there a way to just build the jar files? Building the whole Thunderbird
> Project is very time consuming...

On platforms that support symlinks (linux, OS X), if you do a Thunderbird build we don't even build any jar files and we just symlink everything that's not preprocessed.  So simply restarting Thunderbird should get the changes, although then the startup cache might be a problem.

Although .mozconfig should default to it for Thunderbird, the line to get symlinks is:
ac_add_options --enable-chrome-format=symlink

The startup cache is named PROFILEDIR/startupCache/startupCache.8.little and I fear there are no preferences that cause it not to be updated.

Comment 11

[Rain [:rain1] (inactive)]

Also on Windows: see http://www.less-broken.com/blog/2011/03/hard-linked-chrome-now-supported-on.html Note that (on all platforms) preprocessed files aren't symlinked.

Comment 12

[Joshua Cranmer [:jcranmer]]

(In reply to Thomas Schmid from comment #9)
> In order to get the ball rolling and break down complexity I would start
> with converting messageDisplay and folderDisplay into a JavaScript module
> and use the chance to do some cleaning it up. This means moving
> initialization code into the modules as well as introducing a real
> constructor and getters/setters for both modules of these modules.
> Furthermore I would suggest to get rid of gMessageDisplay (it is used
> according to mxr in just 4 lines of code) and I suppose there are a lot more
> of such leftovers. 

One thing to beware is that addons can use globals in the JS window, so I would be wary of advocating removing features "just because" they aren't used in TB itself. That's not to say that you can't remove them, it just means you need to have good justifications and make sure that any useful functionality they provide is still accessible somehow.

Also, something to point out is that it's not necessarily a good idea to move display logic code into modules, since modules force new globals, as well as various issues around cross-compartment wrappers. I don't work with frontend code enough to know what would and wouldn't be a good idea, but this is something that a reviewer might object to. I'll let those with more expertise here interject.

Comment 13

[Wayne Mery (:wsmwk)]

Sounds like something for 76.

To what extent should this be considered a blocker to fixing the ills related to Bug 714489 ?
And should they be on parallel tracks?

Comment 14

[Magnus Melin [:mkmelin]]

Not sure this bug by itself is important, though bug 487386 is. That may have another solution.

Comment 15

[Thomas D. (:thomas8)]

Looks like this bug is currently being worked on as:
Bug 1729384 - Create message pane as a separate document (implement about:message)
So should we close this bug 545666 as a duplicate of that?

What about the dependent bugs here (bug 545668, bug 809300, bug 809302)? Do they still have useful information for the current operation which is underway?

Related: Bug 1729379 - [meta] Implement 3-pane tab in a separate document

Comment 16

[Wayne Mery (:wsmwk)]

So should we close this bug 545666 as a duplicate of [Bug 1729384]?

This bug has many uplevel dependencies marked, so I think rather than dup and rearrange dependencies, it is less work to just mark this as a dependent on bug 1729379 and/or Bug 1729384

Comment 17

[Thomas D. (:thomas8)]

Ok, thanks.

Comment 18

[Geoff Lankow (:darktrojan)]

I closed the blocking bugs anyway, so we might as well dupe it and move the one blocked bug reference.