Translator: Add romanizations setting in Translation settings by eric-p-hutchins · Pull Request #12829 · koreader/koreader

eric-p-hutchins · 2024-12-02T01:31:20Z

This adds a new setting in "Translation settings" called "Show romanizations" which tells the translation popup to include in the query the parameter dt=rm and then extracts romanizations from the results to display.

This change is

hius07 · 2024-12-02T04:23:38Z

Can it be just a user patch of 2 lines?

eric-p-hutchins · 2024-12-02T04:29:13Z

Can it be just a user patch of 2 lines?

I'm sorry I don't understand what you mean by this.

Are you suggesting I make some change?

@hius07

hius07 · 2024-12-02T05:13:43Z

A niche setting.
This can be solved by a short userpatch

local Translator = require("ui/translator")
table.insert(Translator.trans_params.dt, "rm")

Userpatches are: https://github.com/koreader/koreader/wiki/User-patches.

eric-p-hutchins · 2024-12-02T06:54:20Z

@hius07 So, I have a few points to respond to that with:

It is actually not possible to make that work with user patches. The patch you suggested will add dt=rm to the Google Translate API query but it the Translator:_showTranslation function still won't know how to extract the romanization since they come in from a different part of the response. It is necessary for the Translator module to know about romanizations for this to work, and there's nothing for the user to modify to hack in support for it.
The point is also to be able to conveniently enable/disable it whenever the user wants. It may be more needed for some content than for others. I don't see examples in userpatches of adding to the settings, which is an important part of the change.
It's not clear to me what the reasoning is for whether something should be merged vs. a userpatch. If transliteration is niche, then why is translation itself not niche as well? They show up on the Google Translate UI for a reason, after all. It is not obvious that userpatches even exist, let alone how to install them.
Basic support for the Google Translate API has been in place for years and it is even documented in the comments that this query parameter exists to get transliterations, but it has not been used. I think this is a natural extension of the Translator interface, providing an easily added feature that makes a big difference but is, in fact, not trivial to add without modifying the internals of the module.
The "Translation settings" menu only has 4 items so far. Some settings have pages of items to scroll through. Going from 4 items to 5 seems like not a big deal imo.
If the reasoning for not merging is maintenance burden then I don't see how this code causes any issues with existing functionality. The code already has to be defensive about extracting the real translations since the response arrays may or may not have real results in all the slots, so it checks for string results. Romanizations show up in a different part of the response, so it will never conflict with where it checks for regular translations and vice versa.

hius07 · 2024-12-02T07:15:54Z

It is necessary for the Translator module to know about romanizations for this to work

Good point, agree.

hius07 · 2024-12-02T07:17:02Z

+             rmIndex = i
+             break
+          end
+       end


We have

koreader/frontend/util.lua

Line 323 in 8297d17

function util.arrayContains(t, v, cb)

hius07 · 2024-12-02T07:58:29Z

While we are in Translator.

koreader/frontend/ui/translator.lua

Line 296 in 8297d17

return T(_("Translate to: %1"), self:getLanguageName(lang, ""))

@Frenzie, isn't it more correct Translate into:?

Frenzie · 2024-12-02T08:16:00Z

I think it's a from/to, but into is also fine.

poire-z · 2024-12-02T08:55:07Z

+                    G_reader_settings:flipTrue("translator_from_romanizations")
+                end,
+            },
            {
                text_func = function()
                    local lang = G_reader_settings:readSetting("translator_from_language")


(May be you could name the setting translator_with_romanizations or translator_show_romanizations to avoivd "same prefix but different purpose" with translator_from_language with using from?)

Yeah, I re-used the _from_ segment to mean that the romanizations come from the "from" language (rather than the "to" language).

But maybe that's a little confusing.

Renamed to translator_with_romanizations

poire-z · 2024-12-02T08:58:34Z

+    if G_reader_settings:isTrue("translator_from_romanizations") then
+       table.insert(self.trans_params.dt, "rm")
+    else
+       local rmIndex = nil
+       for i, v in pairs(self.trans_params.dt) do
+          if v == "rm" then
+             rmIndex = i


No other way avoid this and the ugly removal? Dunno (no time to check) how this appears in the query when dt is an array.

Hmm, you're right.

Actually, I only had to do this to make the tests that I added pass since it reuses the Translator between tests. Actually, I'm pretty sure from testing it for real that this doesn't need to be done otherwise.

Do you have a suggestion how to better handle using this in tests?

I moved the initialization of dt to this function so that it doesn't affect the tests. Now the tests still pass and it's not as ugly. Thanks for asking for me to look at this again.

Depending on specifics you can also adjust the test code with for example before each and after each. (Mainly for future reference.)

… the call is made

…r_with_romanizations

poire-z · 2024-12-02T18:17:27Z

        tsel = 0,  -- ?
        -- tk = "" -- auth token
-        dt = { -- what we want in result
-            "t",   -- translation of source text
-            "at",  -- alternate translations
-            -- Next options only give additional results when text is a single word


A bit sad to move this bit of documentation away from all the others :) Please let it here.

That's probably why I ended up with the approach I had at first 😅

I can put the comments back here but I still think that dt should be added later on rather than here, so that it is clearer what's happening later on.

We could put them all (with their documentation) where there are used, but that's a bit more than what this PR is about :)

Why there are here may not have been really thought (I may have just started translator.lua by copying wikipedia.lua), but the side effect of having it there is that people can user-patch it (like we suggested you to do :)) - so by hardcoded the replacement in the body of a function, the user patched stuff will be overridden.

poire-z · 2024-12-02T18:21:17Z

+    if G_reader_settings:isTrue("translator_with_romanizations") then
+       table.insert(self.trans_params.dt, "rm")
+    end
    for k,v in pairs(self.trans_params) do
        if type(v) == "table" then
            for _, v2 in ipairs(v) do


I see that for things in the dt array, we just keep appending them with the same key: dt=aa&dt=bb&dt=cc.
So, may be just append yours after all the ones processed by this generic query construction.
ie. after this for loop:

if G_reader_settings:isTrue("translator_with_romanizations") then query = query .. 'dt=rm&' end

But isn't it more consistent to have all the dt= parts be in the same structure first? Seems confusing to special-case this one.

I don't mind the special case :)
~~But if you really don't want that, you could:~~

local dt = self.trans_params.dt if G_reader_settings:isTrue("translator_with_romanizations") then dt = util.tableDeepCopy(dt) table.insert(dt, "rm"} end

Won't work as we just process the full self.trans_params...
I don't see another clean way.

Alright, I went with appending to the query directly like you suggested.

poire-z · 2024-12-02T19:53:56Z

+        -- dt = { -- what we want in result
+            -- "t",   -- translation of source text
+            -- "at",  -- alternate translations
            -- Next options only give additional results when text is a single word
            -- "bd",  -- dictionary (articles, reverse translations, etc)
            -- "ex",  -- examples
            -- "ld",  -- ?
-            "md",  -- definitions of source text
+            -- "md",  -- definitions of source text
            -- "qca", -- ?


I also don't like you commenting these - while they are actually used.
This documents the whole API - commented out stuff means we won't need and use them (like you "rm"), so the ones we use should stick out from the ones we don't.

Again, can't think of something better than special casing and appending dt=rm to query.

Ok, I put that back.

…ing is enabled

Co-authored-by: Frans de Jonge <fransdejonge@gmail.com>

…er#12829) This adds a new setting in "Translation settings" called "Show romanizations" which tells the translation popup to include in the query the parameter dt=rm and then extracts romanizations from the results to display.

eric-p-hutchins added 3 commits December 1, 2024 05:55

Translator: Show romanizations if enabled

80740c5

Translator: Add unit tests for romanizations option

a9ab292

Fix warning from luacheck

9ef2e6e

pazos reviewed Dec 2, 2024

View reviewed changes

Comment thread frontend/ui/translator.lua

Translator: Add help text for "Show romanizations"

1aeb856

hius07 reviewed Dec 2, 2024

View reviewed changes

poire-z reviewed Dec 2, 2024

View reviewed changes

eric-p-hutchins added 3 commits December 2, 2024 12:43

Translator: Use util.arrayContains for "rm" parameter

d380ab8

Translator: Move initialization of dt query parameter closer to where…

caf3883

… the call is made

Translator: Rename setting translator_from_romanizations to translato…

6c2aae4

…r_with_romanizations

poire-z reviewed Dec 2, 2024

View reviewed changes

Translator: Put back documentation in one place

68670d9

poire-z reviewed Dec 2, 2024

View reviewed changes

Translator: Put back dt initialization and add dt=rm to query if sett…

540292a

…ing is enabled

poire-z approved these changes Dec 2, 2024

View reviewed changes

Frenzie reviewed Dec 2, 2024

View reviewed changes

Comment thread frontend/ui/translator.lua Outdated

Translator: Adjust casing in help text for "Show romanizations"

aa4d6be

Co-authored-by: Frans de Jonge <fransdejonge@gmail.com>

Frenzie added this to the 2025.01 milestone Dec 2, 2024

Frenzie approved these changes Dec 2, 2024

View reviewed changes

pazos approved these changes Dec 2, 2024

View reviewed changes

Frenzie merged commit 76a7633 into koreader:master Dec 2, 2024

Uh oh!

Uh oh!

Conversation

eric-p-hutchins commented Dec 2, 2024 • edited by Frenzie Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

hius07 commented Dec 2, 2024

Uh oh!

eric-p-hutchins commented Dec 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hius07 commented Dec 2, 2024

Uh oh!

eric-p-hutchins commented Dec 2, 2024

Uh oh!

hius07 commented Dec 2, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hius07 commented Dec 2, 2024

Uh oh!

Frenzie commented Dec 2, 2024

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

poire-z Dec 2, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

eric-p-hutchins commented Dec 2, 2024 •

edited by Frenzie

Loading

eric-p-hutchins commented Dec 2, 2024 •

edited

Loading

poire-z Dec 2, 2024 •

edited

Loading