[PATCH] xfree86: Add option parsing for percent options.

[Dan Nicholson]

On Thu, May 13, 2010 at 4:34 PM, Peter Hutterer
<peter.hutterer at who-t.net> wrote:
> On Thu, May 13, 2010 at 04:12:34PM -0700, Dan Nicholson wrote:
>> On Thu, May 13, 2010 at 3:39 PM, Peter Hutterer
>> <peter.hutterer at who-t.net> wrote:
>> > On Thu, May 13, 2010 at 03:19:12PM -0700, Dan Nicholson wrote:
>> >> On Wed, May 12, 2010 at 11:07 PM, Peter Hutterer
>> >> <peter.hutterer at who-t.net> wrote:
>> >> > +
>> >> > +pointer
>> >> >  xf86ReplaceStrOption(pointer optlist, const char *name, const char* val)
>> >> >  {
>> >> >       return xf86AddNewOption(optlist,name,val);
>> >> > @@ -533,6 +565,22 @@ ParseOptionValue(int scrnIndex, pointer options, OptionInfoPtr p,
>> >> >                p->found = FALSE;
>> >> >            }
>> >> >            break;
>> >> > +       case OPTV_PERCENT:
>> >> > +           {
>> >> > +               char tmp[2];
>> >> > +               /* awkward match, but %% doesn't increase the match counter,
>> >> > +                * hence 100 looks the same as 100% to the caller of sccanf
>> >> > +                */
>> >> > +               if (sscanf(s, "%ld%1[%]", &p->value.num, tmp) != 2) {
>> >> > +                   if (markUsed)
>> >> > +                       xf86DrvMsg(scrnIndex, X_WARNING,
>> >> > +                                  "Option \"%s\" requires a percent value\n", p->name);
>> >> > +                   p->found = FALSE;
>> >>
>> >> Why is the warning being suppressed with markUsed? Because we can't
>> >> trust the formatting of the value or so that CheckPercent doesn't see
>> >> a warning or something else?
>> >
>> > yes, exactly. I added this to the commit message, but I guess it should be
>> > in the code as well:
>> >
>> > ""
>> > The percent value parsing differs slightly - if the option is not to marked
>> > as used (e.g. xf86CheckPercentOption()), no warning is emitted to the log
>> > file if the value is not a percent value. This allows double-options (either
>> > as % or as absolute number) without warnings.
>> > ""
>>
>> I guess I meant I need a little more explanation. Why wouldn't we want
>> a properly formatted percentage for the value? Not to mention that
>> FALSE will be returned in that case and CheckPercent will end up using
>> the default passed in. None of the other option types have this
>> special handling. OPTV_FREQ seems like a good example to follow.
>
> oh, right. the main reason is that I expect some options that take percent
> also to take absolute values. markUsed is set when you use the
> xf86SetPercentOption(), which is the preferred one.
> If you use xf86CheckPercentOption() just to check if the value is there,
> then the following construct is possible (from an upcoming synaptics patch):
>
>    percent = xf86CheckPercentOption(opts, "AreaTopEdge", -1);
>    if (percent != -1) {
>        percent = xf86SetPercentOption(opts, "AreaTopEdge", -1);
>        pars->area_top_edge = percent/100.0 * (priv->maxy - priv->miny) + priv->miny;
>    } else
>        pars->area_top_edge = xf86SetIntOption(opts, "AreaTopEdge", 0);
>
> this checks for a percent option and takes if if present, otherwise treats
> it as an integer number. Without the special handling, this would always pop
> a warning into the logs, even if the value is valid.
>
> (the xf86SetPercentOption() is there to make the option appear as selected
> in the log)
>
> the other option is to somehow coerce the xf86Set{Int|Float} options into
> supporting % values as well and returning some magic flag if it is set.

I see what you mean. However, I think this would be better handled in
the driver where it knows it wants this special behavior instead of
making the implementation funky.

top_edge = xf86FindOptionValue(opts, "AreaTopEdge");
if (top_edge && top_edge[strlen(top_edge) - 1] == '%') {
    percent = xf86SetPercentOption(opts, "AreaTopEdge", -1);
    pars->area_top_edge = percent/100.0 * (priv->maxy - priv->miny) +
priv->miny;
} else
    pars->area_top_edge = xf86SetIntOption(opts, "AreaTopEdge", 0);

If I was trying to use a percent option in my code, I wouldn't like
that it behaved differently than its friends.

--
Dan