Desktop: Fixes #10077 - Special characters in notebooks and tags are not sorted alphabetically #10085

cagnusmarlsen · 2024-03-07T20:33:56Z

Summary

Fixes the issue where the names of notebooks and tags are not sorted alphabetically if the name starts with a special character.

Fixes #10077

Testing

Follow reproduction steps in #10077:

Notes: When alphabetical sort order is applied, "normal" and special characters are sorted correctly.
Notebooks and Tags: Verify that both normal and special characters are sorted correctly here as well.

This has been tested successfully on Windows 11 and Ubuntu.

roman-r-m · 2024-03-07T20:56:35Z

packages/app-desktop/gui/TagList.tsx

@@ -29,9 +29,9 @@ function TagList(props: Props) {

 	const tags = useMemo(() => {
 		const output = props.items.slice();
-
+		const collator = new Intl.Collator(undefined, { numeric: true, sensitivity: 'base' });


does it not need to know the locale? looking at the MDN page, it has this example:

// in German, ä sorts with a console.log(new Intl.Collator("de").compare("ä", "z")); // -1, or some other negative value // in Swedish, ä sorts after z console.log(new Intl.Collator("sv").compare("ä", "z")); // 1, or some other positive value

how does your change handle it?

The original issue stated that the names were sorted correctly for Notes and the problem only occurred for Notebooks and Tags.

Assuming that the sorting is fine for Notes, I took a similar approach here. Looking at the code for Notes, I don't think that the locale is being passed as a parameter to Intl.Collator during sorting, which is why I think your example wouldn't work for Notes either.

https://github.com/laurent22/joplin/blob/1b96a165866f3f9ad3499f45e01097e224797a27/packages/lib/models/Note.ts#L1121C2-L1123C3

public static getNaturalSortingCollator() { return new Intl.Collator(undefined, { numeric: true, sensitivity: 'base' }); }

If we want to take the locale into consideration, the changes would have to be made for Notes as well, which I thought was out of the scope of this issue.

If that is something that we actually want to do, I could try to make those changes test it with your example.

Yeah, I'm not sure what's the right thing here.

From what I can see the original issue was only for Turkish language. It can be that the sorting method used for folders produces correct result for Turkish by accident but isn't correct for some other language. Unfortunately, I don't think there's an easy way to check, except maybe ask on the forum.

Hey, I made some changes to the code to also consider the current locale before we do the sorting. I made it so that the notes, folders and tags are all sorted according to the current locale of the system. I tested for German, Swedish and some other languages and it seems to be working correctly, changing the sort order based on the locale. Some screenshots -

For German, ä sorts with a -

For Swedish, ä sorts after z -

I think this approach makes more sense and is more inclusive than the previous commit.

roman-r-m · 2024-03-08T21:03:41Z

packages/app-desktop/gui/TagList.tsx

@@ -13,6 +14,7 @@ interface Props {
 }

 function TagList(props: Props) {
+	const collatorLocale = Setting.value('locale').slice(0, 2);


Consider using one of the functions in packages/lib/locale.ts

roman-r-m · 2024-03-08T21:09:48Z

I wonder if there are other places we order things by title and if it makes sense to apply the same change there. I think there's one in ResourceScreen where it sorts resources by name.
There's also some sorting going on in sortFolderTree in Folder.ts
Also in setTags.ts

May be out of scope for this change though.

JackGruber · 2024-03-09T08:39:21Z

I think it should be noted that this also needs to be adapted for the mobile app. Not that these have different sorting again like in the last issue: #9996

laurent22 · 2024-03-09T11:14:32Z

@cagnusmarlsen, marph91 also addressed this issue in this pull request #10081 but used localeCompare instead, however as a result it doesn't address sorting numbers (1, 10, 2 instead of 1, 2, 10). Could you maybe collaborate with him to see what might the complete solution?

cagnusmarlsen · 2024-03-09T12:13:48Z

Hey laurent, Overall, I think this PR already addresses the following issues,

Sorts numbers properly.
Sorts Notes, Folders and Tags based on the current locale (as per the discussion I had above).

I took a look at marph91's pull request and I think the reason the numbers are not sorted is because the numeric argument was not passed to localeCompare. Plus, I think localCompare also accepts a locale argument. Their PR should work otherwise.

marph91 · 2024-03-09T12:23:58Z

Since the scope gets bigger and bigger, I guess it makes time-wise sense for me to give up my PR and just review the changes here.

I could collect the following requirements:

Sort special characters on desktop correctly for notebooks, tags (and notes). This was the original scope and works at both PR.
Sort numbers correctly. For example 1, 2, 10 instead of 1, 10, 2 (reference). This doesn't work at my PR, even when using the numeric argument. Not sure about yours.
Make sure the mobile app sorts everything in the same way as the desktop app (reference). I mentioned two potential changes here.
Check for other places, where the sorting could be applied (reference).

Not sure if all of them are needed to get the PR accepted, though.

NB: I added tests for 1. and 2. in fd29f7f. You can cherry-pick them if you like.

cagnusmarlsen · 2024-03-09T13:00:17Z

marph91, Thanks for this, I have cherry picked the tests that you wrote.
And yeah, I think the scope has gotten bigger than what the original issue mentioned. As mentioned previously, this PR addresses the following points -

Sorts the Notes, folders and tags based on the currentLocale, while taking care of special characters.
Sorts the numbers properly.

I am also not sure if the rest of the changes that you mentioned should be added to this PR or not, I think the main points of the original issue have been addressed.

packages/lib/models/Folder.test.ts

cagnusmarlsen · 2024-03-09T14:05:50Z

I don't know why the build failed, seems like a weird issue. The logs say this -

/home/runner/work/joplin/joplin/packages/app-desktop/gui/JoplinCloudLoginScreen.tsx:30:8 - Unknown word (applicatio)

Edit: Seems like its a spellchecker issue here -

joplin/packages/app-desktop/gui/JoplinCloudLoginScreen.tsx

Line 30 in d7401d7

const applicatioAuthId = useMemo(() => uuidgen(), []);

marph91 · 2024-03-09T14:34:52Z

It fails on the dev branch, too. I think it's not relevant for this PR.

laurent22 · 2024-03-14T18:22:01Z

packages/lib/models/Folder.ts

+
+

No white space changes please

laurent22 · 2024-03-14T18:25:41Z

packages/app-desktop/gui/TagList.tsx

@@ -13,6 +14,7 @@ interface Props {
 }

 function TagList(props: Props) {
+	const collatorLocale = currentLocale().slice(0, 2);


I think you misunderstood Roman's comment - there are functions in lib/locale to parse these strings. For example the languageName function and you should use this instead of string manipulation

laurent22 · 2024-03-14T18:26:57Z

packages/lib/models/Note.ts

@@ -1125,7 +1126,8 @@ export default class Note extends BaseItem {
 	}

 	public static getNaturalSortingCollator() {
-		return new Intl.Collator(undefined, { numeric: true, sensitivity: 'base' });
+		const collatorLocale = currentLocale().slice(0, 2);
+		return new Intl.Collator(collatorLocale, { numeric: true, sensitivity: 'base' });


You've added the same code in 4 different places. Could you move all this to a function and call it from those 4 places please? A good place for the function could be lib/models/utils for example

Sounds good. I will make these changes

marph91 · 2024-03-17T09:05:18Z

packages/lib/components/shared/side-menu-shared.ts

@@ -72,6 +73,8 @@ export const renderFolders = (props: Props, renderItem: RenderFolderItem) => {

 export const renderTags = (props: Props, renderItem: RenderTagItem) => {
 	const tags = props.tags.slice();
+	const collatorLocale = getCollatorLocale();
+	const collator = getCollator(collatorLocale);


You could remove even more duplicated code by calling getCollatorLocale() inside getCollator(). Either as default parameter or inside the function.

I agree. getCollatorLocale is used only by getCollator so not really needed

I have made it so that getCollatorLocale() is passed as the default parameter to getCollator().

…plin into sort-notebooks-and-tags

laurent22 · 2024-03-20T11:18:32Z

That looks good, thanks @cagnusmarlsen, and thank @roman-r-m and @marph91 for reviewing!

laurent22#10077 - Updated sorting for Folders and tags using collator

01dbbb8

roman-r-m reviewed Mar 7, 2024

View reviewed changes

cagnusmarlsen added 2 commits March 8, 2024 02:40

Tests on lib

a624568

laurent22#10077 - sort notebooks and tags based on locale

55d4421

roman-r-m reviewed Mar 8, 2024

View reviewed changes

roman-r-m added the gsoc-2024 label Mar 8, 2024

used currentLocale instead of Setting.value

13a2821

add tests for folders

3655863

marph91 mentioned this pull request Mar 9, 2024

Desktop: Fixes #10077: Special characters in notebooks and tags are not sorted alphabetically #10081

Closed

updated to use accent in collator

ab8ae4a

marph91 reviewed Mar 9, 2024

View reviewed changes

packages/lib/models/Folder.test.ts Show resolved Hide resolved

Merge branch 'dev' into sort-notebooks-and-tags

17e6ec3

laurent22 reviewed Mar 14, 2024

View reviewed changes

cagnusmarlsen and others added 3 commits March 17, 2024 00:13

moved collator to a function

4c3f7a0

removed comment

3bf52f5

Merge branch 'dev' into sort-notebooks-and-tags

fa04fa1

marph91 reviewed Mar 17, 2024

View reviewed changes

cagnusmarlsen and others added 3 commits March 17, 2024 18:56

removed duplication

8569d53

Merge branch 'sort-notebooks-and-tags' of github.com:cagnusmarlsen/jo…

86ef315

…plin into sort-notebooks-and-tags

Merge branch 'laurent22:dev' into sort-notebooks-and-tags

59e5a6d

laurent22 merged commit e9ebd84 into laurent22:dev Mar 20, 2024
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Desktop: Fixes #10077 - Special characters in notebooks and tags are not sorted alphabetically #10085

Desktop: Fixes #10077 - Special characters in notebooks and tags are not sorted alphabetically #10085

cagnusmarlsen commented Mar 7, 2024

roman-r-m Mar 7, 2024

cagnusmarlsen Mar 7, 2024

roman-r-m Mar 7, 2024

cagnusmarlsen Mar 8, 2024

roman-r-m Mar 8, 2024

roman-r-m commented Mar 8, 2024

JackGruber commented Mar 9, 2024

laurent22 commented Mar 9, 2024

cagnusmarlsen commented Mar 9, 2024 •

edited

marph91 commented Mar 9, 2024 •

edited

cagnusmarlsen commented Mar 9, 2024

cagnusmarlsen commented Mar 9, 2024 •

edited

marph91 commented Mar 9, 2024

laurent22 Mar 14, 2024

laurent22 Mar 14, 2024

laurent22 Mar 14, 2024

cagnusmarlsen Mar 14, 2024

marph91 Mar 17, 2024

laurent22 Mar 17, 2024

cagnusmarlsen Mar 17, 2024

laurent22 commented Mar 20, 2024

Desktop: Fixes #10077 - Special characters in notebooks and tags are not sorted alphabetically #10085

Desktop: Fixes #10077 - Special characters in notebooks and tags are not sorted alphabetically #10085

Conversation

cagnusmarlsen commented Mar 7, 2024

Summary

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roman-r-m commented Mar 8, 2024

JackGruber commented Mar 9, 2024

laurent22 commented Mar 9, 2024

cagnusmarlsen commented Mar 9, 2024 • edited

marph91 commented Mar 9, 2024 • edited

cagnusmarlsen commented Mar 9, 2024

cagnusmarlsen commented Mar 9, 2024 • edited

marph91 commented Mar 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurent22 commented Mar 20, 2024

cagnusmarlsen commented Mar 9, 2024 •

edited

marph91 commented Mar 9, 2024 •

edited

cagnusmarlsen commented Mar 9, 2024 •

edited