Created attachment 55037 [details] [review]
set background image for graphical greeter

Frederic:

I don't think we should be using the same background keys for gdmlogin and
gdmgreeter.  We just fixed gdmgreeter so that it uses a separate key for
specifying the background color.  If you want to also add the ability to have an
image, then defining new keys in gdm.conf for the greeter background_type and
background_image would be better.

Your patch also needs some work:

1) It doesn't compile against CVS head.  We can't make GUI or API changes in 
   the 2.12 branch anymore.
2) You need to update gdmconfig so it works with the new configuration 
   options.  Even if we decided to use the same keys for gdmlogin and
   gdmgreeter, the new options would have to be made available in the
   "gdmgreeter" section of gdmsetup.

Well, patch was done against 2.8.0.6, which is why it doesn't apply to HEAD (but
I can fix that) :)

I'm not sure why we need to specify different keys for gdmlogin and gdmgreeter,
but if it is the path you want to follow, I'll adapt my code.

Just tell me and I'll fix my patch (I really want to get this merged :)

Frederic, I'm glad to hear that you are willing to update the patch.  I've added
Dennis and Calum to the cc:list.  We've been doing a lot of work redesigning
gdmsetup so it works better and it isn't clear to me if it makes the most sense
for gdmlogin/gdmgreeter to share the same keys or have separate keys for
specifying background color/image.  Either way we do it, it will affect the
gdmsetup GUI, and I want to make sure we make the right choices here.

Created attachment 55153 [details] [review]
greeter background, based on HEAD

I've modified my patch so it can be applied to cvs HEAD.

I can modify it easily to support new configuration keys if needed.

Since we already split the color key for gdmlogin and gdmgreeter, I would
recommend just doing the same for the background key.  The new setting will need
to be added to gdmsetup.  On the local/remote tab, I believe we have enough room
to the right of "Background color" to squeeze in a "Background image" setting. 
Well, those are my thoughts.

Created attachment 71604 [details] [review]
updated patch for 2.16 HEAD

Brian, could you comment on previous recommendations. I'm ok to improve the patch for 2.18 development.

Issues I notice:

1) There are no changes to the docs.  This sort of change needs to be documented.
2) We should use a different key for specifying the background image so that 
   gdmlogin and gdmgreeter use different config keys.  Just like we do for
   background color.
3) Also, doesn't look like gdmsetup has been updated to support this option.
   I'd prefer this to be done before accepting the patch.

In fact, it seems all the recommendations above haven't been added to the patch aside from making it compile against head.  Am I missing something?

not at all, seb128 asked me a refreshed version of the patch for current version of gdm, which is why I pinged back, and moreover, since only Dennis gave some recommendations, I wasn't sure I had to follow them or not.

I'll try to implement your requests in september or october, once 2.16.0 is out.

Thanks.  I think it would be better to implement this as a different key.  We've found in the past that trying to share keys between the greeters causes problems.

For example, it is possible to set up GDM so that the console and remote login are using different greeter programs (say gdmgreeter for console and gdmlogin for remote is recommended since gdmlogin is lighter and more appropriate for remote displays).  Since the color schemes can be quite different, you don't really want to share the same color for both.  We've had various distros complain that their default color that looks great with their gdmgreeter theme looks really nasty when users convert to gdmlogin for whatever reason (or log in via XDMCP).

Does this make sense?  It shouldn't be hard to add a new key.  Just add it to gdm.h, look at the code in daemon/gdmconfig.c, and it should be fairly easy to see how to add a new key.  Then just reference the key as appropriate in the greeter code via the get_config functions that are used to get the other config values.

And the docs should obviously be updated when the config file changes.

If you don't implement the gdmsetup code, I will accept the patch, but it would be nice if users could set the color in gdmsetup too.  If we go this way, we can close this bug when the patch is updated and file a new bug about the fact that gdmsetup doesn't support this feature.

Note that in bugzilla bug #355190, that it was mentioned that one cool way to
support this sort of feature would be to enhance the GDM theme XML format so that certain tags could be marked as being a part of the "background".

Then when the GUI is done authenticating, it could simply remove all canvas layers that do not have the background tag.  This would allow theme designers to specify which layers are a part of the background, and have GDM simply continue displaying this "background" until the GUI is torn down.

This is probably a bit better than specifying an image file to be the greeter background since in most cases this just adds extra work for the GDM theme UI designer to provide the "background" part of the theme as a separate "background
image" file.

I'd just add a new attribute tag to the <item> node, so it would look like
this for the circles theme if you wanted the background image and the flower to be the "background":

  <item type="svg" background="true">
    <normal file="background.svg"/>
    <pos x="0" y="0" width="100%" height="-75"/>
  </item>
[...]
  <item type="pixmap" background="true"
    <normal file="flower.png"/>
    <pos x="100%" y="100%" anchor="se"/>
  </item>

Note adding this is pretty easy.  Just modify gui/greeter/greeter_parser.c in
the "parse_item" function to check for the new "background" tag element and add 
a  boolean to the "info" structure to keep track of whether the item is a part
of the background.  You'll have to update the greeter_parser.h file to add the
new boolean to the info structure.

Then after authentication, it would be necessary to add some code to loop over
the displayed theme elements and make the non-background theme elements 
disappear (checking the new info->background boolean).  Note the function 
greeter_item_pam_error_set in greeter_item_pam.c which shows example code of how 
to make libgnomecanvas layers appear and disappear with the 
gnome_canvas_item_show/gnome_canvas_item_hide functions.

Hope this is helpful.

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

funny : three weeks ago (just before my two weeks holiday which are now finished) , I started working on my patch to adapt it to your previous comments (it is working but UI changes in gdmsetup are a little ugly) and I remember regretting lack of a "background" tag in the theme format ;)

I'll try to do another patch to add this tag in theme format and use it instead of the previous solution.

BTW, we also need to apply background when doing autologin (at least, in our case, it allows smoother login).

Brian, I've implemented most of your suggestions but I'm not sure where we should set background : just before exiting greeter ?

Another problem is we still need to change root window, otherwise, as soon as greeter application will be terminated, root window will be displayed again and user will not keep their gdm "background" until login is complete, which is the point of initial patch.

In comment #11, I am just talking about different ideas about how to approach this problem and how we *could* fix it.  Really, any solution that is reasonable I would accept.  There are several things to think about:

1. Do we want the same "background" image to be used for all themes, or 
   do we want each theme to be able to specify its own background, or do we
   want both (e.g. if the theme specifies, then use it, if not use a default).
2. If the "background" image really is the same as a subset of the components
   already in the GDM theme, might it be better to add a feature where the
   theme could specify what elements of the theme are the "background" so
   that GDM could simply build the background image from these parts of the
   theme directly?  The advantage of this is that the theme designer wouldn't
   have to actually create a separate image file that (in reality) is just a
   subset of what is already there).  Note that this approach could be
   supported in addition to or instead of the solution #1.

Assuming we think approach #1 is right and we want the same background for all themes, then your original patch is okay (aside from the fact that it should
use a different greeter-specific key, should be supported in gdmsetup, and should have docs).

If we want to support different background images per-theme, then it would be necessary for the theme xml file to also be able to specify a background image to use (which could perhaps override the one in the GDM configuration file).  Or perhaps we only specify this in the xml file and only support this per-theme?

Some more discussion about the idea proposed in comment #11 to help you understand my thoughts...

It might be cool if, instead of showing a background image, if certain components of the theme just disappeared.  Most themes have a background image already with things like buttons, the username/password entry, etc. layered on top.  The idea was that if we marked certain things in the XML file as background="true", then the "background" items would persist while the ones with background="false" would disappear (buttons, entry field, etc.). 

This might make it a little easier to display the background effect if the theme "background" is really a composite of a bunch of stuff (like several images on top of each other with alpha values, etc.).  If the GDM configuration file *only* works with a background image, then the person would have to create an actual image file to match this to be the "background" graphic to display.

However, if the user *wants* the background image to display after GDM to be different looking than the GDM theme itself, then this approach might need to
be further extended to support this.

If you are still unsure of what I'm describing or want to discuss ideas of how to improve this further, lets talk more in this bug report.

(argg, browser crashes after I wrote my long reply, let's hope I don't forget something this time ;)

While my initial patch was implementing proposal 1a (same background for all), I don't think it should be integrated as it, because it clutters gdmsetup UI and it forces people changing this setting as soon as they change theme. I could be ok in a "vendor" spirit when you optimize only the default settings but we probably shouldn't do that upstream.

So, I favor proposal 1b ie allow each theme to set a background. I don't think it is worth adding yet another configuration setting for this part of login : as said previously, it would clutter gdmsetup UI and this part of login is not important enough to have its own configuration. If a theme doesn't provide a background information, gdm should just do nothing, just like currently.

I've almost finished implementing proposal 1b with "background" attribute in XML.

Now, I'm polishing the patch to fix issues like the menubar hack (which is not in the GreeterItemInfo tree but only in Gnome Canvas) and items size which must be recomputed before using background.

I can post work in progress patch in you want to start reviewing it.

Created attachment 86652 [details] [review]
new implementation, using "background" property on item 

new implementation.

there is an independant trial code cleanup (see gui/greeter/greeter_item_pam.c file) I did at the same time.

I've moved some code from gdmlogin.c to gdmcommon.c to be able to set root window background. The rest is quite easy to understand (I think).

Comments welcome.

You didn't modify docs/C/gdm.xml to explain how the new option works.  It would also be nice if you made at least one of the default themes (circles, happygnome or happygnome-list) use this option to demonstrate the feature.  It would also make it easier for me to test.  Even if you don't add this to one of the default themes, I'd like you to attach a theme that does use it so I can test it.

Created attachment 86696 [details] [review]
new version

new patch version :
-add documentation to docs/C/gdm.xml
-modified shipped themes to use background feature
-remove forgotten g_print debug statement (oops)
-call process_operation (GDM_QUIT, NULL) when pressing ESC when greeter is in development mode : allow to easily test background feature without really logging.

Overall the patch looks good.  I have a few questions for discussion, though, before it goes in.

1) I notice you change the menubar to being saved in the item data structure.
   So it now is item->data.text.menubar.  Could you explain why this is needed?

2) I notice that you create a new item of type "background".  One problem with
   this approach is that the user could define multiple backgrounds, and could
   get odd behavior if they did so.  Might it be better to make "background"
   a property of the <greeter> tag.  Note this <greeter> tag already supports
   the "gtk-theme" property.  So the tag would look like this:

   <greeter background="image.svg" gtk-theme="optional theme to use">

   or

   <greeter background="image.jpg">

   I don't think that the "<pos x="0" y="0" width="100%" height="-75"/>" stuff
   is really meaningful for the bakcground image, since it probably should 
   always be full screen?

   This might be a bit better to ensure the background is only specified in one
   place?

   What do you think?  I'm open to discussion here.

3) Did you test only with type SVG, or also with jpg/png/etc images (pixmap)
   type?

1/ this change was needed, otherwise, when all non background items are hidden from the canvas, you will see the menubar (it was quite surprising initially), because it is only added on canvas and not in the GreeterInfoItem tree.

2/ indeed, you are right : because of the <pos > stuff, you can't really get a fullscreen background image with my current page. But on the other hand, you can still get a reduce set of items which were used for the entire greeter, so people could try to have different elements on the resulting background (for instance, with default theme, I left the nice flower) but this is probably overkill. I'm ok in doing another iteration of the patch with <greeter background="foo.jpg"> if you are ok too. (If we are going this path, it will probably simplify code much more, probably ending with something similar with my initial proposal, with added bonus of caring anymore about canvas and its ugly hack like menubar one :)

3/ yes, I tested also with jpg images (ie Mandriva theme), it worked fine.

This works great.  I just tested it and committed your change.  Though I made a minor change to the happygnome and happygnome-list xml files, which I also think points out a possible bug in the way this is coded:

-  <item type="svg">
+  <item type="svg" background="true">
     <normal file="background.svg"/>
     <pos x="0" y="0" width="100%" height="-75"/>
   </item>
   <item type="rect">
-    <normal color="#000000"/>
+    <normal color="#000000" background="true"/>

Note the background specified in the <normal> tag, which makes the bottom section appear properly.  Instead I do the following, which looks better since it also leaves the black line on the top of the gray section.  Note that I specify background on the <item type="rect"> and also the internal <item type="rect">

  <item type="svg" background="true">
    <normal file="background.svg"/>
    <pos x="0" y="0" width="100%" height="-75"/>
  </item>
  <item type="rect" background="true">
    <normal color="#000000"/>
    <pos x="0" y="-75" width="100%" height="75"/>
    <fixed>
      <item type="rect" background="true">

It seems ugly to specify background on an internal tag (e.g. <normal color>).
I'd think you should *only* be able to specify background on an <item> tag.  I'd expect odd behavior if allow background to be specified for internal tags.  Or is there a good reason for supporting this.  If so, what use case?

I think we should fix this, and fix the parser to only allow this background tag to be specified for actual items.  What do you think?  If you agree could you provide a patch against SVN head to fix this?  Then I'll close the bug.

Also, in happygnome & happygnome-list, I added "background" tag to the foot logo.  This helps people to see what the feature is doing, I think, when using these themes (much like how the flower works in circles).

Note that I just updated the docs for the "background" item node.  According to the docs this should only work for item nodes, so I think this also supports my thinking that it is a bug that it "sort of" works on the <normal> tag.  Please review my docs changes and see what you think.

A few further ideas:

1) I think the current behavior is that if no background items are set, then
   GDM will continue to display the GraphicalThemedColor for a second, still
   creating the "flicker" problem.  Do you think it would be nicer if GDM
   just didn't do this, and instead rendered the entire GDM GUI screen to
   the background so that the transition goes directly from GDM GUI screen to
   session without displaying GraphicalThemedColor in the interim?  This might
   be a better default when no items have "background" than using the current
   logic.  What do you think?  If so, we could deprecate the 
   GraphicalThemedColor configuration key.

2) I assume that if somebody wanted to that they could specify a completely
   different image to use as the "background".  I'd imagine you'd do this
   by creating an item type rect with the background to use after authentication
   (with the background property) and putting another rectangle of the same size 
   with the background to use in the greeter (without the background property).
   Then when the user authenticates, it would look like a different image 
   would appear as the "background".  I'd imagine there might be some cases
   where people might want to do this.  If this sort of technique works (I
   haven't tested it), it might be useful to mention how to do it in the docs.

3) Note that you partially implemented a fix for RFE bug #355190.  This bug
   suggests that GDM should continue managing the background until the session
   tells it to stop, allowing GDM to manage the transition up-to-the-point that
   the session is running.  The idea in this bug report was that we'd use the
   "background" property to specify what would display in the interim.  I 
   rejected the patch in that RFE because they were using a hacky mechanism to
   tell GDM when to stop displaying the background image.  It would be better
   to create some new interface with gnome-session where GDM could ask
   gnome-session "do you support this feature" (perhaps by calling gnome-session
   with a special argument that returns true or false).  If so, then GDM would
   display longer and gnome-session would tell GDM when to quit.  Then GDM
   could display some sort of throbber or something in the interim.

4) If we did the above, it might be interesting if the background could be 
   somewhat "animated".  This could easily be done by supporting additional
   properties for items with the background property specified.

   Like perhaps "background-display-time=5" and "background-delay-time=5"
   (just suggestions off the top of my head).  This way you could specify 
   certain images that would show at certain points in time and for certain
   lengths in time.  For example, this could be used to show little
   "commercial" or eye-candy images while the user waits for the session to 
   start.

   In other words, after authentication GDM would show only the background
   images, show a throbber in the middle of the screen and the 4 corners
   could show various thumbnail sized images that appear and show for 1-2 
   seconds and then disappear.  All specified by the theme file.  It might
   also be cool to support some property like "background transition" so you
   could make the images fade in/out or other effects.

   This wouldn't be that much more work than what has already been done.

(I've changed patch status to committed)

I'm ok with our documentation changes.

I didn't test background properties on non-item node. If it works, it is a bug or unwanted feature :)

Regarding your proposals :
1/ I don't think we should push the gdm graphical greeter rendering to background window if no background node is found : it might confuse users ("wait, did I really logged in") and this can stay a long time until gnome-settings-daemon kicks in and refresh the background (or nautilus) : I have a test system where you have to wait about 15s until background is refresh ...

2/ this technique works, I tested it. You just need to do it in the beginning of the file (to follow parsing order). Probably worth documenting

3/ I agree my patch is a poor man version of bug #355190, after all, initial version was written a long time ago :) I'm not sure it is gdm job to take care progress report for login (and moreover, it is a little too much GNOME specific for my taste). gnome-session splash could perfectly handle this part of the job, moreover since the flickering problem is now fixed with current background patch.

4/ I'm a little afraid, just like I wrote above, doing animated background for login is not really gdm business.

Frederic:

Okay, I agree with your points #1, #3, and #4.  If you want to provide some extra docs to explain #2 a bit more that would be nice.

Regarding supporting "background" properties on non-item nodes, I'm a bit confused because in your original patch, you were applying the following for the happygnome theme.

-  <item type="svg">
+  <item type="svg" background="true">
     <normal file="background.svg"/>
     <pos x="0" y="0" width="100%" height="-75"/>
   </item>
   <item type="rect">
-    <normal color="#000000"/>
+    <normal color="#000000" background="true"/>

Note that the second background is in the <normal> tag.  I assume this was causing the bottom area to be white, or was the parser just silently ignoring this property since it was misplaced?

Created attachment 87553 [details] [review]
improve documentation

I've improved documentation about using a different background for login transition vs greeter.

My original patch was wrong. I think parser doesn't complain about unknown attributes. It is probably better this way, since it allows to use new themes with old version of gdm and maybe to parse future version of theme format with current version of gdm (joy of XML :)

Thanks for the improved docs, and for answering my questions.  I think this is wrapped up now, and am closing this bug as FIXED.