Re: Fix 1120 in a way to avoid issues 1472, 1474 (issue4095041)
Latest version LGTM. Keith, could you send me the git-format patch privately, so that I can push it in 24 hours if nobody complains? http://codereview.appspot.com/4095041/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix 1120 in a way to avoid issues 1472, 1474 (issue4095041)
LGTM.
http://codereview.appspot.com/4095041/diff/52002/lily/separation-item.cc
File lily/separation-item.cc (right):
http://codereview.appspot.com/4095041/diff/52002/lily/separation-item.cc#newcode83
lily/separation-item.cc:83: double horizon_padding = robust_scm2double
(me-get_property (skyline-vertical-padding), 0.0);
Real horizon_padding
http://codereview.appspot.com/4095041/diff/52002/lily/separation-item.cc#newcode94
lily/separation-item.cc:94: double horizon_padding = robust_scm2double
(me-get_property (skyline-vertical-padding), 0.0);
Real horizon_padding
http://codereview.appspot.com/4095041/diff/52002/scm/define-grob-properties.scm
File scm/define-grob-properties.scm (right):
http://codereview.appspot.com/4095041/diff/52002/scm/define-grob-properties.scm#newcode727
scm/define-grob-properties.scm:727: Y-extents and extra-spacing-heights
of the constituent grobs in the
@code{Y-extent}s and @code{extra-spacing-height}s
http://codereview.appspot.com/4095041/diff/52002/scm/define-grobs.scm
File scm/define-grobs.scm (right):
http://codereview.appspot.com/4095041/diff/52002/scm/define-grobs.scm#newcode1081
scm/define-grobs.scm:1081: (extra-spacing-height . (+inf.0 . -inf.0))
fix indent (tab)
http://codereview.appspot.com/4095041/diff/52002/scm/define-grobs.scm#newcode1161
scm/define-grobs.scm:1161: ; Recede in height for purposes of note
spacing,
;;
http://codereview.appspot.com/4095041/diff/52002/scm/define-grobs.scm#newcode1162
scm/define-grobs.scm:1162: ; so notes in melismata can be freely spaced
above lyrics
;;
http://codereview.appspot.com/4095041/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix 1120 in a way to avoid issues 1472, 1474 (issue4095041)
On 2011/02/02 23:14:39, Neil Puttock wrote: LGTM. http://codereview.appspot.com/4095041/diff/52002/lily/separation-item.cc File lily/separation-item.cc (right): http://codereview.appspot.com/4095041/diff/52002/lily/separation-item.cc#newcode83 lily/separation-item.cc:83: double horizon_padding = robust_scm2double (me-get_property (skyline-vertical-padding), 0.0); Real horizon_padding [...] Thanks. All done. http://codereview.appspot.com/4095041/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix 1120 in a way to avoid issues 1472, 1474 (issue4095041)
On 2011/01/31 19:50:38, Carl wrote: Since the 0.1s are all part of a Separation_item, what if we defined a new grob-property default-separation, and set the value to 0.1 for NonMusicalPaperColumn, NoteColumn, and PaperColumn, since these are the grobs having separation_item_interface? Done; and I like it. Only NoteColumn needed the padding to restore 2.12.3s good note spacing. I gave a nonzero padding to NonMusicalPaperColumn because the grobs in such columns all seem to want the same padding. http://codereview.appspot.com/4095041/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix 1120 in a way to avoid issues 1472, 1474 (issue4095041)
LGTM http://codereview.appspot.com/4095041/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix 1120 in a way to avoid issues 1472, 1474 (issue4095041)
LGTM On Tue, Feb 1, 2011 at 7:49 PM, percival.music...@gmail.com wrote: LGTM http://codereview.appspot.com/4095041/ -- Han-Wen Nienhuys - han...@xs4all.nl - http://www.xs4all.nl/~hanwen ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix 1120 in a way to avoid issues 1472, 1474 (issue4095041)
2 issues: * this hardcodes 0.1 in several places. Make a property, so we can override this * rewrite the commit description so it explains what you are doing (ie. what issues 1120, 1472, 1474 are). The desc. should be understandable without access to the bug database. http://codereview.appspot.com/4095041/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix 1120 in a way to avoid issues 1472, 1474 (issue4095041)
On 2011/01/31 11:20:39, hanwenn wrote: * this hardcodes 0.1 in several places. Make a property, so we can override this Agreed in principle; the relevant property extra-spacing-height should absorb these magic numbers, but I am not willing to do so in this patch. The 0.1s were in the previous stable release; commit ee0488 tried to remove them, but had side effects. This patch reverts ee0488 (thus bringing back the ugly 0.1s) then adjusts a few existing properties to have the desired effects of ee0488. I began looking into absorbing the 0.1s into the extra-spacing-height of all the objects who need it, thus completing the job of ee0488. I concluded that negative extra-spacing-height for Lyrics was necessary for that task, so proposed this patch first. * rewrite the commit description so it explains what you are doing (ie. what issues 1120, 1472, 1474 are). I gave words to the issue numbers on Rietveld; the git commit text will be a shortened version in that style. http://codereview.appspot.com/4095041/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix 1120 in a way to avoid issues 1472, 1474 (issue4095041)
On 2011/01/31 19:36:16, Keith wrote: On 2011/01/31 11:20:39, hanwenn wrote: * this hardcodes 0.1 in several places. Make a property, so we can override this Agreed in principle; the relevant property extra-spacing-height should absorb these magic numbers, but I am not willing to do so in this patch. The 0.1s were in the previous stable release; commit ee0488 tried to remove them, but had side effects. This patch reverts ee0488 (thus bringing back the ugly 0.1s) then adjusts a few existing properties to have the desired effects of ee0488. Since the 0.1s are all part of a Separation_item, what if we defined a new grob-property default-separation, and set the value to 0.1 for NonMusicalPaperColumn, NoteColumn, and PaperColumn, since these are the grobs having separation_item_interface? We could then get the value of default-separation from the paper column, and use that value throughout separation-item.cc I agree that it seems like a bit of a pain to do this when you're just wanting to revert a patch that now appears inappropriate. But if this property were added, no recompilation would be required in order to resolve the issues you've been fighting with here. http://codereview.appspot.com/4095041/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix 1120 in a way to avoid issues 1472, 1474 (issue4095041)
On 2011/01/31 19:50:38, Carl wrote: We could then get the value of default-separation from the paper column, and use that value throughout separation-item.cc I had discounted the new property idea because of all the extra property lookups, but it seems your idea costs only one lookup per Column. For a name, default-clearance might better indicate that this is a vertical clearance maintained while choosing horizontal separation between columns. I agree that it seems like a bit of a pain to do [...] Pain only makes us stronger. However, I decided decades ago that I should avoid learning programming languages where one can redefine the + operator. I did not manage to escape them entirely, but might get stuck trying to pass a new property through all the layers. Let's see. http://codereview.appspot.com/4095041/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix 1120 in a way to avoid issues 1472, 1474 (issue4095041)
On 31 January 2011 20:20, k-ohara5...@oco.net wrote: I had discounted the new property idea because of all the extra property lookups, but it seems your idea costs only one lookup per Column. For a name, default-clearance might better indicate that this is a vertical clearance maintained while choosing horizontal separation between columns. Since we already have skyline-horizontal-padding, why not call it skyline-vertical-padding? Cheers, Neil ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix 1120 in a way to avoid issues 1472, 1474 (issue4095041)
On 2011/01/26 18:18:21, Graham Percival wrote: Neil has identified a potential downside to this patch. That was a restoration of 2.12.3 behavior, which I had mentioned in the original email thread but didn't explicitly handle until now. Status of this patch is 1)revert ee0488, which was a global change in effective extra-spacing-height, /except/ keep from ee0488 the addition of beat-slash on the print-callbacks list 2)reviewed the entire list of Layout Objects to specify extra-spacing-height to restore any possible positive effect of ee0488 3)regression test and see only expected changes http://codereview.appspot.com/4095041/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix 1120 in a way to avoid issues 1472, 1474 (issue4095041)
This patch solves Neil's problem with clef spacing. LGTM. Carl http://codereview.appspot.com/4095041/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix 1120 in a way to avoid issues 1472, 1474 (issue4095041)
On 2011/01/25 04:47:23, Carl wrote: This patch seems to have some very good benefits. Neil has identified a potential downside to this patch. Some kind of additional work is required -- maybe adding some special case to avoid changing clefs in this way, or possibly just an argument that the change is good. http://codereview.appspot.com/4095041/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix 1120 in a way to avoid issues 1472, 1474 (issue4095041)
- Original Message - From: k-ohara5...@oco.net To: k-ohara5...@oco.net Cc: re...@codereview.appspotmail.com; lilypond-devel@gnu.org Sent: Monday, January 24, 2011 5:15 AM Subject: Re: Fix 1120 in a way to avoid issues 1472, 1474 (issue4095041) Extended to cover the other issues that were fixed along with 1120. The regression test that /could/ have caught the breakage of issue 1120 is revised so it will (more likely) catch any future breakage. FWIW had I been running my pixel comparator then, I'd have spotted this easily. -- Phil Holmes ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix 1120 in a way to avoid issues 1472, 1474 (issue4095041)
On 1/24/11 11:15 AM, Bernard Hurley bern...@marcade.biz wrote: Is there any point in anyone working on 1474 before this is resolved? I don't think so. 1120, 1472, and 1474 are intimately connected and will be resolved together. Thanks, Carl ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix 1120 in a way to avoid issues 1472, 1474 (issue4095041)
This patch also resolves the problem in issue 1229 of notes extending beyond the right-hand bar line. Adding additional extra-spacing-height to the TimeSignature grob resolves the 1229 issue of overlapping the time signature. This patch seems to have some very good benefits. http://codereview.appspot.com/4095041/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix 1120 in a way to avoid issues 1472, 1474 (issue4095041)
On Mon, 24 Jan 2011 20:47:24 -0800, carl.d.soren...@gmail.com wrote:
This patch also resolves the problem in issue 1229 of notes extending
beyond the right-hand bar line.
Adding additional extra-spacing-height to the TimeSignature grob
resolves the 1229 issue of overlapping the time signature.
This patch seems to have some very good benefits.
The patch doesn't take any specific action for issues 1472 1474 1229. I'd say it
reduces these other issues to their version-2.12.3 status, in which version
they were un-noticed.
I especially like that it reduces back to 2.12.3-levels the frequency of slurs
confusing note separation { g''8( c''- geses'' a'') }.
___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix 1120 in a way to avoid issues 1472, 1474 (issue4095041)
Extended to cover the other issues that were fixed along with 1120. The regression test that /could/ have caught the breakage of issue 1120 is revised so it will (more likely) catch any future breakage. http://codereview.appspot.com/4095041/diff/32001/scm/define-grobs.scm File scm/define-grobs.scm (right): http://codereview.appspot.com/4095041/diff/32001/scm/define-grobs.scm#newcode261 scm/define-grobs.scm:261: (stencil . ,ly:text-interface::print) On 2011/01/23 04:48:19, Keith wrote: extra-spacing-height . (-0.5 . 0.5) for issue 1138 .. is not required for the regtest that raised issue 1138 (figured-bass-extenders-markup) Also, if the line above is added to FiguredBass, it spaces complicated basso continuo too tightly. Figured Bass is more similar to NoteHeads than it is to Lyrics. http://codereview.appspot.com/4095041/diff/32001/scm/define-grobs.scm#newcode586 scm/define-grobs.scm:586: (extra-spacing-height . (-0.5 . 0.5)) CueClef and CueEndClef were added after ee00488 so a simple revert missed these. They should match Clef. http://codereview.appspot.com/4095041/diff/32001/scm/define-grobs.scm#newcode967 scm/define-grobs.scm:967: (stencil . ,system-start-text::print) InstrumentName goes left of the staff. No analogy with the melismata issue 1120 http://codereview.appspot.com/4095041/diff/32001/scm/define-grobs.scm#newcode1183 scm/define-grobs.scm:1183: (stencil . ,ly:measure-grouping::print) On 2011/01/23 04:48:19, Keith wrote: extra-spacing-height analogous to Lyrics? MeasureGrouping does not work analogously to Lyrics. No need to extend a fix for 1120 here. http://codereview.appspot.com/4095041/diff/42001/input/regression/lyrics-melisma-beam.ly File input/regression/lyrics-melisma-beam.ly (right): http://codereview.appspot.com/4095041/diff/42001/input/regression/lyrics-melisma-beam.ly#newcode17 input/regression/lyrics-melisma-beam.ly:17: g4 d8[ b8 d8 g8] g4 Moved some note heads so their stems interfere with lyrics, so that these notes will move should something like issue 1120 recur. http://codereview.appspot.com/4095041/diff/42001/scm/define-grobs.scm File scm/define-grobs.scm (right): http://codereview.appspot.com/4095041/diff/42001/scm/define-grobs.scm#newcode178 scm/define-grobs.scm:178: (BalloonTextItem Similar to Lyrics in its spacing needs http://codereview.appspot.com/4095041/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix 1120 in a way to avoid issues 1472, 1474 (issue4095041)
Needs adjustments as marked, to handle all the other issues that commit ee0488 fixed : 886 (=819) 1138 and 1137(already handled) and some exactly analogous situations not yet reported. New patch within a day, to come along with reg tests. http://codereview.appspot.com/4095041/diff/32001/scm/define-grobs.scm File scm/define-grobs.scm (right): http://codereview.appspot.com/4095041/diff/32001/scm/define-grobs.scm#newcode261 scm/define-grobs.scm:261: (stencil . ,ly:text-interface::print) extra-spacing-height . (-0.5 . 0.5) for issue 1138 http://codereview.appspot.com/4095041/diff/32001/scm/define-grobs.scm#newcode495 scm/define-grobs.scm:495: (stencil . ,ly:text-interface::print) extra-spacing-height for issue 886 (and 819) http://codereview.appspot.com/4095041/diff/32001/scm/define-grobs.scm#newcode856 scm/define-grobs.scm:856: (stencil . ,fret-board::calc-stencil) extra-spacing-height to avoid analog of issue 886 http://codereview.appspot.com/4095041/diff/32001/scm/define-grobs.scm#newcode967 scm/define-grobs.scm:967: (stencil . ,system-start-text::print) extra-spacing-height analogous to Lyrics? http://codereview.appspot.com/4095041/diff/32001/scm/define-grobs.scm#newcode1183 scm/define-grobs.scm:1183: (stencil . ,ly:measure-grouping::print) extra-spacing-height analogous to Lyrics? http://codereview.appspot.com/4095041/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix 1120 in a way to avoid issues 1472, 1474 (issue4095041)
Regtest comparison shows nothing obviously wrong, and it compiles the docs from scratch. http://codereview.appspot.com/4095041/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
