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

Better other default language support #280

Merged
merged 13 commits into from
Jun 27, 2024

Conversation

sdumetz
Copy link
Contributor

@sdumetz sdumetz commented Jun 11, 2024

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 Smithsonian/ff-graph#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.

@sdumetz sdumetz changed the title Default language Better other default language support Jun 11, 2024
gjcope

This comment was marked as outdated.

@@ -84,7 +84,7 @@ export default class LanguageMenu extends Popup
</div>
<div class="ff-flex-row">
<div class="ff-scroll-y sv-scroll-offset" role="listbox">
${language.activeLanguages.map((language, index) => this.renderEntry(language, index))}
<sv-property-options class="ff-flex-column" buttons .property=${language.ins.language}></sv-property-options>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I may be missing something from another commit, but this doesn't seem correct to me. language.ins.language should be all supported languages (i.e. all languages we have dictionaries for) and the Explorer language menu should have all languages actively used in the scene.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opted to split the "list of accepted languages" (a subset of "all dictionaries") into language.ins.languages, whose value is then used as possible options for language.ins.language ; then always propose just this subset of languages until another language is enabled in the scene.

The reason behind this is something I saw a lot of users do during scene editing :

  • edit some annotation/article in a french-only scene
  • click on "Français", switch to "English" (misclick, or looking for something else, or not knowing this scene is not translated)
  • Save the scene at any point in the future
  • The scene is now FR/EN forever, even though most/all EN values are undefined. There is no way to go back to "french-only"

To prevent this I rather liked the concept of this intermediate list of languages that makes it unequivocaly clear what languages are expected to be supported in this scene.

We might go the other route and not have this list. The list of available languages would then be "any language for which something is defined" but then :

  1. we can't easily delete a language for a scene once it has been touched
  2. we have to thoroughly search through the scene for all translation dictionaries
  3. inside the Story editor we will have all available languages to select from. Keeping in mind that I might be required to commit to some basic localization for a large chunk of languages spoken in the EU in the near future, that's a quite a list to have.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the explanation. I understand the motivation, and acknowledge the issue, but am not sure I agree this is the best route forward.

  • edit some annotation/article in a french-only scene
  • click on "Français", switch to "English" (misclick, or looking for something else, or not knowing this scene is not translated)
  • Save the scene at any point in the future
  • The scene is now FR/EN forever, even though most/all EN values are undefined. There is no way to go back to "french-only"

I think this is only true with articles, as switching languages currently creates a new html article asset. With annotations and titles, if the data is undefined on save it shouldn't be getting written to the file. If there are cases where it is, that sound like a bug.

To prevent this I rather liked the concept of this intermediate list of languages that makes it unequivocaly clear what languages are expected to be supported in this scene.

How do you see this list being defined? From my current perspective, this isn't something we could expect from our users.

We might go the other route and not have this list. The list of available languages would then be "any language for which something is defined" but then :

  1. we can't easily delete a language for a scene once it has been touched
  2. we have to thoroughly search through the scene for all translation dictionaries
  3. inside the Story editor we will have all available languages to select from. Keeping in mind that I might be required to commit to some basic localization for a large chunk of languages spoken in the EU in the near future, that's a quite a list to have.

For 3. this is intentional. I don't think it's any different from standard localization lists in many sites/apps.

I'm going to think a little bit more on alternatives to address this issue.

@gjcope
Copy link
Collaborator

gjcope commented Jun 12, 2024

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
Copy link
Contributor Author

sdumetz commented Jun 12, 2024

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
Copy link
Collaborator

gjcope commented Jun 12, 2024

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
Copy link
Collaborator

gjcope commented Jun 12, 2024

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
Copy link
Contributor Author

sdumetz commented Jun 13, 2024

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
Copy link
Collaborator

gjcope commented Jun 13, 2024

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.

this.language.addLanguage(ELanguageType[key]);
). 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.

…le selection

fixup: index for buttons and options change tracking
… lang parameter would get reset on document load. properly update activeLanguages list from CVMeta.
@sdumetz sdumetz force-pushed the default_language_stability branch from efa798d to d051c56 Compare June 17, 2024 08:09
@sdumetz
Copy link
Contributor Author

sdumetz commented Jun 17, 2024

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 :

  1. to English on page load (forced by the string translation system until a dict is available)
  2. to DEFAULT_LANGUAGE (no-op if DEFAULT_LANGUAGE = "EN")
  3. to setup[0].language.language on document load
  4. 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.
  5. 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
Copy link
Collaborator

gjcope commented Jun 25, 2024

Did some quick testing.

One bug I noticed can be reproduced by:

  1. Change to new language
  2. Add new title
  3. Select annotation (with Collection tab still visible)
  4. Change language under annotation
  5. Change back to language from step 1
  6. 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.
  1. 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
Copy link
Contributor Author

sdumetz commented Jun 25, 2024

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 (3cbbeb5).

'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
Copy link
Collaborator

gjcope commented Jun 25, 2024

Did you push 3cbbeb5? 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

languageIns.language.setValue(ELanguageType[id]);
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
Copy link
Contributor Author

sdumetz commented Jun 26, 2024

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
Copy link
Collaborator

gjcope commented Jun 26, 2024

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

@gjcope gjcope merged commit 3cbbeb5 into Smithsonian:master Jun 27, 2024
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