I would accept a patch to fix gdmlogin so it resizes in a better way.

I cant replicate this error for some reason. I'm using gdmsetup launched from within gdm. Changing the logo and clicking on Close does not resize the greeter (I believe logo change event does not cause restart of the greeter). Manually restarting greeter seems to be resizing everything correctly. Can you confirm that either the error still exists in the latest SVN trunk?

Lukasz, are you sure you are using gdmlogin and not gdmgreeter?  gdmsetup should be set so that the Local greeter is set to the "Plain" style.

Note in gdmlogin function gdm_reread_config that there is some code that causes it to put the new logo in the GUI without restarting after it recieves the notify.

So, if the logo isn't changing, then this might mean that the notify signal isn't being sent?  If you look in daemon/gdmconfig.c it looks like we don't pass along the notify event in _gdm_set_value_string in the "/* Handle update */ section.  Probably adding a notify here would cause the notify event to get propegated.

Though fixing this would likely make the error originally reported start to show up again.  Note that after 2.8 was when the config system was rewritten, so the notify getting lost was probably something that happened at that time, oops.

Hope this helps.

Thx :) that is what i thought. I will change the way the notification works just for the sake of fixing/looking into this bug (and i will not submit those notofication changes). I think this part of configuration is non-crucial to the functionality of the deamon and indeed should not cause daemon restart

No, you misunderstand.  The restart is the GUI restart.  In other words, we *want* the GUI's to restart if necessary to support configuration changes as they happen.

That said, if it is possible to code the GUI program so it doesn't need to restart and just start working with the new config option, it's nice to avoid restarting.  

This is why most config parms in the gui/gdmlogin.c code just say "restart if change".  But it looks like someone made the extra effort to avoid the restart for LOGO.  If it is easy to support changing the LOGO without restarting the GUI, then this is nice.  But if it is a pain to make it work, then rip out the code and just make it restart like the others.

Make more sense?

>No, you misunderstand.  The restart is the GUI restart.
Sorry my bad

>That said, if it is possible to code the GUI program so it doesn't need to
>restart and just start working with the new config option, it's nice to avoid
>restarting.  

Hmm indeed it should resize the window once the logo has been changed, but it didnt in my case (maybe the appropreate signal wasn't sent to the greeter).

>If it is easy to support changing the LOGO without restarting the
>GUI, then this is nice.  But if it is a pain to make it work, then rip out the
>code and just make it restart like the others.

Yup changing the logo without the restart seems like a more plausable option

Did you add the notify call to daemon/gdmconfig.c?  I assume the photo will update after making this change, unless the code in gdmlogin gdm_reread_config is broken.  

It's probably okay to just make the gdm_reread_config restart the greeter if the logo notify is received.  It's slightly better to make it work without restart, but not a big deal if it is hard to code, or get working properly (e.g. if it is hard to make the GUI resize properly if the new logo is smaller).

>Did you add the notify call to daemon/gdmconfig.c?  I assume the photo will
>update after making this change, unless the code in gdmlogin gdm_reread_config
>is broken.  

Actually my previous claims about notify function not working were bogus (and were caused by my own stupidity), but i think the code in gdm_reread_config is broken. It does seem to get executed - pixbuffer gets created, then set into the logo image, dimensions are collected and appropriate resize call is executed but to no visual change.

>It's probably okay to just make the gdm_reread_config restart the greeter if
>the logo notify is received.  It's slightly better to make it work without
>restart, but not a big deal if it is hard to code, or get working properly
>(e.g. if it is hard to make the GUI resize properly if the new logo is
>smaller).

Yup restart should be the last resort. Im sure we can get this code working nicely :)

I have found the reason why gdm_reread_config wasnt working properly. GDM_KEY_LOGO seems to contain the old value even though the new value has been changed in the custom.conf file (maybe daemon does not re-read config files in the case of that entry?).

So you are saying that when you run:

   gdmflexiserver --config="GET_CONFIG greeter/Logo"

you get the old value or the new value?

You should get the new value.  If not, then perhaps gdmsetup isn't calling UPDATE_CONFIG properly to let the daemon know to reread the configuration value.

Or are you saying that just the GUI thinks the value is the old value even though the above gdmflexiserver command shows the new value.  If this is the case, then I'd bet you aren't calling notify_displays_string properly.  If you think you are, then I'd turn on enable=true in the [debug] section of the configuration file and see what is going on here.  The notifications and communiation between client and daemon should be echoed to the log so you can follow what might be going on here.

Note that in the gdmlogin gdm_reread_config we call gdm_config_reload_string which should ask the daemon to give it the new value using the same gdmflexiserver command I mention at the top of this comment.   When you do this, it should update the internal cache with the new value.  Maybe something is wrong here, but odd if it just affects this key and not the other keys.

>So you are saying that when you run:
>   gdmflexiserver --config="GET_CONFIG greeter/Logo"
>you get the old value or the new value?

This seems to work correctly i do get the new value.

I have put some debug statements within gdm_reread_config to get the filename of the image, width height etc but they seem to refer to the old image

>Maybe something
>is wrong here, but odd if it just affects this key and not the other keys.

I will look into it

>If this is the
>case, then I'd bet you aren't calling notify_displays_string properly.

Indeed I'm not. I have noticed that only a handful of keys (7 excluding custom command stuff) use notify_displays_strings so i thought that is not the way to go for options of minor importance such as GDM_KEY_LOGO

I think i have found what was causing the inconsitencies in config values. What i noticed that the value (and i think it goes for all the keys) is always equal to the previous value although the new value has been just set. In gdmsetup value change is composed out of two stages:
1. update key - this is where daemon is notified about value change (through GDM_SUP_UPDATE_CONFIG sig) and forced to re-read the config files and update the given key
2. update_greeters () call which sends SIGHUP signal to all the login screens. Each of those signals forces execution of gdm_reread_config

For some reason SIGHUP call gets emitted before the config values get updated, hence the "one step behind" effect. Is this how it's supposed to behave? (If so then ignore my ranting in the remainder of this message)

Id imagine this could be percevied as a separate bug. I think setting [debug] enable=true might not be sufficient in this case so to observe its effect the best way is to put gdm_common_error ("VALUE: %s KEY: %s", result, key); line in gdm_config_get_result function in the gui/gdmconfig.c file, then run gdmsetup and change couple of values whilst watching the log. 

I have only tested plain greeter for this anomally. It might be the case that the themed greeter is suffering the same fate

Interesting.  I think you have just stumbled into one of the ugly areas of GDM.  :0.  This is an area that could probably use quite a bit of cleanup.

There are two ways that the greeters are told to update themselves after a config change:

Method 1 - used by gdmsetup

Note in gdmsetup that most config options use the run_timeout function and the update_greeters call happens in the timeout function.  Also note that update_greeters calls the daemon with GDM_SUP_GREETERPIDS and sends the HUP signal to each running greeter.  I believe the timeout (and the length of time it takes for GDM_SUP_GREETERPID to complete) is intended to ensure that the daemon has time to update the new configuration value before the HUP signals get sent and the greeters try to read the new configuration value.

The GDM_UPDATE_CONFIG command doesn't really do much, it just updates the hash table with the new value, so I think it is a bit odd that the new value would not be updated by the time the gretter receives the HUP and has time to query the daemon for the new values.  But still, it does sound like there is a race condition here.

I'd bet some keys are not using the timeout function, does the LOGO key?  If not, I imagine that adding some code so that it uses the timeout would fix this.

Method #2 - daemon telling the greeters directly when UPDATE_CONFIG is received.

The other method is that when GDM receives the UPDATE_CONFIG command, it sends a notify in daemon/gdmconfig.c.  This approach is probably better than the hacky method #1 in gdmsetup which has timing issues.  When we send the notify this 
way, then we *know* that the hash table has already been updated so there is no
timing issue here.

This approach also works if the configuration value was changed without gdmsetup (e.g. if the user edited the configuration file by hand, and then ran an UPDATE_CONFIG command by hand).  I suppose the user could send the HUP signal
to the greeters by hand to get them to restart, but this is obviously ugly.

So I think this approach is better.  I think method #1 was used because someone thought it was too much work to add all needed notifies and just making gdmsetup send HUP signals to the greeters directly seemed easier.

Summary:

My opinion is that Method #2 is a bit better since it avoids timing issues and
it is a bit more straightforward for the daemon to notify the greeters rather than gdmsetup trying manage the updating.  I suppose we could fix this by getting rid of all the timeout/update_greeter cruft from gdmsetup and adding notifies for all GUI related configuration options to daemon/gdmsetup.c

On the other hand, the quick fix is probably to fix gdmsetup so it uses the run_timeout for all keys that it currently does not so that no keys have such timing issues.  You can tell which update_greeter() calls are made outside of *_timeout functions.  These probably all need to be moved so that they are called from within timeout functions to fix using method #2.

This is just my "off the top of my head" analysis of the situation you describe, so I may not know what I'm talking about.  :)  But, it perhaps gives you a bit better understanding of how GDM works.

>I'd bet some keys are not using the timeout function, does the LOGO key?  If
>not, I imagine that adding some code so that it uses the timeout would fix
>this.

LOGO key uses the timeout function and update_greeters (). A quick fix would be to increase timout intervals giving enough time for the daemon to update the hash tables. Again this would be pure guesswork. Also what i have noticed before, is that these timeouts cause lags within interface responses (i.e. there are pauses betwen selection and interface update). Getting rid of all the timeout functions would make the user experience a bit nicer. 

>My opinion is that Method #2 is a bit better since it avoids timing issues and
>it is a bit more straightforward for the daemon to notify the greeters rather
>than gdmsetup trying manage the updating.  I suppose we could fix this by
>getting rid of all the timeout/update_greeter cruft from gdmsetup and adding
>notifies for all GUI related configuration options to daemon/gdmsetup.c

I do agree method #2 is mouch more plausable. What i was thinking is to add additional command GDM_SUP_UPDATE_CONFIG_REREAD that would work in a very simmilar way to  GDM_SUP_UPDATE_CONFIG but it would trigger a gdm_reread_config function execution in all the login screens (in a simmilar way that notify_displays_*). 
Again we could also modify _gdm_set_value_* functions such that notify_displays_* call would be executed for every key (not for just few selected ones). Again we would be hard-coding these notification in quite few places (i.e. daemon/gdm.h, daemon/gdmconfig.c, daemon/slave.c) which is not very flexible. Additionally this would put strain on the daemon/slaves restarting greeters for very single value.

P.S Should we open a new bug regarding this issue? It seems that the importance of this new discovery is over-shadowed by the topic of this bug. Also we seem to be side-tracking from the issues relevant to this particular bug (however related they are)

Yes, if you don't mind adding a new bug explaining that the timeout mechanism used by gdmgreeter is ugly and should be reworked, then that would be good.  I
do agree that getting rid of all the timeout functions and sending of HUP signals in gdmsetup and just making the daemon send notify for all GUI related configuration is probably a better solution than the current method.

Looking at the code for logo_toggle_timeout, I think the problem is that we are calling gdm_setup_config_set_string and update_greeters in the timeout function.  The gdm_setup_config_set_string function should be called in logo_toggle_toggled
before the timeout to give the daemon time before the update_greeters call.  So if we are to fix this with method #1 (perhaps the right short-term fix), then I think this will fix the problem.

We should probably review the code and fix other areas where we call the gdm_setup_config_set_* function and then update_greeters without the timeout in between.  

Then we should work on the other bug and perhaps get rid of all this cruft and just have the daemon notify the greeters when these keys are updated.

Your thoughts?

>Looking at the code for logo_toggle_timeout, I think the problem is that we are
>calling gdm_setup_config_set_string and update_greeters in the timeout
>function.  The gdm_setup_config_set_string function should be called in
>logo_toggle_toggled...

The logo_toggle\logo_toggle_timeout pair are not responsible for updating GDM_KEY_LOGO but GDM_KEY_CHOOSER_BUTTON_LOGO (which i cant figure out what is used for except a fallback value in gdmsetup in case GDM_KEY_LOGO is missing). GDM_KEY_LOGO is updated by logo_filechooser_response which does not have a timout function and update_greeters is called straight after gdm_setup_config_set_string.  As an experiment i will add a timeout function and see if that fixes it.

>We should probably review the code and fix other areas where we call the
>gdm_setup_config_set_* function and then update_greeters without the timeout in
>between.  
>
>Then we should work on the other bug and perhaps get rid of all this cruft and
>just have the daemon notify the greeters when these keys are updated.

I think we should sort out bug #404891 first as adding more cruft will mean we will have to fix more stuff later on. Im guessing core of the work will have to be done in daemon/gdm.c and daemon/slave.c + removing all the update_greeter calls in gdmsetup.

Okay, if your plan is to fix this bug properly by fixing bug #404891, then lets fix it that way and close this bug when that bug is fixed.  I think that's the best approach, though it might take a bit longer.

gdmlogin doesn't exist anymore, does it?

Thanks for taking the time to report this bug.
However, you are using a version that is too old and not supported anymore. GNOME developers are no longer working on that version, so unfortunately there will not be any bug fixes for the version that you use.

By upgrading to a newer version of GNOME you could receive bug fixes and new functionality. You may need to upgrade your Linux distribution to obtain a newer version of GNOME.
Please feel free to reopen this bug if the problem still occurs with a newer version of GNOME.