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.)

* 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 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.

(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).

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

(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 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. ;-)

(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.