-
Notifications
You must be signed in to change notification settings - Fork 95
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: replace deprecated translation methods #6567
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't dive too deep into the changes, but a quick testing didn't work as expected. While the fake providers provided by the testing
app worked, the providers provided by the translation
app were not listed.
What I did to reproduce:
- Disable app
testing
- Enable apps
assistant
andtranslate
- Run
occ translate:download-models
When I load the translation modal from the assistant menu with this PR applied, the dropdowns "translate from" and "translate to" are empty and don't provide any language. Maybe I missed something?
@mejo- As I understand, the assistant app cannot work without testing app (or AI provider apps). So, disabling testing app means disabling assistant and translation. So, I think the test case doesn't make sense to me. |
There is also translate2, not sure if just an addition or successor. translate doesn't implement the new API. @marcelklehr From an integration team perspective, is it work to keep both APIs available or is just going with the task processing API and ignoring the old translation API fine. |
In my opinion, we should go with the task processing API and ignore the old translation API since it might be complicated to maintain when keeping both APIs. |
We opted to implement neither forward nor backward compatibility for the old Translation API in TaskProcessing. The translate2 app only uses the TaskProcessing API, while the translate app uses the old Translation API. I'd recommend to transition to TaskProcessing directly if you don't need to support nc < 30, as the old Translation API is already deprecated and you will need to transition at some point. If it's too much work for you to add support for task processing, we can talk about adding forward and backward compatibility in server, but I'd rather avoid that. |
@luka-nextcloud I'm still unable to successfully test this using real translation providers. Did you test this with real providers, or just with the testing app? What I did:
Problems:
When opening the assistant modal directly in the header bar (not via Text), translation works as expected, so I guess there's something wrong with the implementation in Text. This is how the dropdowns for selecting languages in the translation modal via Text look in my tests: |
Signed-off-by: Luka Trovic <[email protected]>
e879703
to
08e25e1
Compare
@mejo- I have tested with both apps |
Note: integration_openai also implements a translation provider |
/backport! to stable31 |
Installed successulfy now. Translation menu entry in the AI menu doesn't do anything on click:
|
I can share credentials for easier testing with the integration_openai provider, just ping me if needed |
That looks like you did not compile it yet. Could you try again? We can also jump on a call next week to try it together. |
Yeah, let's do that :) |
notes from test with new build: On opening:
when selecting a new "from language":
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested this with @marcelklehr and the integration worked fine. There were two errors in the logs:
- When changing the
fromLanguage
whiletoLanguage
was not set yet - When clearing the
toLanguage'
.
Both pointed to line 157 of Tranlate.vue
. I'll propose a change that will fix them.
In addition it would be good to also clear the result when the fromLanguage
is changed - to allow for a recomputation.
Other than that this looks fine. I'll approve so you can fix the remaining things and then merge @luka-nextcloud
fromLanguage(newVal) { | ||
if (newVal.id === this.toLanguage.id) { | ||
this.toLanguage = null | ||
} | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Fix the errors thrown in line 157 by handling
null
asnewVal
andthis.toLanguage
well. - When changing
fromLanguage
- also clear the result so it can be recomputed.
fromLanguage(newVal) { | |
if (newVal.id === this.toLanguage.id) { | |
this.toLanguage = null | |
} | |
}, | |
fromLanguage(newVal) { | |
if (newVal?.id === this.toLanguage?.id) { | |
this.toLanguage = null | |
} | |
this.result = '' | |
this.error = null | |
}, |
📝 Summary
🏁 Checklist
npm run lint
/npm run stylelint
/composer run cs:check
)✔️ TODO