[MPlayer-dev-eng] [PATCH] GUI: Corrections for display of video size and track number, playlist

[Ingo Brückl]

Hans-Dieter Kosch wrote on Thu, 29 Nov 2012 01:54:47 +0100:

> Ingo Brückl wrote:
>>
>> Please provide separate patches for each different (class of) issue(s),
>> not a bunch of fixes in one big patch.
> OK. I'll split it up. I'm not familiar with your patch codex (from 'svn
> log' can be seen that it's very detailed).

I can't give a general rule. Try addressing only one issue a time and
describe what you've done and why. You should think of the other developers
who will read the patch and possibly aren't familiar with this particular
issue or the related code. So make as small steps as possible, but as big
ones as necessary to cover the single issue. And describe in a way that will
help to understand the patch (even yourself if you will be confronted with it
in a few years again).

Most developers appreciate separating purely cosmetic issues from functional
ones (see r35459 and r35460, for example). In r35459 you see that it's just a
simple patch, nothing but adding an if clause (but for two different windows
affected by the same issue). And even without knowing anything about GUI
mouse handling you see that there is an array with an index we check now.
Easy to understand at a glance. If I hadn't separated the patch, we would
have had one with 2 x 17 changed lines and it would be very difficult to
understand what happened there and why.

(See r35472 for a bad commit. Renaming the variables should have been a
separate patch. Two - rather simple - changes are now a patch that is
unnecessarily hard to read.)

> The reason for I did it in one step is that I do 'svn diff' against the
> latest SVN revision. If I split it, and one diff is treated, the line
> numbers in the next diff are incorrect. I could make step by step 'diff -u'
> against previous change, or wait until one diff is treated and then update
> working copy and post the next. What do you propose?

I know that, without svn commit permissions, it isn't easy to work as
suggested (and even with, it sometimes isn't). If you're familiar with git,
you can create your own local repository and prepare, rearrange, split or
merge your local patches as needed before posting them.

If you'd like to stick with svn, you probably have to do as you said. Either
work step by step and wait for the commit, or copy your already changed files
and make diffs against them.

> BTW: Is there a preferred coding style? E.g. placing of opening braces (at
> end of line or on separate line), indentation rules, ... Of course, when I
> patch a file, I try to use the style therein.

The preferred coding style is defined by an uncrustify config file in
TOOLS/mp-uncrustify-style.cfg.

Since the GUI has been developed by many people so far, you'll find different
coding styles, because only some of the files have already been converted to
the MPlayer coding style:

  app.c
  cfg.c
  interface.c
  skin/font.c
  skin/skin.c
  ui/actions.c
  ui/render.c
  ui/widgets.c
  util/bitmap.c
  util/cut.c
  util/list.c
  util/string.c
  wm/ws.c

It is simplest to use the style you'll find in the particular file to not
break readability. All files will be converted, of course - step by step
(some time).

Ingo