Better other default language support

sdumetz commented 3 months ago

I did a bunch of work trying to bring the multi-language support to a point where non-english speakers and particularly non-primarily-english scenes have a smooth experience.

There was a number of issues:

In a number of places, ELanguageType.EN was used interchangeably with DEFAULT_LANGUAGE. The fix is straightforward. ELanguageType.EN now used as a default only where it makes sense.
I removed every individual task's ins.language property. They were the source of a number or race conditions, while barely used in the code and I found it easier to remove them and use CVLanguageManageras the unique source of language state everywhere. This should also help avoid regressions in the future.

Some backward-compatible changes :

Fixing a race condition where the scene's language would get set by ExplorerApplication using a queryString then changed back to the document's defaults when the document loads. See 9bacad87 and set the page's queryString to some other language to trigger the race condition. I had to create a new UNSET language value. This has minimal impact on the system because it always get set before it really gets used.

A breaking change that I feel was necessary:

Added a document.setups[0].language.languages property describing the document's allowed locales. Without it I couldn't get the activeLanguages feature to work reliably in every case. Additionally it allows an user to manually choose which languages will be available in the scene. As a nice bonus, the language picker is auto-synchronized to allow only those options everywhere (Tasks, CollectionPanel, LanguageMenu).

It won't break older scenes, gathering the allowed languages from existing data.

Note : it requires https://github.com/Smithsonian/ff-graph/pull/1 to work properly.

I left every commit separate to make those changes more readable: each message should be relatively self-describing. Most of them could be squashed together once this is ready to merge.

We tested it on a lot of our scenes already (French-only, English-only, French-first, English-first) so this shouldn't cause too much unexpected trouble.

gjcope commented 3 months ago

Still considering the "active language" commit, but other than that this PR looks great. Thanks for submitting and I appreciate the commits being broken up!

sdumetz commented 3 months ago

Clearly an area where there is more than one "good" solution. I felt I had to propose a solution as a starting point but I am ready to consider other solutions if they fit the greater picture better.

For the selection of a list of "scene languages", our users generally have a very clear idea of what this list should be, hence the proposal.

If you have something in mind please let me know. Otherwise I'll try to have another look at it shortly, taking your comments into account to see if I can come up with a better solution.

gjcope commented 3 months ago

I think my question re: the list is more about how that is operationalized. Would you as an admin be setting this list before they start in each scene file? Would the end user be setting this list? It isn't clear to me how it would be configured, even if you know what this list would be.

gjcope commented 3 months ago

Talked to some more folks about this, but still really interested in how you see this being configured. But something else to consider:

Language list is still driven by the full list of supported languages. If the list gets too long, there are some UI improvements we could add that are pretty common to long localization selections.
Languages are still added to the 'active' list by selecting in a language dropdown, but adding in a pop-up to give users an out if it was unintended. (i.e. "French is not currently used in your scene. Would you like to add it?")
Providing some sort of UI for removing languages from the supported list which would also remove any associated data from the scene. I'm less clear on how this piece would fit in.

The advantage I see here is not relying on a static list and having the 'active' list be more data driven. But again, looking for your config plan as maybe you were thinking something similar.

sdumetz commented 3 months ago

In our case, the end user is generally the one creating the scene. In the current state it is a list of buttons to choose from in the "Collection" tab, which isn't even greatly discoverable.

I wasn't thinking along those lines but your reasoning made me reconsider. This might well be better for your use case and possibly just as good for me.

On a practical level, it would make setup.language.languages redundant if we can compute a reliable list of active languages when the scene is parsed to replace it. ie. a generalization of what I do in c803129. the upside is we don't change the document's schema, but it adds back some error-prone code that will need to be regularly updated in the future. That'd be a bit of spaghetti code too, scanning through pretty much the whole IDocument.

It would also be possible to call addLanguage() from the initializers of Annotation, Article... but this would add back language management code all over the place, wouldn't it?

To handle the potentially growing list of available languages, one idea would be to be able to group options, presenting the active languages first and the other second. However as I've been thinking about this, I now think that's better left for a future time because :

the Property schema can't handle this right now
this is an area where there is A LOT of dead/convoluted code so I'm reluctant to add features to it before it had a good cleanup
the list of language is not yet too long so this is still a "tomorrow" problem.

So I'll probably just remove the ins.languages.changed block for now.

I'm curious about your opinion on whether setup.language.languages is desirable or not, then I think I'll have everything I need to improve d58414e..26a4574

gjcope commented 3 months ago

In our case, the end user is generally the one creating the scene. In the current state it is a list of buttons to choose from in the "Collection" tab, which isn't even greatly discoverable.

I see. I think the alternate proposal builds it into the existing workflow so may be a little more user friendly.

It would also be possible to call addLanguage() from the initializers of Annotation, Article... but this would add back language management code all over the place, wouldn't it?

We already have this code for annotations and title (e.g. https://github.com/Smithsonian/dpo-voyager/blob/c08cb20741d5f0aaadd0aa8a5229152e30d4e017/source/client/components/CVMeta.ts#L79). It doesn't look like your PR touched it. But it also looks inconsistent for articles, so that would need to be added. I don't think this is "all over the place", just the three components with localized content. If a new content component was added, it would need to be implemented for that too, but that seems reasonable.

I'm not sure how we would centralize this anymore without reimplementing the language enum to encapsulate it but that seems extreme.

To handle the potentially growing list of available languages, one idea would be to be able to group options, presenting the active languages first and the other second. However as I've been thinking about this, I now think that's better left for a future time because :

I was thinking the same thing about pushing the active languages to the top, but agree it is not a priority at the moment.

I'm curious about your opinion on whether setup.language.languages is desirable or not, then I think I'll have everything I need to improve d58414e..26a4574

I don't have a problem with it and see how it could serve as a shortcut to loading active languages from established scenes, but I don't know that it is required if we go with the above changes. My remain issue is how a user would remove things from the active list.

sdumetz commented 3 months ago

I force-pushed to collapse the commits from d58414e onwards because all the reverts started getting messy.

Did a few more tests. Language is set order of precedence :

to English on page load (forced by the string translation system until a dict is available)
to DEFAULT_LANGUAGE (no-op if DEFAULT_LANGUAGE = "EN")
to setup[0].language.language on document load
to the lang queryString value. If it doesn't map to a supported language in the scene, it is forcefully added to the list of active languages.
to the user-selected value from any of the <sv-property-options> selectors (every language available) or LanguageMenu(restricted to CVLanguageManager.activeLanguages

Don't know for sure if 4. is the most desirable behavior? Checking activeLanguages before switching to the value of ?lang seems not practical because it would introduce back a race condition between this and the document load. On the other hand not adding it to activeLanguages would make the user unable to switch to another language than back to it.

Should be OK to merge now, please let me know if there is any remaining issue that I overlooked or if you uncover any bug this introduces.

gjcope commented 3 months ago

Did some quick testing.

One bug I noticed can be reproduced by:

Change to new language
Add new title
Select annotation (with Collection tab still visible)
Change language under annotation
Change back to language from step 1
Collection tab language will now say 'English' regardless of selected language. I assume it is because the index is null for some reason but haven't been able to look into it yet.

to the lang queryString value. If it doesn't map to a supported language in the scene, it is forcefully added to the list of active languages.

'lang' is currently a property that really only applies to explorer, so if it doesn't map to a supported language that would mean there is no content for that language and I would think it should be ignored.

Other than those two things, the PR looks good it just doesn't seem like we are addressing the original issue (unless I'm missing something). Users can still unintentionally add languages with no way to remove them, right? Seems like we still need something like a confirmation pop-up when adding new languages and some way to remove them. But maybe this is for another day. There is still a lot of valuable cleanup in this PR.

sdumetz commented 3 months ago

I assume it is because the index is null for some reason but haven't been able to look into it yet

Looks like a bad use of litElement templates. I can reproduce the issue, the HTML is properly updated (in the inspector), but the wrong value is shown. It looks like a known issue and using the live() directive seems to fix it (3cbbeb57).

'lang' is currently a property that really only applies to explorer, so if it doesn't map to a supported language that would mean there is no content for that language and I would think it should be ignored.

This is reasonable, however I don't know if we can without introducing back some race conditions between this and the document deserializer? Currently we don't know what will be the list of available language from where the queryString is parsed.

Regarding the issue, that's me going all over the place. The core issue would be "there is currently no way to make Voyager reliably stick to a language other than English". It's just that I know I18n when done wrong tends to come back at you and bite hard so I try to think ahead.

I'm Ok with the other issues left for later. For english-first scenes, this PR is effectively just a cleanup job but for scenes that defaults to another language it is already a great improvement.

gjcope commented 3 months ago

Did you push 3cbbeb57? I'm not seeing it.

This is reasonable, however I don't know if we can without introducing back some race conditions between this and the document deserializer? Currently we don't know what will be the list of available language from where the queryString is parsed.

Good point. It's not pretty, but maybe https://github.com/Smithsonian/dpo-voyager/blob/c08cb20741d5f0aaadd0aa8a5229152e30d4e017/source/client/applications/ExplorerApplication.ts#L692 is changed to a new 'initLanguage' function or secondary property or something like that to separate setting language via api/attribute so it can be evaluated when available.

sdumetz commented 3 months ago

Sorry, forgot to push...

If that's OK with you, I'd wait to see if the problem actually happens on a regular basis in the real world before implementing a solution. I don't know if it would actually happen to have a queryString not matching the referenced scene.

If it only happen once in a while, I vote to leave it as it is : it's the simplest solution. And if it becomes a problem we can fix it later using your idea or something else that might come up depending on the context.

gjcope commented 3 months ago

Sounds good. Merged to https://github.com/Smithsonian/dpo-voyager/tree/rc-42

Smithsonian / dpo-voyager

Better other default language support #280