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.

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

fix: Use the user configured UI language for PDFjs instead of browser language #1022

Merged
merged 2 commits into from
Aug 23, 2024

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Aug 22, 2024

PDFjs uses the browser language by default, but users should have consistently the Nextcloud defined language. So now we set the PDFjs language to:

  1. The Nextcloud configured language if supported
  2. The unflavored version if supported (e.g. de instead of de-DE)
  3. Fall back to browser UI language

We need 3 because if we set an unsupported language PDFjs will default to English, which is worse than the browser UI language.

Screen recording

My Nextcloud language: Spanish
My Browser UI: German

Before

Bildschirmaufnahme_20240822_190219.webm

After

Bildschirmaufnahme_20240822_183724.webm

@susnux susnux requested review from skjnldsv and danxuliu and removed request for skjnldsv August 22, 2024 16:56
@susnux susnux added this to the Nextcloud 31 milestone Aug 22, 2024
@susnux
Copy link
Contributor Author

susnux commented Aug 22, 2024

/compile amend/

@susnux
Copy link
Contributor Author

susnux commented Aug 22, 2024

/backport to stable30

@susnux
Copy link
Contributor Author

susnux commented Aug 22, 2024

/backport to stable29

@susnux
Copy link
Contributor Author

susnux commented Aug 22, 2024

/backport to stable28

susnux added 2 commits August 23, 2024 14:38
… language

PDFjs uses the browser language by default, but users should have consistently the Nextcloud defined language.
So now we set the PDFjs language to:
1. The Nextcloud configured language if supported
2. The unflavored version if supported (e.g. `de` instead of `de-DE`)
3. Fall back to browser UI language

We need 3 because if we set an unsupported lanuage PDFjs will default to English,
which is worse than the browser UI language.

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
@susnux susnux force-pushed the fix/use-user-language branch from e37bce4 to 6c641b2 Compare August 23, 2024 12:52
@susnux susnux merged commit 1b1af33 into master Aug 23, 2024
37 checks passed
@susnux susnux deleted the fix/use-user-language branch August 23, 2024 13:02
Copy link

backportbot bot commented Aug 23, 2024

The backport to stable28 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable28
git pull origin stable28

# Create the new backport branch
git checkout -b backport/1022/stable28

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts, resolve them
git cherry-pick 9078680f 6c641b2f

# Push the cherry pick commit to the remote repository and open a pull request
git push origin backport/1022/stable28

Error: Failed to clone repository: Failed to checkout branches: error: Your local changes to the following files would be overwritten by checkout:
js/files_pdfviewer-main.js.map.license
js/files_pdfviewer-node_modules_nextcloud_dialogs_dist_chunks_index-CWnkpNim_mjs.js.map.license
js/files_pdfviewer-public.js.map.license
js/files_pdfviewer-vendors-node_modules_nextcloud_dialogs_dist_chunks_FilePicker-kaHdQqaz_mjs.js.map.license
js/files_pdfviewer-workersrc.js.map.license
Please commit your changes or stash them before you switch branches.
Aborting


Learn more about backports at https://docs.nextcloud.com/server/stable/go.php?to=developer-backports.

Copy link

backportbot bot commented Aug 23, 2024

The backport to stable29 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable29
git pull origin stable29

# Create the new backport branch
git checkout -b backport/1022/stable29

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts, resolve them
git cherry-pick 9078680f 6c641b2f

# Push the cherry pick commit to the remote repository and open a pull request
git push origin backport/1022/stable29

Error: Failed to clone repository: Failed to checkout branches: error: Your local changes to the following files would be overwritten by checkout:
js/files_pdfviewer-main.js.map.license
js/files_pdfviewer-node_modules_nextcloud_dialogs_dist_chunks_index-CWnkpNim_mjs.js.map.license
js/files_pdfviewer-public.js.map.license
js/files_pdfviewer-vendors-node_modules_nextcloud_dialogs_dist_chunks_FilePicker-kaHdQqaz_mjs.js.map.license
js/files_pdfviewer-workersrc.js.map.license
Please commit your changes or stash them before you switch branches.
Aborting


Learn more about backports at https://docs.nextcloud.com/server/stable/go.php?to=developer-backports.

Copy link

backportbot bot commented Aug 23, 2024

The backport to stable30 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable30
git pull origin stable30

# Create the new backport branch
git checkout -b backport/1022/stable30

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts, resolve them
git cherry-pick 9078680f 6c641b2f

# Push the cherry pick commit to the remote repository and open a pull request
git push origin backport/1022/stable30

Error: Failed to clone repository: Failed to checkout branches: error: Your local changes to the following files would be overwritten by checkout:
js/files_pdfviewer-node_modules_nextcloud_dialogs_dist_chunks_index-CWnkpNim_mjs.js.map.license
js/files_pdfviewer-vendors-node_modules_nextcloud_dialogs_dist_chunks_FilePicker-kaHdQqaz_mjs.js.map.license
Please commit your changes or stash them before you switch branches.
Aborting


Learn more about backports at https://docs.nextcloud.com/server/stable/go.php?to=developer-backports.

@ostasevych
Copy link
Contributor

Hey, so I cannot understand the solution. How to translate the strings, update them?

Is transifex supported?

I have modified template file where the strings are localised, plus the Ukrainian version. I can help to migrate main languages to the standard template file, if needed.

After each update I manually correct the template file, add the localiser strings. It is a bit annoying thing. So if no automatised solution found I am thinking just to fork the app.

@susnux
Copy link
Contributor Author

susnux commented Aug 24, 2024

@ostasevych We decided to use the PDFjs translations, but we fixed the language detection, see the closed issue.
Also to improve here @danxuliu is about to upgrade to PDFjs v4 which provides much better translation, even for languages not supported by Nextcloud.

@susnux
Copy link
Contributor Author

susnux commented Aug 24, 2024

Ref: #1022

@ostasevych
Copy link
Contributor

@ostasevych We decided to use the PDFjs translations, but we fixed the language detection, see the closed issue.
Also to improve here @danxuliu is about to upgrade to PDFjs v4 which provides much better translation, even for languages not supported by Nextcloud.

Why? Why not to go in line with nc standards as other apps do?

Which framework to use in order to improve the translations? How to get know if new strings appear, eg transifex informs on that?

@susnux
Copy link
Contributor Author

susnux commented Aug 24, 2024

Why? Why not to go in line with nc standards as other apps do?

This app basically wraps the PDFjs library, a lot of strings are set internally.
So not everything could be translated as not every string is inside the template but is loaded from the JS.

Which framework to use in order to improve the translations? How to get know if new strings appear, eg transifex informs on that?

PDFjs is a mozilla library and uses the Mozilla L10n project for translations, I think all Mozilla products are translated at protoon: https://pontoon.mozilla.org/teams/

@susnux
Copy link
Contributor Author

susnux commented Aug 24, 2024

BTW if you are looking for Ukrainian: It is nearly 100% translated
image

@ostasevych
Copy link
Contributor

ostasevych commented Aug 24, 2024

BTW if you are looking for Ukrainian: It is nearly 100% translated !

I am a volunteer translator in Ukrainian, that is why I am so dissatisfied with the way how this app is built in terms of localisation :(
I feel that I have to continue locally my own way of localising the app with the NC traditional way: just replacing the code manually, adding uk.js and uk.json files to the l10n folder from version to version...

@ostasevych
Copy link
Contributor

ostasevych commented Aug 24, 2024

Don't know where to find those translations for PDFjs

image

Meh, even troubles with logging in there ...

@susnux
Copy link
Contributor Author

susnux commented Aug 24, 2024

Don't know where to find those translations for PDFjs

PDFjs is part of Firefox so the translations are also part of the firefox translation project.

I feel that I have to continue locally my own way of localising the app with the NC traditional way: just replacing the code manually, adding uk.js and uk.json files to the l10n folder from version to version...

But this does not fully work, as the strings are not all in the template, but also within the Javascript code of the library.

@susnux
Copy link
Contributor Author

susnux commented Aug 24, 2024

BTW do you experience any translation issues? (Since the merged fix, so with current master branch)

@ostasevych
Copy link
Contributor

ostasevych commented Aug 24, 2024

BTW do you experience any translation issues? (Since the merged fix, so with current master branch)

Nothing changed with 28.0.9. Instead of Ukrainian I see all strings in English. It is not working completely so I have to change the template and add my translations to my cloud.

viewer.zip
uk_js.zip

Ahhh! What did you do? Now my localisation is not working anymore. First time it shows in Ukrainian and in a second switches to English. :/

@susnux
Copy link
Contributor Author

susnux commented Aug 25, 2024

Nothing changed with 28.0.9.

No it is just in current master branch.

If I set Nextcloud language to Ukrainian and open a PDF file it is now working out of the box:
Bildschirmfoto_20240826_000053

@ostasevych
Copy link
Contributor

Nothing changed with 28.0.9.

No it is just in current master branch.

If I set Nextcloud language to Ukrainian and open a PDF file it is now working out of the box:

Ah, sorry. So, should I manually download and install the app from master branch?

@ostasevych
Copy link
Contributor

ostasevych commented Aug 25, 2024

I am getting error when running npm ci && npm run dev:

Building files_pdfviewer 4.0.0 

[webpack-cli] Failed to load '/www/html/nextcloud/apps/files_pdfviewer/webpack.config.js' config
[webpack-cli] /www/html/nextcloud/apps/files_pdfviewer/build-js/WebpackSPDXPlugin.js:46
	async #findPackage(dir) {
	                  ^

SyntaxError: Unexpected token '('
    at wrapSafe (internal/modules/cjs/loader.js:915:16)
    at Module._compile (internal/modules/cjs/loader.js:963:27)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1027:10)
    at Module.load (internal/modules/cjs/loader.js:863:32)
    at Function.Module._load (internal/modules/cjs/loader.js:708:14)
    at Module.require (internal/modules/cjs/loader.js:887:19)
    at require (internal/modules/cjs/helpers.js:85:18)
    at Object.<anonymous> (/www/html/nextcloud/apps/files_pdfviewer/webpack.config.js:9:27)
    at Module._compile (internal/modules/cjs/loader.js:999:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1027:10)

So, cannot replace the one app, which is provided with 28.0.9, which doesn't show Ukrainian strings (.

@ostasevych
Copy link
Contributor

ostasevych commented Aug 25, 2024

So, I have manually replaced js subfolder from git cloned archive in the app installed from the AppStore with the one, which is in the main branch, checked and it works.
What I would like to ask you to fix is viewer.css file, as it shows big Cyrillic symbols, so I changed the font size (see my viewer.css attached upper in the thread).

@susnux
Copy link
Contributor Author

susnux commented Aug 25, 2024

should I manually download and install the app from master branch?

Just until it is also fixed for 28 and 29 :)

@ostasevych
Copy link
Contributor

Again, no localisation in 28.0.10. The language is still English.

@susnux
Copy link
Contributor Author

susnux commented Sep 30, 2024

Again, no localisation in 28.0.10. The language is still English.

Yes it is only backported to stable30 and stable29. So you would have to update to at least 29.

@ostasevych
Copy link
Contributor

Again, no localisation in 28.0.10. The language is still English.

Yes it is only backported to stable30 and stable29. So you would have to update to at least 29.

What is wrong with backporting to 28.x?..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Localisation is not working properly.
3 participants