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

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

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

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

Note the main reason I put these all in one bug is so that when people are motivated to fix any of these, that they know what other issues are there and hopefully will fix some of the other issues.  Many of them are fairly easy to fix, especially if you are digging into the code and fixing a problem in the same tab.

+ Should be able to specify Use24HourClock since users like to change
  this.  General tab.  This is a checkbox.

In gdm 2.8.x, this key was useful,
but it seems that this key is useless now in 2.14.x.
The translation handle it.

Here is the french translation:
#. Translators: You should translate time part as
#. %H:%M if your language does not have AM and PM
#. equivalent.  Note: %l is a strftime option for
#. 12-hour clock format
#: ../gui/gdmcommon.c:598
msgid "%a %b %d, %l:%M %p"
msgstr "%a %d %B, %H:%M"

I don't have Use24HourClock key set, and I read 13:56 in GDM.

Brian Cameron posted multiple suggestion on gdmsetup. How many of them have been implemented? I'm interested in doing some work with GDM. Is there a developer's guide to follow to build the develop and test environment of GDM? Should I need a new test PC to code, build, and test GDM for these feature requests? Please give me some pointers. Is there a priority of these features?

None of these features have been implemented.  I'd start on the easier tasks listed since they would give you a feel of how gdmsetup works and how to edit the glade file.  I'm happy to help.

First, I'd load the gdmsetup.glade file and try to update the GUI so that it has some of the desired features.  Then note how these are setup in the various setup_xyz_tab functions.  Most types of widgets have setup functions where you simply pass the key in (such as setup_notify_toggle).  For such widget types, 
that's pretty much all you need to do.

However, if using a more complicated widget (such as a file selection widget) the code gets more complicated.  There are examples in the code.

If you have any questions, or need any help, let me know.

I can see you were saying install glade, load gdmsetup.glade, modify, and save it. Before I start, I want to make sure I did './configure' correctly. Below is the output

TCP Wrappers support                  : NO
Xinerama support                      : YES
XDMCP (remote login) support          : YES
Secure remote connection              : NO
DMX (Distributed Multihead X) support : NO
Console helper                        : NO
SELinux support                       : NO
Solaris Trusted Extensions support    : NO
Authentication scheme                 : verify-shadow
Extra utilities built                 : gdmopen

Obviously, I am missing few components. Should I have them in "YES" before I proceed? I am planning on installing gdm on a test PC instead of my development PC to avoid ruining the existing gdm or even Gnome. What do you think?

It looks like your configuration is fine.  TCP Wrappers is a mechanism that allows you to specify which hosts are allowed XDMCP access.  It might be nice if gdmsetup allowed you to edit the TCP Wrappers configuration file, and you would want it enabled to test that.  But there are lots of other things gdmsetup needs, so I wouldn't worry about this first.  You must have libwrap on your system and the GDM configure script needs to find it in order for this to be enabled.  Secure remote connection and DMX are optional features that most people don't use.  You can read about them in the GDM docs if interested.  SELinux is the Linux Security Extensions for auditing, etc.  Solaris Trusted Extensions support is probably not interesting to most users.  PAM is a better authentication scheme than shadow passwords, but I think you need PAM installed on your system.  None of this really matters for the work you are interested in doing, but so you understand.

Note that glade is a bit tricky to use if you have never used it (or a GUI editing tool before).  Also, the gdmsetup GUI is a bit complicated.  I'll explain it a bit, so you will have an easier time editing it.  Hopefully.  :)

In the main glade-2 window, click on a dialog choice to be able to see and edit a particular window in sort of a WYSIWYG where you can click on widgets and edit them (clicking the right mouse button over a widget allows you to do things like edit a particular widget).

setup_dialog is the main gdmsetup window with the tabs at the top.  
xdmcp_dialog is the window that pops up when you click on the "Configure XDMCP"
   button in the "Remote" tab.
xserver_dialog is the window that pops up when you click on the "Configure
   Xserver" button in the "Security" tab.
add_xserver_dialog is the dialog that appears when you click on the "Add
   Server" button in the xserver_dialog.
add_user_dialog is the dialog that appears when you click on the "Add User"
   button on the "Users" tab.

So, if 

Normally, when I use glade, I go to View->Show Widget Tree since this shows you the actual widgets for the window you are viewing.  If you click on a widget in the WYSIWYG editor, the widget will become highlighted in the Widget Tree.  You
can add new widgets by right-clicking on a widget and then you'll notice the container widgets (the widgets that contain the widget you are clicking on)
at the bottom of the pop-up menu.  Go to these and you can select "Insert
Before" or "Insert After".  Play with it a bit.  Note the View->Property Editor.  To get widgets to behave the way you want, you often need to modify properties.
Make sure that, for example, when you resize the window that the various widgets resize reasonably.  If they don't, or if you want something to be center justified, to add spacing/borders, etc, you often have to play with the property settings.

Frustratingly, glade-2 doesn't seem to have an "Undo" feature so I often find that I have to make sure to backup the changes as I go since it is often
easier to go back to a previous version than to try to undo some change by hand (especially if you accidently delete something).

Note that most checkboxes, labels, etc are inside containers (such as VBox, HBox, Table, etc.).  Try to add new things to the GUI in sensible ways, so if the tab currently has checkboxes inside a VBox, put any new ones inside the same VBox, etc.  In other words, if you look at each tab you will notice that there are certain patters to how the containers are laid out.  Try to understand the layout, and follow the existing patterns. 

For example, if you click on the local tab, you will see that this corresponds to the local_tab_vbox widget.  This widget contains the local_style_vbox and the local_welcome_message_vbox1.  These vboxes contain further vboxes that contain the actual widgets.  Some things are put in separate widgets because gdmsetup sometimes does things like make certain areas visible/hidden based on what the user is doing.  For example, when you set the Style between Plain and Themed, you'll notice that this hides/shows different widgets so you have different choices based on which you are editing.  So, if you are adding a config option that only applies to the Plain greeter, you'ld want to add your widget inside local_plain_properties_vbox (you'ld put it in local_themed_properties_vbox for the Themed greeter).

Does this make sense?  Looking at the code in gdmsetup, you can follow the logic and see how the window is managed, but this hopefully gives you a bit of an overview of how to use glade and how the gdmsetup.glade file is organized.

I have set up a test machine running xubuntu to run the latest gdm. I tried glade to add widgets. I have a problem installing the latest gdm unstable build to xubuntu.

1: I observed that there is no configure or options in xubuntu gdm login screen. I checked the ubuntu gdm login screen. There is only 1 clickable widget: Options. It has 7 entries: Select Language, Select Session, Remote Login via XDMCP, Restart, Shut Down, Suspend, Hibernate. I don't see any of the dialogs or widgets available when I open gui/gdmsetup.glade.

2: I tried to uninstall gdm on xubuntu. The dependent package is xubuntu-desktop which will free 12 MB. Is there a way to save xubuntu-desktop when I uninstall the gdm and install the unstable gdm?

I solved the #2 problem. I removed both xubuntu-desktop and the old gdm to install the unstable gdm. For problem #1, I still do not understand why there is a difference between the unstable gdm and the ubuntu's shipped gdm. I don't see any configuration options in the ubuntu's shipped gdm. Perhaps, it is the greeter version of the gdm with themes?

Finally, I got to the point where I can start to hack the code. I built gdm with option -O0. Please consider adding a --debug option in ./configure which adds "-g -O0" to CFLAGS. The gdm login screen has many functions in gui/gdmlogin.c. I want to implement the feature which disable multiple logins from the same user. I was looking at gui/gdmlogin.c line 880 in function gdm_login_enter:
    login_string = gtk_entry_get_text (GTK_ENTRY (entry));
the login_string is supposed to be the username typed in the gdmlogin username entry. When I used gdb to check, I found that it was not the username I typed in the entry. The output of print/c login_string in gdb always gives "16 '\020'".

1. How do I get the username typed in the entry?
2. How do I get the usernames who have logged in?
3. Where should I intercept and check the typed username against the logged in usernames?
4. Where is the face browser? The Actions->Configure Login Manager... has style: Plain with face browser. It has no effect. I don't see any face browsers.
5. Where in the code does the actual username/password get checked against /etc/passwd|/etc/shadow?

If you build GDM from source code, some GDM configuration options may be different.  Your distro may change some default config values.

To be able to go into the configuration screen from the login program you need to have SystemMenu=true and ConfigAvailable=true.  To use the face browser, you need to have Browser=true and either IncludeAll=true or specific users set up (separated by commas) in the Include variable or you won't actually see the face browser (note that for gdmgreeter you need to use a theme with a face browser such as happygnome-list).  I think the GDM documentation is very clear about how all this works.  Have you read it?  You really should familiarize yourself with the manual before making changes.  http://www.gnome.org/projects/gdm/docs.html

Note you can also simply run gdmsetup as root to be able to run the GDM configuration program, you don't need to run it from the login screen.  It's easier to test running it from the commandline as root.

Not sure why running "glade-2 gdmsetup.glade" isn't working for you.  

Anyway, the features that gdmsetup needs are already implemented.  AlwaysLoginCurrentSession, for example.  You shouldn't need to modify gdmlogin or the gdmgreeter code at all.  This bug is simply adding the configuration options to gdmsetup so you can modify the values in the gdmsetup program rather than having to edit the ASCII GDM configuration files directly.  So I'm really not sure why you are trying to edit gdmlogin.  I don't see how any of your 5 questions relate to making changes to gdmsetup (aside from knowing how the face browser works - question #4 - which I think I answered above).

I don't think anyone is doing any work on this bug (last post was quite some time ago) so I have started initial design. Brian, soon i will attach the gdmsetup.glade so you can gave a look at the positioning/design of new widgets. Once we finalise the interface layout I will add all the necessary handlers

Right.  A few people have expressed an interest in looking at this bug, but nobody has yet worked on it.  This may not be the most high-priority bug, but gdmsetup would be a lot more useful if all these issues were addressed.  A lot of people like to modify the configuration via the GUI rather than having to hand-edit the ASCII files (like /etc/X11/gdm/custom.conf).  I think fixing this bug would mean that most features users would want to change would be in the gdmsetup program.

Awesome.  Note bug #340610 and #343206 and #347101 are also gdmsetup usability problems that might be worth fixing at the same time as this bug.  I'm not 
sure if you will fix all these problems, but I recommend that you do as much as you can.  Then we can close this bug and the other 3 bugs (340610, 343206, 347101) and add one new bug that explains any remaining work needing to be done.
Does this sound like a good way to manage these gdmsetup usability issues?

Thanks, I'm looking forward to seeing your work.  You are really helping to make GDM a lot nicer.

Might also be good to review the GDM configuration file and see if there are any other new configuration options that should be added to gdmsetup (such as your new custom commands feature).

Brian

Created attachment 76521 [details]
Initial design of the new gdmsetup interface

Brian, attached is the initial design of the gdmsetup interface (glade-2 format).
Please note that the lables on the new widgets are default ones, i will give them meaningful names when we finalise the desgin. I placed the Configure Custom Commands button on the General tab, but there is no corresponding dialog box as of yet - im still thinkin about it. I managed to include most of the issues raised in this bug but i have couple of questions:

>+ Should be able to specify GtkRC file since it would be nice if users
>  could specify the GTK+ theme to use for gdmlogin.  This also affects 
>  the GTK+ elements in gdmgreeter (entry, F10/Options menu, etc.) so should
>  go in General tab.  Probably should be a file selection dialog.

Is this option really needed? The documentation states:
BEGIN QUOTE
Path to a gtkrc to read when GDM puts up a window.  You should really now use the GtkTheme key for just setting a theme.
END QUOTE
Which kindof suggests that the GtkRC entry is deprecated (correct me if im wrong)

>+ Should be able to set ChooserButton to turn off the ability to launch
>  gdmchooser from login screen.  Also would be nice to be able to set the
>  various [chooser] config options, though not sure what tab this belongs in.

I believe this reffers to XDMCP. If so then on the Local there is a checkbox titled "Include Hostname Chooser (XDMCP) menu item" which i believe (havent checked that tho) gets rid of the chooser launcher. Also on the Remote tab there is a "Configure XDMCP..." button which sets most if not all the relevant chooser options.

>+ Should be able to create new users in gdmsetup.  Probably "Users" tab.
>  Note that the about-me applet has logic to do this, perhaps it could be
>  ported into GDM?

This will probably require more thought than just placing widgets in glade. Perhaps an edit button once a particular user is selected opening separate dialog, or calling the gnome-about-me applet perhaps. Is this feature necessary though? Only root has access to gdmsetup and i cant think of many scenarios where root will need to modify other users details

Anyhow leme know your thoughts

Created attachment 76568 [details]
Initial design of the new gdmsetup interface

Added a customcmd_dialog box that will be used to set up 9 custom commands

Regarding GtkRC/GtkTheme.  I think some people will want to use one
or the other.  Might be good to have a checkbox that lets the user
specify which one they want to use.  Probably should use a file
selection widget to specify the theme or gtkrc file.

Yes, you are right about gdmchooser.  This isn't a bug, people can already turn this on/off in the gdmsetup program.

Yes, the ability to add new users via gdmsetup is probably a bit complicated.
Perhaps we should open a new GDM bug for this issue and address the other issues
in this bug fix?

It's cool that you now allow people to set up the custom commands via gdmsetup.

Any progress on getting the backend code implemented?

Its nearly finished. Im gessing it will be ready at the end of this week. Atm im implementing Default session (i bleive i will have to duplicate some of the code found in gdmsession.c to obtain the list of sessions). Whats left to do is custom command section. There is a little quirk regarding commands (Its halt shutdown and suspend and i believe it will also concern custom commands) which is caused by config parsing by the daemon. Only first working command is retained and that is what is displayed in gdm setup. One can append/type multiple commands (semicolon seperated) but upon gdmsetup restart gain only first working one appears (although the full one is saved)

I am a bit confused by your comment that you will need to duplicate some code from gdmsession.c.  Note gdmsession.c is compiled into the libgdmcommon.a
library that is statically linked into several binaries, including gdmsetup.  In other words, all functions in gdmsession.c should be available to gdmsetup without needing to do extra work.

Perhaps some work may be needed to make the functions in gdmsession.c work in the ways that gdmsetup needs, but that shouldn't be too much work.

Yes.  It sounds like the behavior of the Reboot/Shutdown/Suspend commands is a bit quirky, though it probably isn't a big deal.  Probably most users won't try and type in multiple commands anyway, so the bug would only be seen in odd situations.  Perhaps a nice way to work around this issue would be to disallow entering in multiple commands via the gdmsetup GUI.  In other words, restrict it so that users who use gdmsetup can only set it to a single command.

This is probably reasonable.  I believe the reason GDM allows multiple commands is so that you can specify different commands for different OS, and the reboot/shutdown/suspend will "just work" on different machines.  However, if you are using gdmsetup to set the command, then it is probably reasonable to assume that the user is setting ti to the command that should work for the current machine.

> I am a bit confused by your comment that you will need to duplicate some code
> from gdmsession.c.  Note gdmsession.c is compiled into the libgdmcommon.a
> library that is statically linked into several binaries, including gdmsetup. 
> In other words, all functions in gdmsession.c should be available to gdmsetup
> without needing to do extra work.

Yup its my bad. I got sidetracked by the include dependencies and thought it will be easier to create a separate function in gdmsetup.c rather than modifying gdmsession.[hc] - now i have made appropriate modificatons to gdmsession.

As for the commands. Allowing only single command would be a solution (e.g using file chooser rather than text entry). But this would not allow some of the scenarios e.g.
Theres is an autoupdate mechanism that generates a batch file once the updates are available and removes that batch file once the updates have been applied. This batch file in turn is used by one of the custom cmmds, hence presenting an entry in the greeter when updates are available (This is a similar analogy to pup widget in FC6). By enforcing one command only (through file chooser) this wouldnt be possible. Maybe lets leave it as it is (multiple cmmds) and see how does it work in practice (we can always remove.replace it afterwards)

Created attachment 77677 [details] [review]
built against the latest (today's) cvs HEAD 

Hi!
Attached is the initial patch for this bug. It covers most (if not all) of the features requested here (with the exception of the create user)
It also fixes bugs #343206, #347101 and provides minor tweak for bug #358114.

This patch introduces loads of new functionality/code. I have done some testing (and in the process of doing some more). Please provide me with all the feedback that you can (usability and error wise).

Awaiting your comments

This patch doesn't look quite ready to commit.  Nothing serious, but a few
questions/issues below to address:

In the combobox_changed function, near the bottom there is this line:

                return run_timeout (combobox, 100, combobox_timeout);

A void function cannot return a value so should the return happen after the run_timeout function?

You remove session->clearname setting from the gdmsession.c processing.  The purpose of this is when you display the session list as a list (as it does in the default Circles/Happygnome themes).  This is where it shows you the session names in the combo box in the bottom of the screen.  The clearname is used since the "_" isn't valid here - mnemonics aren't used for combobox selection.  Perhaps setting the name a second time without the mnemonic into "clearname" isn't the neatest way to store this other value (perhaps it would be better to remove "_" from the label on the fly).  But it looks like you just broke the code here rather than fixing clearname to not be needed.

Could you explain why you changed the init function to treat sessnames as *sessnames?  Perhaps add a comment about this?

Why do you need to add gdmwm code to gdmsetup?  Note gui/Makefile.am change.  Note that sucking in the gdmwm code requires X_EXTRA_LIBS.  On my machine it will fail to build because -lXext is not on the command line.  If we really need this code, we need to add X_EXTRA_LIBS to gdmsetup_LDADD in Makefile.am

Why did you remove the ability to set GDM_KEY_ALLOW_ROOT and GDM_KEY_ALLLOW_REMOTE_ROOT from gdmseutp?  Just because we support MinimalUID doesn't make me think that setting these is no longer necessary.

Overall looks very good.  But I have some more comments after
running it and looking at it...

General Tab
-----------

+ Perhaps "Visual feedback" checkboxes should be on
  Security tab since this relates to how password
  field looks?  It affects security because someone
  can look over your head and see how many characters
  are in your password if visual feedback is turned on.
  Slightly more secure to leave this off, though it
  might look worse.  However, for some users (like with
  a11y needs), the feedback may be important to have
  on.

+ The "Note" in the "Command" section says you can add
  different commands separated by semicolons, but it
  isn't clear where you do this?  Perhaps the comment
  should be more clear about how the "Add" button works.
  The comment doesn't explain the Add button at all.

+ Perhaps setting Commands (halt/reboot/shutdown/custom)
  should be on its own tab?

+ Should have better labels to explain difference between
  Label/LRlabel/Text/Tooltip in the Command section when
  setting Custom Commands.

+ Should be an easier way to see which commands are
  enabled/disabled than looking at them and seeing if
  the Path value is blank.  Perhaps the drop-down list
  should say "(enabled)" or "(disabled)" after the command
  name.  So you would see "Halt command (enabled)" in
  the list.

+ When I change custom commands, I see these messages sent
  to the terminal.  Should these debug messages be
  removed?

  Could not access configuration key <customcommand/CustomCommand0=>
  Using compiled in value <> for <customcommand/CustomCommand0=>

+ In "Default session" I see "foo" listed twice, even though
  I don't have any sessions named "foo" in my configuration
  files.  Is this an error?  Also I see "Failsafe _GNOME" and
  "Failsafe _Terminal".  Probably should avoid putting the
  "_" character in there?  This is what clearname was used
  for.

+ "GtkRC Path" label should probably be "GtkRC file" since you
  specify a file, not a path.

Security Tab
------------

+ "Check directory owner", "never place cookies on NFS",
  "login retry delay", "relax permissions", and "always
  restart server" maybe should have better labels so it
  is more clear what these choices do.

  Note relax permissions can only have 3 values (0,1,2)
  so checkboxes with the three choices would probably be
  better here.  No need for the end-user to know that
  in the config file the choices are stored as 0,1,2.

  0 - Only allow login if user $HOME directory permissions
      are secure (default value).
  1 - Allow login if group write permissions on user's $HOME directory.
  2 - Allow login if all write permissions on user's $HOME directory.

+ On Security tab might be nicer if Automatic/Timed were below
  Security section?

+ Might be nice if the label for Minimal UID mentioned that the
  default is "100", so if user changes it they can set it
  back to the default if they want without having to remember
  the original value.

User's Tab
----------

+ Probably should be some message in the Users tab to indicate
  that setting MinimalUID on the Security tab affects what users
  can be added to Include/IncludeAll.

Configure Xserver
-----------------

I like the new Priority spinbutton on the "Configure Xserver" dialog
(from "Security" tab).  Note valid values should only be -20 to +20 
and not 0 to 100.

The issues you pointed out:
> In the combobox_changed function, near the bottom there is this line:
>                return run_timeout (combobox, 100, combobox_timeout);
>A void function cannot return a value so should the return happen after the
>run_timeout function?

Indeed i was trying to be funky with the code. Although it worked it wasnt really "correct". Fixed now.

>You remove session->clearname setting from the gdmsession.c processing.  The
>purpose of this is when you display the session list as a list (as it does in
>the default Circles/Happygnome themes).  This is where it shows you the session
>names in the combo box in the bottom of the screen.  The clearname is used
>since the "_" isn't valid here - mnemonics aren't used for combobox selection. 
>Perhaps setting the name a second time without the mnemonic into "clearname"
>isn't the neatest way to store this other value (perhaps it would be better to
>remove "_" from the label on the fly).  But it looks like you just broke the
>code here rather than fixing clearname to not be needed.

This is my fault (unintentional though). I was quite tired last nite and i did cut'n'paste of the whole function without realising that they differed. Fixed now

>Could you explain why you changed the init function to treat sessnames as
>*sessnames?  Perhaps add a comment about this?

using **sessnames in the function makes *sesnames mutable.

>Why do you need to add gdmwm code to gdmsetup?  Note gui/Makefile.am change. 
>Note that sucking in the gdmwm code requires X_EXTRA_LIBS.  On my machine it
>will fail to build because -lXext is not on the command line.  If we really
>need this code, we need to add X_EXTRA_LIBS to gdmsetup_LDADD in Makefile.am

Unfortunately gdmsession (gdm_session_lookup) is the cultprit. This is what i get without the gdmwm lib.
/usr/src/redhat/BUILD/gdm-2.16.0/gui/gdmsession.c:416: undefined reference to `gdm_wm_query_dialog'
/usr/src/redhat/BUILD/gdm-2.16.0/gui/gdmsession.c:454: undefined reference to `gdm_wm_query_dialog'
/usr/src/redhat/BUILD/gdm-2.16.0/gui/gdmsession.c:477: undefined reference to `gdm_wm_message_dialog' 

Maybe in the future it would be worthwile moving all the GUI specific calls from gdmsetup "up-stream".

>Why did you remove the ability to set GDM_KEY_ALLOW_ROOT and
>GDM_KEY_ALLLOW_REMOTE_ROOT from gdmseutp?  Just because we support MinimalUID
>doesn't make me think that setting these is no longer necessary.

I think you referring to the global variables GdmAllowRoot and GdmAllowRemoteRoot. The only other place of appearance for them was main (this is where they had values assigned), They didnt appear anywhere else in the code so i removed them. Note functionality of GDM_KEY_ALLOW_ROOT and GDM_KEY_ALLLOW_REMOTE_ROOT are handled by setup_notify_toggle ("allowroot", GDM_KEY_ALLOW_ROOT) and setup_notify_toggle ("allowremoteroot", GDM_KEY_ALLOW_REMOTE_ROOT) respectively

>Maybe in the future it would be worthwile moving all the GUI specific calls
>from gdmsetup "up-stream".

I meant from gdmsession not from gdmsetup

Yes, I think it makes sense to move the gdmwm code from gdmsession.c upstream.  Perhaps the code can be reorganized, or this function just moved to gdmlogin and greeter/greeter_session.c code.  I hate duplicating code, but if it can't be reorganized more easily then this is better than making gdmsetup need the gdmwm code.  Otherwise things sound good.  Note my comments in comment #25, which I think also maybe need attention. 

Created attachment 77909 [details] [review]
built against the latest (today's) cvs HEAD 

>Yes, I think it makes sense to move the gdmwm code from gdmsession.c upstream. 
>Perhaps the code can be reorganized, or this function just moved to gdmlogin
>and greeter/greeter_session.c code.  I hate duplicating code, but if it can't
>be reorganized more easily then this is better than making gdmsetup need the
>gdmwm code.  Otherwise things sound good. 

Done. Removed all the gdmwm releated code from gdmsession and adjusted gui/gdmlogin.c and gui/greeter/greeter.c accordingly. Now gdm setup does not need to include any gdmwm libs

>Perhaps "Visual feedback" checkboxes should be on
>Security tab since this relates to how password
>field looks?  It affects security because someone
>can look over your head and see how many characters
>are in your password if visual feedback is turned on.
>Slightly more secure to leave this off, though it
>might look worse.  However, for some users (like with
>a11y needs), the feedback may be important to have
>on.

Hmmm i do see your point. Although the security tab is kind of full and placing another two rows of stuff might elongate it even more. I run 1600x1200 and gdmsetup window looks OK. On 1280x1024 the bottom part of gdm setup will overlap with parts of the greeter. I believe on 800x600 it will probably go outside of viewable area. I think this option has also a visual aspect of it and we can justify its presence on the General Tab. I have added some tooltips to make users aware of the potential dangers

>The "Note" in the "Command" section says you can add
>different commands separated by semicolons, but it
>isn't clear where you do this?  Perhaps the comment
>should be more clear about how the "Add" button works.
>The comment doesn't explain the Add button at all.

Changed the note. Now you can only substitute an existing command with the one selected with Add button. However you can still append multiple commands through the text entry

>Perhaps setting Commands (halt/reboot/shutdown/custom)
>should be on its own tab?

If we intend to add more options to the gdmsetup then yes. Atm splitting General tab in two will create two somehow bare tabs

>Should have better labels to explain difference between
>Label/LRlabel/Text/Tooltip in the Command section when
>setting Custom Commands.

I have added tooltips to each of the fields explaing what they are for. In fact i have added tooltips to all widgits that are not self-explanatory by nature


>Should be an easier way to see which commands are
>enabled/disabled than looking at them and seeing if
>the Path value is blank.  Perhaps the drop-down list
>should say "(enabled)" or "(disabled)" after the command
>name.  So you would see "Halt command (enabled)" in
>the list.

Next to the command chooser widget i have added a status string

>When I change custom commands, I see these messages sent
>to the terminal.  Should these debug messages be
>removed?

>Could not access configuration key <customcommand/CustomCommand0=>
>Using compiled in value <> for <customcommand/CustomCommand0=>

I have not seen this myself. I will look into that

>In "Default session" I see "foo" listed twice, even though
>I don't have any sessions named "foo" in my configuration
>files.  Is this an error?  Also I see "Failsafe _GNOME" and
>"Failsafe _Terminal".  Probably should avoid putting the
>"_" character in there?  This is what clearname was used
>for.

Fixed (hopefully)

>"GtkRC Path" label should probably be "GtkRC file" since you
>specify a file, not a path.

Fixed

>Security Tab
>------------
>"Check directory owner", "never place cookies on NFS",
>"login retry delay", "relax permissions", and "always
>restart server" maybe should have better labels so it
>is more clear what these choices do.

Fixed using tooltips

>Note relax permissions can only have 3 values (0,1,2)
>so checkboxes with the three choices would probably be
>better here.  No need for the end-user to know that
>in the config file the choices are stored as 0,1,2.

>0 - Only allow login if user $HOME directory permissions
>    are secure (default value).
>1 - Allow login if group write permissions on user's $HOME directory.
>2 - Allow login if all write permissions on user's $HOME directory.

Done.

>On Security tab might be nicer if Automatic/Timed were below
>Security section?

I dont think it would look right. Most of the security options are quite narrow (with the exception of permissions radio group)

>Might be nice if the label for Minimal UID mentioned that the
>default is "100", so if user changes it they can set it
>back to the default if they want without having to remember
>the original value.

All the options are read from the config values. For example fedora folks set it to 500 (in defaults.conf), and im not sure about any other distros. So probably adding any info about the default value is more hassle than its worth.

>User's Tab
>----------
>
>Probably should be some message in the Users tab to indicate
>that setting MinimalUID on the Security tab affects what users
>can be added to Include/IncludeAll.

Done

>Configure Xserver
>-----------------
>
>I like the new Priority spinbutton on the "Configure Xserver" dialog
>(from "Security" tab).  Note valid values should only be -20 to +20 
>and not 0 to 100.

Done

Lemme know what you think

This patch is now committed to CVS head.

Awesome patch.  Can you close bug #334186, #343206, and #347101 and open a new bug which explains any issues in these bugs that you have not yet fixed?

I do agree that the "Security" tab looks a bit cluttered.  Would be nice to see if the GUI for this tab could be redesigned so it takes up less room.

Probably also nice to add a vertical scrollbar so that if the window is too big for the screen that the user can use the scrollbar to move up/down to see the different choices.

I think with this fix, gdmsetup is finally pretty much fully useful for all the sorts of configuration people would want to do with GDM.  Might be worth
reviewing the config/gdm.conf.in file and see if there are other configuration
choices that should be also added to the gdmsetup program.

Splendid :) I will do some more testing in the meantime.
Both additional issues (#343206, and #347101) have been fully resolved in this patch.
Also i dont think i have the permissions to close any bugs :( (or maybe i just dont know how)

Oh i forgot.
Lets not close this bug yet. The changes are very fresh and there might be some stuff that we discover (i have found a small bug just now) and want to fix right away.

I'd prefer to close this bug.  It's too long.  Could you open a new bug and explain all the things that you didn't address and we can use the new bug for keeping track of additional issues with this.  Does this sound okay?  I'll close the other 2 bugs also.

I havent found a but i was just being stupid. 
Oki doki if need arises i will create another bug report

I don't think you implemented all the ideas in comment #1 (e.g. Quiver, CheckDirOwner, LockPosition, PositionX, PositionY, TitleBar, SetPosition, ability to create new users).   Might also be useful to review the GDM configuration file and see if there are any other obvious ones that should be added that aren't yet in gdmsetup.  Might also want to mention that the Security tab is a bit long and could probably be made to look a bit better.

I'd think at the very least we'd want to create a new bug to capture that there are still perhaps a few useful features that could be added to gdmsetup that are not yet there.

I think you did the most important ones and gdmsetup is much better...but probably still a few that could be done if someone wants.

Could you perhaps review the GDM configuration file and see if there is anything else you think should be captured in the bug, and then open a new bug with these sorts of comments?  If you don't I will.

Quiver, LockPosition, PositionX, PositionY, TitleBar, SetPosition are relevant to  gdmlogin hence they only appear in the Plain greeter (under Behaviour section)
CheckDirOwner is one of the options under security section in the security tab (between minimal UID and Never place cookies on NFS)

As for add new user feature - yes this feature is not yet implemented. 

Please open a new bug if you can :)

Oh TitleBar isnt there (i kind of missed this feature in this thread). I just cut & pasted the whole thing wouthout thinking. I will add it tomorrow.

Okay, I re-opened bug #304849 which says gdmsetup should support adding new users to cover that.  I'm also re-opening this bug until we get a bit more closure.

Okay, I see the options for Quiver, LockPosition, etc. now.  Thanks for clarifying.

After looking over the config file, some other things that might be nice to add would include the following.  Forgive me if you've already added some of these and I'm not noticing:

Might be nice if on the a11y tab if users could change the AddGtkModules value if they want.

Might be nice if you could set DefaultPath and RootPath also so that the default PATH for the user and root user can be configured.

Might be nice to be able to set SoundProgram

MaxIconHeight/MaxIconWidth allow you to set bigger face images.

Can I propose that it might be time to split gdmsetup into gdm-theme-chooser and
gdm-other-crack ?

Pushing every config option to the preferences ui seems like a really bad idea to me.

A lot of the options are pretty clearly in the "distribution picks the default, the user shouldn't change" category (why would anyone ever want to specify there own Halt command?!)

Any many of the options shouldn't be options at all "always restart server" (what do we gain by not doing it?), "Never place cookies on NFS" (we shouldn't be putting cookies in the users home directory at all, nfs or not.  It especially doesn't make sense to do it if DisallowTCP is true (which is the default)) 

And there are options that are deciding policy in the wrong place: "Allow system administrator login" ? that's something that should be configured from PAM.

These issues haven't been that big of a deal, because we can hide most of them away in /usr/share/gdm/defaults.conf and forget about 'em, but exposing them in the UI is really a bad idea.

Who are we designing the UI for?  Are they going to have any idea what it means to place a cookie (yum!) on NFS (some random techy TLA)?

There's a really good, relevant (if not old) article here:

http://ometer.com/free-software-ui.html

Perhaps a few of the features that have recently snuck into gdmsetup should be removed for good reason you describe above.  What specific ones do you think shouldn't be there.  I think most of the new entries are reasonable.

AlwaysRestartServer is just a performance tuning.  If you want to keep the same server running all the time then it runs a little faster.  But it doesn't work well in some environments.

I agree that NeverPlaceCookiesOnNFS is perhaps one that is too "technical" for gdmsetup.  I do agree that NeverPlaceCookiesOnNFS should only be active if DisallowTCP is false if we do keep it in the GUI.

I'm not sure that the GUI will ever make everyone happy, but it should at least allow people to make the sort of changes that are common.

Sorry for jumping in on this thread so late in the game, but I have a few concerns with this set of changes too.  The dialog is way too long.  It barely fits on my 1280x1024 display.  I think the cause is the new options added to the security tab.  Could some of these options be moved to a new tab (or removed) so that the dialog can return to a sane size?  

My minor nitpicks include: the UI change introduces a number of mnemnonic conflicts; the widget spacing is inconsistent (should be 18 pixels between categories, otherwise usually should be 6 pixels); the "Reboot, Halt, Suspend, and Custom Command" category content is not indented; label capitalization needs a couple fixes; and some labels are missing colons.  Also, tool-tips were added for new options, but tool-tips were not added for options that previously existed.

Created attachment 77971 [details] [review]
built against the latest (today's) cvs HEAD 

Attached a patch that fixes some of the issues rasised yesterday.

>A lot of the options are pretty clearly in the "distribution picks the default,
>the user shouldn't change" category (why would anyone ever want to specify
>there own Halt command?!)

I agree that most of users need not to touch the Halt command (or any system commands). But consider the following two scenarios:
1. Users might want to run they own custom shutdown scipts, for example that will combine patch/software update with system shutdown.
2. In a lab/corporate enviroment admins might want to restrict users from being able to shut down machines as automated maintenance is performed over night.

>Sorry for jumping in on this thread so late in the game, but I have a few
>concerns with this set of changes too.  The dialog is way too long.  It barely
>fits on my 1280x1024 display.  I think the cause is the new options added to
>the security tab.  Could some of these options be moved to a new tab (or
>removed) so that the dialog can return to a sane size?  

Indeed the Security tab is a lil bit over-crowded. Maybe adding another tab (called Advanced) and placing some of the more techincal options would be one of the options. Again with this sort of approach we might end up with a width problem and large number of tabs.

>My minor nitpicks include: the UI change introduces a number of mnemnonic
>conflicts; the widget spacing is inconsistent (should be 18 pixels between
>categories, otherwise usually should be 6 pixels); the "Reboot, Halt, Suspend,
>and Custom Command" category content is not indented; label capitalization
>needs a couple fixes; and some labels are missing colons.  Also, tool-tips were
>added for new options, but tool-tips were not added for options that previously
>existed.

Most of those issues should be fixed.

Also added a Show tool bar option for gdmlgin.

>I do agree that NeverPlaceCookiesOnNFS should only be active if
>DisallowTCP is false if we do keep it in the GUI.

NeverPlaceCookiesOnNFS becomes toggle-able only if DisalowTCP is false (this is just the ui restriction though)

>Who are we designing the UI for?  Are they going to have any idea what it means
>to place a cookie (yum!) on NFS (some random techy TLA)?
.
.
>I'm not sure that the GUI will ever make everyone happy, but it should at least
>allow people to make the sort of changes that are common.
I think there should be something for everyone in the gdmsetup. I think average users will probably only look at the Local tab and play with theme/welcome message settings leaving other tabs alone (so the defaults from /usr/share/gdm/defaults.conf will prevail). However for those that need the extended functionality the right options will be there.

Ray i will have a look at the article you posted :)

>Might be nice if on the a11y tab if users could change the AddGtkModules value
>if they want.
>Might be nice if you could set DefaultPath and RootPath also so that the
>default PATH for the user and root user can be configured.
>Might be nice to be able to set SoundProgram
>MaxIconHeight/MaxIconWidth allow you to set bigger face images.

I will add these options once we decide on the course of action (which options to be added/removed, which tabs added etc).

Awaiting your thoughts

Thanks for the updated patch.  I've committed to CVS head.  It's nicer.

I do have some thoughts based on the comments and some issues I notice...

Perhaps it doesn make sense to move some of the more advanced features to another tab named "Advanced".  Perhaps the Advanced tab should only show up if you run gdmsetup with a special argument like "--crackfeatures" or "--advanced".
What specific options do people think belong on this Advanced tab?  Note the GNOME HIG says you can have up to 6 tabs in a single notebook, so perhaps we
shouldn't do this.  But maybe having a 7th tab is okay if you have to type in an argument when you run the command to see it? 

I think there are two areas where the gdmsetup GUI is too long vertically.  In
the "Local" tab because we added the new Behavior section, and in Security.
Could we do something to make these less vertical.  Long ago, Automatic/Timed login used to be in the General tab, but we removed the General tab.  Perhaps we could move it back.  Could the widgets be reorganized a bit to make the GUI less vertical here?

Perhaps the Beahvior section of the Local tab could be made less vertical too.  Perhaps "Show title bar/Lock position" could be on the same line and also with "Set Position/Position X/PositionY" be on one line.  There seems to be space under Logo/Image that may not be needed.  Probably similar work needed on Remote tab when it is "Plain" style?

I think if Deny TCP connections is going to make "Never Place Cookies on NFS" insensitive, then "Never Place Cookies on NFS" should be right under "Deny TCP connections and indented a bit.

Never Place cookies on NFS probably needs a more descriptive label.  Always Restart Server can be moved to the General tab since it doesn't really affect security - it affects performance.  

I think being able to edit the Custom Commands is useful, regardless of what we decide to do with being able to set Halt/Reboot/Shutdown commands.

Note that when you move the combo box to a Custom Command, it spits out these errors:

Could not access configuration key <customcommand/CustomCommand2=>
Using compiled in value <> for <customcommand/CustomCommand2=>
Could not access configuration key <customcommand/CustomCommandLabel2=>
Using compiled in value <> for <customcommand/CustomCommandLabel2=>
Could not access configuration key <customcommand/CustomCommandLRLabel2=>
Using compiled in value <> for <customcommand/CustomCommandLRLabel2=>
Could not access configuration key <customcommand/CustomCommandText2=>
Using compiled in value <> for <customcommand/CustomCommandText2=>
Could not access configuration key <customcommand/CustomCommandTooltip2=>
Using compiled in value <> for <customcommand/CustomCommandTooltip2=>
Could not access configuration key <customcommand/CustomCommandNoRestart2=>
Using compiled in value <FALSE> for <customcommand/CustomCommandNoRestart2=>
Could not access configuration key <customcommand/CustomCommandIsPersistent2=>
Using compiled in value <FALSE> for <customcommand/CustomCommandIsPersistent2=>

These errors are coming from gui/gdmconfig.c.  Probably we should fix the code so that for the above keys we don't print out the messages.  Normally these messages are acceptable since the GDM programs shouldn't ask for keys that aren't in the config file, but the custom keys are special.

I notice a number of problems with this patch where we are calling g_slist functions with GList variables which seems to generate a lot of warnings.  

"gdmsetup.c", line 300: warning: assignment type mismatch:
        pointer to function(pointer to struct _GtkWidget {struct _GtkObject {..} object, unsigned short private_flags, unsigned char state, unsigned char saved_state, pointer to char name, pointer to struct _GtkStyle {..} style, struct _GtkRequisition {..} requisition, struct _GdkRectangle {..} allocation, pointer to struct _GdkDrawable {..} window, pointer to struct _GtkWidget {..} parent}) returning int "=" pointer to void
"gdmsetup.c", line 324: warning: argument #3 is incompatible with prototype:
        prototype: pointer to void : "/usr/include/glib-2.0/gobject/gobject.h", line 220
        argument : pointer to function(pointer to struct _GtkWidget {struct _GtkObject {..} object, unsigned short private_flags, unsigned char state, unsigned char saved_state, pointer to char name, pointer to struct _GtkStyle {..} style, struct _GtkRequisition {..} requisition, struct _GdkRectangle {..} allocation, pointer to struct _GdkDrawable {..} window, pointer to struct _GtkWidget {..} parent}) returning int
"gdmsetup.c", line 414: warning: assignment type mismatch:
        pointer to struct _GList {pointer to void data, pointer to struct _GList {..} next, pointer to struct _GList {..} prev} "=" pointer to struct _GSList {pointer to void data, pointer to struct _GSList {..} next}
"gdmsetup.c", line 424: warning: argument #1 is incompatible with prototype:
        prototype: pointer to struct _GSList {pointer to void data, pointer to struct _GSList {..} next} : "/usr/include/glib-2.0/glib/gslist.h", line 89
        argument : pointer to struct _GList {pointer to void data, pointer to struct _GList {..} next, pointer to struct _GList {..} prev}
"gdmsetup.c", line 534: warning: argument #1 is incompatible with prototype:
        prototype: pointer to struct _GSList {pointer to void data, pointer to struct _GSList {..} next} : "/usr/include/glib-2.0/glib/gslist.h", line 98
        argument : pointer to struct _GList {pointer to void data, pointer to struct _GList {..} next, pointer to struct _GList {..} prev}
"gdmsetup.c", line 1547: warning: argument #1 is incompatible with prototype:
        prototype: pointer to struct _GSList {pointer to void data, pointer to struct _GSList {..} next} : "/usr/include/glib-2.0/glib/gslist.h", line 98
        argument : pointer to struct _GList {pointer to void data, pointer to struct _GList {..} next, pointer to struct _GList {..} prev}
"gdmsetup.c", line 1985: warning: assignment type mismatch:
        pointer to function(pointer to struct _GtkWidget {struct _GtkObject {..} object, unsigned short private_flags, unsigned char state, unsigned char saved_state, pointer to char name, pointer to struct _GtkStyle {..} style, struct _GtkRequisition {..} requisition, struct _GdkRectangle {..} allocation, pointer to struct _GdkDrawable {..} window, pointer to struct _GtkWidget {..} parent}) returning int "=" pointer to void
"gdmsetup.c", line 2762: warning: assignment type mismatch:
        pointer to char "=" pointer to const char
"gdmsetup.c", line 3450: warning: assignment type mismatch:
        pointer to char "=" pointer to const char
"gdmsetup.c", line 7253: warning: argument #1 is incompatible with prototype:
        prototype: pointer to struct _GSList {pointer to void data, pointer to struct _GSList {..} next} : "/usr/include/glib-2.0/glib/gslist.h", line 50
        argument : pointer to struct _GList {pointer to void data, pointer to struct _GList {..} next, pointer to struct _GList {..} prev}
"gdmsetup.c", line 7253: warning: assignment type mismatch:
        pointer to struct _GList {pointer to void data, pointer to struct _GList {..} next, pointer to struct _GList {..} prev} "=" pointer to struct _GSList {pointer to void data, pointer to struct _GSList {..} next}
"gdmsetup.c", line 7257: warning: argument #1 is incompatible with prototype:
        prototype: pointer to struct _GSList {pointer to void data, pointer to struct _GSList {..} next} : "/usr/include/glib-2.0/glib/gslist.h", line 75
        argument : pointer to struct _GList {pointer to void data, pointer to struct _GList {..} next, pointer to struct _GList {..} prev}
"gdmsetup.c", line 7257: warning: assignment type mismatch:
        pointer to struct _GList {pointer to void data, pointer to struct _GList {..} next, pointer to struct _GList {..} prev} "=" pointer to struct _GSList {pointer to void data, pointer to struct _GSList {..} next}
"gdmsetup.c", line 7260: warning: argument #1 is incompatible with prototype:
        prototype: pointer to struct _GSList {pointer to void data, pointer to struct _GSList {..} next} : "/usr/include/glib-2.0/glib/gslist.h", line 45
        argument : pointer to struct _GList {pointer to void data, pointer to struct _GList {..} next, pointer to struct _GList {..} prev}

Created attachment 78089 [details] [review]
built against the latest (today's) cvs HEAD 

>I notice a number of problems with this patch where we are calling g_slist
>functions with GList variables which seems to generate a lot of warnings.  

Fixed

>Always Restart Server can be moved to the General tab since it doesn't really >affect security - it affects performance.  

Done

>I think if Deny TCP connections is going to make "Never Place Cookies on NFS"
>insensitive, then "Never Place Cookies on NFS" should be right under "Deny TCP
>connections and indented a bit.

Done

>I think there are two areas where the gdmsetup GUI is too long vertically.  In
>the "Local" tab because we added the new Behavior section, and in Security.
>Could we do something to make these less vertical.  Long ago, Automatic/Timed
>login used to be in the General tab, but we removed the General tab.  Perhaps
>we could move it back.  Could the widgets be reorganized a bit to make the GUI
>less vertical here?

Ah after the current re-shuffle the height of the dialog should be much smaller (wonder if it is sufficently smaller though). The two cultprits are the security tab and general tab. General tab contains a fixed size vbox that stores all of the command widgets (it used to be dynamically resized but it looked ugly when custom command appeared for the first time - the height increased by around 200 pixels all of a sudden). I think behaviour section does not contribute to the large vertical size (switching between plain - themed greeters does not cause any resizes). We could re-shuffle the elements around but im kind of hoping that the size is going to be ok now - please let me know if that is the case

>Note that when you move the combo box to a Custom Command, it spits out these
>errors:
>Could not access configuration key <customcommand/CustomCommand2=>
>Using compiled in value <> for <customcommand/CustomCommand2=>
>Could not access configuration key <customcommand/CustomCommandLabel2=>

Hmm i do not get those. Does this happen only when debug mode is enabled?

Also in theis patched fixed some of the gdm compiler warnings

Created attachment 78096 [details] [review]
built against the latest (today's) cvs HEAD 

Ignore previous attachment plz. This is the correct one

Cheers

One idea I had for the general tab, move the custom command stuff to its own dialog and add a "Edit Commands..." button.

>Perhaps it doesn make sense to move some of the more advanced features to
>another tab named "Advanced".  Perhaps the Advanced tab should only show up if
>you run gdmsetup with a special argument like "--crackfeatures" or
>"--advanced".

Indeed sounds like a good idea. After reading the article recomended by Ray i do fee reluctant to put zillions of differnt tabs with scary names like "Advanced" ;). Having loads of options it might discourage ppl from using gdmsetup as it will have a steep learning curve. But maybe with "--advanced" switch users will we somhow warned what they letting themselves in for. As Brian mentioned we need to decide what features need to go where

>One idea I had for the general tab, move the custom command stuff to its own
>dialog and add a "Edit Commands..." button.

Is is an option (this was my original design) but it will make General tab look a bit bare though

Actually I notice that the longest vertical gdmsetup tab is Local when "Style" is Plain.  My suggestions to move some of the checkboxes so that multiple ones appear on the same line when they are related (Show title bar/Lock position) and (Set Position/PositionX/PositionY) might make this even better.

Moving the Commands to a pop-up dialog would probably make it look nicer.

Might be nice to explain what the General/AlwaysRestartServer choice does.  Something like "Restart the Xserver with each login.  Restarting is slower, but better protects against stability issues."  At the very least, it should say "Xserver" instead of just "server".

On Security tab could probably place Allow local/remote system admin login checkboxes on the same line to make less vertical.  Could probably also place
Timed Login pause and "allow remote timed logins" on the same line.

Might look better to move the two spinbuttons to the bottom or top so it doesn't visually switch from checkboxes to spinbuttons to checkboxes to the Permissions radiobuttons.

On Local/Remote tab the "Set position, Lock position, Position X and Position Y" could probably have better labels.  "Set position of window", "Position X of window".

On "Security" tab didn't "Deny TCP connections to Xserver" have a comment that explained what it does that seems to have gone away?  I still think "Never place cookies on NFS" needs a better label."  "Minimal UID" might be good to explain (it only affects the face browser, and what users are valid for automatic/timed login).  Also "check directory owner" should have a better label.  "Only allow login if user owns their home directory."

Note that "Check directory owner" and "Permissions" are related and probably should be next to each other.  Disable multiple logins for a single user isn't really a security issue either and could also probably be moved to the General tab.

Sorry that the comments keep coming, but reviewing GUI's is always a bit of a pain.  I think we are getting close, though.

Mo probs :) this is the only way to make it better

Created attachment 78419 [details] [review]
built against the latest (today's) cvs HEAD 

>My suggestions to move some of the checkboxes so that multiple ones
>appear on the same line when they are related (Show title bar/Lock position)
>and (Set Position/PositionX/PositionY) might make this even better.

I have moved PositionX, PositionY up onto the SetPosition line. I have not moved others as i think two checkboxes on the same line do not look right. (im kikndof hoping that moving SetPosition widgets will be sufficient)

> Moving the Commands to a pop-up dialog would probably make it look nicer.

Done. Commands now reside in a separate dialog. Launchable form general tab through Edit Commands button.

>Might be nice to explain what the General/AlwaysRestartServer choice does. 
>Something like "Restart the Xserver with each login.  Restarting is slower, but
>better protects against stability issues."  At the very least, it should say
>"Xserver" instead of just "server".

Label changed

>On Security tab could probably place Allow local/remote system admin login
>checkboxes on the same line to make less vertical.  Could probably also place
>Timed Login pause and "allow remote timed logins" on the same line.

I dont think two checkboxes next to each other on the same line will look right

>Might look better to move the two spinbuttons to the bottom or top so it
>doesn't visually switch from checkboxes to spinbuttons to checkboxes to the
>Permissions radiobuttons.

Done

>On Local/Remote tab the "Set position, Lock position, Position X and Position
>Y" could probably have better labels.  "Set position of window", "Position X of
>window".

Done. changed to Lock position of the window, Set position of the window, X coordinate, X coordinate

>On "Security" tab didn't "Deny TCP connections to Xserver" have a comment that
?explained what it does that seems to have gone away?  

I have moved the text into the tooltip due to the lack of vertical space

>"Minimal UID" might be good to explain (it only affects the face browser, and >what users are valid for automatic/timed login).  

I think this is mentioned in the tooltip

>Also "check directory owner" should have a better
>label.  "Only allow login if user owns their home directory."

Done

>Note that "Check directory owner" and "Permissions" are related and probably
>should be next to each other.  Disable multiple logins for a single user isn't
>really a security issue either and could also probably be moved to the General
>tab.

Done

>Could not access configuration key <customcommand/CustomCommand2=>
>Using compiled in value <> for <customcommand/CustomCommand2=>

I managed to see those warning messages i will look into that

Latest patch updated in CVS head.  Thanks.

Couple issues I notice:

- I think putting X/Y Coordinate on the Set position line makes the vertical
  issue much better on Plain tab.  Also think the Security tab looks much
  nicer now. 

- Might want to update the tooltip for "Enable debug messages to system log"
  on the Security tab to mention that this should not be enabled unless there
  is a need to collect debug information.  This is just a paranoia issue since
  although I don't think any debug messages expose anything that could 
  compromise security, not logging message is probably more secure.

- On the Local tab there are two things with mnemonic "h" (Show title bar and
  the Help button)

- For some reason the Alt-O key doesn't seem to go to the Login Retry Delay
  spinbutton.  Alt-m goes to _Minimal UID, though.  Any ideas what this issue
  might be?

- Shouldn't the Permissions choices have mnemonics on security tab?

- You aren't responsible for this, but on the Users tab the Add and
  Remove mnemonics only work for the Include area.  Probably need
  different mnemonics for Include/Exclude buttons.  Probably requires
  changing the label on the button "Add to _Include" or something?

- On "Edit Commands" dialog there are two mnemonics with "A" (Add
  button and Apply Command Changes).

- "Maximum pending requests" in "Configure XDMCP" dialog has no
  mnemonic.  Nor does Ping Interval.  Note you have to enable
  Remote on the Remote tab to see this button.  Again, you aren't
  responsible for this - but nice to fix with the other mnemonic
  fixes.

Created attachment 78478 [details] [review]
built against the latest (today's) cvs HEAD 

> - Might want to update the tooltip for "Enable debug messages to system log"

Done

>- On the Local tab there are two things with mnemonic "h" (Show title bar and
>  the Help button)

Fixed

> - For some reason the Alt-O key doesn't seem to go to the Login Retry Delay
>  spinbutton.  Alt-m goes to _Minimal UID, though.  Any ideas what this issue
>  might be?

It had a wrong focus target specified. Fixed now

> - Shouldn't the Permissions choices have mnemonics on security tab?

Done

>- You aren't responsible for this, but on the Users tab the Add and
>  Remove mnemonics only work for the Include area.  Probably need
>  different mnemonics for Include/Exclude buttons.  Probably requires
>  changing the label on the button "Add to _Include" or something?

Strange - this seems to work for me for exclude stuff as well as the include

>- On "Edit Commands" dialog there are two mnemonics with "A" (Add
>  button and Apply Command Changes).

Fixed

>- "Maximum pending requests" in "Configure XDMCP" dialog has no
>  mnemonic.  Nor does Ping Interval.

Had duplicate/unavailable mnemonics assigned. Fixed now

> Again, you aren't responsible for this ....

I think we should fix as many bugs as we can find now, as the whole design and code is in our memory and it doesnt take long to fix simple things.

Also i have fixed a minor bug with commands_dialog. If there were changes to be saved and user clicked on Help button he was prompted for a save/ignore changes response. Now dialog callback function conditions saving on the dialog response, i.e. save only if the response is not equal GTK_RESPONSE_HELP

Thanks, I think the mnemonics are more sensible now.  Looks good to me.  Should we close this bug now?  Anything else to do?

Not that i can think of atm. Imn sure new things will come up at some stage but we can always open a new bug report for that

What about the warning messages in comment #45?

Could not access configuration key <customcommand/CustomCommand2=>
Using compiled in value <> for <customcommand/CustomCommand2=>
Could not access configuration key <customcommand/CustomCommandLabel2=>
Using compiled in value <> for <customcommand/CustomCommandLabel2=>
Could not access configuration key <customcommand/CustomCommandLRLabel2=>
Using compiled in value <> for <customcommand/CustomCommandLRLabel2=>
Could not access configuration key <customcommand/CustomCommandText2=>
Using compiled in value <> for <customcommand/CustomCommandText2=>
Could not access configuration key <customcommand/CustomCommandTooltip2=>
Using compiled in value <> for <customcommand/CustomCommandTooltip2=>
Could not access configuration key <customcommand/CustomCommandNoRestart2=>
Using compiled in value <FALSE> for <customcommand/CustomCommandNoRestart2=>
Could not access configuration key <customcommand/CustomCommandIsPersistent2=>
Using compiled in value <FALSE> for <customcommand/CustomCommandIsPersistent2=>

Another thing to be aware of is that I'm currently working to add patch #360984.  It is being delayed because I am working with Calum to determine exactly which icons we should use.  Once I get that worked out, you might want custom commands to have icons too.  If you wanted to add a patch to allow custom commands to have menu icons like this, we could add it now.   But might also want to add it to gdmsetup to add icons there too.

>What about the warning messages in comment #45?

It has slipped my mind, sorry will look into that

>If you wanted to add a patch to allow custom commands to have menu icons ...

I have not had a look at the new icon patch yet but im guessing you are referring to icons that appear on the action or options menu on the themed greeter right?
I did think about it but i think it will be difficult to find a general description icon that will describe the diveristy of the custom command. Also it is possible for all 9 custom commands to be present at the same time so the icons should differ from each other in some small detail.

>But might also want to add it to gdmsetup to add icons there too

It would be nice to have icons on the Edit Commands button and all (or most) subsequent custom command releted widgets.

One kwik question - are you going to commit patch 78478 or shall i include those changes in the next patch (that hopefully will fix the comment #45 issue)?

I'm confused.  Patch 78478 is already in GDM CVS.  So if you provide new patches, please don't include patch 78478 again.  

In terms of icons for the commands, I was thinking that people might want to specify what icon to use for each new custom command they add.  You suggest that users might want to change the icons for the built-in commands (reboot, suspend, shutdown, configure login manager, login via XDMCP, etc.).  I was thinking it
would be okay to just hardcode the icons for the built-in commands, but if you
think it adds a lot of value to make them configurable, I would be agreeable to
accept a patch to allow them to be changed.  I was just thinking that if we add
icons for the built-in commands, we might also want to allow the config file to
specify what icon to use for each custom command (much like you can currently
specify label text, etc.).

Having said all this, we probably should wait until patch for bug #360984 to go in before we worry about this.  Once it goes in we can decidee what configuration options should be added and how we want gdmsetup to work for it.  I just wanted to alert you that this is a minor change that might affect custom commands.

Some minor comments...

One issue I find with gdmsetup is that when I go to the "Edit Commands" button
and edit a Custom Command, it is not very clear to me what the difference between Label, LRLabel, and Text are.  Perhaps the labels should be updated to be a little more clear about where the labels and text are used.  I assume one is the button label, another is what shows up in the pop-up dialog text.  Also might be nice to use a more clear label than Persistant.  What does persistant mean?  

On the "General" tab the label says "Show visual feedback in the password entry".  Wouldn't it be more clear to say "Show visual feedback (asterisks) in
the password entry".  This way the other label that says "Use circles instead
of asterisks" would make more sense.  On Security tab the Minimal UID tooltip 
says "All users with a lower UID will be excluded from face browser".  What
if the UID is the same value as Minimal UID?  Is it excluded?  I can't remember
if it is "<" or "<=".

Created attachment 78753 [details] [review]
built against the latest (today's) cvs HEAD 

>I'm confused.  Patch 78478 [edit] is already in GDM CVS.  So if you provide new
>patches, please don't include patch 78478 [edit] again.  

oki doki i won't. Was confused because i have not received any status change releted to that patch (i didn't check cvs though).

>warning messages in comment #45

Fixed but please do check.

>One issue I find with gdmsetup is that when I go to the "Edit Commands" button
>and edit a Custom Command, it is not very clear to me what the difference
>between Label, LRLabel, and Text are.  Perhaps the labels should be updated to
>be a little more clear about where the labels and text are used.  I assume one
>is the button label, another is what shows up in the pop-up dialog text.  

I have added some text below each of the command fields explaining briefly what is the field about (more info can be found using tooltips). Please let me know what you think

>Also might be nice to use a more clear label than Persistant.  What does >persistant mean?  

Persistant means that the command can be accessible outside of gdm login screen (in a simmilar way that shutdown, reboot and suspend do on the gnome desktop through shutdown panel widget). Atm this is not used as I'm still awaiting the response from the gnome-panel ppl (i have already submited the patch). Maybe i should give them a little nudge as it has been a while since the initial submission

>On the "General" tab the label says "Show visual feedback in the password
>entry".  Wouldn't it be more clear to say "Show visual feedback (asterisks) in
>the password entry".  

Done

>All users with a lower UID will be excluded from face browser".  What
>if the UID is the same value as Minimal UID?  Is it excluded?  I can't remember
>if it is "<" or "<=".

All the UID's < than the MinimalUID will be excluded. All UID's >= then MinimalUID can be added to the Include list

>In terms of icons for the commands, I was thinking that people might want to
>specify what icon to use for each new custom command they add.  You suggest
>that users might want to change the icons for the built-in commands (reboot,
>suspend, shutdown, configure login manager, login via XDMCP, etc.).  I was
>thinking it would be okay to just hardcode the icons for the built-in commands, >but if you think it adds a lot of value to make them configurable, I would be >agreeable to accept a patch to allow them to be changed.  I was just thinking >that if we add icons for the built-in commands, we might also want to allow the >config file to specify what icon to use for each custom command (much like you >can currently specify label text, etc.).

I was only refering to the custom command icons. Halt/reboot/Suspend, etc should be hard-coded. It shouldn't be too difficult to add additional option to the config for custom commands but i do agree lets wait untill bug #360984 is resolved.

Awesome!  I think that this looks really good.  I think all issues identified have been addressed.  I am going to close this bug now.  Any additional issues can be dealt with by opening new bugs rather than continuing this already overlong bug report.

I know some people have complained that some of the new features are perhaps "too advanced" for most users.  Hopefully now that the labels have better descriptions this is not so bad now.  However, if people think specific new features are inappropriate for gdmsetup, I recommend creating a new bug and we can remove features if people think that they aren't appropriate.

Also, some have complained that the gdmsetup window is too big.  We've made some improvements so it is not as bad now, but if people still find issues with this then we can perhaps address this as a separate bug.  Perhaps some features can be moved to pop-up dialogs like we moved the "Edit Commands" to a separate dialog.
Again, lets open a new bug (hopefully with specific suggestions on how to improve the dialog).

Again, Lucasz, thanks for the hard work.  This makes GDM much more easy to configure, I think.

My pleasure :)
I know this bug has been closed but i do not see the latest patch in cvs head. Did you submit it?

Oops, sorry....committed now.