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

Adds translate status feature (when connected to a Mastodon 4.x server with translation enabled) #252

Closed
wants to merge 4 commits into from

Conversation

danschwarz
Copy link
Collaborator

I've submitted a patch to the mailing list, but hopefully this is cleaner/easier to review.

Implementation as per mastodon/mastodon#19218
and mastodon/mastodon#19328

Screenshots of feature in action.

Screenshot 2022-12-06 192326

Screenshot 2022-12-06 192407

Copy link
Owner

@ihabunek ihabunek left a comment

Choose a reason for hiding this comment

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

Mailing list is fine, as are pulls on Github. The patch you sent to the mailing list does not apply, so check it out.

Thanks, this is a good patch, I have some comments before we merge.

self._emit("translate", status)
return

if key in ("r", "R"):
Copy link
Owner

Choose a reason for hiding this comment

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

You changed the key binding for thread here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ouch. Not sure how that happened. Fixed.

toot/tui/app.py Outdated
@@ -106,6 +106,7 @@ def __init__(self, app, user):
self.timeline = None
self.overlay = None
self.exception = None
self.can_translate = Option.UNKNOWN
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think using Option here is very beneficial and it complicates things. I would just set it to False here, and set it to True later when we determine if translate is available.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

toot/tui/app.py Outdated
"""
def _load_instance():
return api.get_instance(self.app.instance)

def _done(instance):
if "max_toot_chars" in instance:
self.max_toot_chars = instance["max_toot_chars"]
if "translation" in instance:
# instance is advertising translation service
self.can_translate = Option.YES
Copy link
Owner

Choose a reason for hiding this comment

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

Looking at mastodon/mastodon@8046cf3#diff-fdc9457d4797dfa4b6e351a974127f11b1efc7eaeadb84718976f98af3618026

Just having "translation" in instance info is not enough, we need to check if instance["translation"]["enabled"] is True.

Suggested change
self.can_translate = Option.YES
self.can_translate = instance["translation"]["enabled"]

(this is presuming we change this from an Option to bool)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

toot/tui/app.py Show resolved Hide resolved
@danschwarz
Copy link
Collaborator Author

I've pushed fixes for all of the above issues, hopefully all is good now. Enhancements for future consideration:

  • Use a library to translate language codes ('en', 'de') to language names when displaying "translated from..." text
  • Make translations toggleable

@ihabunek
Copy link
Owner

Thanks! I merged this manually and also enabled toggling translation by pressing n repeatedly.

@ihabunek ihabunek closed this Dec 11, 2022
@ihabunek
Copy link
Owner

* Use a library to translate language codes ('en', 'de') to language names when displaying "translated from..." text

We don't need a library, when we can use language codes from wikipedia: 0ab0db0

@danschwarz danschwarz deleted the translate branch December 11, 2022 22:43
@ihabunek
Copy link
Owner

Published in 0.32.0. Test and let me know how well it works.

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.

2 participants