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

Initial libsoup async implementation #236

Merged
merged 17 commits into from
Feb 23, 2022
Merged

Initial libsoup async implementation #236

merged 17 commits into from
Feb 23, 2022

Conversation

rafaelmardojai
Copy link
Member

@rafaelmardojai rafaelmardojai commented Feb 6, 2022

Changes to use libsoup async functions. Now most Translator methods should just process data.

TODO

  • Rewrite the history code that uses trans_queue
  • Rewrite suggestions
  • Rewrite api key
  • Test and fix/implement error handing
  • Optimize the code, various parts have been written just to work
  • Cleanup old code

Notes and open questions

With this implementation all the JSON processing code is run in the main thread, test if this doesn't affect the app fluidity. If so, maybe we can rewrite Session.multiple() and create other helper functions to run the processing callbacks threaded.

We need to figure out what to do with the TTS code. We don't have time to re-implement gTTS. Should we just leave it as is?

It's fine to make requests from Translator.__init__()? Or should we figure out a way to make them from DialectWindow.load_translator() and make Translator() only provide the needed urls and processing functions?

It's Session.multiple() fine or should we make it run one task after another?

Changes to use libsoup async functions on main cases.
Now most `Translator` methods should just process data.
@rafaelmardojai rafaelmardojai marked this pull request as draft February 6, 2022 17:42
@mufeedali
Copy link
Member

With this implementation all the JSON processing code is run in the main thread, test if this doesn't affect the app fluidity. If so, maybe we can rewrite Session.multiple() and create other helper functions to run the processing callbacks threaded.

Doesn't seem to cause any slow downs so far for me.

We need to figure out what to do with the TTS code. We don't have time to re-implement gTTS. Should we just leave it as is?

Yeah, probably better to just leave it as is. Proxy users get broken TTS (lol) but not much we can do there. gTTS uses requests IIRC, so maybe it'll just work?

It's fine to make requests from Translator.init()? Or should we figure out a way to make them from DialectWindow.load_translator() and make Translator() only provide the needed urls and processing functions?

As long as we're using the Session helper class and every request is from the same thread, I don't think it really matters much.

It's Session.multiple() fine or should we make it run one task after another?

Should be fine.

@mufeedali
Copy link
Member

Rewrite the history code that uses trans_queue

Seems like it works just fine with a minor change.

@mufeedali
Copy link
Member

mufeedali commented Feb 15, 2022

So, there are some issues...

  • Live Translation is somewhat broken. I can't figure out the exact reason, but it breaks on fast input or when quickly erasing text.
  • The API keys validation method we implemented doesn't work as intended because LibreTranslate doesn't give an error if we use the wrong API key when API keys are optional. (Will be fixed if and/or when app: Fail when giving invalid API keys LibreTranslate/LibreTranslate#215 is merged.)
  • The settings UI doesnt update appropriately when instance has been reset (i.e. it doesn't hide the api key field).

There are others but I'll have to make sure, so I'll post them later.

@rafaelmardojai
Copy link
Member Author

The settings UI doesnt update appropriately when instance has been reset (i.e. it doesn't hide the api key field).

Fixed this and made me remove an unnecessary request when validating.

rafaelmardojai and others added 3 commits February 18, 2022 14:01
Some api servers might require it
- Now checks if data received is valid.
- The approach itself is still unfortunately broken.
- Remove unnecessary import, fix styling.
@mufeedali
Copy link
Member

mufeedali commented Feb 19, 2022

That should fix all the weird re-translation issues. Hopefully, for good.

Overall, this is very much working out fine for me. I think it might be the right direction to go.

- Separate user selection and programmatic selection.
- Give a function for programmatic selection and give option to not
  cause retranslation. i.e. to not emit user-selection-changed
- Throw away no_retranslate.
Remove unused imports and functions
Add custom exceptions to handle possible translators errors
Show better error messages when loading and translating
@mufeedali mufeedali marked this pull request as ready for review February 23, 2022 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants