Closed Bug 935958 Opened 11 years ago Closed 10 years ago

"unsupported sort key: 17" output from Seamonkey

Categories

(SeaMonkey :: General, defect)

defect
Not set
normal

Tracking

(seamonkey2.24 affected, seamonkey2.25 fixed, seamonkey2.26 fixed, seamonkey2.27 fixed)

RESOLVED FIXED
seamonkey2.27
Tracking Status
seamonkey2.24 --- affected
seamonkey2.25 --- fixed
seamonkey2.26 --- fixed
seamonkey2.27 --- fixed

People

(Reporter: ishikawa, Assigned: ishikawa)

References

Details

Attachments

(1 file)

+++ This bug was initially created as a clone of Bug #895085 +++

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 (mozilla@davidbienvenu.org)

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.

---
Doesn't have r+
Assignee: nobody → ishikawa
Keywords: checkin-needed
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #1)
> Doesn't have r+

Sorry, I should have cloned the bugzilla before I gave checkin-needed to Bug 895085.

I am duplicating this bug since the original bugzilla entry tried to
address the same issue for both
 - TB, and
 - Seamonkey.

Exactly the same JS file was affected by the original bugzilla.

The patch for TB portion was approved, but the reviewer wanted to have
a reviewer for suite portion, and so I split the bugzilla (I should have split it
before I gave the original checkin-needed keyword. I didn't realize this was duped as well.)

The original takes care of TB issue only, and 
this bugzilla entry takes care of Seamonkey issue only.

I have uploaded the patch, which is basically the same patch to be
applied TB, here for Seamonkey.

I will find out who the reviewer should be.

TIA
Summary: "unsupported sort key: 17" output from Thunderbird → "unsupported sort key: 17" output from Seamonkey
> Exactly the same JS file was affected by the original bugzilla.

I should have said 
A file used in TB has exactly the same content as a file, under different name,
used by Seamonkey and both files ought to be fixed.

TIA
Attachment #828622 - Flags: review?(neil)
Comment on attachment 828622 [details] [diff] [review]
Fix to suppress printing of "unsupported key: 17" for "not sorted" folder. (same patch has been approved for TB already).

Adding r? IanN (or whoever gets to review this first)
moa? Mnyromyr.
Attachment #828622 - Flags: superreview?(mnyromyr)
Attachment #828622 - Flags: review?(iann_bugzilla)
Comment on attachment 828622 [details] [diff] [review]
Fix to suppress printing of "unsupported key: 17" for "not sorted" folder. (same patch has been approved for TB already).

I'm not a fan of this approach, although I see the custom column handler code already defaults to dateCol when the sort type is custom but no custom sort key was defined, so I don't know what to say here. (Then again, we should never see a sort key of byNone in SeaMonkey as we don't have that particular search.)
Attachment #828622 - Flags: review?(neil)
(In reply to ISHIKAWA, Chiaki from comment #2)
[…]
> I will find out who the reviewer should be.
> 
> TIA

In case you ever come back to fix other bugs in SeaMonkey (one can always hope ;-) ), I recommend that you bookmark the following two pages: the list of reviewers by module at http://www.seamonkey-project.org/dev/project-areas and the policies about review, superreview, et al. at http://www.seamonkey-project.org/dev/review-and-flags
Comment on attachment 828622 [details] [diff] [review]
Fix to suppress printing of "unsupported key: 17" for "not sorted" folder. (same patch has been approved for TB already).

r/sr=me
[Triage Comment]
a=me for c-a so it lands on the same branches that it did for TB assuming the uplift has happened.
Attachment #828622 - Flags: superreview?(mnyromyr)
Attachment #828622 - Flags: superreview+
Attachment #828622 - Flags: review?(iann_bugzilla)
Attachment #828622 - Flags: review+
Attachment #828622 - Flags: approval-comm-aurora+
Hi ISHIKAWA-san. Did you ever check this in? I don't see this in comm-central, and I did not see a [check-in needed] keyword.
Flags: needinfo?(ishikawa)
(In reply to Philip Chee from comment #8)
> Hi ISHIKAWA-san. Did you ever check this in? I don't see this in
> comm-central, and I did not see a [check-in needed] keyword.

Oh, my. I have obviously forgotten to put the [check-in needed] keyword!
I am just about to do it.
Sorry for my oversight, and thank you for noticing this!

TIA
Flags: needinfo?(ishikawa)
Keywords: checkin-needed
Pushed:
https://hg.mozilla.org/comm-central/rev/8cff2e56d482
https://hg.mozilla.org/releases/comm-aurora/rev/301c46466eac
https://hg.mozilla.org/releases/comm-beta/rev/632d073bfcc5

Missed the uplift so this patch will only make it to SeaMonkey 2.25. Sorry about that.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.27
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.