GNOME Bugzilla – Bug 336155
[patch] port gnome-terminal to goption
Last modified: 2008-01-05 15:59:51 UTC
I'm attaching (in a second) a port for gnome-terminal to use goption. The translations will need to be updated now, however, because I had to strip carriage returns from many of the strings... I don't really know how that process works. Should I work on doing that (fixing all the .po files where carriage returns were removed)?
Created attachment 62095 [details] [review] patch gnome-terminal to use goption
The translators can handle it, don't worry. ;) Thanks for the patch, I'll be looking at it shortly.
OK, that's not fair. I've been talking about this patch but never really got around to doing something or even replying to this report, so here it goes. Yes, we really should get rid of popt. However, this patch still needs some work =/ Marking as so.
gpastore, what needs to be done exactly to get that patch accepted?
Mariano, can you look into this. Thanks.
*** Bug 121491 has been marked as a duplicate of this bug. ***
Can someone please look at this patch ? This gnome goal is fulfilled by a majority of apps by now... http://live.gnome.org/GnomeGoals/PoptGOption
*ping* - mariano, guilherme?
ping Chris.
Created attachment 87664 [details] [review] migrate from popt to goption (work-in-progress) my current state of work-in-progrress I could use some suggestions from the developers. I do not know popt but I presume a lot of parsing/argument evalution was done 'manually' in parse_options_callback(). I suggest we get rid of that callback completely. my main question is: where should we store all the cmd line arguments? In OptionParsingResults *results ? And the deprecated arguments in a dedicated data structure we can free after checking whether they were used?
*ping* - chris, mariano, guilherme? can somebody take a look at christian's patch and give some feedback?
Another ping seems in order, so: ping :)
The patch clearly says "work in progress". If you care about it that much, please finish it. Pings don't help, no.
No one has ever said what was wrong with my initial patch, either, though. I haven't looked at it since I first wrote it, though, so I doubt it even applies cleanly anymore...
Lets see if adding more developers to an already late project helps :) Umm, was going to CC chpe, but he's already CC'ed.
The patch still applies cleanly, except for ChangeLog. It seems mostly ok to me. +/* static void parse_options_callback (poptContext ctx, enum poptCallbackReason reason, const struct poptOption *opt, const char *arg, - void *data); + void *data); */ Just remove this. "working-directory", - '\0', - POPT_ARG_STRING, - NULL, - OPTION_WORKING_DIRECTORY, + 0, + 0, + G_OPTION_ARG_STRING, + &OPTION_WORKING_DIRECTORY, N_("Set the terminal's working directory"), Use G_OPTION_ARG_FILENAME. "default-working-directory", - '\0', - POPT_ARG_STRING, - NULL, - OPTION_DEFAULT_WORKING_DIRECTORY, + 0, + 0, + G_OPTION_ARG_STRING, + &OPTION_DEFAULT_WORKING_DIRECTORY, Same. + context = g_option_context_new (N_("- GNOME Terminal Emulator")); Add a call to g_option_context_set_translation_domain here. + option_group = g_option_group_new (N_("gnome-terminal"), And call g_option_group_set_translation_domain. + option_group = g_option_group_new (N_("gnome-terminal"), + N_("GNOME Terminal Emulator"), + N_("Show GNOME Terminal options"), + results, + NULL); + + g_option_context_add_main_entries (context, option_group, GETTEXT_PACKAGE); + g_option_group_set_parse_hooks(option_group, NULL, post_options_callback); Actually, |group| is unused; just remove it. gtk_init (&argc, &argv); You need to remove this call. Instead, do g_option_context_add_group (context, gtk_get_option_group (TRUE)); And then move this code: /* Do this here so that gdk_display is initialized */ if (results->startup_id == NULL) { /* Create a fake one containing a timestamp that we can use */ Time timestamp; timestamp = slowly_and_stupidly_obtain_timestamp (gdk_display); results->startup_id = g_strdup_printf ("_TIME%lu", timestamp); } g_set_application_name (_("Terminal")); display = gdk_display_get_default (); display_name = gdk_display_get_name (display); results->display_name = g_strdup (display_name); to after the gnome_program_init call.
True, I intentionally said "work-in-progress" because I ran into a point where I needed to make a decision. My patch is not finished. Some questions are still unanswered. Also I'd appreciate some comments on how to migrate to GOption in a nifty and clever way. I mean, an outline of how it should be done.
Apart from the issues I mentioned in comment 16, the patch has the right approach.
Christian, I was confused whether you were referring to Michael's initial patch. In fact -besides 'needs work'- there were no comments on his patch. I took a look again and got some more insight. gnome-terminal basically does a lot of postparsing by having popt call a callback for *every* option. It also parses remaining options that cannot easily be parsed by popt (nor GOption, I believe). For old compatibility options a warning message is displayed only, they are ignored. My last patch and its ideas can savely be dumped at the next junkyard. I plan to look into this during the next days. I also intend to examine Michael Plump's initial patch. some remarks I prepared before concluding I had to completely overhaul my patch: g_option_context_add_main_entries () already sets the translation domain according to devhelp ;)
(In reply to comment #19) > g_option_context_add_main_entries () already sets the translation domain > according to devhelp ;) That doc is misleading; that call only sets the translation domain of the context's main goptiongroup; not of the context itself. Probably should file a docs bug on glib for that :)
Michael, I had a look at your patch. I believe it is not a good idea to have that many callback functions. I think it is not efficient and especially reduces readability of the code.
Today I made quite some progress and all popt stuff is finally gone. I still need to do some testing, tough. I had to remove g_option_context_add_group (context, gtk_get_option_group (TRUE)); because of (gnome-terminal:10356): GLib-WARNING **: A group named "gtk" is already part of this GOptionContext I can attach my work-in-progress later if anybody is intrested.
Created attachment 99177 [details] [review] migrate from popt to goption (partly done) the latest incarnation *all popt stuff gone *parsing of all options shown in --help works for me *deprecated compat options correctly treated * 'gnome-terminal -x' crashes for some reason
Thanks for the new patch! "command", 'e', - POPT_ARG_STRING, - NULL, - OPTION_COMMAND, + 0, + G_OPTION_ARG_STRING, + &option_command, N_("Execute the argument to this option inside the terminal."), NULL Should that be G_OPTION_ARG_FILENAME instead? Not sure... + +/* OPTION_COMMAND */ + if (option_command) Indent the comment. Same for the other comments below in the same func. + if (!g_shell_parse_argv (option_command, NULL, &exec_argv, &err)) Now I'm sure we definitely want G_OPTION_ARG_FILENAME above. + g_printerr (_("Argument to \"%s\" is not a valid command: %s\n"), "--command/-e", err->message); - g_error_free (err); - exit (1); - } + g_error_free (err); + exit (1); I think you should instead use g_set_error for the |error| param passed to the post parse function, and return FALSE. gnome_program_init() will do the rest of the work then. Same for the next occurrences of this pattern in post_options_callback. + option_group = g_option_group_new (N_("gnome-terminal"), The group name shouldn't be marked for translation since it'll not be translated. + option_group = g_option_group_new (N_("gnome-terminal"), + N_("GNOME Terminal Emulator"), + N_("Show GNOME Terminal options"), + results, + NULL); + + g_option_group_set_translation_domain (option_group, GETTEXT_PACKAGE); + g_option_context_add_main_entries (context, goptions, GETTEXT_PACKAGE); + g_option_group_set_parse_hooks(option_group, NULL, post_options_callback); You should add the entries to the option group you just created, *not* the main group (g_option_context_add_main_entries). And then you need to add the group to the context with g_option_context_set_main_group. You do that correctly in handle_new_terminal_event below. + if(g_option_context_parse(ctx, &argc, &argv, &error) < 0) + g_warning ("Error parsing options: %s, passed from terminal child", + error->message); Wrong; use |if (!g_option_context_parse(...))| since this returns a gboolean. Free |error|. And bail out after freeing the option context, before option_parsing_results_apply_directory_defaults etc. --- About the crash: if it still persists after fixing the above, could you post a trace?
Created attachment 99253 [details] [review] migrate from popt to goption (80% done) Thanks for you comments Christian. I incorporated your suggestions. I believe now I understand how GError works. The crash was eventually due to dereferencing NULL-pointers. Removed the bogus lines. * 'gnome-terminal --zoom=2' fails complaining about the option given twice. Debugging I found out that post_options_callback() gets called twice(!) in gnome_program_init(). This of course leads to unexpected problems. I am looking into this.
+ g_option_context_add_group (context, option_group); + g_option_context_set_main_group (context, option_group); The set_main_group call is enough; you shouldn't add it with add_group too. That's probably also why you're seeing the post parse hook being called twice. + if(!g_option_context_parse(context, &argc, &argv, &error)) + g_warning ("Error parsing options: %s, passed from terminal child", + error->message); Still need to free |error| here, and bail out after freeing the option context.
Created attachment 99302 [details] [review] migrate from popt to goption (90% done) Sooo, the latest version * Updated * Tests removed for option given twice and no option given; GOption already takes care of this. * If we want to keep the possibility of giving the --tab and --window options multiple times I will need to create callbacks for those, I believe. GOption ignores additional options given repeatedly. Today I have setup a Gnome svn environment. Wihin that environment the terminal does not start up with my patch. I cann see the process coming up and sleeping but no window appears, no item in the window list. debugging I see that it enters g_main_context_iterate() and then enters kernel land through poll(). -> Can someone confirm this behaviour??
Does it start with --disable-factory? What if you don't have any gnome-terminal running?
It also does not start with the --disable-factory option given. I do not have any terminal running since it does not start up :) I can use the 2.20 distro terminal alright, tough. other applications start up fine. If I unpatch the terminal it starts up fine. *weird*
with the --help option it prints out the help and quits cleanly. I use ddd for debugging.
Created attachment 99305 [details] [review] updated patch Makes sure we open a window if not passed anything.
Yap I figured this out minutes later after spamming people. btw, it->exec_argv = exec_argv; results in many crashes for me. I just do if (results->initial_windows == NULL) it = ensure_top_tab (results); at the beginning of the callback. A question: would it be nifty to move all global option variables into a struct? Sooo, I've implemented callbacks for the --tab, --tab-with-profile, --window and --window-with-profile options. they can now be used repeatedly just like before. We keep compatibility. Stuff like gnome-terminal --window --tab-with-profile=as -e top --tab --active --zoom=0.5 --role=test -t test works, except for --zoom, but zooming also does not work in this case for g-t 2.20 :) stand-alone --zoom works had to implement callbacks for --active and -e in order to make them work on the last tab created in the command line. setting a title does not work; it works neither for 2.20 so no regression here. this is a bug - i can see the title briefly but then it gets 'overwritten' I cannot see any regressions, thus I unleash this patch upon you. Thank you all for your remarks.
Created attachment 99357 [details] [review] migrate from popt to goption (good riddance) migrate from popt to goption. I cannot detect any regressions. Full compatibility to previous options supported. Testing appreciated, tough.
(In reply to comment #32) > A question: would it be nifty to move all global option variables into a > struct? Would be nicer, I think. Some remaining nits: + if (!g_shell_parse_argv (value, NULL, &exec_argv, error)) { - case OPTION_COMMAND: - { - GError *err; - InitialTab *it; - char **exec_argv; - + g_set_error (error, + G_OPTION_ERROR, + G_OPTION_ERROR_BAD_VALUE, + _("Argument to \"%s\" is not a valid command\n"), + "--command/-e"); + return FALSE; + } You need to use a temporary GError for the g_shell_parse_argv (gerror cannot be doulbe-initialised), and in the g_set_error you should put that one's error message too, e.g. _("Argument to %s is not a valid command: %s"), "--command", err->message); + OptionParsingResults *results; + results = data; + + InitialWindow *iw; + iw = add_new_window (results, NULL, FALSE); W: decl-after-statement, doesn't compile on gcc 2.95. + OptionParsingResults *results; + results = data; + + InitialWindow *iw; + const char *profile; Same, and some more times. + /* This automagically makes GOption parse */ + program = gnome_program_init (PACKAGE, VERSION, + &module_info, You can just use LIBGNOMEUI_MODULE directly. + if(!g_option_context_parse (context, &argc, &argv, &error)) + { + g_warning ("Error parsing options: %s, passed from terminal child", + error->message); + g_error_free (error); + g_option_context_free (context); + exit(1); + } This leaks |results|. Looks like you have a mix of tab and spaces indentation...
(In reply to comment #34) > (In reply to comment #32) > > A question: would it be nifty to move all global option variables into a > > struct? > > Would be nicer, I think. Do that, but make sure you don't make it static. Allocate it as an auto variable in handle_new_terminal_event() and pass it as data to g_option_group_set_parse_hooks. This way they don't consume memory after parsing is done. This means moving the goptions struct into that function too. Checking again, isn't 'results' that struct already? I'm confused now. Also in places like: + /* OPTION_EXECUTE */ + if (option_execute) There's no point to have those comments. Remove them. Finally, after you do these, no need to prototype the option callback functions.
(In reply to comment #34) > (In reply to comment #32) > + OptionParsingResults *results; > + results = data; > + > + InitialWindow *iw; > + iw = add_new_window (results, NULL, FALSE); > > W: decl-after-statement, doesn't compile on gcc 2.95. I'm gonna file a bug for gcc :) > Looks like you have a mix of tab and spaces indentation... uhm, I'll file a bug for Anjuta then :) yap, sorry about that. fixing it.
(In reply to comment #35) > (In reply to comment #34) > > (In reply to comment #32) > Checking again, isn't 'results' that struct already? I'm confused now.> Not quite. The command line options given are parsed, the values supplied scrutinised/digested and results are stored in the struct 'results'. Additionally there is no global instance where GOption parsing could store its parsed values. In fact I tried to extend this struct and make GOption fill its values, but it turned out to be a nightmare.
I'm having a hard time moving the goptions variables into a struct ... the compiler (gcc4.3) barks 'initialising element is not constant' for struct elements stated in the goptions struct.
Like this? int main(...) { MyData data; const GOptionEntry entries[] = { { ...., &data.option1, ... }, ... ?
(In reply to comment #38) > I'm having a hard time moving the goptions variables into a struct ... > the compiler (gcc4.3) barks 'initialising element is not constant' for > struct elements stated in the goptions struct. Remove the 'static' modifier.
(In reply to comment #37) > (In reply to comment #35) > > (In reply to comment #34) > > > (In reply to comment #32) > > > Checking again, isn't 'results' that struct already? I'm confused now.> > Not quite. The command line options given are parsed, the values supplied > scrutinised/digested and results are stored in the struct 'results'. > > Additionally there is no global instance where GOption parsing could store > its parsed values. > In fact I tried to extend this struct and make GOption fill its values, but it > turned out to be a nightmare. Make a local options struct and pass that as user data to all goption function calls. Add a link to results from that options struct so you have it around.
Looking around more, I tend to slightly prefer Michael's approach. The patch by Christian basically divides the options into two groups: those that can be done without a callback and those that can't. Then implements the first group without callbacks, just to process the gathered options in a grand post callback and process and stuff them in the results struct. Well, that kinda sounds like abusing GOption to me. If you need to process, just go ahead and add the callback. 10 more callback functions never killed anyone, and I think the callback approach is more readable, not the other one with long functions... My 0.02CAD of course.
Behdad, I do not consent that a dozen of callbacks for command line option processing is 'ok'. I'd say it decreases readablity. I am considering merging these into a single callback immediate_options_callback(). But I agree we are abusing GOption, see blow Just my 0,02€ I've been working on merging the goption variables into the struct results. By doing so we can drop some of the just-copy-into-the-struct action like if (option_default_working_directory) results->default_working_dir = g_strdup (option_working_directory);
Created attachment 101255 [details] [review] migrate from popt to goption (good riddance) I merged the GOption variables into the struct, and did some consolidation within this struct. we now have three main callbacks - unsupported options handling - post-goption-parsing-stuff - 'immediate option' handling
I was in a hurry and wasn't very verbose - some supplementals: * by struct I was referring to "OptionParsingResults" * 'immediate options' are those that need to be examined instantly - they can appear multiple times and often refer to the latest tab/window created * the post-parsing stuff examines the rest of the options and acts upon the values given * Some variables inside the struct were dropped - like "disable_factory" since we already have use_factory. This went along with some other small optimisations.
+ if ( len == 2 ) /* short option */ + while ( option_name[1] != goptions[i].short_name) + i++; + else + while ( strcmp (&(option_name[2]), goptions[i].long_name) ) + i++; + switch (i) + case 0: /* --command */ That makes the code dependent on the order of the options in the GOptionEntry struct... you should at least add a comment to the beginning of that struct, and probably also use an enum for the "0" etc. here. Or maybe just strcmp (option_name, "-c") == 0 || strcmp (option_name, "--command") == 0 ? static GnomeModuleInfo module_info = { @@ -1216,12 +1112,23 @@ You can remove this module_info. reqs[0].module_info = LIBGNOMEUI_MODULE; + module_info.requirements = reqs; and these too. Otherwise this looks good to me.
Created attachment 101317 [details] [review] migrate from popt to goption (good riddance) I incorporated the latest suggestions. I decided to use the strcmp() stuff. I could not find any regressions. Guilherme?
Patch fails to apply for me. (Guilherme is not around g-t anymore. chpe and I maintain it these days)
The only hunk that doesn't apply is obsolete anyway.
Created attachment 101441 [details] [review] just rediff'd
Thanks. I'll review tonight or tomorrow.
(In reply to comment #48) > Patch fails to apply for me. (Guilherme is not around g-t anymore. chpe and I > maintain it these days) you may want to update MAINTAINERS in svn and especially the information at the bottom of http://bugzilla.gnome.org/browse.cgi?product=gnome-terminal then (i think the latter can only be done by guilherme himself, or poke in #bugs).
Behdad, Christian, I made the two of you maintainers (by means of bugzilla). please go to account->Email prefs and watch the "gnome-terminal-maint@gnome.bugs" alias.
Reviewed the patch. It doesn't seem to address any of my previous comments. In short: - Break immediate_options_callback() into 10, 100, whatever needed callbacks. I mean it. "I don't like it" is not enough reason. That approach is seriously inferior, performance-wise, readability-wise, everything-wise. If we wanted to strcmp options, GNU getopt() was there long time ago. - Lets move goptions and parsing_results to auto variables in a function. That saves memory and is cleaner. - This may be a good time to get rid of deprecated options. What do people think? I'm thinking about removing unsupported_option_callback() completely. Other than these, looks good. If you don't want to do these, I can do myself.
(In reply to comment #53) > Behdad, Christian, > > I made the two of you maintainers (by means of bugzilla). > > please go to account->Email prefs and watch the > "gnome-terminal-maint@gnome.bugs" alias. Ughh, just saw this..
Here is some more verbose explanation why I don't like breaking up immediate_options_callback() : * It bloats the code. You'll get 14 more signatures. In my opinion this decreases readablitlity. Then again, I am not an experienced coding wizard. * In each functions we have to replicate some variable declarations. Adds 14*x lines of (redundant) code. Additionally, chpe never complained. After all, I am not maintaining, so I will shape it to your liking, so we can hopefully advance one more step and rid Gnome of legacy dependencies. I am all for ridding deprecated options. However I see no urgent hurry for this. Would be an ideal time now, nonetheless.
(In reply to comment #56) > Here is some more verbose explanation why I don't like breaking up > immediate_options_callback() : > > * It bloats the code. You'll get 14 more signatures. You mean 14 more functions? Sure. More, shorter functions are better than fewer, longer ones. If you really mean signatures, you don't need signatures. > In my opinion this decreases readablitlity. Then again, I am > not an experienced coding wizard. And in my opinion it doesn't. Forget about opinions, yours makes the code inefficient, but gains nothing. > * In each functions we have to replicate some variable declarations. > Adds 14*x lines of (redundant) code. Sure, that's how programming is. That's not redundant, that's just similar code. One can write macros to make those shorter, but *that* decreases readability. Keep it Simple. > Additionally, chpe never complained. It's respectful to respond to all reviews of your patch. > After all, I am not maintaining, so I will shape it to your liking, > so we can hopefully advance one more step and rid Gnome of legacy dependencies. > > I am all for ridding deprecated options. However I see no urgent hurry for > this. Would be an ideal time now, nonetheless. Exactly, no hurry. That's why I want to see this done correctly. Thanks
Created attachment 101957 [details] [review] port gnome-terminal to goption I've broken up immediate_options_callback() as requested. I wasn't able to move struct goptions and struct parsing_results as local variables into a function, and use it in main(), and get it working within hours, I won't be able to do that without help, sorry.
Created attachment 102024 [details] [review] committed patch You can see here what I meant. Your patch had some memory management issues, btw. Anyhow, this if committed and fixed now! Please test.
2008-01-02 Behdad Esfahbod <behdad@gnome.org> Bug 336155 – [patch] port gnome-terminal to goption Based on patch from Christian Kirbach * src/terminal.c: Migrate from popt to GOption, and clean up. * configure.in: Rquire glib-2 >= 2.12.0, libgnome-2 >= 2.14.0
ok i did something similar but moved also gnome_program_init() into the function - it segfaulted deep within gnome_program_init() I tested - the following commands close the terminal and do not work as intended $ gnome-terminal --tab --show-menubar --show-menubar $ gnome-terminal --tab --hide-menubar --tab --show-menubar I will look into this later
They work great here. Open another bug if there are bugs.
new Bug 507503 opened