GNOME Bugzilla – Bug 337000
switch from popt to glib's GOption
Last modified: 2007-01-08 23:17:39 UTC
See the Gnome goals http://live.gnome.org/GnomeGoals/PoptGOption I am working on a patch, but the problem is some of the command line parameters expect a float/long value, but GOption does not support them. The question arises how we should deal with it. I thought about treating the float values as strings and then evaluating the strings to obtain float values. Invalid inputs may give some headache. About the long variables - can we convert them into integer? Please comment. The switches in question are: --cursor-scale-factor=FLOAT cursor scale factor --cursor-color=LONG cursor color (applied to 'black' pixels -z, --zoom-factor=FLOAT zoom (scale) factor used to magnify source display -c, --border-color=LONG border color specified as (A)RGB 23-bit value, Alpha-MSB
Created attachment 62856 [details] [review] switch from popt to GOption get rid of popt however this should be applied for the 2.15 cycle since it requires glib >=2.11 which comes with a new FLOAT command line option type.
We still have a problem with the LONG argument, because if we convert it to integer we will not have precision to represent a 24/32 bits color.
IIRC I was told that it is accepting 32 Bit numbers as integer. hmm in the doc it says G_OPTION_ARG_INT it is of type gint. i could not quickly find the range of numbers it covers.
hmm no it is a 16 Bit number.
I have to correct this again. "Range of an Integer Type [...] There is no general rule; it depends on the C compiler and target machine." according to #gnome-hackers we can assume that every major platform has 32 bit integers today.
if we can assume that every major platform has 32 bit integers today we can apply the patch?
poking Bill ...
Bill, do you have any comment on this?
Christian, I was looking now at GLib 2.11.2 Reference Manual and saw that it's now have G_OPTION_INT64, what resolves the issue. Could you re-make the patch?
Created attachment 69602 [details] [review] This is the same patch with G_OPTION_INT64 for cursor-color and border-color options I don't test the patch yet. I will have to install a new glib to do it!
we also have G_OPTION_ARG_DOUBLE since 2.12 if this is of any help
Created attachment 69642 [details] [review] switch from popt to GOtion follows the latest attachment, fixes a typo can now be applied to cvs head
Thanks for the patch Christian. I will wait a while, before commit, if Bill have something to say about this. Thanks!
I think it may be best to wait until post 2.16 to apply this patch since it will be too difficult to test all the possible options, and there is risk of regression. On the other hand if we can test the following options I'd be OK with applying it now: -t|--target-display STRING -s|--source-display STRING --cursor-size=INT --cursor-color=LONG -v|--vertical -h|--horizontal -m|--mouse-follow -z|--zoom-factor FLOAT (including non-whole-number values!) -f|--fullscreen --ignore-damage --override-redirect
I will hopefully look into this within the next few days. However some help at testing is appreciated.
G_OPTION_ARG_DOUBLEs seem to cause trouble, that is the -z and --cursor-color options do not work. or is it some locale problem?
I tried here with glib 2.12.3 and -z and --cursor-color doesn't worked too.
Christian, any news on the patch? :-)
Actually I had a look at it 2 weeks ago. I have no idea why all options that take a DOUBLE argument are not working. Need to investigate this further, but most likely not before the weekend. Any help is most welcome.
hmm... ABI/API freeze in front and it looks like we will miss this again. :-(
ABI/API freeze aren´t mandatory for desktop modules AFAIK, so if the problems presented with the patch can be solved until 2.18 wen can still apply the patch!
Carlos, as far as I remember many options worked, except the ones taking double arguments. if you could take a look.... I will do, too, as soon as I have a completed jhbuild build again.
actually, i am not able to test --cursor-color even on the unpatched version (because i do not know what values to use, the ones I picked end up in no cursor at all). perhaps my patch was working all the time... can you give me a value to experiment with? or even better, a command line invocation that tests a few parameters magnifier -v -m fooo
ok, found a value to play with. Now, intresting enough, magnifier --cursor-color=16711680 -v -m works perfectly while magnifier -v -m --cursor-color=16711680 does not work at all -- none of the parameters gets applied by the GOption parser! It returns true meaning successful parsing. Did I trigger a bug??
Created attachment 79532 [details] [review] latest patch with some debug output
magnifier -m --cursor-color=16711680 -v also works with my patch. I get the feeling I've hit a glib bug. A minimal testcase works fine. So, regarding comment #14, I've successfully tested the following options: -t|--target-display STRING -s|--source-display STRING --cursor-size=INT --cursor-color=LONG -v|--vertical -h|--horizontal -m|--mouse-follow -z|--zoom-factor FLOAT (including non-whole-number values!) -f|--fullscreen --ignore-damage --override-redirect
Created attachment 79595 [details] [review] get rid of popt, use GOption consider this my "release candidate" As noted before, it's not my fault when swapping command line options confuses the parser.
I filed Bug 393764 about the GOption parser problem
typedef struct { [...] guint32 cursor_color; [...] unsigned long border_color; [...] } MagnifierOptions; + {"cursor-color", 0, 0, G_OPTION_ARG_INT64, &global_options.cursor_color, "cursor color (applied to \'black\' pixels)", NULL}, + {"border-color", 'c', 0, G_OPTION_ARG_INT64, &global_options.border_color, "border color specified as (A)RGB 23-bit value, Alpha-MSB", NULL}, The struct must have gint64's if you use G_OPTION_ARG_INT64 option arguments. The G_OPTION_ARG_NONE struct entries should be gboolean (gboolean==int but for clarity gboolean would be preferred). The struct has |float|s while you use G_OPTION_ARG_DOUBLE, that won't work either.
Created attachment 79639 [details] [review] patch the MagnifierOptions to be succesfully parsed by GOption Thanks for the explanation Christian Persch! I converted all the MagnifierOptions to glib defined types and all the options pointed by Bill worked fine. I will test a little more tomorrow! If you have time Christian Kirbach, also test it, but I think that everything is okay now. Uhuu, we have GOption!
happy happy joy joy (sing along!) okay, hope to see this in soon. :-)
Christian, thank you for casting light on this!
Created attachment 79782 [details] [review] move from popt to GOption Has been tested 25mins and works perfectly for me. Incorporates all suggestions ;) Thanks again for the precious hints
Fixed in the development version. The fix will be available in the next major release. Thank you for your bug report.
i have updated the status at http://live.gnome.org/GnomeGoals/PoptGOption , thanks a lot everybody for getting this fixed!