-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add support for language selection in preferences #2349
Conversation
@@ -204,18 +205,102 @@ def get_image_path(name, default="not_found.png"): | |||
#============================================================================== | |||
# Translations | |||
#============================================================================== | |||
LANG_FILE = osp.join(get_conf_path(), '.langconfig') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok with adding this new file, but don't call it .langconfig
, just langconfig
, so that it's not hidden on Linux :-)
At the end, it's going to be inside of ~/.spyder
, so there is no need to be hidden.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@ccordoba12 I updated with the fixes, when you have some time can you take a look at some questions I left in the description? |
This PR is waiting for some input from the @spyder-ide/core-developers ... There are 2 ways of doing this. I implemented the first one.. but the second one could be also implemented if you consider is better. @ccordoba12 any ideas, thoughts? |
I vote 2 because:
If 2 is chosen, it should be written that it requires a restart. |
In that case of 2, I would make the popup appear at the end when clicking |
Yep. Bonus points if the dialog contains the reason why the restart is needed (as a list so that other settings can give their information too) |
Yep, you read my mind ;-) |
@Nodd ok, this is now updated to ask only after pressing Ok or Apply and displays a list of all the options that require a restart (and were changed). Can you give it a try @ccordoba12? I would expect this to work in mac without changes but, @blink1073 can you give it a try? |
The restart popup message is very nice!! It looks very professional! Thanks a lot for working on it @goanpeca! |
language = DEFAULT_LANGUAGE | ||
else: | ||
spyder_languages = get_available_translations() | ||
for spyder_language in spyder_languages: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's simplify the variables here to make it a bit easier to read. What about this?
for lang in spyder_languages
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok sure, I will adapt ;-)
In python3 I didn't have this problem (the unicode support is way better). |
@@ -2743,6 +2743,9 @@ def restart(self): | |||
startupinfo.dwFlags |= subprocess.STARTF_USESHOWWINDOW | |||
shell = False | |||
else: | |||
# This environment variable is needed so that the new python | |||
# process spawned with Popen sets this as the default encoding | |||
env['PYTHONIOENCODING'] = 'utf-8' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think setting this system-wide is not a good idea. For example, It could mess with Pandas trying to import/export files :-)
If I'm not wrong, redirection of stdio for the internal console happens in https://github.com/spyder-ide/spyder/blob/master/spyderlib/spyder.py#L2435 |
I tried putting @Nodd where @ccordoba12 without success |
@Nodd I think I have it fixed, can you check the latest commit? @blink1073, or @ccordoba12 can you please test on Mac? remember to run using
|
It works for me, other than the fact that the restart dialog was entirely in English. |
Cause we need To rerun the localización scripts 👍 |
@ccordoba12 can we merge this one so that @SylvainCorlay can add this to his icon PR 😄 ? |
Yeah seems I will need to rebase again anyway! |
@@ -169,6 +169,7 @@ def is_ubuntu(): | |||
'animated_docks': True, | |||
'prompt_on_exit': False, | |||
'panes_locked': True, | |||
'interface_language': load_lang_conf(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we don't need to add this here because this is dependent on the user system.
The only options that we add in config.py
are those that can be changed and/or updated in the future (by bumping CONF_VERSION
).
That's why (for example) we don't add here the window size or state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another example of things we don't add are the run configuration options per file :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this option from here to finally merge this PR :-)
If I understand the code correctly:
- It'll be saved to
spyder.ini
by thesave_lang
method ofMainConfigPage
- It can't be retrieved by
CONF
directly.
So there's no need to have it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I don't add that method to the config, Spyder will not be able to load the preferences after a reset, cause there will be no interface_language
setting in the .ini file.
Suggestions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should leave it as it is, otherwise we would have to made unneded changes either to how the preferences are loaded or do an extra check in spyder.py
to see if this is the first time it runs.... here it is just one line of code...
Is there any blocking point for this one? |
@@ -724,6 +724,9 @@ def is_ubuntu(): | |||
CONF = UserConfig('spyder', defaults=DEFAULTS, load=True, version=CONF_VERSION, | |||
subfolder=SUBFOLDER, backup=True, raw_mode=True) | |||
|
|||
# Ensures that the config is present on spyder first run | |||
CONF.set('main', 'interface_language', load_lang_conf()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ccordoba12 based on your previous comment on removing the entry from the default config, I need to add this line here to make sure that on first run of spyder the interface_language
entry is created. On subsequent starts of spyder that line is harmless so this way is not part of the CONF but ensures the correct behavior.
You think is ok like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think it's fine :-) But perhaps it's better to move it to configdialog.py
. Well, if it's possible because there is where this setting is really needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm ok I will take a look if I find a suitable place there
@ccordoba12 this one should be ready to merge as well |
👍 |
# ensure that when changes are applied, they are copied to a | ||
# specific file storing the language value. This only applies to | ||
# the main section config. | ||
if self.CONF_SECTION == u'main': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this change here. Maybe it's better to just move the CONF.set
line from config.py
here to avoid this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, what I don't understand is why you're looking for main
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
main
is the section in charge of setting the interface language, so the _save_lang
only belongs to that section, before I was using if getattr(self, "_save_lang", False):
but I change it so that it was more explicit.
Since this method runs for every child (any other section or plugin) then I need this to avoid an error of calling _save_lang when it does not exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other way would be to make the save_lang method part of one of the ancestors so that EVERY section would alwyas make that save... but since it is only necessary for main...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too much work :-) Then leave this part as it is now.
Please see if you can move the CONF.set
call from config.py
to here so that we can (finally!) merge this PR ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do! ;-)
@@ -151,6 +165,9 @@ def __init__(self, parent=None): | |||
|
|||
self.setWindowTitle(_("Preferences")) | |||
self.setWindowIcon(get_icon("configure.png")) | |||
|
|||
# Ensures that the config is present on spyder first run | |||
CONF.set('main', 'interface_language', load_lang_conf()) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved here, right at the end of the init :-), should work fine now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merging :-) @ccordoba12? |
Yes!! I'm having a problem with Qt5 but it seems to be with my system rather than with this PR (because I'm also having problems in master :-) |
Add support for language selection in preferences
Thanks for your patience by the way ;-) |
No prob ;-) Ok, @SylvainCorlay, now is your turn :-) Just add language_combo = self.create_combobox(_('Language'), choices,
'interface_language',
restart=True) |
Fixes #1001
Description
Add support for language selection in preferences.
This was a bit trickier than expected, namely the configuration of the language needs to be stored in a separate file, because we cannot use CONF at the stage the
_
function is called.I added a
langconfig
in the .spyder
folder, which stores the language value ('en', 'fr' etc)With this change I had to implement some new logic in the config dialog, so now comboboxes (so far) have an extra parameter call
restart
(by default False).Preferences dialog
Pop up message box
When the user changes one or more options flagged with
restart=True
and then clicks on Apply or Ok, the a message box will popup informing the user which options need to restart and asking if he/she wants to restart right away.If the user accepts, any restart specific (defined) settings will be applied and spyder will restart automatically.
Note:
The message box is now available for any other combobox that is created with the
restart=True
parameter. Adding this functionality to other widgets will be left for another PR.Todo
For another PR