Desktop: Fixes #7834: Merged shorthand variants of spellcheck languages #9983

HahaBill · 2024-02-23T00:27:10Z

Summary

Introduction from the Joplin Forum: https://discourse.joplinapp.org/t/introducing-hahabill/36075/3

I fixed the issue regarding the multiple variants of the same spellcheck languages showing duplicate values in the title bar. Merged shorthand variants, e.g. [en-US, en-GB, en-AU] -> 'en'

added "spellCheckerMenu.test.ts" to test the changes and the expected behavior: all passed!
added jest.mock in "jest.setup.js" to mock the Menu from @electron/remote.
modified "buildScriptIndexes" to ignore ".test.ts" files so it does not include them in index.ts. That caused some issues with duplicate identifiers and more errors.

🐛 Fixes: #7834

Testing and Verification Process

1. To ensure the changes are working as expected:

Screen.Recording.2024-02-22.at.23.56.06.mov

2. Unit Testing: showSpellCheckerMenu.test.ts

Notes

I noticed that when I add multiple spellchecker languages, the block expands to the point that it covers the notebook's title.

This is out of the scope of this PR, but I would suggest maybe making the "spellchecker language" block in the title bar horizontally scrollable so that it does not cover the notebook title.

What do you think?
(this suggestion would be out of the scope of this PR, a new issue would be created)

Checklist:

Fixed the issue
Created test units
Complete and working PR

…e bar - Fixed the issue laurent22#7834 - added "spellCheckerMenu.test.ts" to test the changes and the expected behavior: all passed! - added jest.mock in "jest.setup.js" to mock the Menu from @electron/remote. - modified "buildScriptIndexes" to ignore ".test.ts" files so it does not include them in index.ts. That caused some issues with duplicate identifiers and more errors.

github-actions · 2024-02-23T00:27:26Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

…o dev

HahaBill · 2024-02-23T00:33:03Z

recheck

laurent22 · 2024-02-23T09:38:33Z

You need to accept the CLA.

laurent22 · 2024-02-23T09:40:59Z

packages/app-desktop/gui/MainScreen/commands/showSpellCheckerMenu.ts

@@ -30,8 +30,9 @@ export const runtime = (): CommandRuntime => {
 			const s: string[] = [];
 			// eslint-disable-next-line github/array-foreach -- Old code before rule was applied
 			languages.forEach((language: string) => {
-				s.push(language.split('-')[0]);


Obviously it was previously implemented this way for a reason, and you removed that feature.

So what's your reasoning, why do you think it was implemented this way, and is the best fix really to remove it without even a discussion?

Hi, I am sorry that I did not discuss my changes before. I saw in the docs that contributions should be first addressed and discussed in the GitHub issues or Joplin forum. In the future, I will be more aware of that and provide more explicit explanations.

I added only s.push(language) and removed s.push(language.split('-')[0] because to explicitly display the spellcheck languages with different variants like "en-AU, en-GB, en-US" instead of "en, en, en". This is something that tomasz1986 wanted to see and I agree that from the UI/UX perspective, it is better this way.

The changes should not affect SpellCheckerService.ts and the state: AppState.

I might have missed something based on your review, let me know!

Here are a few other options:

1. Only show the region code when it prevents duplicate values from being added to s

Another option might be to only include the region code when there are multiple copies of the same language.

For example,

languages = [ 'en-GB', 'en-US' ] => s = [ 'en-GB', 'en-US' ]

languages = [ 'en-GB', 'es-MX' ] => s = [ 'en', 'es' ].

2. Remove duplicates

For example,

languages = [ 'en', 'en', 'en' ] => s = [ 'en' ]

This is based on one of the list items in "describe what you expected to happen" in the original issue.

2. Remove duplicates

For example,

languages = [ 'en', 'en', 'en' ] => s = [ 'en' ]

This is based on one of the list items in "describe what you expected to happen" in the original issue.

As the author of the original issue, I actually like this approach a lot. It would allow for the language list to take less space, so that more title text could be displayed on the screen, which is precious especially on narrower displays.

I agree, the second solution is good in terms of usability and reduces clutter. @HahaBill, this is what I was trying to get at: first find out why something was done in a particular way. It was indeed to reduce clutter because putting the language as eg fr-FR or it-IT doesn't help the user.

So the idea would be to keep the clutter to a minimum while solving the original bug report. Actually both solutions 1 or 2 would work but indeed 2 seems better.

For me, the second solution also looks better than the first one, and everyone seems to lean towards that. The second solution it is! :)

(at sign)laurent22 I see now! That is a really good point and lesson learned. I will keep that in mind.

Here's the solution:

packages/app-desktop/jest.setup.js

HahaBill · 2024-02-23T14:30:44Z

I have read the CLA Document and I hereby sign the CLA

HahaBill · 2024-02-23T14:31:01Z

recheck

…nu.test.ts"

packages/lib/determineProfileDir.js

personalizedrefrigerator · 2024-02-23T14:47:13Z

packages/app-desktop/gui/MainScreen/commands/showSpellCheckerMenu.ts

@@ -30,8 +30,9 @@ export const runtime = (): CommandRuntime => {
 			const s: string[] = [];
 			// eslint-disable-next-line github/array-foreach -- Old code before rule was applied
 			languages.forEach((language: string) => {
-				s.push(language.split('-')[0]);


Here are a few other options:

1. Only show the region code when it prevents duplicate values from being added to s

Another option might be to only include the region code when there are multiple copies of the same language.

For example,

languages = [ 'en-GB', 'en-US' ] => s = [ 'en-GB', 'en-US' ]

languages = [ 'en-GB', 'es-MX' ] => s = [ 'en', 'es' ].

2. Remove duplicates

For example,

languages = [ 'en', 'en', 'en' ] => s = [ 'en' ]

This is based on one of the list items in "describe what you expected to happen" in the original issue.

personalizedrefrigerator · 2024-02-23T14:53:35Z

packages/tools/gulp/tasks/buildScriptIndexes.js

@@ -12,7 +12,7 @@ async function processDirectory(dir, indexFilePath = null, typeScriptType = null

 	const tsFiles = glob.sync('{**/*.ts,**/*.tsx}', {
 		cwd: dir,
-	}).filter(f => `${dir}/${f}` !== indexFilePath);
+	}).filter(f => `${dir}/${f}` !== indexFilePath && !f.endsWith('.test.ts'));


Suggested change

}).filter(f => `${dir}/${f}` !== indexFilePath && !f.endsWith('.test.ts'));

}).filter(f => `${dir}/${f}` !== indexFilePath)

// Exclude Jest test files to prevent them from being imported

// by index.ts.

.filter(f => !f.endsWith('.test.ts'));

(Optional) A brief comment might be helpful here.

I will do that. Thanks for sharing your implementation!

personalizedrefrigerator · 2024-02-23T14:59:23Z

This is out of the scope of this PR, but I would suggest maybe making the "spellchecker language" block in the title bar horizontally scrollable so that it does not cover the notebook title.

Because it's possible to click on the spellcheck button and show the full list of languages, I think limiting its size and using text-overflow: ellipsis (or something similar in JS) might make sense. I agree that this would probably be best in a separate PR.

tomasz1986 · 2024-02-23T15:26:17Z

This is out of the scope of this PR, but I would suggest maybe making the "spellchecker language" block in the title bar horizontally scrollable so that it does not cover the notebook title.

Because it's possible to click on the spellcheck button and show the full list of languages, I think limiting its size and using text-overflow: ellipsis (or something similar in JS) might make sense. I agree that this would probably be best in a separate PR.

I think there is an issue with the spellcheck button, as it only shows one language enabled in the menu that pops up on click,

even though there are actually four languages enabled. You need to go through the whole "Change language" list to see all of them.

HahaBill · 2024-02-23T21:38:14Z

This is out of the scope of this PR, but I would suggest maybe making the "spellchecker language" block in the title bar horizontally scrollable so that it does not cover the notebook title.

Because it's possible to click on the spellcheck button and show the full list of languages, I think limiting its size and using text-overflow: ellipsis (or something similar in JS) might make sense. I agree that this would probably be best in a separate PR.

Yeah, limiting the size and using something like text overflow makes much more sense. Let's talk about that more after this PR and see if other members agree.

Another example of using 3 languages where it doesn't look nice:

- Test case passed: ['en-GB', 'en-US', 'en-CA', 'es-ES', 'es-MX'] -> 'en, es'

- manually tested locally and the changes work!

HahaBill · 2024-02-23T23:05:06Z

I made all the necessary changes. I hope I got everything.

laurent22 · 2024-02-24T11:43:58Z

Notes

I noticed that when I add multiple spellchecker languages, the block expands to the point that it covers the notebook's title.

This is out of the scope of this PR, but I would suggest maybe making the "spellchecker language" block in the title bar horizontally scrollable so that it does not cover the notebook title.

What do you think? (this suggestion would be out of the scope of this PR, a new issue would be created)

Would you mind creating a new issue for this? As a fix we'll just make the languages not wrap. We don't need to do anything smart because the UI is not really usable at a very small size anyway.

HahaBill · 2024-02-25T10:44:26Z

Notes

I noticed that when I add multiple spellchecker languages, the block expands to the point that it covers the notebook's title.

This is out of the scope of this PR, but I would suggest maybe making the "spellchecker language" block in the title bar horizontally scrollable so that it does not cover the notebook title.
What do you think? (this suggestion would be out of the scope of this PR, a new issue would be created)

Would you mind creating a new issue for this? As a fix we'll just make the languages not wrap. We don't need to do anything smart because the UI is not really usable at a very small size anyway.

Right, also, by removing duplicates, we have partially solved the issue. I think overflowing the text should suffice. I am happy to create a new issue for that!

HahaBill · 2024-02-25T11:36:00Z

Notes

I noticed that when I add multiple spellchecker languages, the block expands to the point that it covers the notebook's title.

This is out of the scope of this PR, but I would suggest maybe making the "spellchecker language" block in the title bar horizontally scrollable so that it does not cover the notebook title.
What do you think? (this suggestion would be out of the scope of this PR, a new issue would be created)

Would you mind creating a new issue for this? As a fix we'll just make the languages not wrap. We don't need to do anything smart because the UI is not really usable at a very small size anyway.

Right, also, by removing duplicates, we have partially solved the issue. I think overflowing the text should suffice. I am happy to create a new issue for that!

Here's the issue: #9998

laurent22 · 2024-02-26T10:27:19Z

That looks good now, thanks everyone!

HahaBill · 2024-03-01T16:29:24Z

@PackElend label me please

HahaBill and others added 2 commits February 22, 2024 23:52

Merge branch 'laurent22:dev' into dev

d1e3652

HahaBill added 3 commits February 23, 2024 00:30

Fixed the build issue from GitHub Actions

9f2b42e

Merge branch 'dev' of https://github.com/HahaBill/joplin-hahabill int…

7f2c5d4

…o dev

Create determineProfileDir.js

a831916

HahaBill marked this pull request as ready for review February 23, 2024 00:46

laurent22 reviewed Feb 23, 2024

View reviewed changes

packages/app-desktop/jest.setup.js Outdated Show resolved Hide resolved

github-actions bot added a commit that referenced this pull request Feb 23, 2024

@HahaBill has signed the CLA in #9983

9a115c9

Desktop: Putting the mock back into the test file "showSpellCheckerMe…

8a936d6

…nu.test.ts"

personalizedrefrigerator reviewed Feb 23, 2024

View reviewed changes

Desktop: Implemented 2nd solution and modified unit test

e934d91

- Test case passed: ['en-GB', 'en-US', 'en-CA', 'es-ES', 'es-MX'] -> 'en, es'

HahaBill changed the title ~~Desktop: Fixes #7834: Provided more specific shorthand variants of spellcheck languages, e.g. en-AU, en-GB, en-US, etc.~~ Desktop: Fixes #7834: Merged shorthand variants of spellcheck languages Feb 23, 2024

HahaBill added 2 commits February 23, 2024 22:50

Deleting an auto-generated file: determineProfileDir.js

12ba64f

Desktop: More clarity on changes in "buildScriptIndexes.js"

d4b6926

- manually tested locally and the changes work!

laurent22 merged commit 64684dc into laurent22:dev Feb 26, 2024
10 checks passed

PackElend added the gsoc-2024 label Mar 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Desktop: Fixes #7834: Merged shorthand variants of spellcheck languages #9983

Desktop: Fixes #7834: Merged shorthand variants of spellcheck languages #9983

HahaBill commented Feb 23, 2024 •

edited

Loading

github-actions bot commented Feb 23, 2024 •

edited

Loading

HahaBill commented Feb 23, 2024

laurent22 commented Feb 23, 2024

laurent22 Feb 23, 2024

HahaBill Feb 23, 2024

HahaBill Feb 23, 2024 •

edited

Loading

personalizedrefrigerator Feb 23, 2024 •

edited

Loading

tomasz1986 Feb 23, 2024 •

edited

Loading

laurent22 Feb 23, 2024 •

edited

Loading

HahaBill Feb 23, 2024

HahaBill Feb 23, 2024

HahaBill commented Feb 23, 2024

HahaBill commented Feb 23, 2024

personalizedrefrigerator Feb 23, 2024 •

edited

Loading

personalizedrefrigerator Feb 23, 2024

HahaBill Feb 23, 2024

personalizedrefrigerator commented Feb 23, 2024

tomasz1986 commented Feb 23, 2024 •

edited

Loading

HahaBill commented Feb 23, 2024 •

edited

Loading

HahaBill commented Feb 23, 2024

laurent22 commented Feb 24, 2024

Notes

HahaBill commented Feb 25, 2024

Notes

HahaBill commented Feb 25, 2024

Notes

laurent22 commented Feb 26, 2024

HahaBill commented Mar 1, 2024

-	}).filter(f => `${dir}/${f}` !== indexFilePath && !f.endsWith('.test.ts'));
+	}).filter(f => `${dir}/${f}` !== indexFilePath)
+	// Exclude Jest test files to prevent them from being imported
+	// by index.ts.
+	.filter(f => !f.endsWith('.test.ts'));

Desktop: Fixes #7834: Merged shorthand variants of spellcheck languages #9983

Desktop: Fixes #7834: Merged shorthand variants of spellcheck languages #9983

Conversation

HahaBill commented Feb 23, 2024 • edited Loading

Summary

Testing and Verification Process

1. To ensure the changes are working as expected:

2. Unit Testing: showSpellCheckerMenu.test.ts

Notes

Checklist:

github-actions bot commented Feb 23, 2024 • edited Loading

HahaBill commented Feb 23, 2024

laurent22 commented Feb 23, 2024

laurent22 Feb 23, 2024

Choose a reason for hiding this comment

HahaBill Feb 23, 2024

Choose a reason for hiding this comment

HahaBill Feb 23, 2024 • edited Loading

Choose a reason for hiding this comment

personalizedrefrigerator Feb 23, 2024 • edited Loading

Choose a reason for hiding this comment

1. Only show the region code when it prevents duplicate values from being added to s

2. Remove duplicates

tomasz1986 Feb 23, 2024 • edited Loading

Choose a reason for hiding this comment

2. Remove duplicates

laurent22 Feb 23, 2024 • edited Loading

Choose a reason for hiding this comment

HahaBill Feb 23, 2024

Choose a reason for hiding this comment

HahaBill Feb 23, 2024

Choose a reason for hiding this comment

HahaBill commented Feb 23, 2024

HahaBill commented Feb 23, 2024

personalizedrefrigerator Feb 23, 2024 • edited Loading

Choose a reason for hiding this comment

1. Only show the region code when it prevents duplicate values from being added to s

2. Remove duplicates

personalizedrefrigerator Feb 23, 2024

Choose a reason for hiding this comment

HahaBill Feb 23, 2024

Choose a reason for hiding this comment

personalizedrefrigerator commented Feb 23, 2024

tomasz1986 commented Feb 23, 2024 • edited Loading

HahaBill commented Feb 23, 2024 • edited Loading

HahaBill commented Feb 23, 2024

laurent22 commented Feb 24, 2024

Notes

HahaBill commented Feb 25, 2024

Notes

HahaBill commented Feb 25, 2024

Notes

laurent22 commented Feb 26, 2024

HahaBill commented Mar 1, 2024

HahaBill commented Feb 23, 2024 •

edited

Loading

github-actions bot commented Feb 23, 2024 •

edited

Loading

HahaBill Feb 23, 2024 •

edited

Loading

personalizedrefrigerator Feb 23, 2024 •

edited

Loading

1. Only show the region code when it prevents duplicate values from being added to `s`

tomasz1986 Feb 23, 2024 •

edited

Loading

laurent22 Feb 23, 2024 •

edited

Loading

personalizedrefrigerator Feb 23, 2024 •

edited

Loading

1. Only show the region code when it prevents duplicate values from being added to `s`

tomasz1986 commented Feb 23, 2024 •

edited

Loading

HahaBill commented Feb 23, 2024 •

edited

Loading