[MPlayer-dev-eng] Re: [PATCH] SSA/ASS subtitles support

Rich Felker dalias at aerifal.cx
Mon Jun 19 00:21:29 CEST 2006


On Sun, Jun 18, 2006 at 04:59:50PM +0300, Uoti Urpala wrote:
> On Sun, 2006-06-18 at 09:38 -0400, Rich Felker wrote:
> > > Even though they have been moved under if() {} ? That's a tradeoff between 
> > > patch readability and code readability :) I prefer the latter.
> > 
> > You can do the cosmetics in a separate subsequent patch. Mixing them
> > makes it hard to review and hard for people going back later to audit.
> 
> That was not an isolated cosmetic change but an indentation change
> related to new functionality. I think it's better to keep those as part
> of the same patch as long as they don't reindent a block of several
> dozen lines.

It's been discussed before and the conclusion was along the lines of
what I said. If you mix the cosmetics in the functional patch, someone
MUST READ those lines and see that nothing actually changed.
Surprisingly (or maybe not surprisingly) this is very difficult for
the human eye.

> > > Freetype can read fonts directly from memory, but fontconfig can't :(
> > 
> > Is there a reason we need fontconfig? I hate it...
> 
> Is there a reason you hate fontconfig? You hate various things, often
> for completely irrational reasons.

Yes, it's bloated and unnecessary.

Rich




More information about the MPlayer-dev-eng mailing list