[MPlayer-dev-eng] [PATCH] New command support select sub by source and ID

[Ulion]

2007/11/15, Alban Bedel <albeu at free.fr>:
> On Sat, 3 Nov 2007 21:14:26 +0800
> Ulion <ulion2002 at gmail.com> wrote:
>
> > Thank you for the notify, here's the updated version, combined with a
> > sub_source property range fix.
>
> Sorry for the delay, but please don't abuse too much of remainder via
> private mail.
>
> > +    case M_PROPERTY_PRINT:
> > +        if (!arg)
> > +            return M_PROPERTY_ERROR;
> > +        return mp_property_sub(prop, M_PROPERTY_PRINT, arg, mpctx);
>
> Calling PRINT on the "sub_source" must print the current subtitle source
> name, anything else is non-sense.

OK.

>
> > +    case M_PROPERTY_SET:
> > +        if (!arg || *(int *) arg < -1 || *(int *) arg >= SUB_SOURCES)
> > +            return M_PROPERTY_ERROR;
>
> It should just clamp the parameter into the valid range instead of
> returning an error.

I already did that, at:
    { "sub_source", mp_property_sub_source, CONF_TYPE_INT,
     M_OPT_RANGE, -1, SUB_SOURCES - 1, NULL },


>
> > +        if (*(int *) arg < 0)
> > +            mpctx->global_sub_pos = -1;
> > +        else if (*(int *) arg != sub_source(mpctx)) {
> > +            if (*(int *) arg != sub_source_by_pos(mpctx, mpctx->global_sub_indices[*(int *) arg]))
> > +                return M_PROPERTY_UNAVAILABLE;
> > +            mpctx->global_sub_pos = mpctx->global_sub_indices[*(int
> > *) arg];
> > +        }
> > +        break;
>
> And it should return the value effectively set (like GET do).

Indeed the code did return the value effectively set, when arg < 0, it
must be -1, we set -1, and the effectively set value also -1, no need
to change the value. when arg >= 0, I have a check call
sub_source_by_pos to see whether the effectively set value is same
with the input arg, when they not match, set fail, else set it, the
input arg is just the effectively set value we make it sure here.

>
> > +    case M_PROPERTY_STEP_UP: {
> > +        int cur_source = sub_source(mpctx);
> > +        for (source = cur_source + 1; source != cur_source; ++source) {
> > +            if (source >= SUB_SOURCES) {
> > +                mpctx->global_sub_pos = -1;
> > +                break;
> > +            }
> > +            if (source == sub_source_by_pos(mpctx, mpctx->global_sub_indices[source])) {
> > +                mpctx->global_sub_pos = mpctx->global_sub_indices[source];
> > +                break;
> > +            }
> > +        }
> > +        break;
>
> You should take the optional argument (and its sign) in account if possible.
> This mean it's generaly better to join STEP_UP and STEP_DOWN and just compute
> the step size with something like this:
> (arg ? *(int*)arg : 1)*(action == M_PROPERTY_STEP_UP ? 1 : -1)

OK, done.

>
> I know the sub property don't do it, but it should. This mean here:
>
> > +    --mpctx->global_sub_pos;
> > +    return mp_property_sub(prop, M_PROPERTY_STEP_UP, arg, mpctx);
>
> You should pass NULL instead of arg, otherwise things will break if the
> sub property get fixed one day.

Fixed.

>
> > +    switch (strlen(prop->name)) {
> > +        case 7: source = SUB_SOURCE_VOBSUB; break;
> > +        case 8: source = SUB_SOURCE_SUBS; break;
> > +        case 9: source = SUB_SOURCE_DEMUX; break;
> > +        default: return M_PROPERTY_ERROR;
> > +    }
>
> This is really a gratious hack, just use strcmp().

Fixed.

>
> That's all from the property API POV I think. For the subtile code
> I can't really comment beside saying that it all (current code and
> this patch) look way, way too messy. Is anybody thinking about
> cleaning this awful mess?
>
>         Albeu

Any direction for the 'cleaning'? What's the 'messy' detail? I think I
didn't get your meaning about the 'messy' and 'cleaning', would you
make it more clear?

Fixed patch is here, all as you wish.

-- 
Ulion
-------------- next part --------------
A non-text attachment was scrubbed...
Name: select_sub_by_source_property3.diff
Type: text/x-diff
Size: 12700 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20071115/cd7a8697/attachment.diff>