376717 - Extensions Should Be Able to Add and Handle Columns (SeaMonkey front-end patch)

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Message Display, enhancement)

Description

[Philip Chee]

Attachment: Changes to commandglue.js to handle custom columns.

This patch adds the front end changes in Thunderbird Bug 348504 to MailNews.

Comment 1

[Philip Chee]

Note: I've reduced the amount of whitespace in this patch compared to the thunderbird patch because the prevailing style in this file appears to be to *not* add blank lines before/after a try/catch block. I can put these back if you think it improves readability.

Comment 2

[Karsten Düsterloh]

Comment on attachment 260820 [details] [diff] [review]
Changes to commandglue.js to handle custom columns.

Basically okay, just some nits:

>Index: base/resources/content/commandglue.js
>===================================================================
>-      dump("unsupported sort column: " + columnID + "\n");
>-      sortKey = 0;
>+

Dump that empty line.

>+      //no predefined column handler - lets check if there is a custom column handler

Space after "//", it's "let's" and probably just say "one" for the final "column handler".

>+      try {
>+        //try to grab the columnHandler (an error is thrown if it does not exist)

Space after "//".

>+        columnHandler = gDBView.getColumnHandler(columnID);
>+
>+        //it exists - save this column ID in the customSortCol property of dbFolderInfo
>+        //for later use (see nsIMsgDBView.cpp)

Space after each "//".

>+        gDBView.db.dBFolderInfo.setProperty('customSortCol', columnID);

"" instead of '', for consistency's sake.

>+        sortKey = nsMsgViewSortType.byCustom;
>+      }
>+      catch(err) {

Space after "catch".

>+    case nsMsgViewSortType.byCustom:
>+      //TODO: either change try() catch to if (property exists) or restore the getColumnHandler() check
>+      try { //getColumnHandler throws an errror when the ID is not handled
>+        columnID = gDBView.db.dBFolderInfo.getProperty('customSortCol');
>+      }

Spaces after "//", and both comments are just misleading: restore which check, whence? How should getColumnHandler throw something if it's not even called?

>+      catch (err) { //error - means no handler

Space after "//".

You should also add the fix from bug 359357 (attachment 244946 [details] [diff] [review]) while you're at it.

Comment 3

[Philip Chee]

> Spaces after "//", and both comments are just misleading: restore which check,
> whence? How should getColumnHandler throw something if it's not even called?

These comments appear to be left over from an earlier draft:

+      try //getColumnHandler throws an errror when the ID is not handled
+      {
+        columnID = gDBView.db.dBFolderInfo.getProperty('customSortCol');
+        //columnHandler = gDBView.getColumnHandler(columnID); //try to grab the columnHandler to make sure it exists
+      } 
+      
+      catch (err) { //error - means no handler
+        dump("ConvertSortTypeToColumnID: custom sort key but no handler for column '" + columnID + "'\n");
+        columnID = "dateCol";
+      }

Since the getColumnHandler line isn't in the patch I'll just remove those comments. Well actually the error message in the catch block, and the try/catch block itself now don't make sense if the getColumnHandler isn't called. So do I restore the getColumnHandler check or remove the try/catch block?

Comment 4

[Philip Chee]

Attachment: Custom Columns for MailNews: Patch v2

I've removed the try/catch block in ConvertSortTypeToColumnID() and changed it to:

+    case nsMsgViewSortType.byCustom:
+      columnID = gDBView.db.dBFolderInfo.getProperty("customSortCol");
+      if (!columnID) {
+        dump("ConvertSortTypeToColumnID: custom sort key but no handler for column\n");
+        columnID = "dateCol";
+      }
+      break;

So far I haven't seen any errors in the Error Console nor in the -console

Comment 5

[Philip Chee]

Attachment: Sample extension to test the patch

This is a modified version of the test extension found in Bug 359357 . Modified to work in SeaMonkey/XPFE and in SuiteRunner. I've also fixed the error generated by the extension by adding a NOOP getRowProperties method to the custom tree views.

Error: 'JavaScript component does not have a method named: "getRowProperties"'
when calling method: [nsIMsgCustomColumnHandler::getRowProperties] = NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED

Comment 7

[Bruno 'Aqualon' Escherl]

I saw too late, that you already tried to implement that :)

I had the same patch except one thing:

> +        columnHandler = gDBView.getColumnHandler(columnID);
Since columnHandler isn't defined anywhere, the assignment can be skipped.

Comment 8

[Philip Chee]

Attachment: Custom Columns for MailNews: Patch v2.1

I seem to have let this patch bit-rot without asking for sr. Updating.

Comment 9

[Philip Chee]

Attachment: Custom Columns for MailNews: Patch v2.2

on #irc <Mnyromyr> said: you could've dropped the "columnHandler = ", too

nits fixed

Comment 10

[[e-mail]]

Comment on attachment 276773 [details] [diff] [review]
 Custom Columns for MailNews: Patch v2.2

>+        //  try to grab the columnHandler (an error is thrown if it does not exist)
Nit: doubled space after //

>+        dump("ConvertSortTypeToColumnID: custom sort key but no handler for column\n");
That's not actually the error that you're testing for :-P

Comment 11

[Philip Chee]

Attachment: Patch with nits fixed for checkin

Fixed whitespace, description in dump statement.

Comment 12

[Karsten Düsterloh]

Landed attachment 276889 [details] [diff] [review] on trunk.