Do not display the translationbar to non-supported languagues #47
Conversation
|
@andrenatal One question. Is there a test for this? |
| @@ -93,4 +93,8 @@ class TranslationNotificationManager { | |||
| const message = { command: "displayStatistics", tabId: this.tabId }; | |||
| this.bgScriptListenerCallback(message); | |||
| } | |||
|
|
|||
| isPageLanguageSupported(){ | |||
| return this.languageSet.has(this.detectedLanguage); | |||
There was a problem hiding this comment.
We should check that language pair page-lang->user-lang or page-lang->en and en->user-lang are supported. Models of opposite directions are completely decoupled from each other and we can support only one-way translation.
There was a problem hiding this comment.
After giving this a second thought there's some assumptions we are making about this issue for free that I'm not sure if they are right:
- We are putting to much trust that the cld module will return the right language, and we already know how naive that module is and its flaws (like confusing es and pt very often)
- We are assuming that the portion of the page's content to be analyzed to always be representative of the majority of the page's language.
So I think this is more complicated than just preventing the infobar of displaying when the supposed page's language do not match what exists in the model registry. Before that we'll need to review the cld and the page's language analysis first and also decide if we want to actually prevent the infobar of displaying or not (maybe we want to always display it and only don't auto select the language we think it represents the page?). Many new sites localize some parts of the page, and mix languages. How do we detect that? In that case a deep learning based cld would work better than ngrams like cld2 which is what we have. I heard cld4(?) is nn based.
In a normal world that would be a UI + product decision, but since we know we don't have that, we'll need to figure out something reasonable ourselves.
There was a problem hiding this comment.
I think we should definitely work on improving cld accuracy but I don't think always displaying the infobar will be useful. Our language coverage is pretty limited atm and displaying just to show that we don't support the page language will be annoying for users. I would suggest assuming cld works correctly and relying on its output here, abstract from it and work on this issue separately.
extension/mediator.js
Outdated
| @@ -61,7 +61,7 @@ class Mediator { | |||
| if (!this.languageDetection.navigatorLanguage.includes(this.languageDetection.pageLanguage.language)) { | |||
There was a problem hiding this comment.
The code inside "if" statement shouldn't be called if language pair is not supported. It triggers calls to infobar and creates unnecessary Translation object. Also we'll need to record "unsupported" metrics from mediator. It means we need to read model registry from mediator and move the check here from TranslationNotificationManager.
There was a problem hiding this comment.
| Also we'll need to record "unsupported" metrics from mediator.
Could you please elaborate on what does this mean? That we are supposed to send a ping every time a page is not supported by the extension (i.e. all the time the user browses to a new page?)
There was a problem hiding this comment.
See this todo. We should always record "unsupported" metric. How the ping sending is triggered is decoupled from recording. In the current implementation, it happens on page closing. If it won't work for us, we can move this metric to a different ping and send it on timer in the future.
There was a problem hiding this comment.
Ok, so this means sending a ping to every page a user browses to any page, no? At least one ping will always be sent if the page's lang is either supported or unsupported?
There was a problem hiding this comment.
Only when language mismatch happens. Yes, exactly one ping will be sent if the user language is different from the page language in the current implementation.
Wanna take a stab at that? |
fixes #34