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

Translate chat messages #9512

Merged
merged 3 commits into from May 12, 2023
Merged

Conversation

marcoambrosini
Copy link
Member

@marcoambrosini marcoambrosini commented May 10, 2023

☑️ Resolves

Requisites

🖼️ Screenshots

image
image

🚧 Tasks

  • Code review
  • Visual check
  • Handle rich content properly

🏁 Checklist

src/components/TranslateDialog.vue Outdated Show resolved Hide resolved
src/components/TranslateDialog.vue Outdated Show resolved Hide resolved
src/components/TranslateDialog.vue Outdated Show resolved Hide resolved
src/components/TranslateDialog.vue Outdated Show resolved Hide resolved
@Antreesy
Copy link
Contributor

Antreesy commented May 11, 2023

So, i get a successful responce on axios.get(generateOcsUrl('/translation/languages')):
{"ocs":{"meta":{"status":"ok","statuscode":200,"message":"OK"},"data":{"languages":[{"from":"de","fromLabel":"German","to":"en","toLabel":"English"},{"from":"en","fromLabel":"English","to":"de","toLabel":"German"}],"languageDetection":true}}}

But when trying to translate with axios.post(generateOcsUrl('/translation/translate'), get an error:
{"ocs":{"meta":{"status":"failure","statuscode":400,"message":""},"data":{"message":"Unable to translate","from":"en"}}}

src/components/TranslateDialog.vue Outdated Show resolved Hide resolved
src/components/TranslateDialog.vue Outdated Show resolved Hide resolved
src/components/TranslateDialog.vue Outdated Show resolved Hide resolved
Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
@Antreesy Antreesy force-pushed the feature/9273/translate-chat-messages branch from 120eaae to 8497bd7 Compare May 12, 2023 13:18
@Antreesy Antreesy changed the title Feature/9273/translate chat messages Translate chat messages May 12, 2023
@nimishavijay
Copy link
Member

I messed around a bit more with how this could look and I'd like to propose one more design change: use a border instead of a background color.

  • Give --border-radius-large border radius to both the texts
  • Use a --color-border border for the source text
  • Use a --color-primary-element border for the target text
  • use --color-main-background background for both the texts

what do you think? :)

Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
@Antreesy
Copy link
Contributor

what do you think? :)

Looks good and a bit less as Google Translate) Also user mentions are visible better now
image

Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
@Antreesy Antreesy enabled auto-merge May 12, 2023 16:03
Comment on lines +369 to +372
isTranslationAvailable: {
type: Boolean,
required: true,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the isTranslationAvailable always supposed to have value from capabilities?
If it is, maybe use capability instead of prop here? Or at least use capabilities as the default value. Then the long list of props to pass will be a little bit less 😀

Copy link
Contributor

Choose a reason for hiding this comment

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

Could be done later, when we'll finally reach Message refactoring =)

Comment on lines +223 to +234
async translateMessage(sourceLanguage = null) {
try {
this.isLoading = true
const response = await translateText(this.message, sourceLanguage, this.selectedTo?.id)
this.translatedMessage = response.data.ocs.data.text
} catch (error) {
console.error(error)
showError(error.response?.data?.ocs?.data?.message ?? t('spreed', 'The message could not be translated'))
} finally {
this.isLoading = false
}
},
Copy link
Contributor

@ShGKme ShGKme May 12, 2023

Choose a reason for hiding this comment

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

On the first modal open it always fails to me from the request in mounted:

Request:

fetch("https://<HOST>/ocs/v2.php/translation/translate", {
  "headers": {
    "accept": "application/json, text/plain, */*",
    "accept-language": "en-US,en;q=0.9,ru;q=0.8",
    "content-type": "application/json",
    "requesttoken": "<DELETED>",
    "sec-ch-ua": "\"Chromium\";v=\"112\", \"Not:A-Brand\";v=\"99\"",
    "sec-ch-ua-mobile": "?0",
    "sec-ch-ua-platform": "\"Windows\"",
    "sec-fetch-dest": "empty",
    "sec-fetch-mode": "cors",
    "sec-fetch-site": "same-origin"
  },
  "referrerPolicy": "no-referrer",
  "body": "{\"text\":\"Hallo!\",\"fromLanguage\":null,\"toLanguage\":\"en\"}",
  "method": "POST",
  "mode": "cors",
  "credentials": "include"
});

Response:

{"ocs":{"meta":{"status":"failure","statuscode":400,"message":""},"data":{"message":"Could not detect language"}}}

Copy link
Contributor

Choose a reason for hiding this comment

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

It's on API side then, we always send text and 'to' language

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, it seems, API is used correctly

Copy link
Member

Choose a reason for hiding this comment

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

That's what I said in the office. You send a single word and fromLanguage is null.
But on a single word language models don't work and respond with a language, so we don't have a from when talking to translations provider and it returns with could not translate

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should not show the error message in this case?
Otherwise, all small messages show an error toast on opening the translation modal.
Also, if only one language is available, we can pre-select it.

Copy link
Member

Choose a reason for hiding this comment

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

Well we could try to hide the error from empty runs yeah, but let's do that in a follow up

@Antreesy Antreesy merged commit 358d043 into master May 12, 2023
19 checks passed
@Antreesy Antreesy deleted the feature/9273/translate-chat-messages branch May 12, 2023 20:46
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.

Translate chat messages [web frontend]
5 participants