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

More robust i18n try4 #560

Merged
merged 5 commits into from
Jul 30, 2015
Merged

Conversation

godiard
Copy link
Contributor

@godiard godiard commented Jul 27, 2015

Replaces #558 addressing comments from @quozl.

We have received a few reports of broken ~/.i18n files
Try to make the code more robust, using "with" to open the files,
and remove redundancy in the code.
Only write the file when set the languages, and flush to
try avoid have a broken file.
When read the languages, get the values from the enviroment variables
set on bin/sugar. If the file ~/.i18n exists, was already read there.
When the user press the cancel button in the control panel section,
the ModelWrapper object (in gui.py) try to revert the changes
and write the initial values, even if nothing has changed.
As the recording of the values is triggered by a timeout,
we don't have another way to avoid race contions that verify
at the moment of write the file.
…#4878

When the user press the (+) button and a new language row is added,
the _add_row method receive locale_code=None. To make the method
manage this case properly, we need do two changes:
* Do not check in the available locales because None will not get any value.
* When add the locale_code to self._country_codes,
  set the locale based in the default values displayed to the user.

A consequence of the method not managing the None case,
id that changes will not be saved until the user change the default
value displayed to the user.
Instead of catch the exception in the model, do it in the view,
and show feedback to the user with the mechanism provided by the
control panel. Disable the check button and update the msg label.
@godiard godiard mentioned this pull request Jul 27, 2015
@tchx84
Copy link
Member

tchx84 commented Jul 29, 2015

Aside from my previous comment, I see changes on top of other changes (e.g, write method is changed in 3 different patches), shouldn't that be consolidated?

Aside from these 2 things, the changes look good.

@godiard
Copy link
Contributor Author

godiard commented Jul 29, 2015

@tchx84 , do you prefer consolidate all the patches in a single patch?

@tchx84
Copy link
Member

tchx84 commented Jul 29, 2015

@godiard no, just in a way where the same function doesn't get completely re-written 3 times :)

@quozl
Copy link
Contributor

quozl commented Jul 30, 2015

Reviewed. I'm fine with the separate patches, as they show the sequence of change. Don't like consolidation, as it hides the sequence. Future bug analysis depends on sequence of change.

@tchx84
Copy link
Member

tchx84 commented Jul 30, 2015

Alright, we are tied now, so its up to @godiard.

@godiard godiard merged commit 3b7cbb0 into sugarlabs:master Jul 30, 2015
@godiard
Copy link
Contributor Author

godiard commented Jul 30, 2015

Ok, I merged this pr. Even when the method is modified in different patches, every patch have a specific purpose, and I have tried to consolidate the changes, and the result is too big.

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.

3 participants