FoundKeyGang/FoundKey
FoundKeyGang/FoundKey
7
19
Fork 22
Code Issues 37 Pull requests 4 Releases Activity

Add LibreTranslate support #224

Merged
norm merged 3 commits from libretranslate into main 2022-11-20 16:21:17 +00:00
Conversation 3 Commits 3 Files changed 11 +301 -60
norm commented 2022-11-01 06:03:24 +00:00
Owner
Copy link

Admins will need to specify the API endpoint of the LibreTranslate instance being used. An API key can be optionally specified if the instance of choice does any ratelimiting on non-authenticated requests.

To-do:

  • Add settings to Admin UI
  • Add support to backend

Resolves: #173
Changelog: Added

Admins will need to specify the API endpoint of the LibreTranslate instance being used. An API key can be optionally specified if the instance of choice does any ratelimiting on non-authenticated requests. To-do: - [x] Add settings to Admin UI - [x] Add support to backend Resolves: #173 Changelog: Added
norm force-pushed libretranslate from dd803e4c9b to f03e64d157 2022-11-01 06:05:03 +00:00 Compare
norm added a new dependency 2022-11-01 06:08:25 +00:00
#173 Add LibreTranslate support
norm commented 2022-11-01 06:22:14 +00:00
Author
Owner
Copy link

A few things I'm kinda wondering about:

  • Should the translation settings live in the meta table like they currently do, or should those be in the config file?
  • I imagine the languages supported by DeepL and LibreTranslate are going to be different. Currently the translate endpoint hardcodes the languages that DeepL supports.
    • Should there be some sort of endpoint that tells the client what languages are supported?
  • Also I'm kinda wondering if #33 should also be handled with this PR or not.
A few things I'm kinda wondering about: - Should the translation settings live in the `meta` table like they currently do, or should those be in the config file? - I imagine the languages supported by DeepL and LibreTranslate are going to be different. Currently the translate endpoint hardcodes the languages that DeepL supports. - Should there be some sort of endpoint that tells the client what languages are supported? - Also I'm kinda wondering if #33 should also be handled with this PR or not.
Johann150 reviewed 2022-11-02 20:00:38 +00:00
packages/client/src/pages/admin/settings.vue Outdated
@ -183,3 +201,3 @@
let swPublicKey: any = $ref(null);
let swPrivateKey: any = $ref(null);
let deeplAuthKey: string = $ref('');
let translationService: string = $ref('');
Johann150 commented 2022-11-02 20:00:38 +00:00
Owner
Copy link

Shouldn't this start out as a "valid" value, e.g. start out as 'none'?

Shouldn't this start out as a "valid" value, e.g. start out as `'none'`?
norm marked this conversation as resolved
norm force-pushed libretranslate from 494b9236db to 64fb594ddd 2022-11-17 04:38:43 +00:00 Compare
Johann150 reviewed 2022-11-17 20:21:43 +00:00
packages/backend/migration/1668661888188-add-libretranslate.js Outdated
@ -0,0 +2,4 @@
name = 'addLibretranslate1668661888188'
async up(queryRunner) {
await queryRunner.query(`CREATE TYPE "public"."meta_translationservice_enum" AS ENUM('none', 'deepl', 'libretranslate')`);
Johann150 commented 2022-11-17 20:21:43 +00:00
Owner
Copy link

Wouldn't it make more sense to use NULL instead of an explicit enum key?

Wouldn't it make more sense to use `NULL` instead of an explicit enum key?
norm commented 2022-11-17 21:22:16 +00:00
Author
Owner
Copy link

Yeah, that makes more sense.

Yeah, that makes more sense.
norm marked this conversation as resolved
norm force-pushed libretranslate from f2be6afd46 to 03338d35f2 2022-11-18 00:23:13 +00:00 Compare
norm force-pushed libretranslate from 03338d35f2 to c24ead3830 2022-11-18 00:26:25 +00:00 Compare
norm commented 2022-11-18 00:29:03 +00:00
Author
Owner
Copy link

This may need the fix from #239 to work properly since I've had some issues with actually getting the libretranslate settings to apply in the admin interface.
Fixed in 28aa440bcc

~~This may need the fix from https://akkoma.dev/FoundKeyGang/FoundKey/pulls/239 to work properly since I've had some issues with actually getting the libretranslate settings to apply in the admin interface.~~ Fixed in https://akkoma.dev/FoundKeyGang/FoundKey/commit/28aa440bcc967224964cadf141924f8960409c9a
norm force-pushed libretranslate from c24ead3830 to 3b2773c3fc 2022-11-18 20:20:19 +00:00 Compare
norm changed title from WIP: Add LibreTranslate support to Add LibreTranslate support 2022-11-18 20:20:31 +00:00
Johann150 requested changes 2022-11-18 21:14:10 +00:00
packages/backend/migration/1668661888188-add-libretranslate.js Outdated
@ -0,0 +2,4 @@
name = 'addLibretranslate1668661888188'
async up(queryRunner) {
await queryRunner.query(`CREATE TYPE "public"."meta_translationservice_enum" AS ENUM('deepl', 'libretranslate')`);
Johann150 commented 2022-11-18 21:08:15 +00:00
Owner
Copy link

I think this does not properly migrate the case when DeepL is already set up.

I think this does not properly migrate the case when DeepL is already set up.
Johann150 marked this conversation as resolved
packages/client/src/pages/admin/settings.vue Outdated
@ -131,3 +131,3 @@
<FormSection>
<template #label>DeepL Translation</template>
<template #label>{{ i18n.ts.translationSettings }}</template>
Johann150 commented 2022-11-18 21:14:03 +00:00
Owner
Copy link

I think this may be a bit hard to find. Maybe it should be on a separate settings page? Before commit 533955f928 it was on the "other" settings page.

I think this may be a bit hard to find. Maybe it should be on a separate settings page? Before commit 533955f928e7739fcec84adeca94ae36271298b1 it was on the "other" settings page.
norm commented 2022-11-18 22:46:58 +00:00
Author
Owner
Copy link

Not really sure why the new page doesn't seem to work properly, but I did add one just now.

Not really sure why the new page doesn't seem to work properly, but I did add one just now.
Johann150 marked this conversation as resolved
Johann150 approved these changes 2022-11-19 22:10:57 +00:00
packages/backend/src/server/api/endpoints/notes/translate.ts Outdated
@ -162,0 +209,4 @@
case TranslationService.DeepL:
return translateDeepL();
case TranslationService.LibreTranslate:
return translateLibreTranslate();
Johann150 commented 2022-11-19 22:10:53 +00:00
Owner
Copy link

I'm not sure if these need to be awaited?

I'm not sure if these need to be `await`ed?
norm commented 2022-11-19 22:12:55 +00:00
Author
Owner
Copy link

Probably a good idea.

Probably a good idea.
norm marked this conversation as resolved
norm commented 2022-11-19 22:11:20 +00:00
Author
Owner
Copy link

Seems like the new settings page decided to work all of a sudden today. Additional testing would be appreciated.

Seems like the new settings page decided to work all of a sudden today. Additional testing would be appreciated.
Johann150 requested changes 2022-11-19 22:28:33 +00:00
packages/backend/src/server/api/endpoints/admin/update-meta.ts Outdated
@ -58,2 +60,4 @@
deeplAuthKey: { type: 'string', nullable: true },
deeplIsPro: { type: 'boolean' },
libreTranslateAuthKey: { type: 'string', nullable: true },
libreTranslateEndpoint: { type: 'string', nullable: true },
Johann150 commented 2022-11-19 22:28:30 +00:00
Owner
Copy link

You need to actually add these to set so they are saved into the DB.

You need to actually add these to `set` so they are saved into the DB.
Johann150 commented 2022-11-19 22:33:59 +00:00
Owner
Copy link

Probably would also be a good idea to somehow make sure that you can't set translation service to something without the credentials for that being set up as well.

Probably would also be a good idea to somehow make sure that you can't set translation service to something without the credentials for that being set up as well.
norm marked this conversation as resolved
norm force-pushed libretranslate from 64fe6655f6 to e03428b83f 2022-11-19 22:48:42 +00:00 Compare
norm force-pushed libretranslate from e03428b83f to 8e818195b1 2022-11-19 23:00:32 +00:00 Compare
norm force-pushed libretranslate from 8e818195b1 to a66ef89059 2022-11-19 23:03:00 +00:00 Compare
norm force-pushed libretranslate from 62f9d43303 to cfe0f3ca67 2022-11-20 04:00:39 +00:00 Compare
Johann150 added 1 commit 2022-11-20 09:38:00 +00:00
fix: translator settings on admin/meta endpoint
Some checks failed
ci/woodpecker/push/build Pipeline was successful
Details
ci/woodpecker/push/lint-foundkey-js Pipeline was successful
Details
ci/woodpecker/push/lint-backend Pipeline was successful
Details
ci/woodpecker/push/lint-client Pipeline was successful
Details
ci/woodpecker/push/test Pipeline was successful
Details
ci/woodpecker/pr/lint-foundkey-js Pipeline was successful
Details
ci/woodpecker/pr/lint-backend Pipeline failed
Details
ci/woodpecker/pr/build Pipeline was successful
Details
ci/woodpecker/pr/lint-client Pipeline failed
Details
ci/woodpecker/pr/test Pipeline failed
Details
aefb11959f
Johann150 approved these changes 2022-11-20 09:38:26 +00:00
norm merged commit 512351746f into main 2022-11-20 16:21:17 +00:00
norm deleted branch libretranslate 2022-11-20 16:21:17 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Johann150
Labels
Clear labels   
feature

task that proposes new feature(s)

  
fix

task to fix behavioral problems in the FoundKey codebase

  
upkeep

task that should make FoundKey smaller and more maintainable

No labels
feature
fix
upkeep
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Blocks
#173 Add LibreTranslate support}
FoundKeyGang/FoundKey
Reference: FoundKeyGang/FoundKey#224
No description provided.
Delete branch "libretranslate"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?