What is up with the thread pane (and friends) UI refactoring on bug 474701
Andrew Sutherland
with the refactoring on bug 474701 (
https://bugzilla.mozilla.org/show_bug.cgi?id=474701 ) might be useful.
Pre-patch, much of the code dealing with the thread pane suffers from
global spaghetti-itis and under-documentation-fever. Specifically, we
have a lot of global variables and global functions that interact in
complex and inconsistent ways spread out through many separate JS files
organized loosely based on reuse across multiple XUL windows. Working
with the code is made more difficult by a lack of comments of what
functions or variables do, or why they do the things they do.
Control-flows have evolved into surprisingly reliable structures, but
ones that I doubt any human comprehensively understands or could
understand on short notice.
This is primarily a result of accumulated technical debt and the
evolutionary path of JavaScript in Thunderbird/MailNews from UI glue to
application logic. This patch tries to 'pay down' as much of that debt
as possible and help avoid us getting into that situation again in the
future.
The patches on bug 474701 attempt to do this by:
- Taking a more modular/object-oriented approach to things. Obviously
not a guaranteed panacea, but we desperately needed some encapsulation
for the affected code.
- Avoiding cargo culting. I've tried to analyze the full mailnews
control flow paths involved and only have our code do the things it
actually needs to do to make the right thing happen. In some cases the
right thing may still be odd/awkward, of course, but we should have less
"// we have to do this because of bug #lownumber" where lownumber turns
out to be an odd bug in layout code in mozilla 0.8.
- Extensive code documentation. Methods that describe what they do and
frequently why they do it. Inside, I try and comment why something is
being done so you won't have to guess what my intent was. If something
is doing something wrong and the comment doesn't at least have an
excuse, the code is probably just wrong.
- Unit tests! xpcshell tests for non-overlay code / code that doesn't
need to involve XUL. mozmill tests for overlay code, things that do
involve a lot of XUL, or more end-to-end/feature tests.
The key implementation abstractions introduced are:
already landed:
- DBViewWrapper. Attempts to abstract all interaction with nsMsgDBView
instances. You tell it about the nsIMsgFolder and it takes care of
building the view. It provides notifications related to the view and
its underlying folders as well as means to manipulate the view.
Search-related abstractions are provided for mail-views, quick searches,
and potentially any other view search predicates you would want to layer
on. Helper abstractions are VirtualFolderWrapper, SearchSpec,
MailViewManager, and QuickSearchManager. DBViewWrapper and its friends
live in JS modules (in mailnews/base/src) and are tested using xpcshell
tests.
in the current patch trying to land:
- FolderDisplayWidget. Wraps the DBViewWrapper and deals with the more
UI-related issues of the thread pane (selection, displayed columns,
updating toolbars, showing account central versus the thread pane, etc.)
especially the ramifications of our multiplexed tab implementation. The
implementation is sub-classed for use by the Search Window and
Standalone Message Window. Arguably the 3-pane window should also have
its own subclass and some of the 3-pane-specific things should be pushed
down into that. It is tested using mozmill tests, although the feature
matrix is really large, so the current set of tests is not exhaustive.
We will be adding tests as regressions are fixed, however.
- MessageDisplayWidget, FolderDisplayWidget's comrade-in-arms. It tries
to take care of the logic related to the message display pane as it
relates to the FolderDisplayWidget, to avoid bloating
FolderDisplayWidget and conflating too many things. This is where
multiple-message summaries are triggered from, for example. It has
subclasses specific to each use (message pane in 3-pane, message tab,
search window (no-ops-ville), and standalone message window) and does a
better job of not having the superclass assume the 3-pane use quite as
much as FolderDisplayWidget.
With the changes for this patch and Standard8's content pane work, we
are potentially close to being able to finally ditch the multiplexed tab
implementation.
There is one further patch 'layer' in bug 474701 coming which is the
actual gloda search that brought about this refactoring. The gloda
search patch provides for a global search using gloda's SQLite FTS3
fulltext index that is crammed into the traditional thread pane we all
know and love. Because of the refactoring we are able to use mail views
and quick-searches against the gloda search results and otherwise remain
reasonably blissfully ignorant that the gloda search is any different
from any other folder or transient search. The gloda search patch needs
some UI work still, but the backend is pretty solid and has had
extensive unit tests for quite some time now.
Because code documentation does not cover the entire documentation
story, I will be making sure that we provide usable documentation on
devmo for extensions authors and introductory documentation for (current
and would-be) Thunderbird developers. The Thunderbird developer
documentation is intended to provide an overview and means of
transitioning to the code documentation and is not intended to duplicate
or supplant it. I say "making sure" because I know at least clarkbw has
foolishly volunteered to try and help out on this front, and of course
jenzed will not escape either. I will provide updates on the newsgroups
as notable milestones in the documentation are reached.
Andrew
Ludovic Hirlimann
I think extension developers might be very interested in knowing all of
this.
--
Ludovic Hirlimann MozillaMessaging QA lead
http://www.spreadthunderbird.com/aff/79/2
Siddharth Agarwal
Extension developers would be interested in this. (_Tsk_'s original
forward was to a misspelled newsgroup.)
Pavol Misik
I'm an extension developer.
I do an extension completely in c++.
I only used js function GetDBView to get instance of nsIMsgDBView.
I know that in TB3 there is new feature called Tabbed Email.
After your patch will land, when I need to get instance of actually
displayed nsIMsgDBView on active tab do I need to call this
"gFolderDisplay.view.dbView" ?
I'm not sure If I can access from c++ to js global variable.
I use this to get instance of nsIMsgDBView from c++:
nsCOMPtr<nsIDocument> doc = do_QueryInterface(m_doc, &rv);
nsIScriptGlobalObject* sgo = doc->GetScriptGlobalObject();
nsIScriptContext* aScriptContext = sgo->GetContext();
JSContext* jscontext =
reinterpret_cast<JSContext*>(aScriptContext->GetNativeContext() );
JSObject* global = JS_GetGlobalObject(jscontext);
ASSERT ( NULL != global ); // toto by asi namalo nastat
if ( NULL == global )
{
return NS_ERROR_INVALID_POINTER;
}
jsval rval;
JSBool ok = JS_CallFunctionName( jscontext, global, "GetDBView", 0,
NULL, &rval );
if ( !ok || !JSVAL_IS_OBJECT( rval ) )
{
return NS_NOINTERFACE;
}
nsCOMPtr<nsIXPConnect> xpc( do_GetService ( nsIXPConnect::GetCID (), &rv
) );
if ( !NS_SUCCEEDED (rv) )
{
return rv;
}
JSObject* pJSObject = JSVAL_TO_OBJECT ( rval );
if ( NULL == pJSObject )
{
msgDbView = NULL;
return NS_ERROR_INVALID_POINTER;
}
nsCOMPtr<nsISupports> holder;
rv = xpc->WrapJS ( jscontext, pJSObject, NS_GET_IID (nsISupports),
getter_AddRefs( holder ) );
if ( !NS_SUCCEEDED (rv) )
{
return rv;
}
msgDbView = do_QueryInterface( holder, &rv );
Could you please global function which returns instance of global
variable or how can I get instance of global variable?
PM-
Pavol Misik
Could you please create global function which returns instance of global
Andrew Sutherland
> I do an extension completely in c++.
> I only used js function GetDBView to get instance of nsIMsgDBView.
What does your extension use the nsIMsgDBView reference for? Are you
adding a custom column?
How does your extension get triggered when it wants to get a reference
to the nsIMsgDBView?
Depending on what you are trying to do, there may be an easier way to
accomplish it. Does your C++ extension expose any XPCOM services? It
might be easier to do more things in JavaScript that call into your
service as needed.
Andrew
Pavol Misik
> What does your extension use the nsIMsgDBView reference for? Are you
> adding a custom column?
I searched or source code and I find out that we use these methods:
- GetURIsForSelection
- GetNumSelected
- GetViewFolder
- GetMsgFolder
We generally need to get uris of selected messages.
An effective way to find out if it is selected any message and number of
selected messages without getting uris. Last think we need is to get
nsIMsgFolder associated with actually displayed nsIMsgDBView.
Btw. meanwhile I forgot what is the difference between GetViewFolder and
GetMsgFolder (noting in nsIMsgDBView.idl)?
> How does your extension get triggered when it wants to get a reference
> to the nsIMsgDBView?
By buttons from toolbar (completely created and handled in c++), when
folder selection if folder pane is triggered and after selection in
"threadTree" is changed.
> Depending on what you are trying to do, there may be an easier way to
> accomplish it.
In our case it was made decision to do it in c++ and I can't change it.
We need to interact with operating system and services on it and much more.
> Does your C++ extension expose any XPCOM services? It
> might be easier to do more things in JavaScript that call into your
> service as needed.
We expose only a few component because we use nsISupport Proxies. We
find better way do that thinks without proxies and we want to get rid
of all exported components.
PM-
Andrew Sutherland
> I searched or source code and I find out that we use these methods:
> - GetURIsForSelection
> - GetNumSelected
> - GetViewFolder
> - GetMsgFolder
>
> We generally need to get uris of selected messages.
> An effective way to find out if it is selected any message and number of
> selected messages without getting uris. Last think we need is to get
> nsIMsgFolder associated with actually displayed nsIMsgDBView.
All of these can be directly retrieved from the nsIMsgDBView. We
continue to expose the gDBView global variable which is the nsIMsgDBView
instance for the currently displayed view. Your existing JS-extraction
code should be able to access the global; just lose the line that calls
the global (function) you retrieve.
Unless you see a major problem with that, I think that is the best
option. Additionally, it should also work in 2.0 if that is relveant to
you. Unless we change the documented mechanism for creating custom
columns, we must continue to provide the gDBView global. (And if we do
change the mechanism, I don't have a problem leaving gDBView exposed.)
> Btw. meanwhile I forgot what is the difference between GetViewFolder and
> GetMsgFolder (noting in nsIMsgDBView.idl)?
When dealing with saved searches the viewFolder is the (artificial)
nsIMsgFolder for the saved search, while the msgFolder is the
nsIMsgFolder for the folder with the actual messages in it.
> In our case it was made decision to do it in c++ and I can't change it.
> We need to interact with operating system and services on it and much more.
I'm not proposing you completely abandon C++ for JavaScript. I just
mean that code that deals with the UI would probably find it easier if
that code is written in JS. It would then call into your C++ XPCOM
components, driving it. It's a similar situation to the discussions
people have about writing things in Python; the general consensus is to
write as much as possible in Python and only move the things into C/C++
when the situation demands, and only then, only move what you must.
Of course, I do understand the inertia of a large C++ code base. Doing
such a thing is not easy. It sounds like gDBView should resolve your
problem so it's not an issue :)
Andrew
Pavol Misik
> All of these can be directly retrieved from the nsIMsgDBView. We
> continue to expose the gDBView global variable which is the nsIMsgDBView
> instance for the currently displayed view. Your existing JS-extraction
> code should be able to access the global; just lose the line that calls
> the global (function) you retrieve.
>
> Unless you see a major problem with that, I think that is the best
> option. Additionally, it should also work in 2.0 if that is relveant to
> you. Unless we change the documented mechanism for creating custom
> columns, we must continue to provide the gDBView global. (And if we do
> change the mechanism, I don't have a problem leaving gDBView exposed.)
I tested what you suggest to get gDBView. I searched
mozilla.dev.tech.js-engine and I find out that I can use function
JS_GetProperty.
I changed this
JSBool ok = JS_CallFunctionName( jscontext, global, "GetDBView", 0,
NULL, &rval );
to this:
JSBool ok = JS_GetProperty(jscontext, global, "gDBView", &rval);
and it seem it woks under TB2.0.0.21.
Thanks
PM-
Andrew Sutherland
> Standard8 made the good point on IRC that an explanation of what is up
> with the refactoring on bug 474701 (
> https://bugzilla.mozilla.org/show_bug.cgi?id=474701 ) might be useful.
This has now landed. Apart from regression fixing, my short-term focus
is to get the documentation for this up and running. In the interim,
here is what you need to know if you are writing extensions or other
chrome code.
Want to know the selected messages? Use:
- gFolderDisplay.selectedCount: To get the number of things selected.
- gFolderDisplay.selectedMessages: To get a list of the nsIMsgDBHdrs
corresponding to the selected messages. If a collapsed thread is in
there and working with collapsed threads is enabled, this will include
the headers for the messages in that collapsed thread.
- gFolderDisplay.selectedMessageUris: To get a list of the URIs
corresponding to the selected messages. Also includes messages in
selected collapsed threads when so enabled.
- gFolderDisplay.selectedMessage: To get the first selected header.
Only use this when you know there is only one message happening or only
care about one. In some cases I believe this is used as an (erroneous)
proxy for knowing some attribute about all the messages in a selection.
Want to select some messages? Use:
- gFolderDisplay.selectMessages(aListOfMessageHeaders): Select all the
headers in the list you pass. It will attempt to scroll the view so the
range is visible, but obviously, with huge selections it can only show
part of it (and we choose the first part).
- gFolderDisplay.selectMessage(aMessageHeader): Select just the one message.
- gFolderDisplay.clearSelection(): Select no messages!
Want to know what folder is being displayed? Use:
- gFolderDisplay.displayedFolder: The nsIMsgFolder being displayed.
Want to know what message is being displayed in the message pane, if
any? Use:
- gMessageDisplay.displayedMessage: An nsIMsgDBHdr (or dummy that's
trying to pretend to be one, in the case of an .eml file), or null if no
message is being displayed.
Okay, so that's pretty useful so I'm going to throw this up on devmo
right now, as newsgroups are probably not most people's idea of the
place to go for documentation.
Andrew
Mark Banner
> Standard8 made the good point on IRC that an explanation of what is up
> with the refactoring on bug 474701 (
> https://bugzilla.mozilla.org/show_bug.cgi?id=474701 ) might be useful.
Hi folks,
In case you haven't been watching the bug or the checkin logs, this
patch has now landed and is in today's nightly builds.
Please keep an eye out for regressions. We have a meta bug tracking
regressions from the landing:
https://bugzilla.mozilla.org/show_bug.cgi?id=497199
Please check there before filing new bugs (and search as well, just in
case they aren't listed), and when filing new bugs add them as blocking
that bug if you think they were caused by the update.
Thanks,
Standard8
Andrew Sutherland
> Please keep an eye out for regressions. We have a meta bug tracking
> regressions from the landing:
>
> https://bugzilla.mozilla.org/show_bug.cgi?id=497199
>
> Please check there before filing new bugs (and search as well, just in
> case they aren't listed), and when filing new bugs add them as blocking
> that bug if you think they were caused by the update.
A fast way to check if the bug is known is to look at the dependency
tree for the bug:
https://bugzilla.mozilla.org/showdependencytree.cgi?id=497199&hide_resolved=1
(That saves you from having to hover over the bugs or click on them all.)
Andrew
AlfredKayser
Version Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1pre)
Gecko/20090614 Lightning/1.0pre Shredder/3.0b3pre ThunderBrowse/3.2.5
is not usable at all. Clicking on a row in the folderPane just doesn't
display the message. Doubleclicking however does open the message in
its separate window.
Error displayed in Error Console:
Error: gRightMouseButtonDown is not defined.
Source File: chrome://tbrowse/content/tbmain.js
It looks like Bug 497279 - Can't switch to View -> Threads ->
Threads with unread,
but I am not sure whether it is the same, and whether it has enough
attention to fix it real soon, and make an update available.
The issue is now there since June 13...
Greetings, Alfred
Thomas Stache
But the error includes "chrome://tbrowse/content/tbmain.js", and your
user agent includes "ThunderBrowse/3.2.5". Please disable that
extension, and check back.
Andrew Sutherland
> Version Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1pre)
> Gecko/20090614 Lightning/1.0pre Shredder/3.0b3pre ThunderBrowse/3.2.5
> is not usable at all. Clicking on a row in the folderPane just doesn't
> display the message. Doubleclicking however does open the message in
> its separate window.
>
> Error displayed in Error Console:
> Error: gRightMouseButtonDown is not defined.
> Source File: chrome://tbrowse/content/tbmain.js
It looks like this is an error in the ThunderBrowse extension caused by
the removal of a global variable in the refactoring. Presumably
ThunderBrowse is replacing Thunderbird's onselect handler for the thread
pane (the folder pane is the folder tree on the left) and using the
variable itself, or it contains copied and pasted code from the original
method.
Double-click handling happens via a different method, which is probably
why it is not broken.
I would suggest disabling the ThunderBrowse extension for the time being.
Andrew
G4TechTV
See, no one tells me they are gonna change this. I've patched up most
of the conflicts in 3.2.6, which I've uploaded to AMO a couple of
minutes ago. The new messagepane also screwed up stuff as well, so did
deleting a lot of the UI we hook into.
But yes, it's not overridden, it's actually copy pasted and then
hotpatched to suit my methods.
I know about the bug, it's been fixed and now we have to wait for
Mozilla to push it to public. I also have a build that will fix these
errors for the time being on my blog.
http://www.thunderbrowse.com/thunderblog/download-manager.php?id=9