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

Fix bug 1212738: Send proper plural_form when updating translations. #208

Merged
merged 1 commit into from
Oct 12, 2015

Conversation

Osmose
Copy link
Contributor

@Osmose Osmose commented Oct 9, 2015

Turns out my initial assessment in bug 1212738 about the fix was wrong; the UI depends on pluralForm being -1 for locales with nplurals == 1, regardless of what is stored in the database as the plural form of the string.

This fixes the issue on our current UI, but it's a bandaid. The hopeful future redesign/re-implementation will have to take a few things into consideration:

  1. This was caused by a conflation of UI logic and app logic. Because we determined the plural form from which DOM elements were visible, the UI requirement of hiding the plural tabs for locales with nplurals == 1 got conflated with our logic of what plural form to store in the database.
  2. We don't currently have any good way to represent in the database whether an entity is pluralized or not. There's checking for string_plural but that seems to be not quite right. vcs_models uses a dict with a None key for singular entities and numbers for pluralized entities, but I'm not entirely happy with that either.

In any case, this should fix things for us now.

@Osmose
Copy link
Contributor Author

Osmose commented Oct 9, 2015

@mathjazz r?

@mathjazz
Copy link
Collaborator

r+

Filing a bug to redesign plurals:
https://bugzilla.mozilla.org/show_bug.cgi?id=1213779

mathjazz added a commit that referenced this pull request Oct 12, 2015
Fix bug 1212738: Send proper plural_form when updating translations.
@mathjazz mathjazz merged commit 3d32627 into mozilla:master Oct 12, 2015
@mathjazz
Copy link
Collaborator

Actually: why not putting the code you added inside getPluralForm()?

@Osmose
Copy link
Contributor Author

Osmose commented Oct 12, 2015

Actually: why not putting the code you added inside getPluralForm()?

Because getPluralForm is used in a bunch of places for UI logic, like finding the right translation to display after deleting one or going to the next plural form after saving a translation. I don't know if those are broken (no one appears to have reported anything) and I don't want to potentially break them if they're not.

@Osmose Osmose deleted the fix-plural-translation branch October 12, 2015 15:38
@mathjazz
Copy link
Collaborator

I checked the code and we use getPluralForm(false) 3 times:

  • getHistory()
  • updateOnServer()
  • isPluralized()

Your fix is sufficient for all 3 cases and the fix in getPluralForm() would also work fine. So let's leave it as it is and come up with a complete fix in bug 1213779.

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