GNOME Bugzilla – Bug 108820
Ability to change language in the GUI on the fly
Last modified: 2007-07-17 05:26:11 UTC
(this is about the graphical greeter but may apply to the standard greeter too) When i represented GNOME Germany on CeBIT (in the OpenBooth in the LinuxPark) this year, i was asked why gdm doesn't have the possibility to choose a language for gdm's ui. The guy who asked me and I came to the solution, that it would be nice if one could choose which language gdm starts by default. Then, if a user selects »his« language from the languages menu gdm should change the ui language on-the-fly. It should also switch automtically to a user's language once he/she entered his/her username.
I'd also love to have an option to choose gdm's startup language and make UI and messages come up in that language. Perhaps, we can have this option set by super-user in gdm-setup's menu. Also, changing ui languages on-the-fly will be high requirement when diffrent language users may share the same system. But, I'm concerned about switching ui when user puts his/her username. To do that, gdm has to know the last login language of the user, but this information shouldn't be read from the user's preference file until user authecation is completed with user's password.
Hi, I'm now working on the fix of this bug and was able to have greeter restarted when users select new language. I'm creating another "login interrupt" protocol passed from greeter to gdm-slave process. gdm.h ----- /* Different login interruptions */ #define GDM_INTERRUPT_REFRESH_GREETER 'R' slave.c ------- static gboolean check_for_interruption (const char *msg) { ... case GDM_INTERRUPT_REFRESH_GREETER: ve_setenv ("LANG", &msg[2], TRUE); ve_setenv ("LC_ALL", &msg[2], TRUE); ve_setenv ("LC_CTYPE", &msg[2], TRUE); ve_setenv ("LC_NUMERIC", &msg[2], TRUE); ve_setenv ("LC_TIME", &msg[2], TRUE); ve_setenv ("LC_COLLATE", &msg[2], TRUE); ve_setenv ("LC_MONETARY", &msg[2], TRUE); ve_setenv ("LC_MESSAGES", &msg[2], TRUE); do_restart_greeter = TRUE; break; ... } gui/greeter_action_language.c ----------------------------- void greeter_action_language () { ... case GTK_RESPONSE_OK: if (dialog_selected_language) { current_language = g_strdup (dialog_selected_language); tmp = ve_locale_to_utf8 (current_language); printf ("%c%c%c%s\n", STX, BEL, GDM_INTERRUPT_REFRESH_GREETER, tmp); fflush(stdout); g_free(tmp); } break; ... } This is at least the starting point and I'm going to finish all the change needed and test them. Chookij V.
Hi George, I have been working on this bug and I have got some snapshots of UI change. I'd like to ask help to review the UI change and any feedback would be really appreciated to get this bug out of the list. Here are the reasons of UI change. 1) Once gdm2 starts up, there is NO-WAY to switch its greeter GUIs to any other languages without restarting gdm2 with the new languages/locales (before users loggining to the system). 2) "Language" menu at greeter is using for the case when users want to have their own sessions running in different languages(locales) other than the language (locales) specified in ~/.dmrc user configuration file and also, greeter doesn't restart/refresh GUI to be displayed in that particular language on the fly. Ex: ~/.dmrc [Desktop] Session=gnome.desktop Language=ja_JP.SJIS In CDE, dtlogin/dtgreeter has the same feature but, dtgreeter is also restarted/refreshed on the fly when users select new language from "language" menu, even the language selected is actually using for users's own session after users's logging-in. I'd like to point out that this behavior in dtlogin/dtgreeter is causing the demand to have gdm greeter to restart greeter's GUI on the fly after selecting new language. The point is that, actually, there are two different things which can be applied with "language". 1) Language applying to greeter (gdmlogin, gdmgreeter) 2) Language applying to users' session Another word is that "Language" menu GUI is a little bit easier to confuse users' behavior between greeter's GUI and users' session GUI. Here are the list of UI change (feature changed/added). 1) Language selection for log-in screen (greeter) and users's session is seperated at the "Language" menu which will have 2 options, "Greeter Language" and "Users' session Language". Attached snapshots: o gdmgreeter_lang_stock_button_action.jpeg.gz When "Language" stock button clicked, there will be the pop-up window for users to select if language is for greete or for user's session. 2) "Greeter Language" will restart log-in screen in new language selected. Attached snapshots: o gdmlogin_greeter_lang.jpeg.gz When selected, gdmlogin will be restarted to the new language selected. There are no "Last" and "System Default" entries. o gdmgreeter_lang.jpeg.gz If user choose language for greeter, language selection window will be pop-uped. There are no "Last" and "System Default" entries. After new language selected and "Ok" button clicked, gdmgreeter will be restarted with new language. 3) "Users' session Language" will have the new extra pop-up sub-menu for experience users to select their preference of "character set" they want for their session, otherwise, gdm2 will assign the default character set associated with language users select for their session. Attached snapshots: o gdmlogin_usr_sess_lang.jpeg.gz When selected, gdmlogin will use the language selected for user's session. gdmlogin won't be restarted. There are "Last" and "System Default" entries. o gdmlogin_usr_sess_charset.jpeg.gz User can choose "character set @ modifier" for particurlar language selected for user's session, otherwise "character set @ modifier" will be default (according to /etc/X11/gdm/locale.alias) o gdmgreeter_usr_sess_lang.jpeg.gz If user choose language for user's session, language selection window will be pop-uped. There are "Last" and "System Deafult" entries. There is new stock button named "Information" at the dialog. When "Information" stock button clicked, users can change "character set @ modifier" of language selected for user's session. o gdmgreeter_usr_sess_charset.jpeg.gz Users can choose "character set @ modifier" for language used within session. 4) A new tab called "Session Languages" tab is for customizing the default detail information of each language for users's session by super-user through the gdmsetup tool. Attached snapshots: o gdmsetup_lang_tab.jpeg.gz This new session language tab is used to change the default "character set @ modifier" for any languages which will be used as the default for users' session if users don't activate "Information" stock button. Please let me know any suggestion. In the next week or so, I'll give you the coding change (patch). Thanks, Chookij V.
Created attachment 21283 [details] Snapshot of UI change in "Language" selection of gdm2
snapshot.tar.gz contains 8 jpeg files. a ./gdmgreeter_lang.jpeg 313K a ./gdmgreeter_lang_stock_button_action.jpeg 282K a ./gdmgreeter_usr_sess_charset.jpeg 311K a ./gdmgreeter_usr_sess_lang.jpeg 308K a ./gdmlogin_greeter_lang.jpeg 171K a ./gdmlogin_usr_sess_charset.jpeg 69K a ./gdmlogin_usr_sess_lang.jpeg 208K a ./gdmsetup_lang_tab.jpeg 87K Chookij V.
Date: Tue, 11 Nov 2003 09:39:38 -0800 From: "George" <jirka@5z.com> Subject: Re: UI change of "Languge" selection in gdm2 To: "Chookij Vanatham" <Chookij.Vanatham@Eng.Sun.COM> Cc: "George" <jirka@5z.com>, "Jerry Wall" <Jerry.Wall@Sun.COM>, Robert.Doolittle@Sun.COM, Chookij.Vanatham@Eng.Sun.COM, gdm@sunsite.dk On Mon, Nov 10, 2003 at 03:34:20PM -0800, Chookij Vanatham wrote: > So right now, first thing I'm really focusing on is delivering > "a sane language-on-the-fly" PATCH in the following ways. > > 1) MINIMAL change to gdm's user interface > There is NO NEED to seperate langauge option between greeter > and users's session. > 2) NO NEED for persistent-greeter-language change > 3) Some text from the slave (including pam) will need to be > in the same/new langauge users select. > (I already did this by having "slave process" change its locale > to the new one: setlocale (LC_ALL, <new language>): then, some > message: ex: "Password" will be displayed in new language). > 4) Text that goes to the syslog needs to be in system language > which is the language when system boots up. > (I'll figure this out) I think 3-4 should be done slightly differently. Instead of just setlocale for the whole slave, remember the language in the slave and only do setlocale to the users language when we're about to say something to the user. Would be perhaps useful to have a 'language stack', so that you can do something like (in the slave) gdm_push_user_language () ... some code here ... gdm_pop_language () Say put this around the pam calls. Those calls could however call some methods that require the system language, thus it would have to be a stack. Perhaps this is not required, perhaps just setting and unsetting the system langauge around all gdm_info/error would be enough. Also you have to make sure to set back to the system language when the login failed or the greeter is reset from the slave (do this in the slave). > Please help me clearify the right behavior when users select option "Last" > langauge. What langauge we should have "greeter" to be restarted on the fly ? I think restarting the greeter on the fly after the user enters their login name would be incredibly annoying. If this is not possible to do without restarting greeter (and it is not with the graphical greeter at least since geometry is managed very insanely there), then we shouldn't switch the language. IMO at least. Loading the greeter can be somewhat expensive, especially over the network. On the other hand we could on the fly switch tha language in the slave, that way at least the slave messages (the actual questions the user answers) can come in translated. Note that you need to make sure to get encodings right. The greeter is all utf8 no matter what the encoding is and the slave is the encoding of the locale. If the language of the slave will be different from the language of the greeter, the slave must somehow also give the greeter the locale it's working in. If I remember correctly communication is done in system locale whatever that is and it's up to the greeter to make it utf8 (but I could be wrong, I'm not looking at the code right now) > Currently, gdm is able to provide list of various charsets > for one particular language by adding the message (Charset name) > after the langauge name (please see snapshots: gdm_various_charsets.rs.gz > for Chinese langauge as an example) but I have got the request that users > don't like long list of the same langauge in many choices. It is up to the administrator to setup the language alias file with only those that make sense. I think in the future we're really moving to all utf8 and languages with more then one possibility of charset will become more and more of a rarity so IMO this is not as useful. Not to mention that as a user I should be presented with a language, not the charset. A regular user will have no clue what a charset is. If you need to regularly use more then one charset something is very broken. George -- George <jirka@5z.com> Then, when you have found the shrubbery, you must cut down the mightiest tree in the forest... with... a herring! -- The Knights Who Say 'Ni'
Any updates on this one?
The giulia cvs project does seem to contain code that might be useful for this purpose.
If there is a patch, please attach it to this bug.
Created attachment 80299 [details] [review] Patch for daemon/gdm.h, daemon/misc.c, daemon/slave.c, gui/gdmlogin.c, gui/greeter/greeter.c and gui/greeter/greeter_action_language.c This is a patch to change the locale of gdmgreeter dynamically. 1. If a locale is choosed on the gdm locale table, gdmgreeter is restarted except for the LAST language. 2. If the choosed locale is different from the system locale or user locale in .dmrc, The confirmation dialog is launched: "You have chosen %s for this session, but your default setting is %s." 3. If user click "Make _Default", $HOME/.dmrc is updated and next gdmgreeter shows that locale. If user click "Just For _This Session", next gdmgreeter revert to the system locale but user's .dmrc is not changed. If user click "Cancel", any session won't be started.
This is a nice change. I think it is a big value-add to be able to change language on the fly. That said, I suspect this patch may need a bit more work before it is ready for production. One obvious thing is that we should really make the code work the same for gdmlogin and gdmgreeter. It is ugly when the two programs behave differently, especially for a feature like this. I notice the patch changes both gdmlogin and gdmgreeter, but I would like you to confirm that the patch makes both programs work the same way. I think it is a bit nasty to restart gdmgreeter without warning. Some users may be in the middle of entering their username/password. It would obviously be nicer if we could teardown and rebuild the GUI in the new language if possible without restarting. However, I suspect this would be much more work and not really worth the effort. But worth thinking about. Perhaps it would be better if after changing the language in the dialog (or menu for gdmlogin), a pop-up appears and asks the user if they wish to restart GDM in the new language in addition to setting the session language. This might be nicer than just changing the language and restarting the greeter on the fly without warning. Or perhaps it would make sense to just add a new menu option that says "Restart the greeter". Users who want to change the language, then restart the greeter will see the new language. This makes it take two steps, but not unreasonable, I think. The language dialog could have a label telling people to choose "restart the greeter" if they want to see the new language immediately. What about the situation where a user by accident changes the language to one they didn't intend? Or if a user changes the language and then leaves GDM running in a different language. Is it easy to figure out how to change it back? Perhaps it would be nice to have something that returns GDM to its "default" state? My idea of adding a menu option that "restarts the greeter" might meet this need. Also, does it make sense for this to be configurable? Perhaps there should be a configuration setting which is true/false for supporting gdmgreeter restarting in different languages.
Created attachment 81731 [details] [review] Revised patch from the feedback of #80299 OK, I've updated tha greeter locale patch. > One obvious thing is that we should really make the code work the same for gdmlogin and gdmgreeter. It is ugly when the two programs behave differently, I have compiled some dupilcated functions between gdmlogin and gdmgreeter. I had two problems in this request. One is process_operation() is hardly unified between login and greeter because there are several static variables in both gdmlogin.c and greeter_action_language.c so I unified GDM events only in process_operation(). Another is the current lang menus in gdmlogin is not good for l10n. i.e. it has "A-M" and "N-Z" but most translators keeps the tag as English then the languages are separated by two groups without any correct collations and I also think it's impossible to separate whole the UTF-8 code maps by two groups. (Actually it's CR 4990120 in opensolaris bug). I'm ok with the suggested patch. What do you think? > I think it is a bit nasty to restart gdmgreeter without warning. OK, I added the confirmation dialog. Could you have a look at that? I feel a little risk to change UI message only without restarting. Since gdm has the parent/child processes structure and the parent is the daemon, is it reasonable to restart greeter? > Perhaps it would be better if after changing the language in the dialog (or menu for gdmlogin), a pop-up appears and asks the user if they wish to restart GDM in the new language in addition to setting the session language. Thanks for the suggestion. I also guess if user click "OK" button with Shift key event, it may be useful to restart greeter without the confirmation. > What about the situation where a user by accident changes the language to one they didn't intend? Or if a user changes the language and then leaves GDM running in a different language. My suggestion is to choose "Default" language, then gdmgreeter is restared on the system locale. > Also, does it make sense for this to be configurable? I think this is a user preference but not an admin one so the configuration would be done on the greeter menu. Could you review the patch? Thanks.
The patch looks okay to me. Since we are passed UI freeze, though, and since this patch changes the behavior in a significant way, I'd prefer to add this to GDM 2.19. Takao, if you want me to add this to our Solaris 2.18 internal builds for Solaris users to have this feature sooner, then let me know and I'll patch our release to add this feature to 2.18 Solaris.
Thanks for the review. If you could add the patch in Solaris internal build, please go ahead. I also thought the same plan. I just noticed gdm-05-system-locale.diff is not upstreamed yet in Solaris build. Are there any reasons? My patch expects the gdm_system_locale variable.
*** Bug 411695 has been marked as a duplicate of this bug. ***
Coming from bug 411695, I need to ask if this only applies to the login screen or if there would be a way to change locale when already logged into a session (that was the purpose of my bug report, and if it's a different issue, I would have to reopen it I guess)
Jeff, Sorry I misunderstood the bug 411695 - I thought you were talking about language support in GDM since you filed a GDM bug. However, if you are talking about changing the language in your GNOME session on-the-fly, then this isn't really a GDM bug. I'd file a bug against the gnome-session module, since that is the program that manages your session.
Thanks Takao. I filed bug #417997 to reflect the 3 dependant patches that we want to go into GDM for this patch to work. This way others can review these patches.
Takao, this patch is now in our Solaris builds, so please test this when the next vermillion-devel comes out and verify it works okay for me. Thanks.
I have some minor issues with the patch after further review: - I would rename the function greeter_language_append_restart_menu to greeter_lanaguge_append_lang_menu or something. Having "restart" in the name makes it sound like it is being used for the "Reboot/Restart" feature. - The same function you are calling before greeter_system_append_system_menu. I'd call it after so it appends the radio buttons at the bottom of the menu. It looks bad if you set SystemMenu=true in the config file and you have menu options above and below the radio buttons. - You define restart_grp in gdm_login_language_menu_new but it doesn't seem to be used. - You seem to only have added the "Restart" radio button to gdmgreeter and not gdmlogin. Please add it to the gdmlogin menu also, at the bottom. - The new menu choices are: "Always _Restart Login Dialog" "Ask _To Restart Login Dialog" It isn't clear at all that these features only apply to when you change the language. It should be clear that these only apply to when you change the language. In fact, I'd think it might be nicer to just pop-up a dialog after the user changes language to ask them if they want to restart or not rather than having this as a menu radio button choice. For example, if the user has it set to "Ask" and changes the language, then it doesn't restart. The user then realizes they wanted it to restart, so they have to change the radio button choice, then change the language to something else (causing a restart), then change it back to the language they really want (causing a 2nd restart). Isn't this a bit ugly. Why not just pop-up another dialog and ask "Restart or no"?
Sorry for the late response. Our Japanese staffs had a office moving a week ago and I'm back two days ago. Thanks for your integration. I'll investigate your queries asap.
Created attachment 84566 [details] [review] Patch over patch id=81731 OK, the request seems to remove greeter_language_append_restart_menu(). What about the attached patch?
I think this enhancement also can resolve bug 415600.
I'm confused by two things: 1) The updated patch simply removes the code to add the radio buttons from the menu, but doesn't seem to add any new code. Are you just assuming the code will always restart the GUI now? I thought we agreed that it would be better to pop-up a dialog to ask the user if they want to Restart or not after changing language. Please clarify how you think this should work. Do you not think such a dialog is needed? 2) How does this address bug 415600? It doesn't seem to change how the grouping is done at all. Or did you attach the wrong patch? I'd think the updated patch would also add some code, not just remove the two radio buttons.
Sorry, I might not understand your request. > 1) I thought greeter_language_ask_restart() satisfies the function of the pop-up dialog which asks restarts. Are there any differences between your pop-up dialog and greeter_language_ask_restart() ? The radio button's function was whether the pop-up dialog itself is launched or not. > 2) The patch replaces gdm_login_language_handler() with greeter_language_handler(), then the language menu is replaced with the language dialog so it no longer use the grouping. The current menu separation does not work on locales but one dialog with the scroll bar works fine with any locales as gdmgreeter has ever done. It also supposes the request of removal of the duplicated codes: >>> One obvious thing is that we should really make the code work the same for
Created attachment 84709 [details] [review] updated patch This patch combines the two patches and applies cleanly against 2.18.
I just took a look at how the new patch works, and I'm not really happy with how the pop-up operates. I notice after you change language it says: "Do you accept to restart login to change the locale of login next time?" This sentence doesn't make much sense, especially since the button choices are "Just for this session", "Cancel" and "Make default". It seems that there really are 2 things we are asking for: 1) Do you want to restart GDM or not with the new language 2) Do you want the language to be your default language or only for one session. I also notice that if you pick "Just for this session" in the dialog, and then authenticate, it again throws up a popup asking you if you want the new language for "Just this session" or "Make default". This seems really annoying. I'd think that after changing your language in the language selection box it should ask you simply: "Do you want to restart GDM in the new language" with "Yes and No" buttons. Then after you authenticate it should ask you the "Just for this session" or "Make default" choice. What do you think?
I'd like to get these last issues fixed so this patch can go into the new 2.19 branch. Can you update the patch?
I have a question. > "Do you want to restart GDM in the new language" with "Yes and No" buttons. If you select "No", what do you expect? As you know, the previous suggestion is: "Just for this session" -- will ask next time "Cancel" -- don't restart. don't change languages. "Make default" -- restart. change languages. I'ld like to ask whether you think the "No" change languages or not. It was an reason I added "Just for this session" since I might not understand the request. Another concern is that the dialog is for anonymous users before users input their userID. It means if the first user agreed click "Yes", the second user cannot choose "No". It was an reason I added the radio button in greeter/login. What do you think?
My suggestion is that after changing the language in the language dialog it asks you: "Restart GDM with the new language?" If yes, it is restarting with the new language. If no, then GDM does not restart and keeps running in the same language. The 3 choices in the menu mean the following: - Just for this session - This choice avoids saving the language in your $HOME/.dmrc file so that future logins will have your old language choice. This is useful for some cases, like if you are a tester and you want to occasionally log into different language for testing. Or if you speak multiple languages, and use different languages on occasion. - Cancel - Go back to login. - Make default - save the new language selection in your $HOME/.dmrc file so that future logins will be in this new language by default. Perhaps you didn't understand the purpose of this dialog since you seem to think it has something to do with whether GDM should itself restart or not? So what I'd expect to happen is this: 1. I change the language in the language dialog 2. I'm asked if I want to restart greeter. If I pick yes, then GDM restarts in the new language. If I pick No, then GDM doesn't. 3. I authenticate 4. GDM reads my $HOME/.dmrc file and checks if my default language is the same as picked in the language dialog. If not, then I'm asked if I want the new language to be my new default (and put in my $HOME/.dmrc file) or if I want to use this new language just for this session, or if I want to "Cancel" to go back to the login screen and pick a different language. I'm not sure I understand your concern about anonymous users. If one user changes the language for GDM and leaves it in a different language, then this could be annoying for the next user if they can't speak this language. The second user would need to figure out how to navigate to the language dialog and change the language back in this case. I doubt this situation would happen often, though. If we want to nicely deal with that sort of problem, perhaps adding a button that restarts GDM would be good so the user would just have to know to hit the restart button to get GDM to restart in the default language? Or perhaps GDM could timeout after a certain amount of inactivity and restart into the default language when the user changes language but then doesn't actually login.
Created attachment 85033 [details] [review] Patch over patch id=84709 Thanks for your explanation. I understood and agreeed with your suggestion. How about this attachment?
Takeo, thanks. I'll look at this. Some other localization issues that should be looked at are. Could you check these? bug #109837 bug #167650
Created attachment 85042 [details] [review] attaching an updated patch that applies cleanly against head. Thanks for the updated patch. This does work better. I made some changes to your patch: - The dialog now says "Do you wish to restart %s with the chosen language" I think this is better wording. - The dialog 2nd message says "You will restart %s with the %s locale." Also better wording. - The dialog now has choices "Yes" and "No" instead of "OK" and "Cancel" which I think is more appropriate. - I removed references to restart_item from greeter_action_language.c since I think this references the radio button we removed. Can you verify that these changes are okay. However, I still have two issues with this patch. - The most serious issue is that after you pick "Yes" in the "do you want to restart" dialog, you are never asked if you want to restart it again. Even if you login and logout, change the language, it just restarts without showing the pop-up. How do you change the language again if you never get asked again? I would think we would want to ask if the user wants to restart *every* time they change the language. - I notice you include greeter_action_language.h and greeter_configuration.h in gdmlogin.c, and make use of the greeter_language_handler(), greeter_language_initialize_model(), greeter_language_op_lang(), greeter_language_op_slang(), greeter_language_op_setlang(), greeter_language_op_always_restart() functions. Note the file gui/gdmlanguages.[ch] is intended for common language code used by both gdmlogin and gdmgreeter. It would be cleaner to move the common code to this file so that gdmlogin doesn't need to link against greeter code, and we should avoid building gui/greeter/libgdmgreeter.a. The logic for making greeter/libgdmgreeter.a in gui/Makefile.am is gross. Could you fix this?
In other words, I think just because one user picked "Yes" to the dialog, this shouldn't mean that the value should stay "Yes" forever. It would be okay for it to ask all the time, or for it to start asking again after you logout and log back in.
> - The most serious issue is that after you pick "Yes" in the "do you want In fact, I meant the same issue as the anonymous users. > shouldn't mean that the value should stay "Yes" forever. It would be okay for it to ask all the time, or for it to start asking again after you logout and log back in. Personaly I prefer asking per login to asking every time. > - I notice you include greeter_action_language.h and greeter_configuration.h Hmm.., one problem is that greeter_language_handler() has the deep dependencies. (Maybe GreeterItemInfo) +libgdmgreeter_a_SOURCES = \ + greeter_action_language.c \ + greeter_events.c \ + greeter_item.c \ + greeter_item_customlist.c \ + greeter_parser.c \ + greeter_session.c + I felt it was a bit complicated to separate the source files as far as I remembered.
I don't think there are deep dependencies. Note the GreeterItemInfo is passed into greeter_language_handler and only used to pass it into greeter_language_ask_restart, which doesn't use the variable. Why not just get rid of this variable that is unused from the function call? Then you should have an easier time moving it. I'm okay with fixing GDM so it asks the dialog after you log out and log back in, but not every time you change language. I notice it has the following behavior: If I pick "No", it asks me every time even if I change language again If I pick "Yes", it stops asking me if I change language again Does it make sense for it to keep asking if I say No, but not when Yes? I'm not sure here.
> I don't think there are deep dependencies. Note the GreeterItemInfo is passed I need your help. greeter_language_handler() is called by greeter_item_register_action_callback() and greeter_item_run_action_callback() assignes GreeterItemInfo. What do you think to remove GreeterItemInfo in the callback in greeter_item_run_action_callback? > I notice it has the following behavior: I'll look at it later.
Note that greeter_language_handler does not actually use info. It only has this argument because the language handler prototype requires it. I'd split this into 2 functions. The first takes the *info argument and is registered as the handler to meet the prototype requirement, and all it does is call the 2nd function which does not take the argument. This way the 2nd function can be moved to gui/gdmlanguages.c, and the 1st function can stay in gui/greeter. Does this make sense?
Created attachment 85519 [details] [review] Patch over patch id=85042 OK, I updated the patch. Could you review the attachment? - Agreed to separate greeter_language_handler(). - Moved greeter_action_language.[ch] to gdmlanguages.[ch] and changed the function names "greeter_language_foo()" to "gdm_lang_foo()". - Added the logic to ask restarts again after users log into the sessions. - Changed "Do you want to restart %s" to "Do you wish to restart %s". Maybe it's typo. - Small fix in gdm_lang_handler() I think the patch does not resolve the external libgdmgreeter.a yet. It needs greeter_custom_set_language().
Takao: This looks much better than before, but in testing I notice one serious bug: + When you pick a different language, it asks you if you want to restart GDM with the new language. If you pick "No", then after you authenticate it doesn't ask you if you want this language to be your "default", or "just for this session", etc. So if you pick "No" for restart, it acts like you don't want the language selection to be used at all. Shouldn't it still honor the language choice even if the user doesn't restart GDM? + It seems like you didn't run "make clean" and remake GDM after making the above changes. This is something you should do always, especially after making any changes to header files. I had to make many changes to your patches to get the code to compile because you didn't do this. For example, some files in gui/greeter were still including the now removed greeter_action_language.h file. + I fixed the code so it no longer needs libgdmgreeter.a. You did most of the work moving the code to gui/gdmlanguages.[ch], but I addressed the issue of greeter_custom_set_language. Basically the intent of this function is to act as a callback so that the GUI can do extra things when the language changes. For the greeter this is needed because it supports the language combo box, and is needed to keep them in sync. To fix this I simply added a similar function to gdmlogin that does nothing. I changed the function name to lang_set_custom_callback since this is more general than having "greeter" in the name. Speaking of this, does your code work reasonably if the user changes languages via the combo box? Note that the default themes (circles and happygnome have the language combo box in the lower left near the Options button. So you can change language just by using the combo box. I'd think that picking language this way should also ask the user if they want to restart GDM. I'm not sure it does. I haven't tested. It may work okay since it internally causes the selection in the treeview to get modified as well, but you should test. If you have only tested with the Sun Nimbus theme, you probably haven't noticed this feature. I believe that the Nimbus theme will use this in the future since it is in the Coolstart spec, though. I have committed your patch to SVN head with my fixes. Could you please provide future patches against SVN head? Also, in the future, I don't really like patches built on top of other patches. They are often hard to apply.
Takao: Some good news and some bad news. The good news is that I fixed all the problems I identified above. Including: - I tested with the language combo/list style selection that you see in the default GDM theme, circles, or with the happygnome/happygnome-list themes. The gui/greeter/greeter_item_customlist.c function needed to be enhanced to call the same logic to pop-up the dialog and set the language. I ran into a bit of trouble getting this to work because changing the language in the dialog causes the combo/list to reset its value and this was causing GDM to fall into an infinite loop since it also wanted to call the logic to pop-up the dialog again. I fixed this by changing the logic so that if the set_lang function is called with the same language already selected, it simply avoids popping up the dialog and does nothing. This means if you select the same language multiple times in the dialog or the combo box, it only asks you if you want to restart the first time. This might be a little less friendly for users who mean to pick "Yes" but accidently pick "No", then the user must pick a different language and pick the language they really want and select "Yes" this time. Perhaps a bit annoying, but reasonable, I think. - I also fixed the code so that when you pick "No" it actually calls gdm_set_lang but doesn't restart the greeter. This was easy to fix. The previous logic was only calling gdm_set_lang if the response was YES. I just took out this if-test. In other words, now picking YES or NO in the dialog causes the language to be set for the user session. Picking YES now just causes GDM to restart in the selected language and picking NO causes the language only to be used in the user's session. I think this is the behavior we want. So, now, I am happy with how the code works in SVN head. Perhaps, over time, additional improvements will be made to the code, but I think it is working reasonably now. The bad news... Unfortunately, the way this patch made it into SVN head was really messy. You never really provided a working patch. Instead you provided patches on top of patches, and I ended up fixing the last remaining issues separately. The patches-on-top-of-patches caused a bad merge and caused bug #427022 which was fixed in SVN head by William Jon McCann. I know you wanted to get this into GNOME 2.16 so we could patch our Solaris build to have this feature sooner. However, I think it would be a good amount of work to backport this to GNOME 2.16 and have it work properly. Currently we are applying a broken older version of this patch in our GNOME 2.16 builds. Since it doesn't work, I am now taking this patch out of our Solaris builds. If you can provide me with a GDM 2.16 patch that works and has all the features in SVN head, then I will readd it to our GNOME 2.16 builds. Otherwise, I think we need to wait until GNOME 2.20 before this fix goes into Solaris. Takao, what do you think? I am closing this bug since the issue is fixed, but we can continue discussing a GDM 2.16 patch in this bug report, or you we can take this offline and discuss via email if you like.
Sorry for the late response. I'll get time in this weekend. > + It seems like you didn't run "make clean" and remake GDM after making the above changes. This is something you should do always, especially Sorry, It's obviously my failure. I skimped the work a little. > but I addressed the issue of greeter_custom_set_language. Basically the intent of this Thanks much for this help. > - I also fixed the code so that when you pick "No" it actually calls gdm_set_lang but doesn't restart the greeter. This was easy to fix. The Hmm.., probably I don't understand you mean. I asked the issue prevously. >> I'ld like to ask whether you think the "No" change languages or not. I'll try to get your change and understand the meanings later. My personal problem is that my ITops runsocks script does not work with the external DNS and IP address, runsocks svn co http://82.211.81.213/svn/gdm2/trunk, so I just checked your change via web browser. > - I tested with the language combo/list style selection that you see in the default GDM theme Great, thanks much for your fixes. I haven't checked the other themes so much. I'll check your change later. > Also, in the future, I don't really like patches built on top of other patches. They are often hard to apply. Sorry, I completely misunderstood the patch request, I thought you like the patch on patch. I have worked in Sun build sources instead of SVN HEAD. I'll provide the patch for SVN. But I need to fix my socks issue.. > The patches-on-top-of-patches caused a bad merge and caused bug #427022 which was fixed in SVN head by William Jon McCann. OK, that's a problem. Did you reproduce the bug? I'll get time in this weekend.
Takao: I explained before that the dialog that asks if you want to restart GDM in the new language should *only* determine whether GDM is restarted in the new language. Regardless of the language picked, the choice should be applied to the user's session after they authenticate. I hope this makes more sense. So this is why I fixed the code so that regardless of whether you answer Yes or No to "Do you want to restart GDM", it still calls gdm_lang_set. The only difference about picking "Yes" is it causes GDM itself to restart in the new language. I hope this is more clear. Yes, in the future (e.g. if you backport the fix to GDM 2.18, please test with the default GDM themes (circles, happygnome, happygnome-list) and make sure the feature works with the language combo selection list (you will see it in the bottom panel). I fixed it in SVN head so it works properly. If you have trouble getting the code from SVN head, let me know and I can send you a tarball that contains the latest code for you to work with. Do you have a GNOME SVN account? If so, I can show you how to access from in the SWAN? I don't know how to access GDM SVN anonymously from within SWAN. I only know if you have a GNOME SVN account. If you don't have a GNOME SVN account, you should apply for one. You have done enough work on GDM to warrant having an account, I think. The process for asking for an account is here: http://live.gnome.org/NewAccounts?highlight=%28accounts%29 If you have an account (or if you get one), send me an email at Brian.Cameron@sun.com, and I can explain this to you off line. Not much sense having this discussion in a bug report. :) It's okay that you sent patch-on-patch. I think I was able to get things working with this, but in the future, let's not do things that way. It's better to update with a single patch that works. It's easier to avoid merge conflicts. Yes. Bug #427022 is now fixed. I was able to reproduce it, and it is fixed after Jon made his fix.
Created attachment 88610 [details] [review] Patch for system locale Thanks for your help. Now I can look at gdm SVN. I tried to use it but the system locale does not work as I expected. Could you review the attachment?
Created attachment 88611 [details] [review] Patch to have the tests for gdm HEAD on Solaris When I tried to use HEAD, gdm caused SEGV on Solaris SPARC because "lo_ss" is NULL. I guess this is a know issue but just heads up. Also gdm_lang_set() does not work as I expected. the "language" parameter is suddenly changed from "ja_JP.UTF-8" to "" in gdm_lang_set. When I commented out gtk_tree_view_scroll_to_cell(), gdm_lang_set() works fine. I'm not sure if you also encounter the same problem.
Takao: I applied your patch for the system locale, and also the patch to fix the problem with lo_ss being NULL. However, I didn't apply the fix to correct th eproblem where the parameter is suddently changed from a valid value to "". I think more research is needed here. Could you identify more specifically what is causing the scroll to cell to be resetting the value to ""? Are you using gdmlogin or gdmgreeter? If gdmgreeter, then what theme are you using when you see this problem? Is it the circles or happygnome themes which have a language change combo box? I suspect the problem may be in the code that tries to keep the language combo box and the language dialog in sync, so if you change the language value in one it updates the other. Note that these functions are listening to "change" events so the scroll that you comment out is probably causing the "change" to not get triggered...so this is probably not the right fix. The logic is in gui/greeter/greeter_item_customlist.c. Note that we have the following logic in setup_combo_customlist. This is called when the combo changes and it calls gdm_lang_set_restart_dialog to update the dialog. g_signal_connect (G_OBJECT (combo), "changed", G_CALLBACK (combo_selected), item); Also, the lang_set_custom_callback function manages when the dialog changes to update the combo box. There may be something funny going on here which is causing the value to get unset. Could you take a closer look at this part of the code and let me know if you find what is causing the problem?
Created attachment 89531 [details] Reference of my build Thanks for your quick fixes. > However, I didn't apply the fix to correct th eproblem where the parameter is suddently changed from a valid value to "". Probably my comment @ 2007-05-22 did not describe my problem exactly. I'ld like to clarify if my build is ok. The attachment is the diff to build GDM SVN HEAD with OpenSolaris build files. 1. Install Solaris 11 build 65 SPARC. 2. Install the latest JDS AKA Vermillion_66. 3. Check out GDM HEAD and archive gdm2.tar.bz2 with the SVN tree directly. (I didn't use make dist.) 4. Build GDM HEAD with the attachment and replace SUNWgnome-display-mgr only. 5. Launch GDM and change the local theme of gdmgreeter to "Sun Nimbus" with gdmsetup. (as the default is gdmlogin.) My problem is gdm-binary in SUNWgnome-display-mgr doesn't change the locale of gdmgreeter when I change the locale with the language combox. However I see my problem can be resolved with the rebuilt gdm-binary: # cd gdm2/daemon # touch slave.c # make # svcadm disable gdm # mv /usr/sbin/gdm-binary /usr/sbin/gdm-binary.orig # cp gdm-binary /usr/sbin/gdm-binary # svcadm enable gdm The manual make is ok but the gdm-binary with pkgbuild is strange. It means I can reproduce my problem whenever I revert the gdm-binary.orig I'll try to see my problem later.
Takao. It looks like you are not using the latest gdm.spec file from spec-files SVN head. The changes that you have made there are already in spec-files SVN head - are you using the 2.18 branch or using the (slightly old snapshot of spec-files from opensolaris.org)? For doing 2.19 work, I recommend using spec-files svn head. Contact me via email if you don't know how to access spec-files svn head and I'll give you instructions.
Created attachment 89533 [details] Reference of my build Sorry, I didn't update gdm.spec since I actually used GDM SVN HEAD. I updated the reference at the moment.
Sorry for bothering you. It may be better to file another bug. > However I see my problem can be resolved with the rebuilt gdm-binary: It was not true. I still can see the problem after touched files. It seems the behavior is different by the system locale. I put LANG=C in /etc/default/init It seems the behavior is also different by locale which is choosed in lang box. I have choosed French, Japanese, Korean and Chinese. > g_signal_connect (G_OBJECT (combo), "changed", > G_CALLBACK (combo_selected), item); It seems combo_selected is not called when I change the language. > Also, the lang_set_custom_callback function manages when the dialog changes to update the combo box. Now I also found the lang_set_custom_callback() and gdm_lang_set() has the similar codes. I still don't know why the language parameter is changed to "" at the gtk_tree_view_scroll_to_cell() line. All I noticed is lang_set_custom_callback() returns soon because it's not initialized yet.
Takao I'm sorry it has taken me so long to respond. I was on vacation for a few weeks this month, and have been busy working on other things. Could you retest with the latest GDM 2.19 code using GNOME from here: http://dlc.sun.com/osol/jds/downloads/current/ And tell me exactly what problems you are seeing. Is the only problem you are seeing is that the language parameter is changed to "" or are you also seeing other problems? I cannot replicate this problem. Does the problem only happen if you set LANG in /etc/default/init? It would be very useful to me if you could explain how to recreate the problem so I can see it, then I could help with debugging it. I don't see this problem. If you are using the Nimbus theme, then this theme doesn't have a combo dialog for changing the language list. Therefore it is expected that it would notice that the lang_set_custom_callback function would return immediately since language_widget should be NULL and it will return immediately. Therefore I suspect the problem isn't related to the combo-box and language dialog having issues syncing the data. Perhaps you could test with the circles or happygnome gdmgreeter themes. These do have the combo box. I'd expect you'ld like see the same problem with these themes? Let me know. Therefore I suspect the problem has something to do with gui/gdmlanguages.c in the code for gdm_lang_set. This function must be being called again. Perhaps the function selection_changed is being called again for some odd reason. Note the code looks like this: static void selection_changed (GtkTreeSelection *selection, gpointer data) { GtkTreeIter iter; if (gtk_tree_selection_get_selected (selection, NULL, &iter)) { g_free (dialog_selected_language); gtk_tree_model_get (GTK_TREE_MODEL (lang_model), &iter, LOCALE_COLUMN, &dialog_selected_language, -1); } } Perhaps adding some protection code and if gtk_tree_model_get returns "" for some odd reason we shouldn't be freeing dialog_selected_language and resetting it? Does this help? I wonder if an extra bad selection changed event is happening on the widget causing a problem. Might be worth running a debugger to see how dialog_selected_language is being managed here.
Created attachment 91471 [details] [review] Patch for gui/gdmlanguages.c Your observation is right. After I applied the attached patch, my problem is fixed. Could you review this patch?
This patch looks wrong to me, and I'm confused how this fixes your problem. When you say that my observation is right, could you explain which observation I was right about? I'm not clear. Looking at the code, I don't see any place where the code frees dialog_selected_language in an inappropriate way. The docs for gtk_tree_model_get say the following, which say that it is appropriate for the GDM code to free the value. --- For example, to get a value from column 0 with type G_TYPE_STRING, you would write: gtk_tree_model_get (model, iter, 0, &place_string_here, -1), where place_string_here is a gchar* to be filled with the string. If appropriate, the returned values have to be freed or unreferenced. --- Perhaps what the code should be doing is getting the value into a temporary value, then checking the temporary value to ensure it isn't empty or NULL. Then it should only free and reset dialog_selected_language if the gtk_tree_model_get function returns a valid value. Does this work? If so, it would be a cleaner patch I think.
As you may notice, it works fine from the point of the allocation of the dialog_selected_language variable with my patch, i.e. the transition of value is NULL -> g_free() -> g_strdup() -> g_free () -> g_strdup() ... : static void selection_changed (GtkTreeSelection *selection, gpointer data) { ... g_free (dialog_selected_language); gtk_tree_model_get (GTK_TREE_MODEL (lang_model), &iter, LOCALE_COLUMN, &dialog_selected_language, -1); if (dialog_selected_language) dialog_selected_language = g_strdup (dialog_selected_language); ... } Yes, I also noticed the doc of gtk_tree_model_get(). If I understand correctly, it seems gtk_tree_model_get() does not allocate the new heap but just returns the pointer. However since we continue to refer the LOCALE_COLUMN values in gdm_lang_set(), I think we need a new allocation for dialog_selected_language by clicking language names on the language chooser dialog. void gdm_lang_set (char *language) { ... do { gtk_tree_model_get (GTK_TREE_MODEL (lang_model), &iter, LOCALE_COLUMN, &locale, -1); } } while (gtk_tree_model_iter_next (GTK_TREE_MODEL (lang_model), &iter)); ... } I think, if we should free the strings in lang_model, we could do in a destroy function. I noticed gdm_lang_initialize_model() uses gtk_list_store_set() and it allocates heaps for the LOCALE_COLUMN values internally but I think we don't have the destroy function for the lang_model at the moment. free(3C) has no action if the argument is a null pointer. What do you think?
I think something weird is going on here, and we probably need to do some more research. Your analysis doesn't really match the code exactly. For one thing, the tree model and the treeview that uses it are both never freed (at least until gdmgreeter exits when all the memory is freed). It is created once and reused each time the language dialog is displayed. Note that the function gdm_lang_initialize_model is only called once from main() and not each time you display the language dialog. You suggest that gdm_lang_set frees the memory and that we should instead free in a destroy function. Looking at the gdm_lang_set code we call gdm_tree_path_free, but this only frees the GtkTreePath, which I believe is appropriate. Note the docs for gtk_tree_model_get_path says you need to free the path after using it. Or am I missing something here? I suspect that something else might be going on, perhaps some memory corruption that adding the extra g_strdup hides.
> Your analysis doesn't really match the code exactly. Hmm.., I may not understand what is matter in my view. > For one thing, the tree model and the treeview that uses it are both never Do you mean even though we add the destructor for lang_model, it does not work as I expected since "gdm_lang_initialize_model is only called once from main() and not each time you display the language dialog" ? If so, we may need to move the initialization of lang_model to a new constructor. > You suggest that gdm_lang_set frees the memory and that we should instead free in a destroy function. Yes, GtkTreePath is freed in gdm_lang_set but I don't mention that point. I mean gtk_tree_model_get() is used in gdm_lang_set() for LOCALE_COLUMN variable and also the same function is used in selection_changed() so I think we need the g_strdup() for dialog_selected_language so that we don't free any values in LOCALE_COLUMN variables. My understanding is gtk_tree_model_get() does not allocate new heaps as I mentioned the previous thread. > I suspect that something else might be going on, perhaps some memory corruption that adding the extra g_strdup hides. Probably I don't understand your point or I'm not sure you may miss something?
Very sorry, my comment was wrong. It seems gtk_tree_model_get() allocates a new string. I'll provide the better fix.
Takao. Yes, since gtk_tree_model_get returns a new string, I'm not sure why your proposed fix actually fixes the problem. Also, we don't anywhere set a destructor for lang_model. I don't think the code actually ever throws away this tree model. It just allocates it once and reuses it over and over each time you pop-up the language dialog. So, I'm wondering if there is memory corruption or something going on that the extra g_strdup is hiding, or if there is something going on that we just don't understand yet. It would be good to do a bit more analysis to see if we can understand why it is failing. I appreciate that you are looking into this.
Created attachment 91599 [details] [review] Patch for gui/gdmlanguages.c I updated the patch. selection_changed() is called by gtk_tree_selection_unselect_all() and gtk_tree_selection_select_iter() in gdm_lang_set(). The address of dialog_selected_language is changed by selection_changed() so the address of language in both gdm_lang_set() and gdm_lang_set_restart_dialog() is not correct. I think the attached patch is the easy fix. What do you think?
Ok, now fixed. Thanks for helping to look into this problem. I think we finally figured out exactly what was causing the problem, and this patch looks good to me. This bug report is really long with 59 comments. So if you find more issues with language selection, lets open new bugs rather than opening this one. Ok?