-
Notifications
You must be signed in to change notification settings - Fork 92
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
Signed-off-by: Luka Trovic <[email protected]>
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: |
📝 Summary
🏁 Checklist
npm run lint
/npm run stylelint
/composer run cs:check
)