[MPlayer-dev-eng] [PATCH] Optional parameter for switch_audio

[The Wanderer]

kiriuja wrote:

>>> +    case MP_CMD_SWITCH_AUDIO : {
>>> +        int v = demuxer_switch_audio(demuxer, cmd->args[0].v.i);
>>> +        if (identify)
>>> +          mp_msg(MSGT_GLOBAL, MSGL_INFO, "ID_AUDIO_TRACK=%d\n", v);
>>> +    } break;
>> 
>> Why create the variable "v" at all, instead of simply calling
>> demuxer_switch_audio directly into the mp_msg call?
> 
> You probably noticed that mp_msg is only called when -identify is on.
> If you put the function call inside it, it won't work when -identify
> is off.

Thank you. I'd begun to realize Rich's explanation before I posted, but
this is more conclusive still; my only excuse it that I've been short on
sleep for a week, and my brain was nearing daily shutdown.

>>> -/** \brief Given a matroska track number, find the subtitle number that mplayer would ask for.
>>> +/** \brief Given a matroska track number and type, find the id that mplayer would ask for.
>> 
>> As long as we're changing this line anyway, why not capitalize
>> "Matroska" and "MPlayer" (and possibly "ID")? (Unnecessary, but
>> keeping the documentation good English is my primary
>> contribution... and code comments are a form of documentation,
>> doxygen comments especially, so if the opportunity comes up to make
>> improvements without creating clutter why not take advantage of
>> it?)
> 
> The problem of English and writing style is not local to just that
> one line, it's all over the place. If you would like to improve it,
> first specify the rules that should be followed and then submit a
> patch. Fixing it line by line when opportunity comes up clearly won't
> do.

I know the problem isn't localized, but I don't think that that means
that opportunities to fix part of the problem should not be taken. I
think it likely that even if a comprehensive fix-all-comment-English
patch were worked up and submitted, people would object on the grounds
of "this accomplishes nothing useful" (i.e. creating CVS clutter) and
that various particular comments didn't need to be changed, and...

In any case, although you may be right that opportunistic fixes won't
do, I don't think the word "clearly" applies.

-- 
       The Wanderer

Warning: Simply because I argue an issue does not mean I agree with any
side of it.

A government exists to serve its citizens, not to control them.