Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 9 commits into from
Feb 26, 2024
Merged

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b07903c
Desktop: Made the same spellcheck languages more specific in the titl…
HahaBill Feb 22, 2024
d1e3652
Merge branch 'laurent22:dev' into dev
HahaBill Feb 23, 2024
9f2b42e
Fixed the build issue from GitHub Actions
HahaBill Feb 23, 2024
7f2c5d4
Merge branch 'dev' of https://github.com/HahaBill/joplin-hahabill int…
HahaBill Feb 23, 2024
a831916
Create determineProfileDir.js
HahaBill Feb 23, 2024
8a936d6
Desktop: Putting the mock back into the test file "showSpellCheckerMe…
HahaBill Feb 23, 2024
e934d91
Desktop: Implemented 2nd solution and modified unit test
HahaBill Feb 23, 2024
12ba64f
Deleting an auto-generated file: determineProfileDir.js
HahaBill Feb 23, 2024
d4b6926
Desktop: More clarity on changes in "buildScriptIndexes.js"
HahaBill Feb 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .eslintignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,7 @@ packages/app-desktop/gui/MainScreen/commands/showNoteProperties.js
packages/app-desktop/gui/MainScreen/commands/showPrompt.js
packages/app-desktop/gui/MainScreen/commands/showShareFolderDialog.js
packages/app-desktop/gui/MainScreen/commands/showShareNoteDialog.js
packages/app-desktop/gui/MainScreen/commands/showSpellCheckerMenu.test.js
packages/app-desktop/gui/MainScreen/commands/showSpellCheckerMenu.js
packages/app-desktop/gui/MainScreen/commands/toggleEditors.js
packages/app-desktop/gui/MainScreen/commands/toggleLayoutMoveMode.js
Expand Down
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,7 @@ packages/app-desktop/gui/MainScreen/commands/showNoteProperties.js
packages/app-desktop/gui/MainScreen/commands/showPrompt.js
packages/app-desktop/gui/MainScreen/commands/showShareFolderDialog.js
packages/app-desktop/gui/MainScreen/commands/showShareNoteDialog.js
packages/app-desktop/gui/MainScreen/commands/showSpellCheckerMenu.test.js
packages/app-desktop/gui/MainScreen/commands/showSpellCheckerMenu.js
packages/app-desktop/gui/MainScreen/commands/toggleEditors.js
packages/app-desktop/gui/MainScreen/commands/toggleLayoutMoveMode.js
Expand Down
40 changes: 40 additions & 0 deletions packages/app-desktop/gui/MainScreen/commands/showSpellCheckerMenu.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
import { runtime } from './showSpellCheckerMenu';
import { AppState } from '../../../app.reducer';

describe('mapStateTotitle', () => {

test('should return null if spellchecker.enabled is false', () => {

const mockState: Partial<AppState> = {
settings: {
'spellChecker.enabled': false,
'spellChecker.languages': ['en-GB'],
},
};
const result = runtime().mapStateToTitle(mockState);
expect(result).toBeNull();
});

test('should return null if spellChecker.languages is empty', () => {
const mockState: Partial<AppState> = {
settings: {
'spellChecker.enabled': true,
'spellChecker.languages': [],
},
};
const result = runtime().mapStateToTitle(mockState);
expect(result).toBeNull();
});

test('should return list of countryDisplayName with correct format', () => {
const mockState: Partial<AppState> = {
settings: {
'spellChecker.enabled': true,
'spellChecker.languages': ['en-GB', 'en-US', 'en-CA', 'es-ES', 'es-MX'],
},
};
const result = runtime().mapStateToTitle(mockState);
expect(result).toBe('en-GB, en-US, en-CA, es-ES, es-MX');

});
});
3 changes: 2 additions & 1 deletion packages/app-desktop/gui/MainScreen/commands/showSpellCheckerMenu.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@HahaBill HahaBill Feb 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@tomasz1986 tomasz1986 Feb 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

@laurent22 laurent22 Feb 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's the solution:

Screenshot 2024-02-23 at 22 41 55

s.push(language);
});

return s.join(', ');
},
};
Expand Down
12 changes: 12 additions & 0 deletions packages/app-desktop/jest.setup.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,18 @@ jest.mock('@electron/remote', () => {
};
});

// Mocking bridge().Menu for showSpellCheckerMenu.test.ts
jest.mock('./services/bridge', () => ({
__esModule: true,
default: () => ({
Menu: {
buildFromTemplate: jest.fn().mockReturnValue({
popup: jest.fn(),
}),
},
}),
}));

laurent22 marked this conversation as resolved.
Show resolved Hide resolved
// Import after mocking problematic libraries
const { afterEachCleanUp, afterAllCleanUp } = require('@joplin/lib/testing/test-utils.js');

Expand Down
16 changes: 16 additions & 0 deletions packages/lib/determineProfileDir.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
'use strict';
Object.defineProperty(exports, '__esModule', { value: true });
personalizedrefrigerator marked this conversation as resolved.
Show resolved Hide resolved
const os_1 = require('os');
const path_utils_1 = require('./path-utils');
exports.default = (profileFromArgs, appName) => {
let output = '';
if (profileFromArgs) {
output = profileFromArgs;
} else if (process && process.env && process.env.PORTABLE_EXECUTABLE_DIR) {
output = `${process.env.PORTABLE_EXECUTABLE_DIR}/JoplinProfile`;
} else {
output = `${(0, os_1.homedir)()}/.config/${appName}`;
}
return (0, path_utils_1.toSystemSlashes)(output, 'linux');
};
// # sourceMappingURL=determineProfileDir.js.map
2 changes: 1 addition & 1 deletion packages/tools/gulp/tasks/buildScriptIndexes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will do that. Thanks for sharing your implementation!


tsFiles.sort();

Expand Down
Loading