After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 337000 - switch from popt to glib's GOption
switch from popt to glib's GOption
Status: RESOLVED FIXED
Product: gnome-mag
Classification: Deprecated
Component: magnifier-utility
0.12.x
Other Linux
: Normal normal
: ---
Assigned To: bill.haneman
bill.haneman
Depends on:
Blocks:
 
 
Reported: 2006-04-03 08:37 UTC by Christian Kirbach
Modified: 2007-01-08 23:17 UTC
See Also:
GNOME target: ---
GNOME version: 2.13/2.14


Attachments
switch from popt to GOption (11.10 KB, patch)
2006-04-06 13:48 UTC, Christian Kirbach
none Details | Review
This is the same patch with G_OPTION_INT64 for cursor-color and border-color options (10.67 KB, patch)
2006-07-25 19:44 UTC, Carlos Eduardo Rodrigues Diógenes
none Details | Review
switch from popt to GOtion (11.16 KB, patch)
2006-07-26 07:55 UTC, Christian Kirbach
needs-work Details | Review
latest patch with some debug output (10.66 KB, patch)
2007-01-06 16:06 UTC, Christian Kirbach
needs-work Details | Review
get rid of popt, use GOption (10.82 KB, patch)
2007-01-07 02:12 UTC, Christian Kirbach
none Details | Review
patch the MagnifierOptions to be succesfully parsed by GOption (1.82 KB, patch)
2007-01-07 16:02 UTC, Carlos Eduardo Rodrigues Diógenes
none Details | Review
move from popt to GOption (12.58 KB, patch)
2007-01-08 21:09 UTC, Christian Kirbach
committed Details | Review

Description Christian Kirbach 2006-04-03 08:37:29 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
Comment 1 Christian Kirbach 2006-04-06 13:48:55 UTC
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.
Comment 2 Carlos Eduardo Rodrigues Diógenes 2006-04-25 21:59:27 UTC
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.
Comment 3 Christian Kirbach 2006-04-27 19:21:03 UTC
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.
Comment 4 Christian Kirbach 2006-04-29 11:40:54 UTC
hmm no it is a 16 Bit number.
Comment 5 Christian Kirbach 2006-04-29 11:52:33 UTC
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.
Comment 6 Carlos Eduardo Rodrigues Diógenes 2006-05-03 12:30:18 UTC
if we can assume that every major platform has 32 bit integers today we can apply the patch?
Comment 7 Christian Kirbach 2006-05-03 12:45:33 UTC
poking Bill ...
Comment 8 Carlos Eduardo Rodrigues Diógenes 2006-06-12 23:17:16 UTC
Bill, do you have any comment on this?
Comment 9 Carlos Eduardo Rodrigues Diógenes 2006-07-24 01:24:22 UTC
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?
Comment 10 Carlos Eduardo Rodrigues Diógenes 2006-07-25 19:44:16 UTC
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!
Comment 11 Christian Kirbach 2006-07-26 07:53:01 UTC
we also have G_OPTION_ARG_DOUBLE since 2.12 if this is of any help
Comment 12 Christian Kirbach 2006-07-26 07:55:47 UTC
Created attachment 69642 [details] [review]
switch from popt to GOtion

follows the latest attachment, fixes a typo
can now be applied to cvs head
Comment 13 Carlos Eduardo Rodrigues Diógenes 2006-07-26 11:14:06 UTC
Thanks for the patch Christian. I will wait a while, before commit, if Bill have something to say about this. Thanks!
Comment 14 bill.haneman 2006-07-27 13:32:01 UTC
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
Comment 15 Christian Kirbach 2006-08-01 09:26:05 UTC
I will hopefully look into this within the next few days.
However some help at testing is appreciated.
Comment 16 Christian Kirbach 2006-08-10 22:07:42 UTC
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?
Comment 17 Carlos Eduardo Rodrigues Diógenes 2006-09-22 20:24:01 UTC
I tried here with glib 2.12.3 and -z and --cursor-color doesn't worked too.
Comment 18 André Klapper 2006-11-29 15:25:43 UTC
Christian, any news on the patch? :-)
Comment 19 Christian Kirbach 2006-11-29 16:01:32 UTC
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.
Comment 20 André Klapper 2007-01-05 14:39:55 UTC
hmm... ABI/API freeze in front and it looks like we will miss this again. :-(
Comment 21 Carlos Eduardo Rodrigues Diógenes 2007-01-05 16:47:45 UTC
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!
Comment 22 Christian Kirbach 2007-01-05 17:20:07 UTC
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.
Comment 23 Christian Kirbach 2007-01-06 01:46:03 UTC
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
Comment 24 Christian Kirbach 2007-01-06 15:59:34 UTC
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??
Comment 25 Christian Kirbach 2007-01-06 16:06:37 UTC
Created attachment 79532 [details] [review]
latest patch with some debug output
Comment 26 Christian Kirbach 2007-01-07 01:59:18 UTC
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
Comment 27 Christian Kirbach 2007-01-07 02:12:47 UTC
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.
Comment 28 Christian Kirbach 2007-01-07 02:22:26 UTC
I filed Bug 393764 about the GOption parser problem
Comment 29 Christian Persch 2007-01-07 12:21:01 UTC
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.
Comment 30 Carlos Eduardo Rodrigues Diógenes 2007-01-07 16:02:58 UTC
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!
Comment 31 André Klapper 2007-01-07 17:07:29 UTC
happy happy joy joy (sing along!)
okay, hope to see this in soon. :-)
Comment 32 Christian Kirbach 2007-01-08 15:42:38 UTC
Christian, thank you for casting light on this!
Comment 33 Christian Kirbach 2007-01-08 21:09:14 UTC
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
Comment 34 Carlos Eduardo Rodrigues Diógenes 2007-01-08 22:19:03 UTC
Fixed in the development version. The fix will be available in the next major release. Thank you for your bug report.
Comment 35 André Klapper 2007-01-08 23:17:39 UTC
i have updated the status at http://live.gnome.org/GnomeGoals/PoptGOption , thanks a lot everybody for getting this fixed!