436357 - Don't use RDF for the filter list dialog tree

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[[e-mail]]

As a bonus we get more realistic refresh behaviour. (The filter DS does not receive any change notifications so everyone has to rebuild it all the time.)

Comment 1

[[e-mail]]

Attachment: Proposed patch

* Creates a tree view in JS
* Changes toggleFilter to take an index
* Changes get/currentFilter and getFilterList to access the saved filter list
* Replaces blunt refresh with appropriate invalidate/rowCountChanged
* Fixes space key handling

Comment 2

[Karsten Düsterloh]

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

First of all, this patch regresses deleting filters: the respective filter name is cleared in the tree, but the checkbox is still there. Errors get dumped when moving the mouse over inhabited parts of the tree:

WARNING: row count changed unexpectedly: 'mRowCount == rowCount', file /home/kd/projekte/mozilla/mozilla.org/src/trunk/mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp, line 2798
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIMsgFilterList.getFilterAt]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://messenger/content/FilterListDialog.js :: getRowProperties :: line 124"  data: no]
************************************************************
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIMsgFilterList.getFilterAt]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://messenger/content/FilterListDialog.js :: getCellProperties :: line 128"  data: no]
************************************************************
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIMsgFilterList.getFilterAt]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://messenger/content/FilterListDialog.js :: getCellText :: line 146"  data: no]
************************************************************
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIMsgFilterList.getFilterAt]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://messenger/content/FilterListDialog.js :: getCellProperties :: line 128"  data: no]
************************************************************


And some more nits: ;-)

>Index: mailnews/base/search/resources/content/FilterListDialog.js
>===================================================================
>+var enabledAtom = Components.classes['@mozilla.org/atom-service;1']
>+                            .getService(Components.interfaces.nsIAtomService)
>+                            .getAtom('Enabled-true');

If defined globally, this should be a const.
But since it's only used inside gFilterTreeView, just make it a member there.

>+var gFilterTreeView = {
>+  mTree: null,
>+  get tree() {
>+    return this.mTree;
>+  },

The file's bracing style is rather inhomogeneous, so I'd prefer using the braces-on-their-own-line style.

>@@ -292,22 +358,12 @@
>     if (currentIndex == -1)
>       return null;
>     

Empty that almost empty line, while you're at it. ;-)

>+    return gFilterTreeView.filterList.getFilterAt(currentIndex);

Just call getFilter here - or probably define gFilterTreeView.getFilter and get rid of the global getFilter?

>@@ -644,7 +646,7 @@
> function onFilterTreeKeyPress(event)
> {
>   // for now, only do something on space key
>-  if (event.keyCode != 0)
>+  if (event.charCode != " ".charCodeAt(0))

Oh, come on, be serious! :-D
Please port ;-) bug 368218, it's on our list anyway.

Comment 3

[[e-mail]]

(In reply to comment #2)
> (From update of attachment 322983 [details] [diff] [review])
> First of all, this patch regresses deleting filters:
Strange, I thought I'd tested that, but I'll double-check.

> >+var gFilterTreeView = {
> >+  mTree: null,
> >+  get tree() {
> >+    return this.mTree;
> >+  },
> 
> The file's bracing style is rather inhomogeneous, so I'd prefer using the
> braces-on-their-own-line style.
I'm not too keen on that style for JS objects.

> >@@ -644,7 +646,7 @@
> > function onFilterTreeKeyPress(event)
> > {
> >   // for now, only do something on space key
> >-  if (event.keyCode != 0)
> >+  if (event.charCode != " ".charCodeAt(0))
> Oh, come on, be serious! :-D
> Please port ;-) bug 368218, it's on our list anyway.
OK, I'll switch back to the keyCode if you insist (tree.xml uses charCode).

Comment 4

[[e-mail]]

Attachment: Addressed review comments

* Moved enabledAtom to gFilterTreeView.mEnabledAtom
* Simplified currentFilter()
* Fixed row count change calculation when deleting filters
* Switched to using keyCode to detect space

Comment 5

[[e-mail]]

(In reply to comment #2)
>(From update of attachment 322983 [details] [diff] [review])
>>+    return gFilterTreeView.filterList.getFilterAt(currentIndex);
>define gFilterTreeView.getFilter and get rid of the global getFilter?
I was thinking of moving everything into methods on gFilterTreeView but I thought it was better to do it in two stages i.e. conversion then cleanup.

Comment 6

[Karsten Düsterloh]

Comment on attachment 323205 [details] [diff] [review]
Addressed review comments

>> The file's bracing style is rather inhomogeneous, so I'd prefer using
>> the braces-on-their-own-line style.
> I'm not too keen on that style for JS objects.

I'm not so much "concerned" about the (outer) object, but functions and if structures: for example ...

>Index: mailnews/base/search/resources/content/FilterListDialog.js
>===================================================================

... like here ...

>+  get tree() {
>+    return this.mTree;
>+  },
>+  mFilterList: null,
>+  get filterList() {
>+    return this.mFilterList;
>+  },

... or here (and elsewhere) ...

> function onNewFilter(emailAddress)
> {
...
>+  if (args.refresh) {
>+    gFilterTreeView.tree.rowCountChanged(0, 1);
>+    gFilterTree.view.selection.select(0);
>+  }
> }

And the keyCode has even been fixed by another bug.


r=me given some extended bracing style consideration. ;-)

Comment 7

[[e-mail]]

(In reply to comment #6)
>(From update of attachment 323205 [details] [diff] [review])
>> function onNewFilter(emailAddress)
>> {
>...
>>+  if (args.refresh) {
>>+    gFilterTreeView.tree.rowCountChanged(0, 1);
>>+    gFilterTree.view.selection.select(0);
>>+  }
>> }
> r=me given some extended bracing style consideration. ;-)
Fix checked in with this bracing revised as agreed on IRC.