895085 - "unsupported sort key: 17" output from Thunderbird

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[ISHIKAWA, Chiaki]

Attachment: Fix to suppress printing of "unsupported key: 17" for "not sorted" folder.

unsupported sort key: 17

During the run of DEBUG BUILD of thunderbird, I get an output like
the following 34 times.

unsupported sort key: 17


I tracked down to this output to the following code in
ConvertSortTypeToColumnID() in 
https://mxr.mozilla.org/comm-central/source/mail/base/content/commandglue.js

See line 261

187 function ConvertSortTypeToColumnID(sortKey)
188 {
189   var columnID;
190 
191   // Hack to turn this into an integer, if it was a string.
192   // It would be a string if it came from localStore.rdf
193   sortKey = sortKey - 0;
194 
195   switch (sortKey) {
196     case nsMsgViewSortType.byDate:
197       columnID = "dateCol";
198       break;
199     case nsMsgViewSortType.byReceived:
200       columnID = "receivedCol";
201       break;

	  ...

244     case nsMsgViewSortType.byAttachments:
245       columnID = "attachmentCol";
246       break;
247     case nsMsgViewSortType.byCustom:
248 
249       //TODO: either change try() catch to if (property exists) or restore the getColumnHandler() check
250       try //getColumnHandler throws an errror when the ID is not handled
251       {
252         columnID = gDBView.curCustomColumn;
253       }
254       catch (err) { //error - means no handler
255         dump("ConvertSortTypeToColumnID: custom sort key but no handler for column '" + columnID + "'\n");
256         columnID = "dateCol";
257       }
258 
259       break;
260     default:
261       dump("unsupported sort key: " + sortKey + "\n");   <------ HERE
262       columnID = "dateCol";
263       break;
264   }
265   return columnID;
266 }


Basically, the code is printing "unsupported sort key:" message
since the key is not handled in the switch statement.

But then what is this "17", then?

I looked for the defines for the values used in case labels.
They are defined in the following file.

https://mxr.mozilla.org/comm-central/source/mailnews/base/public/nsIMsgDBView.idl#60

60 interface nsMsgViewSortType
61 {
62   const nsMsgViewSortTypeValue byNone = 0x11; /* not sorted */
63   const nsMsgViewSortTypeValue byDate = 0x12;
64   const nsMsgViewSortTypeValue bySubject = 0x13;
     ...
79   const nsMsgViewSortTypeValue byCustom = 0x22;
80   const nsMsgViewSortTypeValue byReceived = 0x23;
81 };


17 is 0x11, the value of |byNone|.
So the code was complaining that a folder (or is it a newsgroup?)
is "not sorted" somehow.

I don't know the life cycle of a folder, but somehow such "not sorted"
folder is created during the test run of "make mozill" of thunderbird.
(And for that matter, I see this unsupported key: 17 message 
during my use of TB at the office. Usually, I see it when I do a
global search across folders. So maybe one or more folders in my setup have this "not sorted" property somehow.)

When a folder is "not sorted", the code sets the sortKey |columnID| to
"dateCol", which seems a reasonable default.
So maybe we don't have to print out the warning in the case of
|byNone|.
A fix is suggested as attached.

Since the code is very old(?) and I could not find meaningful hg blame
output, I am requesting review from Daivd ([e-mail])

TIA


Actually there is similar code in SeaMonkey as well.
I searched the string using the following command and got the output
as below. 
find . \( -name ".hg" -prune \) -o \( -type f -print \)  | xargs egrep "unsupported sort key"
./mail/base/content/commandglue.js:      dump("unsupported sort key: " + sortKey + "\n");
./suite/mailnews/commandglue.js:      dump("unsupported sort key: " + sortKey + "\n");

The same fix is needed for both
TB and SeaMonkey. So I am attaching the patch for both of them.

PPS: I noticed during the run of "make mozmill" test suite of
DEBUG BUILD of thunderbird. 

PPPS: Patch was checked in TB tryserver submission.
(No new problems were found other than the permanent orange type of errors.)

https://tbpl.mozilla.org/?rev=13f6001f5c01&tree=Thunderbird-Try

PPPPS: If a folder should not have "not sorted" property, then
maybe this warning output is useful.
But as far as I know, my office TB usage printed out this warning for a long time (at least a few years). So it seems OK to have "not sorted" property for folders.

Comment 1

[:aceman]

Comment on attachment 777337 [details] [diff] [review]
Fix to suppress printing of "unsupported key: 17" for "not sorted" folder.

Great catch:) I was also seeing this warning.

Comment 2

[ISHIKAWA, Chiaki]

(In reply to :aceman from comment #1)
> Comment on attachment 777337 [details] [diff] [review]
> Fix to suppress printing of "unsupported key: 17" for "not sorted" folder.
> 
> Great catch:) I was also seeing this warning.

Thank you for fixing the MIME format of the uploaded file. The unbearable heat in Tokyo area distracted me obviously.

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 777337 [details] [diff] [review]
Fix to suppress printing of "unsupported key: 17" for "not sorted" folder.

Review of attachment 777337 [details] [diff] [review]:
-----------------------------------------------------------------

r=Standard8 for the mail/ part with the comment added (please find a separate reviewer for the suite/ part).

::: mail/base/content/commandglue.js
@@ +192,5 @@
>    // It would be a string if it came from localStore.rdf
>    sortKey = sortKey - 0;
>  
>    switch (sortKey) {
> +    case nsMsgViewSortType.byNone:

We should add a comment here such as:

// In the case of None, we default to the date column
// This appears to be the case in such instances as
// Global search, so don't complain about it.

Comment 4

[ISHIKAWA, Chiaki]

(In reply to Mark Banner (:standard8) from comment #3)
> Comment on attachment 777337 [details] [diff] [review]
> Fix to suppress printing of "unsupported key: 17" for "not sorted" folder.
> 
> Review of attachment 777337 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=Standard8 for the mail/ part with the comment added (please find a
> separate reviewer for the suite/ part).
> 
> ::: mail/base/content/commandglue.js
> @@ +192,5 @@
> >    // It would be a string if it came from localStore.rdf
> >    sortKey = sortKey - 0;
> >  
> >    switch (sortKey) {
> > +    case nsMsgViewSortType.byNone:
> 
> We should add a comment here such as:
> 
> // In the case of None, we default to the date column
> // This appears to be the case in such instances as
> // Global search, so don't complain about it.

Thank you for your review.

I have decided to create a new bugzilla entry for the suite portion so that the fix here can be applied first, and then when I find some reviewer (I will post an inquiry to mozilla.dev.apps.seamonkey) for
suite part, he/she can decide to pick up the patch, etc.

TIA

TB-only patch with your comment follows.

Comment 5

[ISHIKAWA, Chiaki]

Attachment: Final patch (split for TB only) (carrying over r=Standard8+)

See previous comment.
Suite portion is split into another bug.

TIA

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/e7fd81e33b3d