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

[Hans-Dieter Kosch]

Ingo Brückl wrote:
> 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).
It's like the difference between a manuscript and a well formatted print layout 
for a book: both contain the same info, but the formatted layout is much more 
easy to read (often, misunderstandings come from misreading).
Would you appreciate help on converting?
> 
Many thanks for the very exhaustive explanations! I understand. I'm familiar 
with SVN since its beginning in my professional job, but we do the commits 
somewhat more "sloppy", as anybody involved has full insight. Git is quite new 
to me, but who knows...

Hans-Dieter