Topic title translations #79

angusmcleod · 2022-07-28T12:09:07Z

Note This PR relies on the existence of a new plugin outlet in the topic title. Please review that first: discourse/discourse#17647 before merging this.

Adds topic title translations. The UI is intended to be similar to the post translation UI, with some slight modifications for the different needs of topic titles.

The site setting translator_default_title_languages, for certain locales to be auto-translated on topic creation, is there because the utility of translating topic titles includes that of users reading translations in the topic list itself. Requiring user interaction for each translation would not allow for this.
The site setting translator_show_topic_titles_in_user_locale is there to also allow for the default behaviour with post translations, i.e. that a user only sees the translation upon clicking the translate button. Leaving this off and leaving translator_default_title_languages blank would allow a site admin to exactly replicate the translation behaviour with posts.
Unlike for post translations, the original text is not visible by default after translation, however the user can see it in the tool tip. This could alternatively be added beneath the main title text, however consider that due to the need to show topic titles translations directly on the topic list (as mentioned above), if the topic view were to show the original title first, and the translation below (i.e. exactly matching the post translation UI) this would be an incongruous experience, as the user has only seen the translated text in the topic list.

Notes

The Amazon translation service has been incorrectly saving post translations like this: { “de” => [“de”, “translated de text”] } (they should be saved like this: { “de” => [“translated de text”] } . This has been also fixed as part of this PR (as the code needed updating to support title translations).
The Microsoft translation service was previously the only service where if the post was longer than the service's character limit the translation would be rejected entirely instead of the post content being truncated to conform to the limit. This has been changed to the truncation approach in line with how the other services work. Again, this code needed to be updated anyway to add support for topic title translations.

pmusaraj

Thanks for this @angusmcleod, I left a few comments and asked tgx to have a look as well.

assets/javascripts/discourse/components/translate-topic-title.js

config/locales/server.en.yml

pmusaraj · 2022-08-05T01:59:18Z

assets/javascripts/discourse/templates/components/translate-topic-title.hbs

@@ -0,0 +1,4 @@
+<a {{action "translate"}} title={{i18n "translator.view_translation"}} class="translate-title" role="button">


I am seeing this button even when my interface language is English (the post translation button is not visible in that context). We shouldn't show it in that context.

In fact, I am thinking that it might make sense to not have a separate button for the title but instead just include the title translation when translating a post.

I am seeing this button even when my interface language is English (the post translation button is not visible in that context). We shouldn't show it in that context.

Resolved here angusmcleod@ef6a943

In fact, I am thinking that it might make sense to not have a separate button for the title but instead just include the title translation when translating a post.

While I can see where you're coming from, I disagree. As mentioned in the overview video, there are different considerations with respect to titles, particularly with respect to the topic list, that mean I don't think it would make sense to bundle them.

pmusaraj · 2022-08-05T02:17:46Z

plugin.rb


-      self.custom_fields.delete(DiscourseTranslator::DETECTED_LANG_CUSTOM_FIELD)
-      self.custom_fields.delete(DiscourseTranslator::TRANSLATED_CUSTOM_FIELD)
+  on(:post_created) do |post, options, user|


This should only run if post.is_first_post? otherwise it will send translation requests for the title on every post that is created.

🤦‍♂️ fixed: angusmcleod@a4385a7

pmusaraj · 2022-08-05T02:30:17Z

plugin.rb

      end
    end

  end

+  add_to_serializer(:topic_list_item, :title) do
+    SiteSetting.translator_show_topic_titles_in_user_locale ? object.translated_title : object.title


Having titles automatically be translated in the topic list feels a little odd because when you click through to the topic view, only the title is automatically translated, the posts aren't.

I would also like to avoid making so many additions to the topic_list_item and topic_view serializers. Maybe we can find a way to have one button in topic lists that can trigger a translation for all the titles in the list? Not sure if that is better, but it would at least match the behaviour on the topic view.

Hm again, I disagree here for the same reasons mentioned above. This approach was also independently suggested by both the WMF community and another multilingual community we're working with (i.e. they both proposed the same thing independently of each other). I'd also be worried about bundling up the functionality too much here. Some communities may not want title translations. I'd also note that we may want to add the title translations in the search pipeline at some point, which is part of the motivation of having a "default" title locales that get auto-translated, i.e. this would ensure they're there for use in search.

tgxworld · 2022-08-05T03:37:56Z

Thank you @angusmcleod for the work on this and we definitely welcome this change. Just a couple of quick feedback before I review:

I see in notes that there are two bug fixes being addressed in this PR. Will you be able to split them up into separate PRs since they those changes are not really tied this the change being discussed here.
Will you also be able to provide screenshots or screen recordings for the behavior that is introduced so that we can first agree on the UI/UX before addressing the actual code changes.

Thank you once again!

angusmcleod · 2022-08-12T10:40:18Z

Thanks for your reviews guys, and sorry it's taken me a while to respond. I'll deal with @tgxworld 's comments first as they're more general in nature.

I see in notes that there are two bug fixes being addressed in this PR. Will you be able to split them up into separate PRs since they those changes are not really tied this the change being discussed here.

@tgxworld As mentioned, these two bugs were fixed as part of the changes for this PR (i.e. the same lines of code are affected). I made them separate notes as the fixes may result in new (i.e. working) behaviour for the users of the plugin (i.e. so people know why the behaviour changed). I can make them separate PRs, but keep in mind that the relevant code will be changed again as part of this PR. Let me know if you still want those as separate PRs first.

Will you also be able to provide screenshots or screen recordings for the behavior that is introduced so that we can first agree on the UI/UX before addressing the actual code changes.

Sure, I've made a recording going through the functionality. I'd also recommend reading the Feature topic on meta first for context if you haven't already. Sorry, I assumed that as context for this (I should have linked it on the OP of the PR).

Co-authored-by: Penar Musaraj <pmusaraj@gmail.com>

…leod/discourse-translator into topic_title_translations

angusmcleod · 2022-08-12T11:33:08Z

@pmusaraj Thanks also for your review and comments! I've dealt with them inline.

Regarding the html entity issue seen in the video, I've gone back to square one there and used the standard translated title instead of a translated_fancy_title: angusmcleod@9cd69f7 . Curious on your thoughts there about what "fancy title support" there should be here?

Regarding the product question that @pmusaraj raised about potentially bundling the two features, i.e. post and title translations, perhaps we could continue that conversation on the feature topic on meta if you want to pursue it? It'd give the chance for some of the folks looking to use this the chance to chime in.

If you're overriding core methods the add_to_class plugin api method is not appropriate as it will force the core methods to return nil when the plugin is not enabled.

tgxworld · 2022-08-23T08:36:41Z

@angusmcleod Just leaving a note here to let you know I've not forgotten this but have been tied up with other things. I aim to get this reviewed this week 🙇

angusmcleod · 2022-08-23T09:39:48Z

@tgxworld No worries, thanks Alan.

tgxworld · 2022-08-25T06:16:42Z

Sorry small road block but we will be introducing a new API for plugins to "transform" the topic title starting with discourse/discourse#18081. That will most likely alter how you've implemented the feature here.

angusmcleod · 2022-08-25T13:44:58Z

@tgxworld I think that new API is not quite suited to this implementation as we don't have the same need for client-side switching as the encrypt plugin has. Serializing the already-translated titles seems to make more sense here. Particularly as one of the needs to meet is having already-translated titles show up in the topic list without needing further user interaction (if you guys agree that's something that should be supported).

tgxworld · 2022-08-29T01:29:18Z

assets/javascripts/discourse/templates/components/translate-topic-title.hbs

@@ -0,0 +1,4 @@
+<a {{action "translate"}} title={{i18n "translator.view_translation"}} class="translate-title" role="button">


I'm curious why a link tag is used here instead of a button because I believe using a button here will work.

tgxworld · 2022-08-29T01:38:26Z

assets/javascripts/discourse/components/translate-topic-title.js

+            title: res.translation,
+            fancy_title: res.translation,


This makes me a little nervous because if there is a bug somewhere, a user could inadvertently edit a topic's title into the translated language. Is there a way we can do this without messing with the model's attributes?

tgxworld · 2022-08-29T01:41:50Z

plugin.rb

-      self.custom_fields.delete(DiscourseTranslator::TRANSLATED_CUSTOM_FIELD)
+  on(:post_created) do |post, options, user|
+    if SiteSetting.translator_default_title_languages.present? && post.is_first_post?
+      Jobs.enqueue(:translate_topic_title, topic_id: post.topic.id)


Suggested change

Jobs.enqueue(:translate_topic_title, topic_id: post.topic.id)

Jobs.enqueue(:translate_topic_title, topic_id: post.topic_id)

tgxworld · 2022-08-29T01:45:20Z

plugin.rb

+    def find_object
+      raise Discourse::InvalidParameters.new unless params[:post_id] || params[:topic_id]
+      @object = params[:post_id] ? Post.find_by(id: params[:post_id]) : Topic.find_by(id: params[:topic_id])


I'll prefer if we can just implement two different controller actions here. I'm not a big fan of having an API endpoint dealing with two different models because it leads to a bunch of conditional statements in the code down the line.

tgxworld · 2022-08-29T01:46:59Z

services/discourse_translator/base.rb

+    def self.get_text(object, max_length = nil)
+      case object.class.name
+      when "Post"
+        text = object.cooked
+        text = text.truncate(max_length, omission: nil) if max_length
+        text
+      when "Topic"
+        object.title
+      else
+        nil
+      end
+    end
+
+    def self.get_custom_field(object)
+      case object.class.name
+      when "Post"
+        DiscourseTranslator::DETECTED_LANG_CUSTOM_FIELD
+      when "Topic"
+        DiscourseTranslator::DETECTED_TITLE_LANG_CUSTOM_FIELD
+      else
+        nil
+      end
+    end


Kind of related to https://github.com/discourse/discourse-translator/pull/79/files#r956832812, I'll prefer if we can implement different methods for dealing with Post and Topic so that we can avoid all these conditionals trying to figure out what type of object we're dealing with.

tgxworld · 2022-08-29T01:50:01Z

@angusmcleod I'll prefer it if we can split this PR into smaller components. The way I see it is that we can first introduce the changes to translate a topic's title by clicking on the globe button. Once the code for that is in place, then we can deal with the changes for SiteSetting.translator_default_title_languages. I expect the latter to be tricker to review because I'll like to figure out how we can provide the proper extension points to override the display of the default topic title instead of overriding the model's attribute in a plugin.

Thank you for the contribution here! Supporting translating the topic title's is something we've wanted to add support for but have never found time to do it internally.

mnalis · 2023-03-14T21:54:52Z

So, did this PR continue somewhere else; I seem unable to find it? Or was it dropped (or maybe even implemented)?

There is interest for Topic title translation in Discourse (e.g. https://community.openstreetmap.org/t/translation-of-thread-title/6326), and it would be a shame if it did not get implemented (especially after all the effort @angusmcleod put into it)

angusmcleod added 8 commits July 25, 2022 11:38

Add topic title support to all translation services

9b85aa8

First complete version

36293e3

Update translator controller spec to cover topic titles

7b15451

Convert post clear translator fields callback to use plugin api

3fec030

Ensure we're only restoring the original title of translated titles

55e1337

Update "translator show topic titles in user locale" description

f4437b8

Update to use new plugin outlet name

9da7e8e

Fix template linting

c83abab

pmusaraj requested a review from tgxworld August 5, 2022 01:37

pmusaraj reviewed Aug 5, 2022

View reviewed changes

angusmcleod and others added 7 commits August 12, 2022 11:56

Minor cleanups

7eeb501

Use this in promise callback instead of assigning self

08c3297

Update config/locales/server.en.yml

8557284

Co-authored-by: Penar Musaraj <pmusaraj@gmail.com>

Ensure topic title icon only shows when title can be translated

ef6a943

Merge branch 'topic_title_translations' of https://github.com/angusmc…

fcd940f

…leod/discourse-translator into topic_title_translations

Ensure title translations are only run if post is first post.

a4385a7

Use translated_title instead of translated_fancy_title

9cd69f7

Use module prepension approach to serializer changes && add specs

89208b4

If you're overriding core methods the add_to_class plugin api method is not appropriate as it will force the core methods to return nil when the plugin is not enabled.

tgxworld mentioned this pull request Aug 23, 2022

DEV: Add plugin API for customizing user menu items discourse/discourse#18046

Closed

FIX: only serialize can_translate_title when title_language is present

0d2e27e

angusmcleod mentioned this pull request Aug 25, 2022

DEV: Add model transformer plugin API discourse/discourse#18081

Merged

tgxworld reviewed Aug 29, 2022

View reviewed changes

angusmcleod closed this Sep 9, 2022

		@@ -0,0 +1,4 @@
		<a {{action "translate"}} title={{i18n "translator.view_translation"}} class="translate-title" role="button">

	Jobs.enqueue(:translate_topic_title, topic_id: post.topic.id)
	Jobs.enqueue(:translate_topic_title, topic_id: post.topic_id)

Topic title translations #79

Topic title translations #79

Uh oh!

Conversation

angusmcleod commented Jul 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Notes

Uh oh!

pmusaraj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

angusmcleod Aug 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmusaraj Aug 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

angusmcleod Aug 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tgxworld commented Aug 5, 2022

Uh oh!

angusmcleod commented Aug 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

angusmcleod commented Aug 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgxworld commented Aug 23, 2022

Uh oh!

angusmcleod commented Aug 23, 2022

Uh oh!

tgxworld commented Aug 25, 2022

Uh oh!

angusmcleod commented Aug 25, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tgxworld commented Aug 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mnalis commented Mar 14, 2023

Uh oh!

Uh oh!

angusmcleod commented Jul 28, 2022 •

edited

Loading

angusmcleod Aug 12, 2022 •

edited

Loading

pmusaraj Aug 5, 2022 •

edited

Loading

angusmcleod Aug 12, 2022 •

edited

Loading

angusmcleod commented Aug 12, 2022 •

edited

Loading

angusmcleod commented Aug 12, 2022 •

edited

Loading

tgxworld commented Aug 29, 2022 •

edited

Loading