Issue 2758042: Doc: NR 4.1: Reorganize, clarify details. (Closed)
|
Created:
14 years, 6 months ago by Mark Polesky Modified:
14 years, 5 months ago Reviewers:
Keith, carl.d.sorensen, Neil Puttock, Trevor Daniels, joeneeman, Mark Polesky, Graham Percival (old account) CC:
lilypond-devel_gnu.org Visibility:
Public. |
DescriptionDoc: NR 4.1: Reorganize, clarify details.
Patch Set 1 #
Total comments: 17
Patch Set 2 : Lots of changes. #Patch Set 3 : Explain \paper vs. \layout; fix compile; nitpicks. #
Total comments: 2
Patch Set 4 : Improve some details. #
Total comments: 1
Patch Set 5 : Refer to installed files for defaults. #
Total comments: 36
Patch Set 6 : Make requested changes. #Patch Set 7 : Nitpicks. #
MessagesTotal messages: 21
Okay guys, here's my first draft patch for a rewrite of NR 4.1 Paper and Pages. Just to clarify, this is totally unrelated to my rewrite of NR 4.4.1 Vertical spacing inside a system (http://codereview.appspot.com/2642043/) So, I guess this is a big patch. Sorry about that, but there was so much information missing. I also moved things around quite a bit; please let me know if I made anything worse or harder to find or something like that. I'm pretty sure this is an improvement over the current version, but I'm starting to lose a little focus with this. Hopefully you'll all love it and there won't be too many complaints! Joe, is it asking too much to have you double-check my revised variable descriptions for accuracy? Anyone else so inclined, feel free to make comments. I'm starting to wonder if "@subsection Page formatting" should be split up into smaller subsections, since now it's quite lengthy. But I might want to take a break from this soon. So let me know what you guys think. Thanks again, - Mark
Sign in to reply to this message.
Mark, What a lot of work! Thanks for taking this on! I think that this is a big improvement, because of its thoroughness. I think that your subheadings need to become nodes (subsubsections?). I think that it would be nice if the defaults are easily accessible in a file somewhere to point us to them instead of explaining them all. If they're not easily accessible, then go ahead with what you've got here. Thanks, Carl http://codereview.appspot.com/2758042/diff/1/Documentation/notation/spacing.i... File Documentation/notation/spacing.itely (right): http://codereview.appspot.com/2758042/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:246: the default paper size, @code{paper-height} is @code{297\mm}. I don't like the idea of putting the default in the docs in this way, because it's a detail to try to keep up. A reference to a file that declares the default is better long-term. http://codereview.appspot.com/2758042/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:275: @c TODO: why? -mp Because it's much nicer in a sparsely-populated page to have the space at the bottom of the page than spread out among all the staves in the system. http://codereview.appspot.com/2758042/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:590: the default is @code{(10@tie{}*@tie{}@var{w}/210)@tie{}\mm}, where Can these defaults be looked up easily in a file somewhere? If so, I think a reference would be better here. There's too much detail, and I can't easily see the big picture. If this is defined in a file somewhere, then I think this constitutes "talking through the code" of the file where it's defined. http://codereview.appspot.com/2758042/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:647: @subsubheading Two-sided mode I think reorganization is needed, because this should be a node, IMO.
Sign in to reply to this message.
http://codereview.appspot.com/2758042/diff/1/Documentation/notation/spacing.i... File Documentation/notation/spacing.itely (right): http://codereview.appspot.com/2758042/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:428: @c TODO: Where do headers/footers fit in? -mp headers/footers have no effect on reference points (ie. the reference point of the top/bottom of the page is the bottom/top of the appropriate margin). Their only effect comes from avoiding overlaps. That is, they affect the behaviour of padding in the same way that a low note on a staff does. http://codereview.appspot.com/2758042/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:775: TODO: this variable is used, but I don't know what it does. -pm The penalty for having a blank page after the end of one score and before the next. By default, this is smaller than blank-page-force, so that we prefer blank pages after scores to blank pages within a score. http://codereview.appspot.com/2758042/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:854: @funindex page-limit-inter-system-space I don't think this exists any more... http://codereview.appspot.com/2758042/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:861: @funindex page-limit-inter-system-space-factor I think this is gone too
Sign in to reply to this message.
http://codereview.appspot.com/2758042/diff/1/Documentation/notation/spacing.i... File Documentation/notation/spacing.itely (right): http://codereview.appspot.com/2758042/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:209: by setting @code{paper-height} or @code{paper-width}, even though Too much warning for a user to remember unless he has already had the problem. Consider just identifying the affected dimensions, as at line 226, then "Note that the set-*-paper-size functions will scale all these variables, even those that you set yourself earlier in the \paper block. In most cases you want to set these variables after set-*-paper-size" http://codereview.appspot.com/2758042/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:275: @c TODO: why? -mp On 2010/10/28 13:29:57, Carl wrote: > [rather] than spread out among all the staves in the system. This space can also be put between the two or three systems, where it is helpful, and 2.13 can do a good job with an override. The same advice existed in the 2.12 manual; I learned to disregard it. http://codereview.appspot.com/2758042/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:280: @c TODO: pieces that amply fill two pages... "true"->"false"? -mp Yes to the change. ragged-last-bottom=##f for the amply-filling case http://codereview.appspot.com/2758042/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:303: Inter-system spacing is controlled by grob properties, with Inter means between. Intra means within. They are often confused, and I think this is the wrong, one but it strains my brain too much to figure out. I strongly recommend Within- / Between- in general. http://codereview.appspot.com/2758042/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:339: @item @code{stretchability} -- a unitless measure of the Drop 'unitless' One word that could be a big distraction. I read the thread and know what you mean, but users don't care. In the analogous TeX glue, stretchability has units of length and the pulling force is quietly taken to be dimensionless. Note that the default value (space - minimum-distance) has units of staff spaces. http://codereview.appspot.com/2758042/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:854: @funindex page-limit-inter-system-space Confirmed that setting it has no effect in 2.13.37, while it did in 2.12.3 http://codereview.appspot.com/2758042/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:861: @funindex page-limit-inter-system-space-factor confirming, gone
Sign in to reply to this message.
http://codereview.appspot.com/2758042/diff/1/Documentation/notation/spacing.i... File Documentation/notation/spacing.itely (right): http://codereview.appspot.com/2758042/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:275: @c TODO: why? -mp On 2010/10/28 13:29:57, Carl wrote: > Because it's much nicer in a sparsely-populated page to have the space at the > bottom of the page than spread out among all the staves in the system. Now I see Carl's point. I was used to 2.12, where the two settings of ragged-bottom chose whether the extra space went between *systems* or at the bottom, either of which gives a decent page. 2.13 (without adjustment) puts so much of the extra space between staves, that orchestral scores require ragged-bottom=#t for reasonable output. Long way of saying, that it is safest to leave this advice in place.
Sign in to reply to this message.
Mark, I wanted to flag a bit of cleanup, and this reasonably-related patch still shows as 'open'. http://codereview.appspot.com/2758042/diff/1/Documentation/notation/spacing.i... File Documentation/notation/spacing.itely (right): http://codereview.appspot.com/2758042/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:1050: \override VerticalAxisGroup #'minimum-Y-extent = #'(-6 . 6) The minimum-Y-extent of a VerticalAxisGroup no longer has effect, by my test, which was prompted by Mark's catch of the convert-ly bug in re issue 1299. (Other objects still use minimum-Y-extent.) Suggest simply cutting this one \context block, because your other doc patch (line 1684) contains an example showing the new spacing method in better context, and re-enforces the use of \layout.
Sign in to reply to this message.
Guys, I totally reorganized things yet again, and added a lot of useful info. I am aware that many of your previous comments have not been addressed yet (I'm not ignoring them). But I'd like to know what you guys think of the new text and organization. This is still a work in progress.... Thanks. - Mark
Sign in to reply to this message.
It looks better. I added a couple of minor comments which I noticed in passing, but I've not checked any of the detail. Trevor http://codereview.appspot.com/2758042/diff/19002/Documentation/notation/spaci... File Documentation/notation/spacing.itely (right): http://codereview.appspot.com/2758042/diff/19002/Documentation/notation/spaci... Documentation/notation/spacing.itely:278: declarations. I don't think we need repeat this. http://codereview.appspot.com/2758042/diff/19002/Documentation/notation/spaci... Documentation/notation/spacing.itely:530: There are a few variables that determine the horizontal dimensions Lose "There are a few "
Sign in to reply to this message.
Patch set 4 uploaded. - Mark
Sign in to reply to this message.
LGTM Trevor
Sign in to reply to this message.
Looks ok. http://codereview.appspot.com/2758042/diff/23001/Documentation/notation/input... File Documentation/notation/input.itely (right): http://codereview.appspot.com/2758042/diff/23001/Documentation/notation/input... Documentation/notation/input.itely:759: @snippets Err, the @snippets is used for "selected snippets", i.e. links to stuff in Documentation/snippets/. The following stuff aren't snippets -- are you intending to turn them into snippets in the future? I think this doc portion is fine as it is; just remove the @snippets line and leave the rest as it is.
Sign in to reply to this message.
Hi guys, Patch set 5 finally makes the changes that Carl requested from the beginning: it's better to refer to a file where default values are defined, than to list them in the property descriptions. I've now done this, and I think this thing is ready to commit. However, I'd like to point out that I tried to organize paper-defaults-init.ly a little bit, and I need to know if I recklessly broke anything there by changing the order of the declarations. Can someone confirm that it's fine? Also, I should probably break this up into several smaller commits since it's more than 1600 lines long. - Mark
Sign in to reply to this message.
LGTM. THanks, Mark. Carl http://codereview.appspot.com/2758042/diff/35001/Documentation/notation/spaci... File Documentation/notation/spacing.itely (right): http://codereview.appspot.com/2758042/diff/35001/Documentation/notation/spaci... Documentation/notation/spacing.itely:112: scores. Settings that can appear in a @code{\paper} block Do settings in a \paper block inside a \book block apply to the entire document, or just to the entire book? Alternatively, do you consider a \book to be a document? http://codereview.appspot.com/2758042/diff/35001/Documentation/notation/spaci... Documentation/notation/spacing.itely:267: The vertical dimensions affected by automatic scaling are: IMO the colon is not necessary here and should be removed.
Sign in to reply to this message.
On 2010/11/18 09:18:27, Mark Polesky wrote: > However, I'd like to point out that I tried to organize > paper-defaults-init.ly a little bit, and I need to know if I > recklessly broke anything there by changing the order of the > declarations. Can someone confirm that it's fine? Have you done a regtest check comparison? If the regtest checker found no problems, then you're off the hook. If the regtest check shows some changes when you didn't expect any, then you're hooked. http://lilypond.org/doc/v2.13/Documentation/contributor/identifying-code-regr...
Sign in to reply to this message.
http://codereview.appspot.com/2758042/diff/35001/Documentation/notation/input... File Documentation/notation/input.itely (right): http://codereview.appspot.com/2758042/diff/35001/Documentation/notation/input... Documentation/notation/input.itely:774: @code{make-footer} and @code{make-header}, defined in @code{make-header} and @code{make-footer} http://codereview.appspot.com/2758042/diff/35001/Documentation/notation/input... Documentation/notation/input.itely:776: @file{ly/paper-defaults.ly} and @file{ly/titling-init.ly}. @file{ly/paper-defaults-init.ly} http://codereview.appspot.com/2758042/diff/35001/Documentation/notation/input... Documentation/notation/input.itely:782: creates the actual page given the system to put on it. This seems to be seriously out of date; neither function exists (and while both are present in 2.8, they're commented out). http://codereview.appspot.com/2758042/diff/35001/Documentation/notation/input... Documentation/notation/input.itely:793: \bold \fontsize #3 \on-the-fly #print-page-number-check-first indent http://codereview.appspot.com/2758042/diff/35001/Documentation/notation/input... Documentation/notation/input.itely:794: \fromproperty #'page:page-number-string @} @} newline for @} @} http://codereview.appspot.com/2758042/diff/35001/Documentation/notation/input... Documentation/notation/input.itely:796: \bold \fontsize #3 \on-the-fly #print-page-number-check-first indent http://codereview.appspot.com/2758042/diff/35001/Documentation/notation/input... Documentation/notation/input.itely:797: \fromproperty #'page:page-number-string @} @} newline for @} @} http://codereview.appspot.com/2758042/diff/35001/Documentation/notation/spaci... File Documentation/notation/spacing.itely (right): http://codereview.appspot.com/2758042/diff/35001/Documentation/notation/spaci... Documentation/notation/spacing.itely:116: @item the @code{set-paper-size} scheme function, @item the @code{set-paper-size} scheme function, @item ... etc. http://codereview.appspot.com/2758042/diff/35001/Documentation/notation/spaci... Documentation/notation/spacing.itely:278: @code{paper-height} is @code{297 \mm} and the @code{paper-width} 297\mm http://codereview.appspot.com/2758042/diff/35001/Documentation/notation/spaci... Documentation/notation/spacing.itely:279: is @code{210 \mm}. 210\mm http://codereview.appspot.com/2758042/diff/35001/Documentation/notation/spaci... Documentation/notation/spacing.itely:321: If set to true, systems will not spread vertically across the `vertically across' sounds a bit strange to me; I think I'd prefer something like `vertically down'. http://codereview.appspot.com/2758042/diff/35001/Documentation/notation/spaci... Documentation/notation/spacing.itely:348: The titles (from the @code{\header@{@}} section) are treated as a @code{\header} block http://codereview.appspot.com/2758042/diff/35001/Documentation/notation/spaci... Documentation/notation/spacing.itely:438: re-defines the variable: redefines http://codereview.appspot.com/2758042/diff/35001/Documentation/notation/spaci... Documentation/notation/spacing.itely:555: @item paper-width @item paper-width etc. http://codereview.appspot.com/2758042/diff/35001/Documentation/notation/spaci... Documentation/notation/spacing.itely:646: @item two-sided @item two-sided etc. http://codereview.appspot.com/2758042/diff/35001/Documentation/notation/spaci... Documentation/notation/spacing.itely:748: @item max-systems-per-page @item max-systems-per-page etc. http://codereview.appspot.com/2758042/diff/35001/Documentation/notation/spaci... Documentation/notation/spacing.itely:901: in the output. Normally only the piece and opus header variables @code{piece} and @code{opus} http://codereview.appspot.com/2758042/diff/35001/Documentation/notation/spaci... Documentation/notation/spacing.itely:972: @item the @code{layout-set-staff-size} scheme function, @item the @code{layout-set-staff-size} scheme function, etc. http://codereview.appspot.com/2758042/diff/35001/Documentation/notation/spaci... Documentation/notation/spacing.itely:985: @item @code{ragged-right} @item @code{ragged-right} http://codereview.appspot.com/2758042/diff/35001/Documentation/notation/spaci... Documentation/notation/spacing.itely:997: \context @{ \Staff \context @{ \Staff http://codereview.appspot.com/2758042/diff/35001/Documentation/notation/spaci... Documentation/notation/spacing.itely:998: \override VerticalAxisGroup #'minimum-Y-extent = #'(-6 . 6) replace http://codereview.appspot.com/2758042/diff/35001/Documentation/notation/spaci... Documentation/notation/spacing.itely:1000: \context @{ \Voice \context @{ \Voice http://codereview.appspot.com/2758042/diff/35001/ly/paper-defaults-init.ly File ly/paper-defaults-init.ly (right): http://codereview.appspot.com/2758042/diff/35001/ly/paper-defaults-init.ly#ne... ly/paper-defaults-init.ly:54: top-margin-default = 5 \mm % scaled to paper-size 5\mm etc. http://codereview.appspot.com/2758042/diff/35001/ly/paper-defaults-init.ly#ne... ly/paper-defaults-init.ly:59: % TODO: Can these two be removed now? -mp yes http://codereview.appspot.com/2758042/diff/35001/ly/paper-defaults-init.ly#ne... ly/paper-defaults-init.ly:141: '((font-family . feta) (font-encoding . fetaMusic))) indent
Sign in to reply to this message.
Content looks fine. A few typographical comments. http://codereview.appspot.com/2758042/diff/35001/Documentation/notation/spaci... File Documentation/notation/spacing.itely (left): http://codereview.appspot.com/2758042/diff/35001/Documentation/notation/spaci... Documentation/notation/spacing.itely:793: @seealso Ordering http://codereview.appspot.com/2758042/diff/35001/Documentation/notation/spaci... File Documentation/notation/spacing.itely (right): http://codereview.appspot.com/2758042/diff/35001/Documentation/notation/spaci... Documentation/notation/spacing.itely:332: If set to false, systems will spread vertically across the last "vertically across" again http://codereview.appspot.com/2758042/diff/35001/Documentation/notation/spaci... Documentation/notation/spacing.itely:522: @seealso Order in @seealso should be NR, Snippets, Installed Files http://codereview.appspot.com/2758042/diff/35001/Documentation/notation/spaci... Documentation/notation/spacing.itely:555: @item paper-width No. @item paper-width is correct within @table http://codereview.appspot.com/2758042/diff/35001/Documentation/notation/spaci... Documentation/notation/spacing.itely:646: @item two-sided No, @item two-sided is correct within @table http://codereview.appspot.com/2758042/diff/35001/Documentation/notation/spaci... Documentation/notation/spacing.itely:722: @seealso Ordering http://codereview.appspot.com/2758042/diff/35001/Documentation/notation/spaci... Documentation/notation/spacing.itely:748: @item max-systems-per-page No. @item max-systems-per-page is correct within @table http://codereview.appspot.com/2758042/diff/35001/Documentation/notation/spaci... Documentation/notation/spacing.itely:835: @seealso Ordering
Sign in to reply to this message.
This last patch set (#6) assumes this patch: http://codereview.appspot.com/3199041/ I'm leaving most of the @item stuff as is. We can always debate it more, but it doesn't need to hold up this patch. - Mark http://codereview.appspot.com/2758042/diff/35001/Documentation/notation/spaci... File Documentation/notation/spacing.itely (right): http://codereview.appspot.com/2758042/diff/35001/Documentation/notation/spaci... Documentation/notation/spacing.itely:522: @seealso On 2010/11/20 09:17:47, Trevor Daniels wrote: > Order in @seealso should be NR, Snippets, Installed Files CG says: NR, Installed Files, Snippets, IR. Is that wrong?
Sign in to reply to this message.
On 2010/11/20 19:34:44, Mark Polesky wrote: > CG says: NR, Installed Files, Snippets, IR. > Is that wrong? Hhm. Seems like I mis-remembered where Installed Files goes. I'd argue it's wrong though, if the order is based on easiest to hardest, or on recommended order of reference - that's why I thought Installed Files should go at the end. I'm happy with the other changes. Trevor
Sign in to reply to this message.
Patch set 7. We're down to nitpicks here... I can't really think of a good way to break this up, except into 2 commits, the 2nd being far larger than the first. 1) Organize paper-defaults-init.ly. 2) Doc: NR 4.1, 4.2: Reorganize, clarify details. ---------------------------------------------- Reorganize node structure. Organize \paper variables. Revise variable descriptions. Explain \paper and \layout blocks. Improve examples. Clean up. The second commit will be almost 1600 lines long. Okay to push? - Mark
Sign in to reply to this message.
Looks good to go to me Trevor
Sign in to reply to this message.
On 2010/11/21 08:55:01, Trevor Daniels wrote: > Looks good to go to me > > Trevor Patch pushed and issue closed. Thanks to everyone who helped out. This is a big change and I'm glad to see it finished! - Mark
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
