359357 - Custom column does not appear except in view All

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Kent James (:rkent)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1) Gecko/20061010 Firefox/2.0
Build Identifier: version 2 beta 1 (20061102)

When using the new custom columns feature in an extension, the custom column is blank except in the All view. As a variation, if when opening Thunderbird a view is displayed that is not All for a particular folder (like unread for example), the custom column information appears initially, but after switching to the All view does not appear there nor in other views, including the original view.

Reproducible: Always

Steps to Reproduce:
1.Install an extension using custom columns, for example https://bugzilla.mozilla.org/attachment.cgi?id=243161
2.Select the Inbox folder (information appears for custom column)
3.Change view to "unread" (no information appears for custom column)

Actual Results:  
Information disappears from the custom column. In the case of the mentioned extension, instead of seeing "0" or "100" for junkscore, blank is seen.

Expected Results:  
column information is displayed, for example "0" or "100" instead of blank.

Comment 1

[David :Bienvenu]

when we switch views, you need to re-register your custom column handler with the new view. In particular, you'd need to overlay here - http://lxr.mozilla.org/seamonkey/source/mail/base/content/commandglue.js#655 

or perhaps here: http://lxr.mozilla.org/seamonkey/source/mail/base/content/commandglue.js#717

Comment 2

[Kent James (:rkent)]

OK, I can solve this problem by adding to my extension a custom CreateDBView function in my overlay javascript. I tried this, and it solves the problem. Is that what you meant by "overlay" a javascript file?

If so, then I am not very comfortable with that approach. First, it makes the extension fairly fragile wrt release version, as any changes to the CreateDBView function in a new release are overwritten by an old extension. Second, multiple extensions that all followed this approach would collide with each other.

Perhaps some sort of list of registered custom column handlers could be added to CreateDBView, so that extensions could add to the list and play nice with each other.

Comment 3

[David :Bienvenu]

perhaps SwitchView should maintain the existing custom column handlers - I'll see how possible that is...

Comment 4

[Kent James (:rkent)]

It seems to me that the root cause of this problem is that the storage structures for the custom column handlers, m_customColumnHandlers and m_customColumnHandlerIDs in nsMsgDBView, are constantly destroyed as nsMsgDBView instances are recreated. In the most common usage of a custom column, they would be created once and should persist throughout the application.

Can't these data structures be made persistent, either by making them static members of nsMsgDBView, or by putting them in a separate class whose lifetime is independent of nsMsgDBView?

Comment 5

[Kent James (:rkent)]

Attachment: Make custom column handler datastructures static

For example, this patch makes the custom column handler datastructures static in nsMsgDBView. (Sorry, the patch is against my local CVS so the version numbers are off from the public CVS) I have compiled it and tested it briefly (Windows 2000 on the Mozilla1_8 branch) and it seems to work, and solves the problem mentioned in the current bug.

The overlay interface to make this work is identical to that shown in https://bugzilla.mozilla.org/attachment.cgi?id=243161 except I have added an additional line to function addCustomColumnHandler():

window.document.getElementById("folderTree").removeEventListener("select", addCustomColumnHandler, false);

If you decide to adopt this approach to this bug, perhaps you could also suggest a cleaner interface for the overlay. Is there an event that I can respond to that will have gDBView defined, instead of this three step process of 1) listen for load event, 2) on load add listener to folderTree, 3) after initial folderTree remove listener. Or perhaps I could load an instance of nsMsgDBView myself in the js code to add the custom column listener.

Comment 6

[David :Bienvenu]

argh, firefox hung as I was adding a long comment...

Thanks for the patch, Kent. It's attractive because it fixes your problem in a simple way. But I have some concerns about this approach. 

1. Semantically, it's odd. You add a custom column handler to a view, and it now applies to all views ever created. That happens to be what you want, but for other extensions, it might not be - for example, if an extension wants a custom column handler whenever a mail folder is selected, but not a newsgroup, they'd need to remove their custom column handler when the newsgroup is selected, and add it back when a mail folder is loaded. (I guess this isn't quite true, since they really need to remove the custom columns from the thread pane).

2. Making the view code hold onto the custom column handlers statically means that extensions need to be vigilant about removing custom column handlers when they're unloaded, or else they'll get held onto until shutdown...

So I'm not sure - I need to think about this a bit. 

Comment 7

[David :Bienvenu]

I could add an empty OnViewOpened function that gets called when a view is opened, and you could overlay that empty js function to add your custom column handler. But there would still be the problem of multiple extensions wanting to add columns trying to overlay the same method.

Or, I could add proper notifications, so that multiple extensions could add themselves as event listeners, and each extension would get the event. I'm not sure where I'd add the event listener (un)registration methods - perhaps on nsIMessenger or nsIMsgWindow. I don't think there are any existing notifications I can take advantage of, since nsIFolderListener doesn't seem appropriate.

Comment 8

[Kent James (:rkent)]

Attachment: Add observer to CreateDBView

How about this simple patch, that adds an observer to CreateDBView in the command glue? This also works with my (modified) test extension. As a bonus, I can easily call IsnewsURI on the passed msgFolder to not add the custom column for a news folder, as you requested. I have tested this and it also solves the problem of this bug.

However, I would need to do more work to make the actual columns disappear in the news views. By not adding the column observer, they have no content, but the custom columns still appear with blank content since they are persisted in the tree.

Comment 9

[David :Bienvenu]

Comment on attachment 244864 [details] [diff] [review]
Add observer to CreateDBView

I like this - but I might change the topic to MsgCreateDBView since topics are global...

Comment 10

[David :Bienvenu]

Attachment: the fix as checked in

I changed the notification topic to MsgCreateDBView

Comment 11

[David :Bienvenu]

Kent, can you add a snippet of js code showing how to listen for this event - I've cc'ed Eric, who's helping document these interesting extension capabilities.

Comment 12

[Kent James (:rkent)]

I will add a sample working extension here after the patch shows up on the nightly builds.

Comment 13

[David :Bienvenu]

should be in tomorrow's trunk build. I'll land it for 2.0 builds tomorrow...

Comment 14

[David :Bienvenu]

fixed on 2.0 branch - thx, Kent!

Comment 15

[Kent James (:rkent)]

Attachment: Sample extension using new observer

This attachment demonstrates the use of the new observer in providing a custom column handler. It also disables the custom columns in news views, as requested in comment #6. This extension works in the 20061109 nightly build of TB 2.0 prebeta1.

The extension itself does nothing useful under current builds, because the db-stored junkscore is not really the score from the bayesian filter, but only has values or 0 or 100. Fixing this is the subject of bug #209890.

Comment 16

[Gil]

I've noticed that current solution does not work when I select "Grouped by Sort". Is it correct. I created some simple extension that works fine, unless "grouped by" option is selected.

Am I doing something wrong, or indeed there is a problem?

Comment 17

[David :Bienvenu]

there's a problem, I bet. Grouped by Sort needs to know about custom columns, and I suspect it does not.

Comment 18

[Gil]

Should I open a new bug over this, or it belongs to current thread?

Comment 19

[David :Bienvenu]

a new bug, please. Assigned to me. Thx!