[MPlayer-dev-eng] text_font_scale_factor

[Reimar Döffinger]

On Fri, Jul 06, 2007 at 11:03:24AM +0300, Anatoli Marinov wrote:
>  The attached file is my patch that enables runtime changing of osd
>  font scale factor.
>  Please let me know if there is something wrong and I will fix it as
>  fast as I can.
>  For me was so hard to find unused keys. Finally I've binded scale
>  factor increment to *b* and decrement to *n* (default values). Of
>  course if you have a different vision about that it will be ease for
>  me to change them.

IMO don't bind them at all by default, we have too many default keybinds
already. Feel free to add them to our example input.conf though.

>    { MP_CMD_VF_CHANGE_RECTANGLE, "change_rectangle", 2, { {MP_CMD_ARG_INT,{0}}, {MP_CMD_ARG_INT,{0}}, {-1,{0}}}},
> -
> +  { MP_CMD_SUB_FONT_SCALE_FACTOR, "sub_font_scale_factor",1,  { {MP_CMD_ARG_FLOAT,{0}}, {MP_CMD_ARG_INT,{0}}, {-1,{0}} } },
>  #ifdef HAVE_NEW_GUI  

leave the empty line before the #ifdef in.
Also you are only using one argument but specify that it accepts two...
It probably would be nicer to add this as a property, too, if you have
the time.
The things you need for that are in command.c, esp. the mp_properties
table around line 1461 and if you want to keep the osd message the
set_prop_cmd array around line 1648.
Maybe mp_property_volume or so is a good template for a property
handling function.
And I agree with Compn suggestion for shorter names (both command string
and both defines).

> @@ -1670,6 +1671,7 @@
>      { "sub_pos", MP_CMD_SUB_POS, 0, 0, -1, MSGTR_SubPosStatus },
>      { "sub_alignment", MP_CMD_SUB_ALIGNMENT, 1, 0, -1, MSGTR_SubAlignStatus },
>      { "sub_delay", MP_CMD_SUB_DELAY, 0, 0, OSD_MSG_SUB_DELAY, MSGTR_SubDelayStatus },
> +    { "sub_font_scale_factor", MP_CMD_SUB_FONT_SCALE_FACTOR, 0, 0, OSD_MSG_SUB_FONT_SCALE_FACTOR, MSGTR_SubScaleFactor },
>      { "sub_visibility", MP_CMD_SUB_VISIBILITY, 1, 0, -1, MSGTR_SubVisibleStatus },
>      { "sub_forced_only", MP_CMD_SUB_FORCED_ONLY, 1, 0, -1, MSGTR_SubForcedOnlyStatus },

Hmm.. while this is what I suggested to do if you implement it as a
property, it is wrong to add it here if you do not implement it as a
property as this patch does.

> +	case MP_CMD_SUB_FONT_SCALE_FACTOR:
> +           if ((cmd->args[0].v.f < 0 && text_font_scale_factor > 0.1) ||
> +                     (cmd->args[0].v.f > 0 && text_font_scale_factor < 100)) {

There are some useless () and it's not aligned nicely, should be more
like this:

>    if (cmd->args[0].v.f < 0 && text_font_scale_factor > 0.1 ||
>        cmd->args[0].v.f > 0 && text_font_scale_factor < 100) {

Should you implement it as a property you won't need this kind of check
though since you can specify the limits in the mp_properties array.

> +               text_font_scale_factor += cmd->args[0].v.f;
> +		 force_load_font = 1;

You used tabs here while the surrounding code does not.

Greetings,
Reimar Döffinger