Skip to content

Fix incorrect preferred locale selection with multiple languages configured #70

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

Merged
fluiddot merged 5 commits into from
May 6, 2024
Merged

Fix incorrect preferred locale selection with multiple languages configured #70

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fa84beb
Use `getLocale` to determine the supported locale
fluiddot Apr 30, 2024
497f9d0
Remove `getPreferredSystemLanguages` as it's no longer needed
fluiddot Apr 30, 2024
a058684
Update unit tests related to supported locale
fluiddot Apr 30, 2024
e5ff9ee
Merge branch 'trunk' into fix/supported-locale-based-on-app-locale
fluiddot May 6, 2024
34e7f3c
Remove unneeded electron mock
fluiddot May 6, 2024
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/__mocks__/electron.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ export const app = {
getFetch: jest.fn(),
getPath: jest.fn( ( name ) => `/path/to/app/${ name }` ),
getName: jest.fn( () => 'App Name' ),
getPreferredSystemLanguages: jest.fn( () => [ 'en-US' ] ),
getLocale: jest.fn( () => 'en-US' ),
};

export const BrowserWindow = {
Expand Down
12 changes: 6 additions & 6 deletions src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,6 @@ const onAuthorizationCallback = ( mainWindow: BrowserWindow | null, url: string
async function appBoot() {
let mainWindow: BrowserWindow | null = null;

const locale = getSupportedLocale();
const localeData = getLocaleData( locale );
defaultI18n.setLocaleData( localeData?.locale_data?.messages );

app.setName( packageJson.productName );

Menu.setApplicationMenu( null );
Expand Down Expand Up @@ -178,13 +174,17 @@ async function appBoot() {
}

app.on( 'ready', async () => {
// Set translations based on supported locale
const locale = getSupportedLocale();
const localeData = getLocaleData( locale );
defaultI18n.setLocaleData( localeData?.locale_data?.messages );
Comment on lines +177 to +180

@fluiddot fluiddot Apr 30, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The supported locale uses app.getLocale that is only available when the app is read.

console.log( `App version: ${ app.getVersion() }` );
console.log( `Built from commit: ${ COMMIT_HASH ?? 'undefined' }` );
console.log( `Local timezone: ${ Intl.DateTimeFormat().resolvedOptions().timeZone }` );
console.log( `App locale: ${ app.getLocale() }` );
console.log( `System locale: ${ app.getSystemLocale() }` );
console.log( `Preferred languages: ${ app.getPreferredSystemLanguages() }` );
console.log( `Used language: ${ getSupportedLocale() }` );
console.log( `Used language: ${ locale }` );

// By default Electron automatically approves all permissions requests (e.g. notifications, webcam)
// We'll opt-in to permissions we specifically need instead.
Expand Down
19 changes: 4 additions & 15 deletions src/lib/locale.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,22 +26,11 @@ const supportedLocales = [
'zh-tw',
];

export function getPreferredSystemLanguages() {
if ( process.platform === 'linux' && process.env.NODE_ENV !== 'test' ) {
// app.getPreferredSystemLanguages() is implemented by g_get_language_names on Linux.
// See: https://developer-old.gnome.org/glib/unstable/glib-I18N.html#g-get-language-names
// The language tags returned by this system function are in a format like "en_US" or "en_US.utf8".
// When these sorts of tags are passed to Intl.getCanonicalLocales() it throws an error.
return app
.getPreferredSystemLanguages()
.filter( ( lang ) => supportedLocales.includes( lang ) );
}

return app.getPreferredSystemLanguages();
}

export function getSupportedLocale(): string {
return match( getPreferredSystemLanguages(), supportedLocales, DEFAULT_LOCALE );
// `app.getLocale` returns the current application locale, acquired using
// Chromium's `l10n_util` library. This value is utilized to determine
// the best fit for supported locales.
return match( [ app.getLocale() ], supportedLocales, DEFAULT_LOCALE );
}

export function getLocaleData( locale: string ): LocaleData | null {
Expand Down
47 changes: 9 additions & 38 deletions src/lib/tests/locale.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,71 +5,42 @@ import { app } from 'electron';
import { createI18n } from '@wordpress/i18n';
import { getLocaleData, getSupportedLocale } from '../locale';

jest.mock( 'electron', () => ( {
app: {
getPreferredSystemLanguages: jest.fn().mockReturnValue( [ 'en-US' ] ),
},
} ) );

function mockPreferredLanguages( languages: string[] ) {
( app.getPreferredSystemLanguages as jest.Mock ).mockReturnValue( languages );
function mockAppLocale( language: string ) {
( app.getLocale as jest.Mock ).mockReturnValue( language );
}

describe( 'getSupportedLocale', () => {
it( 'converts a language-region pair to a glotpress locale slug', () => {
mockPreferredLanguages( [ 'en-US' ] );
mockAppLocale( 'en-US' );

expect( getSupportedLocale() ).toBe( 'en' );
} );

it( 'returns English if preferred language is unsupported', () => {
mockPreferredLanguages( [ 'mi-NZ' ] );
it( 'returns English if app locale is unsupported', () => {
mockAppLocale( 'mi-NZ' );

expect( getSupportedLocale() ).toBe( 'en' );
} );

it( "falls back to lesser preferred languages if the most preferred isn't supported", () => {
mockPreferredLanguages( [ 'mi-NZ', 'fr-FR', 'en-US' ] );

expect( getSupportedLocale() ).toBe( 'fr' );
} );

it( 'ignores region if the best match is a matching language with a different region', () => {
mockPreferredLanguages( [ 'mi-NZ', 'pt-PT' ] );

expect( getSupportedLocale() ).toBe( 'pt-br' );
} );

it( "prefers an exact language-region match, even if it's lower in the preference order", () => {
mockPreferredLanguages( [ 'mi-NZ', 'pt-PT', 'zh-CN' ] );

expect( getSupportedLocale() ).toBe( 'zh-cn' );
} );
Comment on lines -31 to -47

@fluiddot fluiddot Apr 30, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We no longer need to test preferred languages as we don't use it.

it( 'returns zh-cn variant', () => {
mockPreferredLanguages( [ 'zh-cn', 'zh-tw' ] );
mockAppLocale( 'zh-cn' );

expect( getSupportedLocale() ).toBe( 'zh-cn' );
} );

it( 'returns zh-tw variant', () => {
mockPreferredLanguages( [ 'zh-tw', 'zh-cn' ] );
mockAppLocale( 'zh-tw' );

expect( getSupportedLocale() ).toBe( 'zh-tw' );
} );

it( "prefers a language with a different region over an exact language _and_ region match which is further down the user's preference list", () => {
mockPreferredLanguages( [ 'fr-PL', 'pt-BR' ] );
expect( getSupportedLocale() ).toBe( 'fr' );
} );

it( 'returns the Simplified Chinese zh-cn option when the user preference is zh-Hans', () => {
mockPreferredLanguages( [ 'zh-NZ', 'zh-Hans' ] );
mockAppLocale( 'zh-Hans' );
expect( getSupportedLocale() ).toBe( 'zh-cn' );
} );

it( 'returns the Traditional Chinese zh-tw option when the user preference is zh-Hant', () => {
mockPreferredLanguages( [ 'zh-Hant' ] );
mockAppLocale( 'zh-Hant' );
expect( getSupportedLocale() ).toBe( 'zh-tw' );
} );
} );
Expand Down
19 changes: 6 additions & 13 deletions src/lib/tests/site-language.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ import { getPreferredSiteLanguage } from '../site-language';

const originalFetch = global.fetch;

function mockPreferredLanguages( languages: string[] ) {
( app.getPreferredSystemLanguages as jest.Mock ).mockReturnValue( languages );
function mockAppLocale( language: string ) {
( app.getLocale as jest.Mock ).mockReturnValue( language );
}

function mockFetchTranslations( wpVersion: string, translations: string[] ) {
Expand Down Expand Up @@ -62,15 +62,15 @@ describe( 'getPreferredSiteLanguage', () => {
];

it( "returns 'en' as default language", async () => {
mockPreferredLanguages( [] );
mockAppLocale( 'mi-NZ' );

expect( await getPreferredSiteLanguage() ).toBe( 'en' );
} );

it.each( LATEST_WP_VERSION_LOCALES )(
"returns '$expected' for language '$locale'",
async ( { locale, expected } ) => {
mockPreferredLanguages( [ locale ] );
mockAppLocale( locale );

expect( await getPreferredSiteLanguage() ).toBe( expected );
}
Expand Down Expand Up @@ -108,17 +108,10 @@ describe( 'getPreferredSiteLanguage', () => {
{ locale: 'zh-Hant-TW', expected: 'en' },
];

it( "returns 'en' as default language", async () => {
mockPreferredLanguages( [] );
mockFetchTranslations( WP_VERSION, AVAILABLE_LOCALES );

expect( await getPreferredSiteLanguage( WP_VERSION ) ).toBe( 'en' );
} );
Comment on lines -111 to -116

@fluiddot fluiddot Apr 30, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This test case is no longer needed as app.getLocale always returns a value.

it.each( WP_5_0_LOCALES )(
"returns '$expected' for language '$locale'",
async ( { locale, expected } ) => {
mockPreferredLanguages( [ locale ] );
mockAppLocale( locale );
mockFetchTranslations( WP_VERSION, AVAILABLE_LOCALES );

expect( await getPreferredSiteLanguage( WP_VERSION ) ).toBe( expected );
Expand All @@ -130,7 +123,7 @@ describe( 'getPreferredSiteLanguage', () => {
/* NOOP */
} );

mockPreferredLanguages( WP_5_0_LOCALES.map( ( item ) => item.locale ) );
mockAppLocale( WP_5_0_LOCALES[ 0 ].locale );

@fluiddot fluiddot Apr 30, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We pick the first locale but any valid locale should work.
mockFetchTranslations( 'unknown', [] );

expect( await getPreferredSiteLanguage( WP_VERSION ) ).toBe( 'en' );
Expand Down