Created attachment 73559 [details] [review]
created using FC6 Test3 src rpm (gdm-2.16.0-4.src.rpm) and added after Patch24: gdm-2.15.6-wtmp.patch

Attached patch for the issues mentioned above

Issues:

+ Perhaps this should be made a bit more generic.  What if somebody wants
  to add commands to reboot into MacOS, or some other OS?  Perhaps
  rather than making the key GDM_KEY_WINDOWS it could be GDM_KEY_CUSTOM_CMD
  or something.  Then rather than putting "Restart into _Windows" string
  directly into the dialog, this string could also be configured in the 
  gdm.conf file perhaps with GDM_KEY_CUSTOM_CMD_LABEL.

  This way if a user wants to add a new command to do xyz, they could add the 
  GDM_KEY_CUSTOM_CMD to do whatever they want, and set the 
  GDM_KEY_CUSTOM_CMD_LABEL so the dialog/tooltips/etc. say the right thing.
  Then this can be used for all sorts of useful things, not just for booting
  into Windows.  For example, perhaps some user wants to add an option that
  will launch some program they want to run sometimes from the login screen.

  This way we don't have to hack GDM to add a new command each time somebody 
  wants it to do something new.  Some of the gdm_info lines would also need to 
  change like gdm_info (_("Restarting computer into windows...")) could instead 
  say "Running custom command", and function names could be written to have 
  "custom" in their name rather than "windows".

  One disadvantage to this approach is it only allows one custom command. 
  Might be cool to figure out how to make it support an arbitrary number
  of commands that can be configured.  But for now, perhaps we just add one.

  Note this is a lot like the old GDM bug #71238.

  http://bugzilla.gnome.org/show_bug.cgi?id=71238

+ You only seemed to add the code to the gdmgreeter program in
  gui/greeter, and not to gui/gdmlogin.c.  The code should work in both GUI
  programs.  Or am I missing something in reviewing your patch?

+ You don't include any documentation in docs/C/gdm.xml.  I won't accept
  a patch that adds configuration options without documentation.

+ Your configure.ac change hardcodes the command for freebsd and the default
  host and sets it to "/sbin/init 6" on Solaris.  This seems a bit wrong
  since the Solaris command doesn't really boot into Windows.  Shouldn't
  we *not* show this option on systems that won't support it?

  I'd recommend that we *not* set this command to anything by default.  I
  recommend it should be off by default.  We should not assume that the typical
  installation will be on a multiboot system.  We should set this command
  to nothing by default so the option does not appear in the GUI.  The comments
  in the GDM configuration file can explain that this option can be used to 
  reboot into Windows and show example commands for getting it to work on 
  different OS's.  I'd prefer if users who want this feature have to turn it 
  on rather than users who don't want the feature to have to turn it off.
  If a distro wants to turn it on by default, then that's okay, but not in
  the default GDM CVS.

+ You added a number of comments like "Lukas 23 added this if-test" or 
  "Added by Lukas".  Could you remove these comments?  Nobody else has comments 
  like this in the code.  Add yourself to the AUTHORS file if you want credit.
  Comments that explain how the code works are okay, but I'd prefer not to
  clutter the code with comments that don't explain functionality.

+ The patch should not include configure since it is generated from
  configure.ac.

+ The patch looks like tabbing is a little inconsistant.

Please address the above issues and resubmit the patch, or we can discuss if 
you don't agree with my suggestions, ideas, etc.

Hi!
Thx for the info :). I definitely will make the sugested changes. For my purpose windows button did the trick but you are right a generalisation is needed. For very long time i had an idea of creating a pool (maybe represented by linked list) of custom tag labels/stock buttons - i will work on that, but for the time being single item will have to do. I didnt add any documentation as i wasnt sure if you guys will be interested. Now that i know there is a possibility i will provide the proper documentation. As for Lukas comments all over the place this was for my own benefit rather than the credit. It allowed me to quickly grep for it and find the files which i have modified in preparation for next rpm patching.

Created attachment 73865 [details] [review]
created using FC6 Test3 src rpm (gdm-2.16.0-4.src.rpm) and added after Patch24: gdm-2.15.6-wtmp.patch

Hi!
I have made the changes that you suggested:
Now the CustomCommand has four configurable options in the [daemon] section:
CustomCommand=<path to custom script>;.....    (no default provided)

CustomCommandLabel=_Foo
This options determines the labels on the buttons and menu items. If not defined the default is _Custom (which is hardcoded into the code)

CustomCommandLRLabel=_Perform Foo
This option determines the labels on the radio buttons and list items. if not defined the default is _Executing custom command (which again is hardcoded into the code)

CustomCommandText=Are you sure you want to perform custom action?
This option determines the text in the Yes/Cancel warning dialog box once button/menu item/radio button/list item is selected. if not defined the default is Are you sure? (which again is hardcoded into the code)

CustomCommandTooltip=Executes the custom command
This option determines the tooltip text. if not defined the default is Execute custom command (which again is hardcoded into the code)

I have tested this with buttons, menu items and radio buttons through graphical and plain managers and it seems to work. I havent tested this using list as im not entirely sure how to create custom list with stock items as elements - if you could point me in the right direction that be great :). I have also updated the gdm.xml file

NOTE:
This is still build against fedora rpm (rather than gdm src), has dodgy indentations (im not sure if they my doing or redhat's doing) and Lukas tags all over the code. These just help me to develop and test the code. Once we agree that there is nothing to be changed/added i will provide the final patch that will be built against the gdm src and will have all the proper indentations and will not contain any Lukas tags


SOME THINGS IM NOT HAPPY WITH/HAVE QUESTIONS ABOUT:
1. All the custom cmd handlers perform _exit(DISPLAY_CUSTOM_CMD) in a simmilar fashion to reboot/halt handlers - this is ok if the command is used to boot another OS. If the command is used to execute some utility gdm restarts - command does get executed but the screen flickers - this is not very good from end user point of view.

2. gdm_wm_query_dialog does not provide the default OK button (when posbutton parameter is NULL). Maybe i missed something and this was done deliberately but it would be nice to have a standard OK stock button if no custom label is given. Also all posbutton buttons are missing a OK button icon (return arrow symbol), where the cancel still retains the red cross icon.

3. change_to_first_and_clear only takes a boolean value as a parameter giving two possible outcomes "System is restarting, please wait ..." or "System is shutting down, please wait ...". This does not give a lot of choice. What one could do is to provide a enum type flags, use integer as a parameter to the function and use binary & to find the right match

4. For one custom command, placing all the configurable options in the [daemon] section is not too outrageous. However if arbitrary number of custom command is on the cards maybe a separate section (like [CustomCommands]) would make things more clearer and transparent


FINALLY:
If you decide to go through with this patch i will add some Polish docs

Anyhow let me know what you think


Luk

Oh one more thing.
I have provided and example in the Example Configuration section of the docs, that is based around booting windows. Im not sure what are the legal impications of using the word Windows. If this has any implications it can be changed for some flavour of Linux

Looks good.  A few minor comments.

I really don't like the "Added by Lukas for windows thingy" (or "win thingy" and 
such comments in  the code.  Since this is now a "custom" command, there shouldn't be a need to mention "windows" Or "win" in comments in the code.  Please remove these.  Also you really don't need to include "Added by Lukas" so much in the code.  Just add your name to the AUTHORS file.

Let's replace the term "Windows" in the docs with "Windoze" or something that lets people know what we're talking about while avoiding using their trademark.

Feel free to commit after fixing up the comments, by the way.  Please commit into 2.17 (CVS head) only.  This changes interfaces and documentation, so is not appropriate for 2.16 branch.

Oh sorry, I didn't respond to your questions:

1) I think we should add another configuration option for this.  A boolean
   that says whether or not the custom command causes reboot, if true then
   we execute this, if not then we don't.  Does this resolve the problem?

2) Can't we put in default text if the user didn't specify a label?

3) Yes an enum sounds better.

4) Yes, might be good to have a separate section.  Though right now we are just
   supporting 1 custom command.  If, in the future, people want more, then we
   can revisit this then.  Unless you want to extend this so that an arbitrary
   number of commands can be added, which would be cool.

After thinking about this.  It's probably not ready to commit to CVS head.  Why don't you address the above issues (at least 1-3) and then resubmit the patch for review?

Also, your configure.ac changes seem to use environment variables to set the default values if the user wants to.  Wouldn't it be better to use --with configure options?  Might be better not to change configure.ac at all and just let users patch the config/gdm.conf.in file if they want to change the default values.  Note that most config values don't allow configuration via configure script.  I'd recommend not modifying configure.ac in this way.  Using undocumented environment variables to set configuration options seems bad to me.

Also, I notice your patch doesn't include modifications to config/gdm.conf.in.  Note that new configuration commands should have documentation also placed here with the default configuration options included, so it is easier to see how to change them.  Can you add this?  If you do keep configure.ac logic to set the default values, note that you should also set them in the gdm.conf.in file like we do for the others.  Refer to HALT_COMMAND, SUSPEND_COMMAND, etc. for examples.

Created attachment 74163 [details] [review]
Created using gdm-2.16.0.tar.gz src from gnome.org

Hi!
Submitting new and hopefully final patch.
Created using gnome src 2.16.0 with (hopefully) proper alignment and Lukas comments removed.
Took me a little while because i had to change the custom command worked:
1. For restart mode option it behaves as the restart/shutdown options
2. For non-restart mode option it sends an interrupt which is handeled by the slave/daemon

As for the answers to your answers ;)

1. Done. New option (CustomCommandNoRestart=) specifies is gdm/system should be restarted/halted (false) or not (true)

2. Users do not have control over this (This is the Cancel/OK button). At the end i have not modified the actual function but passed GTK_STOCK_OK as the pos_button label and that did the trick

3. I have not changed this option at the end as for non-restart custom command this actually does not get called, for restart option restart message seems just fine

Changes in configure.ac are necessary for the code to compile - i have not assigned any default values there, only defied them there. The defaults are actually hard coded and are used if the config values are not present

I have also added the sample options to gdm.conf.in but i the vagueness of this option makes creation of a good example very difficult so i provided the one with Windoze

Any how lemme know what you think

Regards

Luk

We have been talking about the idea that it would be nice if you could set multi
ple custom commands rather than just one.  One way this could be done, I think, 
is if in the config file you specify the keys like this:

CustomCommand1=foo
CustomCommand2=bar
CustomCommandLabel1=foo label
CustomCommandLabel2=foo label.

Then when getting/setting this key, we could loop over the numbers.  This would 
require a little special code in daemon/gdmconfig.c to handle these keys special
 so it knows to append the integer and loop over them until it doesn't find any 
more.  Then when using the key, again simply loop over them appending the intege
r to the end.

This would also require that the GDM_SOP_CUSTOM_COMMAND look like "%s "%ld %ld" 
so that the
command # selected could be passed along to the daemon.

I think this would work fairly cleanly and wouldn't clutter up the config file t
oo much since people who don't use these Custom Commands wouldn't have them defi
ned at all, and only users who actually want to use them would set them.


Couple questions:

1) Why is change_to_first_and_clear only needed on Linux?

2) In daemon/gdm.c, in the custom_cmd() function, you call fork() but do not
   seem to be checking its return code properly.  Normally, fork() returns
   the pid of the child process so the parent process gets returned the pid
   of the child process while the child process gets 0.  You seem to incorrectly
   be assuming that non-zero means an error.  The way it is written, it probably
   works, but the parent probably always spits out an error message.  There are
   examples of how to use fork() in daemon/slave.c

3) Did you test your changes to the DTD by actually trying to validate some
   of the greeter.xml files?  Would be nice if you checked this if you have
   time.

4) You have set up the config/gdm.conf.in file incorrectly.  The values should 
   be the default values.  So please set them up like this, so they are all
   blank.

  #CustomCommand=
  #CustomCommandText=

  Then in the comments above the examples, explain how to set up the values.
  
5) You should not have to modify configure.ac.  You only need to set up the
   values here because you are using the values in gdm.h.  Just hardcode the
   values in gdm.h to empty.  Like how it is done for daemon/FailsafeXServer.
   Don't use CUSTOM_COMMAND, CUSTOM_COMMAND_LABEL, etc. at all.

Thoughts?

Hi!
As for multiple custom commands i gave it a bit of thought and i came up with two approaches:

1. Predefined number of possible custom commands
Lets say we give users 4 commands to customise. Having the prior knowledge on the numbers gives us a nice bootstrap on the implementation side and i belive we could have it ready in a couple of days. We could use the existing config parsing techinque (either by adding separate variables for each of the custom commands or wrapping them up into arrays), or using the looping thechnique that you suggested. Also moving them into their own section of the config (rather than having all of them in the [daemon]), e.g. [customcommands] would make the config file look neeter, e.g.

[customcommands]

CustomCommand1=foo
CustomCommand2=bar
CustomCommandLabel1=foo label
CustomCommandLabel2=bar label
.
.
.

Next we could modify the custom command handler (in gdm.c) to take gchar* custom_command as a parameter rather than void. This will make the use of the GDM_SOP_CUSTOM_COMMAND without too much overhead, maybe as you suggested with "%s %ld %ld" or with "%s %ld %s" (where the last parameter would be the actual command). As fot the GUI parts the handlers in gdmlogin.c and greeter_system.c could also take string, or int as a parameter (in a simmilar fashion to gdm.c handlers). 
Additional changes would require modifying all the code that parses xml files, adds buttons, menu items, radio buttons and list items.

What i dont like about this approach is it will produce bloated code that will contain repetitions that in essence do more less the same thing. This isnt particularly a good programming style but compromises have to be made somewhere i guess. We could probably generalise all the handler functions, and provide the repetitions for the interface widget thingies, e.g.
if(GdmCustomCommand1Found){
add button/menu item/radio button/list item
}
else if(GdmCustomCommand2Found){
.
.
}
else if .....and so on



2. Arbitrary number of commands.
I havent given this apporoach as much thought as the previous one as i think this one will require a lot of redesign of the current code/approach.
A. Custom storage container wold have to be added (maybe linked list or vector) - one could argue that the arrays could be used, but the lack of prior knowledge on the number of elements would make adding elements a bit of a painful process.
B. Custom parser would have to be implemented (as there is no upper bound and there would be nothing to stop ppl leaving gaps between command numbers). Also parsing of xml files and dtd validation seems like a quite big hurdle. 
C. The relationship of widgets and their handlers would have to change. Atm every widget has a handler assigned. Probably this will have to be reversed, usch that every handler will have a number of widgets assigned. I think we could probably bootstrap the number of handlers but we wouldnt be able to do that with the number of widgets.

SUMMARY:
I think we should go for the 1st approach for the time being. I do agree that 1 command is not enough, and probably will be used for booting another OS leaving no freedom for anything else. 4 seems a good number to start with imho. Im not sure how much noise are ppl making about this stuff but if 4 isnt good enough it could always be upped to a higher number. I think there is not much point in spending long time on redesigning the code/algorithms if no one is going to use it (unless someone comes back and says: actually i want this feature because the numner of commands depends on this script that parses the defaults.conf dynamically and that varies ....)

Now as the answers to your questions:
1. This is my bad. My orginal code was developed on patched version from redhat's rpm (could of been the recent one or the one from last year). I wanted to conform to the general gdm flow of code as close possible, and thats how the restart and halt handlers were implemented. I will change that

2. I was only interested in a child execution, all the other two cases were assumed to be conceptually incorrect (caused restart of gdm soon after the execv was called, well the parent process did). Again i was trying to be too clever for my own good and dint want to have too many if else statements and duplicate execv calls. What i will do i will provide separate function that will deal with the NoRestart case. There the return codes from fork will be handled properly

3. I havent actually performed the validation (i will try that tomorrow).

4 & 5. I will sort it out tomorrow

Regards

Luk



 

Shall i make changes and re-submit the patch for one custom command for the time being,
or shall i work on multiple command one (this might take a while)

Luk

I think we should fix this correctly.  Let's get multiple custom commands 
working, resubmit the code, and I'll commit it after patch approval (or you 
can commit after approval if you have commit permissions).  Please make sure
that the patch is based on gdm CVS head and doesn't include other random 
Red Hat patches (e.g. issue #1 change_to_first_and_clear we just talked about).
Yes, I think it would be nicer to cleanup the fork() code a bit (issue #2).


I think it isn't as complicated as you make it.  I'd say simply loop over them to figure out how many there are.  First try 1, then 2, then 3, etc. and when you find the first one that doesn't return a value from config, then you know how many there are.  I'd say we make it a rule that users cannot skip numbers (like if you defined command #1, #2, and #4 that #4 would be ignored).  I 
think this is reasonable behavior and the best way to address the "upper bound" 
issue.  I'd just make sure the documentation is clear that this is how it works.

Regarding the handlers:

There is no good reason to have separate handlers for each command 
(reboot/halt/suspend).  Might be better to just rewrite the function as a
single function.  Note that currently we set the callback data for the handlers
to NULL (last argument to g_signal_connect).  e.g.

                g_signal_connect (G_OBJECT (w), "activate",
                                  G_CALLBACK (greeter_chooser_handler),
                                  NULL);

Instead we could pass in a structure for each one that would contain:

struct HandlerCallbackInfo {
   gchar *msg;
   gboolean exit;
   gint exit_rc;
}

Then set up the structure and pass it into the g_signal_connect function as
the pointer data.  Then check the various values to figure out what to do
in the handler.  Note that in the greeter the greeter_config_handler does some
special work, so I'd keep this one alone as a unique separate handler.  Refer
to the CallbackInfo structure in greeter_events.c for an example of using
such callback structure with handlers.

Aside from the greeter version of the config_handler, we could probably move 
the generic handler function into /gui/gdmcommon.c and share them between 
gdmlogin and the greeter code so we don't have the code duplicated.

Doing things this way, it should be possible to simply loop over the custom
commands and setup the g_signal_connect calls without having to store the
data in an array/hash/etc.  Instead they are just setup to pass the data along
as userdata to the handler function.

Does this make sense?  I think this is probably the way to do it making it
the least work, though it does require reorganizing the code a bit.  But
it would also nicely cleanup this messy part of the code.

Created attachment 74421 [details] [review]
Created using gdm-2.16.0.tar.gz src from gnome.org

Hi!
This is a patch for single CustomCommand.
I think i have cleaned up the issues that you have raised before (wrt to the single CustomCommand patch).
If this looks ok to you i will focus on the multiple commands version.
Im not sure how long it is going to take so maybe for the time being adding a single command (as a taster for the things to come is a good thing)

Regards

Luk
Luk

Coninued:
.... single command (as a taster for the things to come is a good thing) is a sensible option.

As for the issue #1 (change_to_first_and_clear) this is part of gdm2.16.0 which i have obtained from gnome.org (two of my last patches are built against that).

As i was typing this message your reply came through. I will read it and provide a response of some kind.

Any updates?

Sorry Brian,
was quite busy at work didnt get a chance to do too much of gdm stuff.
Will let you know as soon as i got something in a workable order :)

Regards

Luk

Hi!
I have finished developement and now doing some testing.
I will submit the patch tomorrow and give you some detailed description of what i have done :)

Regards

Luk

Created attachment 75322 [details] [review]
Created using gdm-2.16.0.tar.gz src from gnome.org

Hi!
Attached the latest patch. Now there are 10 custom commands available (controlled by the GDM_CUSTOM_COMMAND_MAX (defined in gdm.h). It probably would be possible to dynamically control the number of command option (maybe as a optional config entry) but that would require re-writing the config file parser.

I have based this approach on the interrupts rather than exit codes.
The custom command is self-contained in the gdm - i have not added anything to the gdm_do_logout_action and gdm_try_logout_action function. I believe this will not respond to the desktop commands whilst the user is logged in (if im not mistaken). The solution that i came up with (if its needed of course) is to create
GDM_LOGOUT_ACTION_CUSTOM_CMD_BASE,
GDM_LOGOUT_ACTION_CUSTOM_CMD_END = GDM_LOGOUT_ACTION_CUSTOM_CMD_BASE + NO_OF_CMDS

and then use the default section of the switch statement to find the right command.
This is a bit ugly but can be generalised. Other option is to hardcode
GDM_LOGOUT_ACTION_CUSTOM_CMD0,
GDM_LOGOUT_ACTION_CUSTOM_CMD1,
.
.
.
etc
but i beleive that does not scale very well :(

NOTE:
I have not updated the gdm.xm, ChangeLog and gdm.conf.in files yet i will do that first thing 2moz the morning

Let me know what you think

Regards

Luk

\
This patch looks really good.  Obviously docs/C/gdm.xml needs to be updated to indicate the new keys, and the gdm.conf.in file seems to have the old keynames without the number.  Probably need to mention the MAX_COMMANDS is 10 in the comments and the docs.

Some specific testing I'd recommend:

I'd test with gdmflexiserver --command="GET_CONFIG keyname" to verify that this works, and then change the value in the config file, and run gdmflexiserver --command="UPDATE_CONFIG keyname", then re-run the GET_CONFIG and verify that the command changes okay.  Would also be good to verify that if the greeter is showing and you run these commands that the greeter restarts.  This is managed by the notify_displays calls in daemon/gdmconfig.c.  Note that certain keys indicate that they send a notify.  If this doesn't work, you might need to add these keys to send notifies as well.  If you find problems with any of these tests, the code might need a few more tweaks to deal with the fact that we are managing these keys a bit different with the number appended as a suffix.

It seems that custom_cmd_no_restart and custom_cmd_restart have very similar code.  Might be good to put the common code in a function and call it rather than duplicating code.

Otherwise, very good.  I think this patch is very close to going into CVS head.

*** Bug 71238 has been marked as a duplicate of this bug. ***

Also, please verify that the custom commands appear in the F10 menu.  Just hit the F10 key when the greeter is showing.

1. F10 menu. Yes they apear there and are executable (i believe this is equivalent to the options_button id/options stock in the themed greeter)

2. Tests with gdmflexiserver --command="GET/UPDATE_CONFIG keyname":

[root@mypc ~]# gdmflexiserver --command="GET_CONFIG customcommand/CustomCommand0"
OK /etc/gdm/scripts/boo
[root@mypc ~]# gdmflexiserver --command="UPDATE_CONFIG customcommand/CustomCommand0"
OK
[root@mypc ~]# gdmflexiserver --command="GET_CONFIG customcommand/CustomCommand0"
OK /etc/gdm/scripts/foo

The restart is only enabled for CustomCommandX= option. We could enable it for other CustomCommand thingies but i believe it would be just a cosmetic improvement. 

as custom_cmd_no_restart and custom_cmd_restart the ammount of code that both of those share is not that big i think having them this way makes reading of the code easier.

P.S. I will attach the diff later on.

Good, sounds like the tests are all working.  I look forward to seeing the final patch ready for commit.  

Created attachment 75410 [details] [review]
75322: Created using gdm-2.16.0.tar.gz src from gnome.org

Hi!
I have made all the necessary modification. Please let me know if it needs any changes.

There is one additional thing that needs testing:
QUERY_LOGOUT_ACTION
SET_LOGOUT_ACTION
SET_SAFE_LOGOUT_ACTION
commands. I shall do that pretty soon.

Regards

Luk

I have thested the above QUERY_LOGOUT_ACTION, SET_LOGOUT_ACTION and SET_SAFE_LOGOUT_ACTION
This is what i get

[lukas@mypc ~]$ gdmflexiserver -a --command QUERY_LOGOUT_ACTION

(gdmflexiserver:20502): GLib-GObject-CRITICAL **: gtype.c:2240: initialization assertion failed, use IA__g_type_init() prior to this function

(gdmflexiserver:20502): Gdk-CRITICAL **: gdk_display_get_name: assertion `GDK_IS_DISPLAY (display)' failed
OK HALT;REBOOT;CUSTOM_CMD0;CUSTOM_CMD1;CUSTOM_CMD2

[lukas@mypc ~]$ gdmflexiserver -a --command "SET_LOGOUT_ACTION CUSTOM_CMD0"

(gdmflexiserver:20568): GLib-GObject-CRITICAL **: gtype.c:2240: initialization assertion failed, use IA__g_type_init() prior to this function

(gdmflexiserver:20568): Gdk-CRITICAL **: gdk_display_get_name: assertion `GDK_IS_DISPLAY (display)' failed
OK
[lukas@mypc ~]$ gdmflexiserver -a --command "SET_LOGOUT_ACTION CUSTOM_CMD5"

(gdmflexiserver:20569): GLib-GObject-CRITICAL **: gtype.c:2240: initialization assertion failed, use IA__g_type_init() prior to this function

(gdmflexiserver:20569): Gdk-CRITICAL **: gdk_display_get_name: assertion `GDK_IS_DISPLAY (display)' failed
ERROR 7 Unknown logout action, or not available

[lukas@mypc ~]$ gdmflexiserver -a --command "SET_SAFE_LOGOUT_ACTION CUSTOM_CMD5"

(gdmflexiserver:20570): GLib-GObject-CRITICAL **: gtype.c:2240: initialization assertion failed, use IA__g_type_init() prior to this function

(gdmflexiserver:20570): Gdk-CRITICAL **: gdk_display_get_name: assertion `GDK_IS_DISPLAY (display)' failed
ERROR 7 Unknown logout action, or not available
[lukas@mypc ~]$ gdmflexiserver -a --command "SET_SAFE_LOGOUT_ACTION CUSTOM_CMD0"

(gdmflexiserver:20571): GLib-GObject-CRITICAL **: gtype.c:2240: initialization assertion failed, use IA__g_type_init() prior to this function

(gdmflexiserver:20571): Gdk-CRITICAL **: gdk_display_get_name: assertion `GDK_IS_DISPLAY (display)' failed
OK

Indicates that custom command options work but there are other problems.
I just run those commands in the console window whilst logged in to gnome desktop (OS: FC6 x64)

Created attachment 75468 [details] [review]
Created using gdm-2.16.0.tar.gz src from gnome.org

Hi!
Just made small modifications to the documentation

Regars

Luk

Thanks.  I checked this into CVS head.  I also updated the docs a bit and cleaned up the code so the syntax is more consistant with the rest of GDM (note GDM code uses spaces like this when calling fucntion

   foo (arg1, arg2)

and not

   foo(arg1, arg2).

I corrected some grammar and spelling issues in the docs, and also tried to make the docs a bit more clear.  Could you check out GDM CVS head and verify that you like the changes to the docs, and that the code in CVS head works okay?  Just to make sure I didn't break anything.  That would be helpful.

Thanks.  This is a great patch.

If you make any further changes to the code, please provide patches based against CVS head and not 2.16.0 anymore.  Okay?  Thanks.

Sounds great :) thx
But i cant obtain the modified code yet (i did cvs co gdm2 on anonymous cvs server).
Am i doing the right thing? (i followed the instructions found on http://developer.gnome.org/tools/cvs.html)

I had another thought. What would be nice is the ability to execute custom command as a logout action from the desktop (by usiong shutdown widget for example). Most of the code is already in place (for gdm) the only two things we need to add is:

1. Another variable that will indicate if the particular custom command should be visible outside gdm login space (i.e. to the desktop for example):
CustomCommandPersist[0-9]=false (default)

2. Another handler GDM_SUP_QUERY_CUSTOM_CMD_LABEL in gdm_handle_user_message (gdm.c) which will return the label of the requested command (as i believe nothing has the access to the gdm's config hash) - behaving in a simmilar fashion to GDM_SUP_QUERY_LOGOUT_ACTION.

Further modifications would have to be made in gnome-desktop (mainly panel-gdm.[hc] and panel-logout.[hc] which im looking at atm.

What do you think?
Is it worth doing?

Luk

Right
i have the new CustomCommand functionality working.
It only appears on the desktop if ustomCommandPersistent[0-9] is set to true.

Also depending on the CustomCommandNoRestart setting (i knew eventually i will find the right functionality for this setting ;)) the option will appear in the Log Out dialog (true) or Shutdown (false)

All i need is the updated gdm src such that i can make further modifications.

P.S. Do i need to submit a new patch for gnome-panel or can i do it through this bug?

Regards

Luk

Sorry, just did the commit.  Should be in CVS head now.  Looking forward to seeing your updated patch.  Please provide patch based on CVS head.

Created attachment 75622 [details] [review]
built against the latest cvs head (obsolete DO NOT use!)

Hi!
Added new config option and couple of commands (in preparation for gnome-panel interaction).
Cleaned up bits and pieces

Regards

Luk

Created attachment 75624 [details] [review]
buit against gnome-panel-2.16.0.tat.gz from gnome.org

Hi!
Im not sure if this is the best place to attach it (maybe i should create a new bug)
but here it is - gnome-panel patch that will allow custom command execution from Log Out/Shut Down dialogs.

The only possible issue that i can think of is:
All the buttons are appended onto on row. If all the custom commands were to be used then it would make one loooong and ungly dialog box. Maybe splitting them onto two rows depending on their numbers would be a nice feature

What do you think?

Luk

Created attachment 75655 [details] [review]
built against the latest cvs head (as of yesterday)

Made some small changes compared to 75622 nothing major (just removed couple of obsolete extern variable declarations)

Okay.  The patch in comment #34 is committed.  Please submit the patch in comment #33 to the gnome-panel category in bugzilla and lets see if the panel maintainer will accept it.  You can refer this bug report in the new bug report you sumbit.  Sound good?

Could you please check out the code from CVS head and test it and make sure that it works as expected.  Just to make sure all your changes got in.  Thanks.

Oki doki will check the code tonight :)

As for the panel i will submit a new bug. Thx


Luk

I have tested the code and it seems fine as far as i can tell :)
It would be nice though if other ppl tested it (the more the better)

Luk

Ah one more question.
Which gdm release is this patch gonna make its way into?

Thx

Luk

Thanks so much for your help on this enhancement.  There are lots more GDM work if you want to review the existing bugs, I'd be happy to help you do more if you want to keep working on GDM.

I'm closing this bug now.  The patch went into GDM CVS head and is in 2.17.1 release.  First stable release will be 2.18.0 when it comes out later.

My pleasure :)
Yes i would love to do some more work on GDM. 
Please give me some pointers on how to proceed from now on.
Is there any place (like an IRC channel) i can get hold of you?

Regards

Luk