Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1748)

Issue 6345088: Fixes all black bars in NR (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 years, 11 months ago by PhilEHolmes
Modified:
6 years, 11 months ago
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

Just a few changed files. A number of these are in /snippets and I have updated the LSR to match them already - these are just to illustrate the changes work. These changes get rid of all instances of 'overfull hbox' in the texi2pdf logfile and therefore of all the black sidebars caused by this. In a few places I've been forced to use new lines and fiddle image widths - the latter because of issue 766.

Patch Set 1 #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+143 lines, -395 lines) Patch
M Documentation/included/chord-names-jazz.ly View 1 chunk +0 lines, -1 line 0 comments Download
M Documentation/included/display-predefined-fretboards.ly View 1 chunk +11 lines, -14 lines 0 comments Download
M Documentation/notation/ancient.itely View 2 chunks +8 lines, -4 lines 0 comments Download
M Documentation/notation/changing-defaults.itely View 1 chunk +1 line, -1 line 0 comments Download
M Documentation/notation/chords.itely View 1 chunk +2 lines, -1 line 0 comments Download
M Documentation/notation/fretted-strings.itely View 3 chunks +7 lines, -7 lines 2 comments Download
M Documentation/notation/input.itely View 3 chunks +3 lines, -3 lines 1 comment Download
M Documentation/notation/notation-appendices.itely View 29 chunks +32 lines, -31 lines 1 comment Download
M Documentation/notation/pitches.itely View 1 chunk +1 line, -1 line 0 comments Download
M Documentation/notation/repeats.itely View 1 chunk +2 lines, -1 line 0 comments Download
M Documentation/notation/simultaneous.itely View 3 chunks +5 lines, -3 lines 0 comments Download
M Documentation/notation/spacing.itely View 1 chunk +1 line, -1 line 0 comments Download
M Documentation/notation/staff.itely View 1 chunk +3 lines, -1 line 0 comments Download
M Documentation/notation/wind.itely View 2 chunks +5 lines, -4 lines 0 comments Download
M Documentation/notation/world.itely View 1 chunk +1 line, -1 line 0 comments Download
M Documentation/snippets/ancient-headword.ly View 1 chunk +0 lines, -15 lines 0 comments Download
M Documentation/snippets/changing-the-breath-mark-symbol.ly View 1 chunk +2 lines, -1 line 0 comments Download
M Documentation/snippets/chant-or-psalms-notation.ly View 1 chunk +8 lines, -2 lines 0 comments Download
M Documentation/snippets/chords-headword.ly View 1 chunk +0 lines, -14 lines 0 comments Download
M Documentation/snippets/editorial-headword.ly View 1 chunk +0 lines, -13 lines 0 comments Download
M Documentation/snippets/expressive-headword.ly View 1 chunk +0 lines, -5 lines 0 comments Download
M Documentation/snippets/figured-bass-headword.ly View 1 chunk +0 lines, -7 lines 0 comments Download
M Documentation/snippets/fretted-headword.ly View 1 chunk +0 lines, -6 lines 0 comments Download
M Documentation/snippets/making-some-staff-lines-thicker-than-the-others.ly View 1 chunk +2 lines, -1 line 0 comments Download
M Documentation/snippets/new/chant-or-psalms-notation.ly View 1 chunk +9 lines, -2 lines 0 comments Download
M Documentation/snippets/new/chords-headword.ly View 1 chunk +0 lines, -6 lines 0 comments Download
M Documentation/snippets/new/fretted-headword.ly View 1 chunk +0 lines, -6 lines 0 comments Download
M Documentation/snippets/new/staff-headword.ly View 1 chunk +0 lines, -7 lines 0 comments Download
M Documentation/snippets/simultaneous-headword.ly View 4 chunks +40 lines, -229 lines 1 comment Download
M Documentation/snippets/staff-headword.ly View 1 chunk +0 lines, -7 lines 0 comments Download

Messages

Total messages: 6
PhilEHolmes
Lots of changes here. The NR still builds successfully following them all, so I'm fairly ...
6 years, 11 months ago (2012-07-11 13:54:26 UTC) #1
Trevor Daniels
LGTM Trevor
6 years, 11 months ago (2012-07-11 14:23:14 UTC) #2
lemzwerg
LGTM. Thanks a lot! http://codereview.appspot.com/6345088/diff/1/Documentation/snippets/simultaneous-headword.ly File Documentation/snippets/simultaneous-headword.ly (right): http://codereview.appspot.com/6345088/diff/1/Documentation/snippets/simultaneous-headword.ly#newcode16 Documentation/snippets/simultaneous-headword.ly:16: #'((alignment-distances .(12))) Why removing the ...
6 years, 11 months ago (2012-07-11 15:23:22 UTC) #3
email_philholmes.net
----- Original Message ----- From: <lemzwerg@googlemail.com> To: <PhilEHolmes@googlemail.com>; <graham@percival-music.ca>; <tdanielsmusic@googlemail.com>; <pkx166h@gmail.com> Cc: <lilypond-devel@gnu.org>; <reply@codereview-hr.appspotmail.com> Sent: ...
6 years, 11 months ago (2012-07-11 15:36:35 UTC) #4
Graham Percival
I've only looked at the first few files, but I have grave concerns with this ...
6 years, 11 months ago (2012-07-11 19:45:08 UTC) #5
email_philholmes.net
6 years, 11 months ago (2012-07-11 20:17:12 UTC) #6
----- Original Message ----- 
From: <graham@percival-music.ca>
To: <PhilEHolmes@googlemail.com>; <tdanielsmusic@googlemail.com>; 
<pkx166h@gmail.com>; <lemzwerg@googlemail.com>; <email@philholmes.net>
Cc: <lilypond-devel@gnu.org>; <reply@codereview-hr.appspotmail.com>
Sent: Wednesday, July 11, 2012 8:45 PM
Subject: Re: Fixes all black bars in NR (issue 6345088)


> I've only looked at the first few files, but I have grave concerns with
> this patch.
>
> I feel terrible about it, though.  Please, PLEASE, anybody who wants to
> make lots of changes to the docs -- spend *at most* one hour working on
> your changes, then submit them to get feedback.  It must have taken a
> long time to do all this, and if it doesn't get accepted this will be a
> real shame.
> (some parts of this patch are obviously good and could be pushed
> directly, but they're all mixed in with parts that I have concerns
> about, so it's problematic)

Probably about 4 or 5 hours, so it's not desperate.  The problem with stuff 
like this is that there's no real point in doing a little bit - you either 
fix the lot or none.  Don't worry about adverse comments - I'm happy to 
receive them providing we remember the aim is to make things better - 
occasionally we stray into keeping the _very_ poor status quo instead of 
implementing a _slightly_ poor change.

>
>
http://codereview.appspot.com/6345088/diff/1/Documentation/notation/fretted-s...
> File Documentation/notation/fretted-strings.itely (right):
>
>
http://codereview.appspot.com/6345088/diff/1/Documentation/notation/fretted-s...
> Documentation/notation/fretted-strings.itely:1134: The file
> @file{predefined-ukulele-fretboards.ly} contains the fret
> I'm not a fan of this change.  I'd rather have a manual line-break on
> its own line:
>
> ... are contained in the file
> @*
> @file{blah}

David doesn't like @* so I avoided it.  I'll use @*

>
http://codereview.appspot.com/6345088/diff/1/Documentation/notation/fretted-s...
> Documentation/notation/fretted-strings.itely:1404:
> @file{ly/predefined-guitar-fretboards.ly}, @*
> oh god.  Seriously?!  there _has_ to be a better way to make tex behave.
>
> hmm, maybe these could be wrapped in an @example?  it's icky, though.
> Not a serious suggestion.

To be honest, it didn't seem worth worrying about too much.  It's basically 
a list of filenames.  Separating them with line break isn't a big deal, and 
at least allows the user to read them?

>
http://codereview.appspot.com/6345088/diff/1/Documentation/notation/input.itely
> File Documentation/notation/input.itely (right):
>
>
http://codereview.appspot.com/6345088/diff/1/Documentation/notation/input.ite...
> Documentation/notation/input.itely:1359:
> @lilypond[verbatim,papersize=a8landscape]
> why is this necessary?  Does lilypond-book not select the appropriate
> paper size when there's a \book{} inside the example?  If that's the
> case, it should be solved with lilypond-book, not by manually changing
> every @lilypond that involves \book.
>
> (this sounds slightly familiar.  Didn't we have a round of bugfixes in
> lilypond-book on this problem?)

Kind of, but not this one specifically.  Look at the old version - it's 
crap.  This makes it work for an odd example that depends on multiple page 
examples, which are rare.  Also check the CG - 
http://lilypond.org/doc/v2.15/Documentation/contributor/lilypond-formatting  
- a8landscape is specifically recommended.

>
http://codereview.appspot.com/6345088/diff/1/Documentation/notation/notation-...
> File Documentation/notation/notation-appendices.itely (right):
>
>
http://codereview.appspot.com/6345088/diff/1/Documentation/notation/notation-...
> Documentation/notation/notation-appendices.itely:48: @c The line width
> is a hack to allow space for instrument names
> I really don't like seeing hacks like this in the docs; it's just
> wall-papering over flaws in lilypond itself.  There should be a way to
> tell lilypond "you have this much width on the page", and then lilypond
> should select an appropriate line-width for the first line of the score
> in order to have enough space for the instrument names.
>
> http://codereview.appspot.com/6345088/

Kind-of agreed.  But if there's a flaw in lilypond, we shouldn't make that 
flaw a defining feature by emphasising it in the docs.  My suggestion is 
that you should either fix the flaw or accept the change....

--
Phil Holmes 

Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b