GNOME Bugzilla – Bug 522890
desire a new button to select desktop environment
Last modified: 2010-06-04 20:45:35 UTC
looking at http://www.jirka.org/gdm-documentation/x1454.html , the only way to choose desktop envinronment seems to use id="session_button", which beleaves as a list . I need a single button for each available session. May it be possible to add this kind of feature? Other information:
The latest GDM docs are located at http://www.gnome.org/projects/gdm/docs.html The docs at www.jirka.org are really old. GDM currently does not have any mechanism for providing a button that corresponds to a specific session. However, if someone wanted to provide a well-written patch to provide such a feature, it could go upstream.
sorry. I'm not a programmer. anyway, if you show me the exact point to touch the code, I can have a look to it.
If you look at gui/greeter/greeter_parser.c, you will see the code that parses the XML file. It would probably be necessary to define new button types with a new "id" value that cooresponds to a specific session button, and probably necessary to parse a new element that would specify which session to launch. So you would perhaps define it in the xml like this: <item type="button" id="start_session" session_name="gnome"> Then notice how in greeter_item_session_setup() in greeter_session.c that we register an action callback for the "session_button" and specify a handler. You would need to add a new handler type for the "start_session" id. The callback function for this new button type would do something similar to what greeter_session_handler does, except instead of launching the pop-up dialog and having the user pick a session, you would just set the session to the specific value associated with the button.
thank you. now wait for a couple of mounth :).
sorry. I've read the code, but I'm not able to make the patch because I'm not able to add new types and other elements. anyway, I think that the feature I'm asking for is important for gdm and is very simple to enforce for you, because it's a basic feature in the end-user point of view. so, please add it to gdm.
Created attachment 110724 [details] [review] proposed patch patch tested with gentoo gdm-2.20.5. I don't know if it's well written, but it works for me.
Created attachment 110725 [details] xml example file
Created attachment 110726 [details] [review] new version sorry, changes in greeter.c was unuseful. new patch submitted.
I am glad that you were able to get GDM to do what you want. That hardly took any time at all. Congragulations on figuring out the code so quickly. However, the patch would need some work to go upstream. There are a few issues: 1) There are two ways to show buttons. One (the way you implemented) is to use a "rect" and set the "button" element to "true". This method is the old way of defining a button. It isn't the best way since buttons created with this mechanism aren't true GTK+ buttons and can not support a11y. The better way is to use the type "button" which uses a real GTK+ button. Ideally it is best to support both methods (then the theme designer can decide how they want it to work, and if they want to support a11y). 2) In greeter_start_handler, you seem to be setting up the session dialog, but then not really showing the dialog, and then calling greeter_set_session with the value from the button. This is unnecessary. Why not avoid the dialog code and just set the session value? 3) The parse_SessionName function and the use of the sessionName global is not the right way to parse the value. It would be better to parse the element in the parse_rect and parse_button functions and store the data in a new variable in the GreeterItemInfo structure (see greeter_item.h). Note in greeter_item.c there is a function greeter_item_info_new where a new string value should be initialized to NULL and a greeter_item_info_free where a string value should be freed when we are done using it. I realize this is a bit more work, but it doesn't make sense to follow a different convention when adding a new element to the DTD. It also might be better to use a element name data="name-of-session" instead of sessionName="name-of-session". "data" is more generic. Then buttons with id="session_name" would understand that the data value is the session name. Then, in the future, other things that want to use this data field could do so rather than having to add a new element for each new desired feature like this. For example, it would be easy to add a similar button for setting language to a specific value using this technique. In other words, I would change "sessionName" to "data", parse the value in the parse_rect and parse_button functions (similar to how it is done for other elements), and store the value in the info stucture. Then you can retrieve the value from the info structure rather than using the global. I am not sure if you are willing to do all this work. You can, of course, use your patch to modify GDM to do what you want. But if you want this modification to go upstream, we need to follow the conventions.
> I am not sure if you are willing to do all this work. infact. I'm not willing. :) anyway, I'll have a look to it. but my times are very slow and I cannot assure a good work, because I've not any notion of programming c and c++. I even need time to understand your suggestions. likely, gmd is well written, and it's easy to imitate. thank for your help.
GDM 2.22 is being rewritten, and it is not clear if it will even support the gdmgreeter or its XML file format, so it might not be worth the time to make this work and get the code clean enough to get upstream. However, if you were willing to do the work, I'd be agreeable to get the change into the 2.20 branch. At any rate, you have a patch that makes GDM work as you need, so that might be sufficient for you.
(In reply to comment #11) > GDM 2.22 is being rewritten > and what about the feature I'm looking for? anyway, it makes no sense to work for a patch which is going obsolete.
No, the new GDM 2.22 rewrite does not support buttons like gdmgreeter. However, if there is enough user demand, I imagine that the new GDM will be enhanced to support the old GDM greeter themes. At the moment, the new GDM rewrite has a completely new GUI which is themed differently. Note that the rewrite will probably take another 6 or so months to get accepted into GNOME, so there is time to evaluate it and make suggestions on how it should work. This bug report should be enough to make people aware that this is a desired enhancement in the rewrite. I was just trying to help you meet your immediate need by helping you get it working in the existing GDM 2.20 codebase.
(In reply to comment #9) > > The better way is to use the type "button" which uses a real GTK+ button. > changing the xml file: <!--item type="rect" id="start_session" button="true" sessionName="kde-3.5"--> <item type="button" id="start_session" sessionName="e16"> but I forced to add a stock tag such as: <stock type="options" /> instead of the text tag inside the label. but the stock tags works bad for me, because it uses a predefined subset of types. why it happens? may it be possible to allow button tags to use labels? sorry for my english: what does it means ally? :)
Hmmm. To get this working properly is starting to seem like it will be a bit more work than it seems at first. I see the following issues: 1) All existing GDM buttons are pre-defined so that labels can be handled via "stock" labels. The advantage of stock labels is that they are easy to translate to other languages. However, this new type of button you want to add does not have a constant label. This makes it a bit unique. It obviously can be any string associated with the session name. I notice that in the <item type="rect" button="true"...> case, we specify the label via putting an <item type="label"...> inside the "rect". Can this technique be used with buttons as well? If not, perhaps we should add the ability to define arbitrary labels for buttons by using an embedded <item type="label"> just like you can with the <item type="rect button="true"...> case. 2) Another complication is that the session desktop file actually contains the translations. For example, my /usr/share/xsessions/gnome.desktop has these values: Name=GNOME Name[af]=GNOME Name[ar]=\330\254\331\206\331\210\331\205 [...] So if we want to support language translation, we should really get the localized string for the label from the desktop file, not hardcode the label in the theme XML file. Based on this, I think my suggestion in #1 above to support arbitrary labels for buttons is perhaps not needed for this feature. Instead we could just add a new stock label type for the new Session button, and this new stock label type would mean "get the translated string from the session desktop file" rather than translating it directly like the other stock labels. I think the session dialog code already has some logic to pull out the translated text from the desktop file, so it might not be too hard to figure out how to do something similar for our new button type. 3) "a11y" is shorthand for "accessibility". In other words, functionality to support users with disabilities (e.g. blind users, paralyzed users, etc.) I hope this is clear. I feel like I'm rambling a bit.
(In reply to comment #15) 1) > > Based on this, I think my suggestion in #1 above to support arbitrary labels > for buttons is perhaps not needed for this feature. > I not agree. the ability of stock label to translate a text it's important, but it should not a condamnation. some users may prefer a label such 'my button', with no predefined translation. so, a button should have the choice to use stock label, free text label or no label at all (for example, an image may substitute the label). free text label seems easier to implement, so it may be the first to be added :). 2) about the <item type="label"...> inside the "button". I think it's currently no working. That's the reason because I choose the "rect" in my xml example: I spent some time to play with buttons and labels, but with no luck.
Yes, it would be ideal to enhance GDM so that both #1 and #2 are addressed. I was trying to suggest that #1 might not be required to implement this feature. If you want to address both #1 and #2 that's fine with me. - It might not be so easy to support "free text labels" for <item type="button">. I think this might be a bit tricky to code, which is why I suggested going with option #2 which I think would be easier to get working for both "rect" buttons and "button" buttons. - Note the choice of "no label" only makes sense for "rect" buttons. - It might be okay if we support the "stock label" choice for both "rect" and "button" style. And then only support the options of having a custom label or no label if "rect" is used. This way it is possible to make things work with either the rect or button style. However, if you want to fix the button style so you can specify custom button labels, that's also okay with me.
New patch version submitted. it's not complete. but it should be better, and I need your opinion about it. in this moment, just rect type is supported. changements: 1) new tab "keys" added o charge name of session; 2) new parse_keys function added. it undipendent from key names. I had some trouble to learn building it, but I think it's good; 3) new keys structure added in greeter_item.h. It's separed from data structure. I don't know if this agrees with conventions.
Created attachment 111090 [details] [review] new patch for rect type
Created attachment 111091 [details] new xml example
Overall, this looks much better and the code looks cleaner, some comments: 1) There is a lot of whitespace changes in the patch. I'd prefer the patch didn't change whitespace, especially in areas of the code you are not otherwise changing. 2) You seem to have chosen the element "keys" rather than "data". To me, "keys" implies something used for security interaction. I'd prefer a more general name for what we intend to be "generic data" that could be used in different ways by different item types. Likewise in the data structure you call the variable "name_of_session". It would be better to likewise call this something generic like "data", and the different item types could use it in different ways. The start_session item type can use it for the name of session, but other things might want to use the field differently, right? A comment explaining this should be in greeter_item.h, I think. 3) The docs for "start_session" in gdm.xml do not really explain how to use the new "keys" option. 4) Now we just need to make it work for button types also.
(In reply to comment #21) > > The start_session item type can use it for the name of > session, but other things might want to use the field differently, right? > that's the reason because I've made the parse_keys independent from key names and I've choose a new structure for key variables. I've named it keys such as key:value pairs in python dictionaries. and I put it out of preexinsting data structure to avoid paths to became long too much, as in if G_UNLIKELY (!parse_keys (child, info, error, &(info->data.keys.name_of_session), "name_of_session")) anyway, I'll move keys structure inside data. if you suggest a new name for it, without security implications, I'll use it. thank you for your comments.
Ah, sorry, I didn't realize the "union" is called "data" already. I didn't mean to be confusing. I would recommend not putting this extra variable inside the union. The union is really for data values that are specific to an item type (like rect or button). Instead I would just create a new variable in the structure called something like this: char * item_data; Then, I'd do something like this: if (G_UNLIKELY (!parse_keys (child, info, error, &(info->item_data), "data")) Then in the XML you add a key named "data" for any kind of item type or id type. Each item type or id type can decide whether to do anything special if a value is provided for "data". So then in the XML you could specify this: <item type="rect" id="start_session" button="true" data="e16"> Then in the callback function, the code just knows that "data" for "rect" items with id of "start session" is the session name. Also when we make this work for button types, it could use the same id="start_session" and data="e16" and it would just work the same way. Is this more clear? I am just trying to suggest how to make this a bit more generic. This way if we want to add other special features to other item types or id types, the code could easily be extended to support this.
(In reply to comment #23) > > if (G_UNLIKELY (!parse_keys (child, info, error, &(info->item_data), "data")) > ok. item_data should be the name of the structure, but I'm not sure it may be possible to use just a simple variable for all keys, instead of a structure with a different variable for any key. that's to avoid overwriting of item_data values. for the same reason, I've discovered a bug in my patch: it just works with two buttons, because the second one overwrites the first. perhaps, I'm force to parse the key in the greeter_session.c file, but so I come back to the first version of my patch. > > So then in the XML you could specify this: > > <item type="rect" id="start_session" button="true" data="e16"> > that makes simpler xml, but not more general. a new item_data tag may charge more than one key.
I think I see the problem. Note the parse_id function adds all items with an "id" value to a hash. Obviously you can only have a single item with an identical id value. It looks like the current theme code assumes that you cannot have multiple objects with the same id value. Obviously the code and possibly the XML format would need some more significant redesign to overcome this restriction. I'm not sure the best way to deal with this, though I'm open to suggestions.
sorry. I think to have a solution for our problem, but I'm fighting against a technical problem. No, I want to share a function between greeter_parse.c and greeter_session.c files. so I have add two new file (name it greeter_foo.c and greeter_foo.h) to put there my function. but I'm not able to handle makefile. I've put here a "bad patch" to add greter_foo to show myu problem: http://pastecode.net/?action=viewpost&tag=2914 may please you show me is my mistake?
I've added gui/greeter/greeter_foo.c to po/POTFILES.skip and it seems work better. sorry for this unuseful post.
Created attachment 111956 [details] [review] patch for multiple buttons ok. I had a bad, bad experience with a missing -N option in diff command. ... and one another with flux direction in gdm program. in gdm design, any id xml option correspond to just one action. so to add a new button, you need to current xml id, fixed to the same "start_session" value for all buttons. after, it's sufficent to call start_handler function to have a working button. but I spent a lot of time to understand that it's impossibile to call start_handler with greeter_item_session_setup, defined in greeter_session.c file, because of this function runs BEFORE parsing! so I choose to put start_handler function directly in greeter_parser.c without other changes. if you like to call put it in greeter_session.c and use greeter_item_session_setup function to call actions, you have to modify the flux of the program. but I think it does not make sense. This patch works both for rect and button types, using just a start_session option and no changes to info structure. I've not done nothing about free labels for buttons, because that a new problem, undipendent from this. may it be good for another bug. just some observations about it. stock labels have a complex function with multiple features. they are not only for label translation, but they curry an action which overwrites the id action. for example stock="option" have a different behaviour then stock="session" for the same id="button". that's not transparent for the user.
Created attachment 111957 [details] [review] example xml with rect and buttons
The more I think about this problem, the more I think it is a bigger problem that will likely take some significant effort to get working properly. As I have mentioned before, the fact that the new GDM 2.22 rewrite does not support the gdmgreeter XML theming format makes me think we should not invest too much time to redesign the greeter_parser. It probably makes more sense to invest time making the new GDM 2.22 rewrite meet your needs rather than hacking on the 2.20 code, which will soon become obsolete. Some thoughts about the latest patch: The XML is looking like it is in better form, and it is nice that the code now works with both "rect" and "button" style buttons. However, if we are going to go through the trouble to make gdmgreeter support dynamic buttons, then we should be doing more to extend the framework to make it easier to add more types of dynamic buttons in the future. In other words, I think this patch just hacks the greeter_parser code to make it work for one type of dynamic button rather than making the code truly manage dynamic buttons. For example, I think that the XML having an element called "name_of_session" which only works if the item id value is "start_session" is a bit ugly. Though I've said this before. Looking at the design of greeter_parser, it seems that this code is supposed to be fairly self-contained with logic to just handle parsing of the XML. You will notice that the higher level greeter code calls into greeter_parser, but not the other way around. Your making greeter_parser call into greeter_set_session breaks this model. You are making the XML parsing code know specifics about how the greeter is implemented and how the button is implemented, which the current design seems to be making efforts to try and hide. I understand why you are doing this. It is easier to hack together code that "works" this way. However, please keep in mind that the code quickly can become hard to maintain when too many people add changes which does not follow a consistent design approach. ---- What do others in the GDM community think about this enhancement request? It will be easier to get this patch into GDM 2.20 if we can confirm the need.
(In reply to comment #30) > It probably makes more sense to invest > time making the new GDM 2.22 rewrite meet your needs rather than hacking on > the 2.20 code, which will soon become obsolete. > I agree with you. our discussion is a demostration of design problems of the actual gdm version. now, as a user, I think my desire is elementar and should be grant in a native way. anyway, after a lot of attemps, I've reached the conclusion this hack is the only simple solution and I've proposed it. is your responsibility to accept it or to reject. for me, that was a good opportunity both to learn programming and to work inside a real situation. thank's a lot for that.
I do appreciate your patch. When I first started working with you on this, I wasn't aware myself of the design limitations in GDM. I was hoping that we would be able to add the functionality you desire without it being too complicated. However, as we discovered the design limitations, it seems that this isn't easy to get working unless we slightly violate how the code is designed. Even if the feature doesn't go upstream, at least we now have a patch that you can use to make GDM work as you want, and others can try it out to see if it is useful. I am also glad to hear that this was a useful experience for you to learn programming and the GDM codebase. I might be willing accept the patch upstream even though it slightly violates the design if there is a real clear need for it. At this point, it isn't clear to me if this is a feature that only you will use, or if this is a feature that many users, theme writers, and/or distros will adopt if the code were to go upstream. If other people add comments to this bug report to let us know that this feature is desired, then that will help make a decision. It might be useful to send an email to the gdm-list@gnome.org and tell people about the work you have done, and a pointer to this bug report and patch. This way we could get some feedback about whether others find this useful. Also, we might get some feedback about whether the approach taken in your patch makes sense to others, or if people think we need to do more work to make the code more general, or better fit with the design.
Created attachment 112876 [details] [review] new version new patch. it's not simple as the hack, but it parses start_session ids in a dinamical way, using a recursive list. so the action is correctly applied in greeter_session.c file. this method is undipendent from type of buttons, so it works for rect, buttons and so on. to avoid problems about option names, I've named uname the unic name option for start_session objects (data is used for other purposes). Names of variables, insted, are specific for start_session, because they reserve some specific location for my feature. anyway, I don't care about names. sorry for long delay :)
Created attachment 112877 [details] xml example file
Overall looks pretty good. Some minor comments: + The docs (docs/C/gdm.xml) says only "Select one single available session, adding uname option." This doesn't do a great job of explaining to me how to use this new feature, or how the XML is structured. An example would be useful. + greeter_start_session.h just defines a single structure. I'd think it would be more clean to just put this structure in the greeter_item.h file. Thus avoiding needing to modify Makefile.am. + Why does greeter_session include greeter_start_session.h when it doesn't seem to add any lines that make reference to the structure defined there? + If the purpose of "uname" is to create a general purpose field that can store data for a given node, why do we call the data structure "StartSessionList"? Wouldn't uNameList be more generic? Likewise wouldn't it be more clear for the info structure defined in greeter_item.h to contain uNamePtr and uName rather than startSessionPtr and startSessionName? Why assume that the uname has to be a session name? + Since the info node already has a pointer to the parent node, why is it necessary to have a unamePtr that points specifically to the unamePtr value parent->startSessionPtr. Couldn't you just navigate to the parent's uname value without needing to store an additional pointer using the parent pointer directly?
(In reply to comment #35) > > + The docs (docs/C/gdm.xml) ... > you know I'm a noob. Im working only with vi, which is wonderful to write code, but is not confortable for documentation. May you please suggest me some better instrument to handle gdm.xml and to look the effects of my changes? :) > > Wouldn't uNameList be more generic? ... > I have a lot of doubts about names, because the problem is not simple a flame. This method to handle a dynamic list is general, and it may be applied for other purposes, but memory locations has to be distinct, to avoid overpositions. So, may it be useful to change the prototype (currently in greeter_start_session.h) in such a way: typedef struct _UNameList UNameList; but not to change names of variables. uNamePtr is an instrument specific for start_session feature and cannot be used for a new hypothetic "end_session" one... Same remark for opion names. If somebody wants to use this list, it has to create new variable location and to generalize parse_uname function with a callback mechanism.Pay attention that currently my function has a general name but not a general behaviour (^_^). anyway gdm-2.20.* is becaming obsolete... That's the same reason because I don't understand why to choose a generic name for the option. > > why is it necessary to have a unamePtr that points specifically to the unamePtr value parent->startSessionPtr. > unamePtr is a pointer to share the static startSessionList structure in greeter_item_info_new function. initializing a copy of the pointer for each node, I avoid to navigate the info tree in parse_uname function up to the top root node. Pay attention that I don't know how deep is the tree. So I think my solution is better. Instead, a possible alternative solution should it be a lookup inside parse_uname function in the same way then in greeter_item_session_setup. If you like, may I study it. + now, I'm waiting for your conclusively decisions about names to make the final version for the patch.
Created attachment 113078 [details] [review] last patch? not yet, unlikely now the patch seems really good. infact the parse_uname function is completly general and names are chooses in a way that distinguish clearly general structures and general variables from strictly bound to start_session feature. but I want an improvement again. the call to greeter_item_register_action_callback in greeter_session.c is not undipendent from start_session feature. If some body wants to use this patch for other purposes, he has to copy my code, not just to call a anonimus function. that's a problem for me, because I'm currently not able to manage the code of ActionFunc, and I have to study how to make varibale the name of the function. so. if you like it, I'm happy, but if not, you need to wait some time again.
Created attachment 113151 [details] [review] new patch
Created attachment 113218 [details] [review] final optimization sorry if I've produced too much patch versions, and you have no time to look at all them. This one should really be the last. + new UNAme type defined to add just one memory element for any new dinamyc id. to add a new dynamic id, you just need to: 1) add a new UName element in _GreeterItemInfo. for example: UName startSessionStruct2; 2) add a new static UNameList pointer in greeter_item_info_new. for example: static UNameList *startSessionPtr; 3) initialize it in this way: info->startSessionStruct2.uNamePtr = &startSessionPtr; if (parent == NULL) { info->id = g_strdup((const char *)"root"); *info->startSessionStruct2.uNamePtr = NULL; } 4) parse it, using parse_uname function in this way: parse_uname(node,info,info->startSessionStruct2); 5) apply the action in greeter_item_session_setup: greeter_uname_action_setup(info,info->startSessionStruct2.uNamePtr,my_handler); old style no-uname ids, such as sessionButtonStruct for id="session", are supported.
Using statics means that all info structures will have the same data, no? How does this work with multiple session buttons? + static UNameList *startSessionPtr; + static UNameList *sessionButtonPtr; What's the difference between startSessionPtr and sessionButtonPtr + *info->startSessionStruct.uNamePtr = NULL; + *info->startSessionStruct.uNamePtr = NULL; Why is the same line duplicated? Looking at the parser code, it seems you set both startSessionPtr and sessionButtonPtr to the "uname" value. What's the point of having two structures if they have the same value. It seems the sessionButtonPtr gets set to the "session" value only if "uname" isn't set. But I'd think it wouldn't work if uname isn't set. You also set up two handlers, one for both startSessionStruct and the other for sessionButtonStruct. I'd think just one would be needed.
Created attachment 113366 [details] [review] another attemp I'm really stupid. and I'm loosing your time. the mistake is owned to an attemp of reunification of "session" and "start_session" ids. but I came back from it because "session" ids not allows uname. a "session" id with an optional uname option has to carry two different actions, and this is not good nor for the user than for the code (UName lists are accessed sequentially as they are parsed, and this makes unwrittable greeter_uname_action_setup). at the end, I prefear to preserve two different ids, one for "session" and one for "start_session" as in our initial goals. anyway, even something looks bad in the actual code, the final patch behaves correctly, because the action for "session" buttons is pointed by info->sessionButtonStruct.uNamePtr in greeter_item_session_setup. about the initialization duplicated: *info->startSessionStruct.uNamePtr = NULL; it is not really essential. the program works the same because the pointer is null by itself. but I don't know if this happens for some precise C rule or if it is a behaviour depending from gcc version or some other variable factor. So, I prefear to preserve it. now, I've corrected the code for your last observations. but a more enhancement should be useful for our patch. I'm thinking to add a new structure IdStruct such this: struct _IdStruct { char *idName; ActionFunc func; UName uNameStruct; int isDynamic; }; and a vector such this: IdStruct vIdStruct[] = { {"session",greeter_session_handler,info->startSessionStruct,0}, {"session_start",greeter_start_session_handler,info->sessionButtonStruct,1} }; this vector may be used to make automatic all parsing instructions for all possible ids. so the user will just have to add a new line to vIdStruct, one new UName element, and his specific handler. some problem may arise from header file, because some type definitions, such as ActionFunc, loop inside greeter_item.h, and is intersteing for me to find a solution.
I still don't understand why it is necessary for there to be two variables startSessionStruct and sessionButtonStruct which both have the same handler. I don't think we should even support "start_session". Isn't the format "id=start_session"? so start_session shouldn't be a separate element, it's just a value for "id". This just seems redundant to support two element names that do the same thing. How does the code work if you specify uname without "id=start_session"?
(In reply to comment #42) > I still don't understand why it is necessary for there to be two variables > startSessionStruct and sessionButtonStruct which both have the same handler. > + greeter_uname_action_setup(info,info->startSessionStruct.uNamePtr,greeter_start_session_handler); + greeter_uname_action_setup(info,info->sessionButtonStruct.uNamePtr,greeter_session_handler); they have non the same handler. in xml file, try to change: <item type="list" id="session" combo="true"> with a "start_session" id and you have a crash.
Created attachment 113469 [details] [review] new patch making a new handler for an option id="start_session" was our initial goal. but now, code is simpler enough to allow joining handlers and adding uname option to previous id="session" buttons.
Created attachment 113470 [details] xml file example
Comment on attachment 113469 [details] [review] new patch diff -ruN gdm-2.20.5.orig/docs/C/gdm.xml gdm-2.20.5/docs/C/gdm.xml --- gdm-2.20.5.orig/docs/C/gdm.xml 2008-04-07 22:33:06.000000000 +0200 +++ gdm-2.20.5/docs/C/gdm.xml 2008-06-26 17:02:01.948101077 +0200 @@ -1740,7 +1740,7 @@ <sect2 id="configfile"> <title>The Configuration Files - GDM System Defaults Configuration File - and GDM Custom Configuraiton File</title> + and GDM Custom Configuration File</title> <para> GDM uses two configuration files: the GDM System Defaults Configuration @@ -6579,11 +6579,23 @@ </listitem> </varlistentry> + <varlistentry> + <term>start_session</term> + <listitem> + <para> + Select one single available session. It needs a unic name option, which is named "uname". An example of use for start_session id is: <item type="button" id="start_session" button="true" uname="gnome">. + </para> + </listitem> + </varlistentry> + <varlistentry> <term>session_button</term> <listitem> <para> List and select from available sessions. + If you need a button for one single available session use the option "e;uname" as in these examples: + <item type="button" id="session" button="true" uname="gnome"> + <item type="rect" id="session" button="true" uname="gnome"> </para> </listitem> </varlistentry> diff -ruN gdm-2.20.5.orig/gui/greeter/greeter_item.c gdm-2.20.5/gui/greeter/greeter_item.c --- gdm-2.20.5.orig/gui/greeter/greeter_item.c 2008-04-07 22:29:38.000000000 +0200 +++ gdm-2.20.5/gui/greeter/greeter_item.c 2008-06-26 16:57:27.517130260 +0200 @@ -49,10 +49,18 @@ GreeterItemType type) { GreeterItemInfo *info; + static UNameList *sessionButtonPtr; int i; info = g_new0 (GreeterItemInfo, 1); + info->sessionButtonPtr = &sessionButtonPtr; + if (parent == NULL) + { + info->id = g_strdup((const char *)"root"); + *info->sessionButtonPtr = NULL; + } + info->item_type = type; info->parent = parent; Files gdm-2.20.5.orig/gui/greeter/.greeter_item.c.swp and gdm-2.20.5/gui/greeter/.greeter_item.c.swp differ diff -ruN gdm-2.20.5.orig/gui/greeter/greeter_item.h gdm-2.20.5/gui/greeter/greeter_item.h --- gdm-2.20.5.orig/gui/greeter/greeter_item.h 2008-04-07 22:29:38.000000000 +0200 +++ gdm-2.20.5/gui/greeter/greeter_item.h 2008-06-26 17:07:43.455102807 +0200 @@ -23,6 +23,7 @@ typedef struct _GreeterItemInfo GreeterItemInfo; typedef struct _GreeterItemListItem GreeterItemListItem; +typedef struct _UNameList UNameList; typedef enum _GreeterItemState GreeterItemState; typedef enum _GreeterItemType GreeterItemType; typedef enum _GreeterItemSizeType GreeterItemSizeType; @@ -81,9 +82,16 @@ GREETER_ITEM_SHOW_REMOTE = 1<<3 }; +struct _UNameList { + char *uName; + UNameList *prev; +}; + struct _GreeterItemInfo { GreeterItemInfo *parent; - + + UNameList **sessionButtonPtr; + GtkAnchorType anchor; float x; float y; Files gdm-2.20.5.orig/gui/greeter/.greeter_item.h.swp and gdm-2.20.5/gui/greeter/.greeter_item.h.swp differ diff -ruN gdm-2.20.5.orig/gui/greeter/greeter_parser.c gdm-2.20.5/gui/greeter/greeter_parser.c --- gdm-2.20.5.orig/gui/greeter/greeter_parser.c 2008-04-07 22:29:38.000000000 +0200 +++ gdm-2.20.5/gui/greeter/greeter_parser.c 2008-06-26 16:56:54.087101742 +0200 @@ -104,20 +104,51 @@ return info; } +static void +parse_uname (xmlNodePtr node, + GreeterItemInfo *info, + UNameList **uName) +{ + xmlChar *prop; + UNameList *tmpList; + prop = xmlGetProp (node, (const xmlChar *) "uname"); + + if (prop) + { + tmpList = *uName; + *uName = malloc(sizeof(UNameList)); + (**uName).uName = info->id; + (**uName).prev = tmpList; + } + xmlFree (prop); +} + static void parse_id (xmlNodePtr node, GreeterItemInfo *info) { xmlChar *prop; - prop = xmlGetProp (node, (const xmlChar *) "id"); - + prop = xmlGetProp (node, (const xmlChar *) "uname"); if (prop) { info->id = g_strdup ((char *) prop); g_hash_table_insert (item_hash, info, info); + parse_uname(node,info,info->sessionButtonPtr); xmlFree (prop); } + else + { + prop = xmlGetProp (node, (const xmlChar *) "id"); + if (prop) + { + info->id = g_strdup ((char *) prop); + g_hash_table_insert (item_hash, info, info); + if (strcmp ((char*) prop, "session") == 0) + parse_uname(node,info,info->sessionButtonPtr); + xmlFree (prop); + } + } } /* Doesn't set the parts of rect that are not specified. @@ -1884,6 +1915,7 @@ root = greeter_item_info_new (NULL, GREETER_ITEM_TYPE_RECT); + g_hash_table_insert (item_hash, root, root); res = parse_items (node, &items, root, error); /* Now we can whack the hash, we don't want to keep cached diff -ruN gdm-2.20.5.orig/gui/greeter/greeter_session.c gdm-2.20.5/gui/greeter/greeter_session.c --- gdm-2.20.5.orig/gui/greeter/greeter_session.c 2008-04-07 22:29:38.000000000 +0200 +++ gdm-2.20.5/gui/greeter/greeter_session.c 2008-06-26 16:56:06.638103212 +0200 @@ -214,6 +214,12 @@ GSList *tmp; int ret; + if (strcmp(info->id,"session") != 0) + { + greeter_set_session ((char *) info->id); + return; + } + /* Select the proper session */ tmp = session_group; while (tmp != NULL) @@ -262,10 +268,23 @@ } } +static void +greeter_uname_action_setup (GreeterItemInfo *info,UNameList **uNamePtr,ActionFunc func) +{ + while (*uNamePtr != NULL) + { + greeter_item_register_action_callback (g_strdup((char *) (**uNamePtr).uName), + func, + NULL); + *uNamePtr = (**uNamePtr).prev; + } +} + void greeter_item_session_setup () { - greeter_item_register_action_callback ("session_button", - (ActionFunc)greeter_session_handler, - NULL); + GreeterItemInfo *info; + + info = greeter_lookup_id ("root"); + greeter_uname_action_setup(info,info->sessionButtonPtr,greeter_session_handler); }
sorry. comment #46 was an attemp to correct the previous patch, removing unuseful line: +typedef struct _UName UName;
up. what about the patch #44 ?
Looking much better. Some questions and comments: - In the docs you say "It needs a unic name option". Do you mean unique? I would reword the docs as follows: The "start_session" item id is used to create a button that will start a specific session when the button is pressed. The "start_session" item id requires that there is also a "uname" element in the same item tag. This "uname" element specifies which session to start. - In the docs, I would also explain how it behaves if the session specified in uname doesn't exist or is left out. Does this leave a non-functional button on the GUI, or does the button get removed from the GUI if it is known to not work? - I don't think we should support the syntax where you use "session_button" with a "uname" value. It should be fine just to use the new "start_session" id for this feature only. I don't think we should change the "session_button" code at all. Also, I suspect the generic "session_button" feature to start up the dialog is broken now. You set sessionButtonPtr for "root" to NULL. Then in greeter_item_session_setup, you call greeter_uname_action_setup with the root node's sessionButtonPtr value. In greeter_uname_action_setup, it does nothing if passed in NULL for this value. Therefore, I'd think that the normal session dialog you get when you press on the session button probably doesn't work anymore. - Note that the spacing in the gdm.xml file tries to keep each line 80 characters long at most. Your line if very long. Would be nice to break it up into separate lines like the rest of the file.
Created attachment 114061 [details] [review] new improvement (In reply to comment #49) thanks for doc hints. and sorry for my poor english. > > - I don't think we should support the syntax where you use "session_button" > with a "uname" value. > I agree, but I removed the start_session id when you ask me for a single handler. In my mind, this patch wants one handler for one item id. Even if the handler code is minimal. Otherwise, the handler is forced to check for the current info->id in a redundant way. > Also, I suspect the generic "session_button" feature to start up the dialog > is broken now. You set sessionButtonPtr for "root" ... > why broken? don't worry, that's the better part of the patch. greeter_item_session_setup does not know anything about info tree, but needs just a way to find the recursive list of button elements. info->sessionButtonPtr is not a pointer but a pointer to a pointer, which is duplicate for all info elements. it carries the actual position of buttons elements. it allows sharing static *sessionButtonPtr inside greeter_item.c (which is a simple pointer) between remote functions, such as parse_id and greeter_item_session_setup, but also shares *sessionButtonPtr between independent info elements. info->sessionButtonPtr is updated once for all info elements in parse_uname: (**uName).prev = tmpList; and is accessed in greeter_item_session_setup with a simple: root = greeter_lookup_id ("root"); the instruction you are looking at is not very important. It is unnecessary in my environment, because root->sessionButtonPtr is NULL but itself, but I leave it because greeter_uname_action_setup loops while (*uNamePtr != NULL), and a random value for it can do a mess. No I've removed it, to avoid confusion. I've dubts about about initialization rules because I've no experience in programming. new improvement: parse_id function and greeter_item_session_setup are maked independent from particular ids. This allows a user to add a new id, with a unique (not unic :) ) uname value just adding a new handler and adjusting greeter_item.* files.
Created attachment 114062 [details] xml file example
up. sorry, what about this patch?
Sorry, I am on vacation this week. I will probably have time to respond early next week.
(In reply to comment #53) > Sorry, I am on vacation this week. I will probably have time to respond early > next week. > ok. I'will be too on vacation next week, and I'will not read your answer immediatly. so, I hope this patch does not need me anymore. :)
well, now I'm back at home. what about the patch?
Overall, I think the patch is looking better. However, I am still having a hard time understanding why some of the code is needed. It seems that in order to get this feature working, you are adding some fairly complicated stuff into the GreeterItemInfo structure. Specifically you are adding vIdHead, vIdPtr, sessionButtonPtr, startSessionPtr. When you process each "uname", you compare each value against the idName. However, I don't really see anywhere that you set the "idName" value in the first place. So I'm not sure how it actually ever passes the following if test: if (strcmp ((char *)prop,(char *)(*info->vIdPtr).idName) == 0) If it does pass the if-test, then it malloc's a new structure and sets (*info->vIdPtr).uNamePtr->prev to whatever was the previous (*info->vIdPtr).uNamePtr value. Could you explain the purpose of this data structure a bit more, and how it works. I get the feeling that you really should be using a hash table here rather than setting up your own linked list and looping through them looking for id string matches. Wouldn't it make more sense to setup a hash table with the hash-key being the id value of nodes which have a uname, and the hash-value being the actual uname value? Then you could avoid all the complicated code which seems to be just looping through a linked-list to get the same value? You can see in the code already uses g_hash functions, so it should be pretty easy to quickly learn how to use them. Then in greeter_item_session_setup () you could just loop through the hash table to set up the various action_setup functions. No? This seems a better approach since it just creates a single hash table (which wouldn't need to be stored in the GreeterItemInfo structure at all and avoids so much complicated code.
(In reply to comment #56) > Could you explain the purpose of this data structure a bit more, and how it > works. > a. the patch is built to be easy used for a general purpose, as in our initial goal. the user just needs to do these three steps: 1. copy how is used the variable startSessionPtr: cloc3@s939 ~/gdm-2.20.5 $ grep -rHn -B1 startSessionPtr . ./gui/greeter/greeter_item.h-105- UNameList **sessionButtonPtr; ./gui/greeter/greeter_item.h:106: UNameList **startSessionPtr; -- ./gui/greeter/greeter_item.c-53- static UNameList *sessionButtonPtr; ./gui/greeter/greeter_item.c:54: static UNameList *startSessionPtr; -- ./gui/greeter/greeter_item.c-67- info->sessionButtonPtr = &sessionButtonPtr; ./gui/greeter/greeter_item.c:68: info->startSessionPtr = &startSessionPtr; -- ./gui/greeter/greeter_item.c-73- vIdStruct[0].uNamePtr = info->sessionButtonPtr; ./gui/greeter/greeter_item.c:74: vIdStruct[1].uNamePtr = info->startSessionPtr; 2. initialize the vector vidStruct, appending his own new line: static IdStruct vIdStruct[] = { {"session",greeter_session_handler,NULL}, {"start_session",greeter_start_session_handler,NULL}, {NULL,NULL,NULL} }; 3. make his own handler as a global function in greeter_item.* b. the patch works with the vIdStruct vector and the _UNameList structure. The first one substitutes an hash table and the second a recursive list. I think it's no so complex, because it uses just two elements, such as in the solution that you suggest. off course, I agree that using hash table would be prefereable, because this makes it possible to use standard code and may it be faster a bit. to explain in a detailed way how the patch works, observe that the third element for each line in vIdStruct is a variable pointer to pointer, initialized once for root node in greeter_item.c in such a way: vIdStruct[0].uNamePtr = info->sessionButtonPtr; vIdStruct[1].uNamePtr = info->startSessionPtr; afterwords, it's updated inside parse_uname function, which carries the pointer to pointer (*info->vIdPtr).uNamePtr in the uName variable. (*info->vIdPtr).uNamePtr is the actual position of the current list element. it's saved in tmplist and is overwitten with malloc. at the end, the tmplist content is stored in prev to rewind sequentially the list, just once. note that GreeterItemInfo strucure contains just pointers, and not list elements, so is not so heavy as it looks. > I get the feeling that you really should be using a hash table ... I agree with you, but now I'm busy in another job for two months. so, your wish remain for me a todo goal, which I promise to realize later.
Okay, I think I see the confusion we have had. When I was suggesting that "Uname" be a general interface, I didn't mean that this needed to relate with how the action handlers are managed. Instead, I was suggesting that "items" with different "id" values might use Uname in different ways. For example, a different id might use it for a special label that is specific to the item type. If you read back in comment #9, bullet #3, you can see that all I was trying to say is that we should add a single element called "data" that is processed for all id types rather than using "SessionName" which is an element name that implies a specific usage, and then storing this in the info structure. In other words, I was just talking about how we do the parsing and setting data in the info structure. I think it makes the code too cumbersome to make it so generic to add special signal handlers based on Uname values. I just don't think this feature will have other uses. We could revisit this sort of added complexity in the future if we find more desired features that are so similar in the future. It seems the patch would be much smaller, easier to read, and simple if we just put the session names in a hash table. Then in greeter_item_session_setup we can just set up the normal greeter_session_handler action callback, and loop over the hash table and set up the other handlers. It's okay if it takes a while to get the patch in good shape. It's normal for a patch to go through some cycles, especially when it adds new API. It's better to get it right at the beginning, rather than suffer with poor API choices later. Even if we decided to stay with the linked-list approach, we should be using the Glib linked-list interfaces not coding the linked-list by hand. Likewise Glib has hash interfaces we should use if we agree to go that way. http://library.gnome.org/devel/glib/stable/glib-Singly-Linked-Lists.html http://library.gnome.org/devel/glib/stable/glib-Doubly-Linked-Lists.html http://library.gnome.org/devel/glib/stable/glib-Hash-Tables.html
(In reply to comment #58) > Okay, I think I see the confusion we have had. no. the only problem stays in my unexperience, not in your suggestions. when I do something new (for me), I prefear to remake all the code by myself instead of using standard routines. > > When I was suggesting that > "Uname" be a general interface, I didn't mean that this needed to relate with > how the action handlers are managed. > Instead, I was suggesting that "items" > with different "id" values might use Uname in different ways. ok, but I think the patch can do this, actually. You can easy substitute the action handler with something else without changing code. I think the problem is not to revisit the patch but just to simplify it. > > I think it makes the code too cumbersome to make it so generic to add special > signal handlers based on Uname values. I just don't think this feature will > have other uses. We could revisit this sort of added complexity in the future > if we find more desired features that are so similar in the future. > > It seems the patch would be much smaller, easier to read, and simple if we just > put the session names in a hash table. Then in greeter_item_session_setup we > can just set up the normal greeter_session_handler action callback, and loop > over the hash table and set up the other handlers. > I don't agree. you want to parse and store data in some place, but you don't want to do something with them. that's make no sense. if you need an information you must forsee an handler to use it. instead, you are wright thinking there is a way to reduce complexity storing handlers inside GList structures, linked by hash table elements. actually, in may work, the self made list _UName carries too few information. > > It's okay if it takes a while to get the patch in good shape. > yes, that's not a problem. but be aware I've really no time to code for a couple of month. anyway, I promise to come back working again. :)
> I don't agree. you want to parse and store data in some place, but you don't > want to do something with them. that's make no sense. if you need an > information you must forsee an handler to use it. I wasn't suggesting that we wouldn't do something with this. Since our usage of Uname is session specific, there is no reason why the code couldn't just set up all the session handlers after parsing, perhaps add a function to loop through the hash table and set up the handlers. We don't really need to make this code generic enough so that you could, in the future, add other Uname things that also setup signal handlers in this way. In fact, I'm not sure that special session buttons with Uname values need a separate signal handler. See my comment below. > instead, you are wright thinking there is a way to reduce complexity storing > handlers inside GList structures, linked by hash table elements. actually, in > may work, the self made list _UName carries too few information. I'm not sure why both a GList and a UName is needed. Is this because you might have multiple buttons which have the same session id? If so, that makes sense. Since the session_handler functions are passed in the appropriate "GreeterItemInfo" structure, if this structure contained just the string value of the Uname value, then this could be referenced, and you could look in the hash table to figure out which action handler to really call. In other words, you could probably use the same greeter_session_handler handler for all session buttons (the one to launch the dialog and ones with "Uname" values). The function could just look in the Uname string field in the Info structure, then if the item node contains a value for Uname it just starts that session, otherwise it launches the dialog. This seems a more simple solution to me. What do you think? Is there a reason you are aware of that this approach wouldn't work. You've been looking at this code more than me, so I might be missing something.
Thanks for taking the time to report this bug. However, you are using a version that is too old and not supported anymore. GNOME developers are no longer working on that version, so unfortunately there will not be any bug fixes for the version that you use. By upgrading to a newer version of GNOME you could receive bug fixes and new functionality. You may need to upgrade your Linux distribution to obtain a newer version of GNOME. Please feel free to reopen this bug if the problem still occurs with a newer version of GNOME.