1439900 - Move unifinder related functions to calUnifinderUtils.jsm

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

This is a fully manual change aimed at moving the unifinder related methods out of calUtils. I've consolidated some of the methods to make the file more compact, since these methods will probably not be used externally anyway.

Comment 1

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Fix - v1

Comment 2

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Successful try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=6bf2037ffe4e067c007a0e4fe0dd6eb94c986c84&selectedJob=164056900


(note that try may look like it doesn't have all bugs, but there was a previous failed try run, so the newest one isn't showing all changesets. See https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ae663d89c9caf8d2f0917f5c83168d8540100ea1&selectedJob=163954434 for the previous run)

Comment 3

[[:MakeMyDay]]

Comment on attachment 8952701 [details] [diff] [review]
Fix - v1

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

r+ with the comments below considered. On top of this, I would appreciate if you can add unit tests for the functions in calendarUnifinderUtils, at least for the two you reworked.

::: calendar/base/modules/calUnifinderUtils.jsm
@@ +9,5 @@
> +
> +this.EXPORTED_SYMBOLS = ["calunifinder"]; /* exported calunifinder */
> +
> +var calunifinder = {
> +    getItemSortKey: function(aItem, aKey, aStartTime) {

Please add doc blocks for the four functions in calUnifinderUtils.jsm

@@ +79,5 @@
> +
> +    sortEntryComparer: function(sortType) {
> +        const compNameMap = {
> +            number: function(a, b, modifier) {
> +                return compNameMap.general(Number(a), Number(b));

please add the modifier when calling general.

@@ +105,5 @@
> +                if (a.length == 0 || b.length == 0) {
> +                    // sort empty values to end (so when users first sort by a
> +                    // column, they can see and find the desired values in that
> +                    // column without scrolling past all the empty values).
> +                    return -(a.length - b.length);

This doesn't neccessarily provide -1, 0 or 1. Just use

return compNameMap.number(a.length, b.length, modifier);

::: calendar/base/modules/calUtilsCompat.jsm
@@ +87,5 @@
> +        sortEntryComparer: "sortEntryComparer",
> +        getItemSortKey:  "getItemSortKey",
> +        getSortTypeForSortKey: "getSortTypeForSortKey",
> +        // compareNative*, compareNumber, sortEntry, sortEntryItem,
> +        // sortEntryKey is no longer available. There is a new

...are no longer available

Comment 4

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to [:MakeMyDay] from comment #3)

> Please add doc blocks for the four functions in calUnifinderUtils.jsm
Done, also added this file to eslint.


> @@ +105,5 @@
> > +                if (a.length == 0 || b.length == 0) {
> > +                    // sort empty values to end (so when users first sort by a
> > +                    // column, they can see and find the desired values in that
> > +                    // column without scrolling past all the empty values).
> > +                    return -(a.length - b.length);
> 
> This doesn't neccessarily provide -1, 0 or 1. Just use
> 
> return compNameMap.number(a.length, b.length, modifier);
I believe it doesn't have to, this is also the scoring that was used before. .sort() will happily accept a value -3 and consider it the same as -1. I did add the missing modifier though.

Looking into unit tests next.

Comment 5

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: bug-1439900-move-unifinder-related-functions-to-calunifinderutilsjsm-v2.diff

This was a useful exercise, I found two regressions that were avoided. I've also removed another of the functions to condense the code, and moved the comptor map into global scope so it isn't re-created on each run.

There are also unit tests now.

Comment 6

[[:MakeMyDay]]

Comment on attachment 8962259 [details] [diff] [review]
bug-1439900-move-unifinder-related-functions-to-calunifinderutilsjsm-v2.diff

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

Thanks, looks good.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/e4188d5b03a3
Move unifinder related functions to calUnifinderUtils.jsm. r=MakeMyDay

Comment 8

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

https://hg.mozilla.org/releases/comm-beta/7b17cda8efa8
Bug 1439900 - Move unifinder related functions to calUnifinderUtils.jsm. r=MakeMyDay

Comment 9

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/releases/comm-beta/rev/7b17cda8efa8
Bug 1439900 - Move unifinder related functions to calUnifinderUtils.jsm. r=MakeMyDay