I think this is an artifact of your patchy - i didn't get any warnings, neither with master nor with my patch.
I'm setting this patch to push, as it has already been on the countdown for 5th July (the latest change is just a whitespace change and property reordering, it doesn't change anything in the logic itself).
Labels: -Patch-review Patch-push
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
* commit [r0bc7f77ff63d9aa044f7d75f9cce255ed2afc0f2]
|\ Merge: f4b89d7 d6604b0
| | Author: Janek Warchoł <lemniskata.bernoullego@gmail.com>
| | Date: Sat Jul 5 23:11:40 2014 +0200
| |
| | Issue 3978: Merge alignment cleanup
| |
| | Make DynamicTexts, TextScripts and many other grobs use aligned_on_parent,
| | which makes their behaviour consitent, predictable and thus more user-friendly.
| |
| * commit [rd6604b0444dfea8c1ad571273c78222ad17ccd2d]
| | Author: Janek Warchoł <lemniskata.bernoullego@gmail.com>
| | Date: Fri Jul 4 22:42:08 2014 +0200
| |
| | define-grobs.scm: reorder properties alphabetically
| |
| * commit [r6f3f8f0fceed3b318c2572337e7dca83e4a11f6c]
| | Author: Janek Warchoł <lemniskata.bernoullego@gmail.com>
| | Date: Mon Jun 23 23:30:49 2014 +0200
| |
| | TextScript, CombineTextScript: use aligned_on_parent
| |
| | I think this makes more sense than just using "self-alignment".
| | It makes these grobs consistent with Lyrics and Dynamics, and
| | allows users to override their alignment more predictably.
| |
| | Expected changes in output: TextScripts and CombineTextScripts
| | will be aligned slightly different when the user sets self-alignment-X
| | explicitly (parent notehead will be included in alignment, as in Lyrics).
| | Default placement should remain the same.
| |
| | I'm also changing one regtest, just to make sure that despite slightly
| | changed alignment the markups will stick out far enough to the left.
| |
| * commit [r1d765020f867995879c761f2c9351c9dc74f1df8]
| | Author: Janek Warchoł <lemniskata.bernoullego@gmail.com>
| | Date: Mon Jun 23 23:04:26 2014 +0200
| |
| | Replace XY-offset closures with aligned_on_parent where possible
| |
| | It doesn't make sense to specify placement using multiple added
| | callbacks or closures when we can use aligned_on_parent. What's
| | more, aligned_on_parent should expose more consistent interface
| | for the users - affected grobs should now behave similarly to
| | LyricTexts and DynamicTexts. For example, this
| |
| | \relative c'' \context Voice {
| | \compressFullBarRests
| | \override MultiMeasureRestNumber.self-alignment-X = #RIGHT
| | [r1]*100
| | }
| |
| | will result in right edge of the MultiMeasureRestNumber being
| | aligned on the right edge of the MultiMeasureRest.
| |
| | Affected grobs:
| | - AccidentalSuggestion
| | - ClefModifier
| | - DoublePercentRepeatCounter
| | - Fingering
| | - GridLine
| | - MultiMeasureRestNumber
| | - MultiMeasureRestText
| | - PercentRepeatCounter
| | - StemTremolo
| |
| | Expected changes in output: none.
| |
| * commit [r09412c25cce3fdffce7cefe491cb3849ece0ae3e]
|/ Author: Janek Warchoł <lemniskata.bernoullego@gmail.com>
| Date: Sat Mar 30 00:11:28 2013 +0100
|
| Clean up DynamicText horizontal alignment.
|
| Until now, DynamicText alignment was messy, as there were 3 different
| callbacks involved - some of them interacting in a confusing way:
| - in define-grobs.scm, X-offset property was initialized to
| ly:self-alignment-interface::x-aligned-on-self,
| - Dynamic_engraver called set_center_parent on every DynamicText
| attached to a note, so that half a NoteHead width was always added
| to its X-offset (producing confusing results for example when
| user requested dynamics to be left-aligned),
| - DynamicTexts living in a Dynamics context used a completely
| different offset callback, which aligned them on NoteColumns.
|
| Since aligned_on_parent is now able to correctly align grobs
| with PaperColumn parents (issue 3254), we can use it for all
| DynamicTexts and have a single interface for the job.
|
| Expected changes in output: DynamicTexts in Dynamics context
| aligned to suspended noteheads may be placed up to 1/4 NoteHead
| width further to the right. This shouldn't be a problem.
|
Labels: -Patch-push Fixed_2_19_10 Status: Fixed
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Originally posted by: janek.li...@gmail.com
(No comment was entered for this change.)
Owner: janek.li...@gmail.com
Originally posted by: pkx1...@gmail.com
Passes make, make check and a full make doc.
However make test shows all the chord labels left aligned on the reg tests (they were centered before).
See attached. This still needs some work.
Labels: -Patch-new Patch-needs_work
Originally posted by: janek.li...@gmail.com
keep old value for TextScript alignment (fix fingering diagrams)
http://codereview.appspot.com/105410046
Summary: Patch: cleanup alignments of various grobs (follow-up to 3254 and 3962)
Labels: -Patch-needs_work Patch-new
Originally posted by: janek.li...@gmail.com
(No comment was entered for this change.)
Blocking: lilypond:3979
Originally posted by: pkx1...@gmail.com
Passes make, make check and a full make doc.
One reg test diff (attached)
Labels: -Patch-new Patch-review
Originally posted by: janek.li...@gmail.com
This change is expected, and as far as I know it doesn't make the regtest unable to detect the bug it was originally written for.
Labels: -Type-Enhancement Type-Defect
Originally posted by: janek.li...@gmail.com
rebase on master, slightly adjust one regtest
http://codereview.appspot.com/105410046
Labels: -Patch-review Patch-new
Originally posted by: pkx1...@gmail.com
Passes make, make check and a full make doc - reg test diff attached
Labels: -Patch-new Patch-review
Originally posted by: janek.li...@gmail.com
yes, the change is exactly as expected.
Originally posted by: janek.li...@gmail.com
(No comment was entered for this change.)
Blocking: lilypond:2245
Originally posted by: pkx1...@gmail.com
Patch on countdown for July 5th 2014
Labels: -Patch-review Patch-countdown
Originally posted by: janek.li...@gmail.com
whitespace fixes and property reordering (shouldn't affect countdown)
http://codereview.appspot.com/105410046
Labels: -Patch-countdown Patch-new
Originally posted by: pkx1...@gmail.com
Passes make, make check and a full make doc.
I also noticed this:
--snip--
Writing /tmp/build-lilypond-autobuild/out/lybook-testdb/e1/lily-69780993-systems.tex...
Writing /tmp/build-lilypond-autobuild/out/lybook-testdb/e1/lily-69780993-systems.count...
Writing timing to e1/lily-69780993.profile...
-programming error: Parsed object should be dead: #<Grob Slur >
-continuing, cross fingers
-programming error: Parsed object should be dead: #<Context_def Score /tmp/lilypond-autobuild/input/regression/mozart-hrn3-defs.ily:10:3>
-continuing, cross fingers
-programming error: Parsed object should be dead: #<Prob: Music C++: Music((origin . #<location /tmp/lilypond-autobuild/input/regression/mozart-hrn3-rondo.ily:70:13>) (span-direction . 1))((display-methods #<procedure #f (event parser)>) (name . BeamEvent) (types general-music post-event event beam-event span-event)) >
-
-continuing, cross fingers
input/regression/mozart-hrn-3.log
--snip--
Could be an artefact of my Patchy and they are '-' so are not being added on this run, but I thought it might be interesting to someone.
Anyway, the reg test diff is the same as comment #8
Labels: -Patch-new Patch-review
Originally posted by: janek.li...@gmail.com
I think this is an artifact of your patchy - i didn't get any warnings, neither with master nor with my patch.
I'm setting this patch to push, as it has already been on the countdown for 5th July (the latest change is just a whitespace change and property reordering, it doesn't change anything in the logic itself).
Labels: -Patch-review Patch-push
Originally posted by: janek.li...@gmail.com
Pushed as merge commit:
* commit [r0bc7f77ff63d9aa044f7d75f9cce255ed2afc0f2]
|\ Merge: f4b89d7 d6604b0
| | Author: Janek Warchoł <lemniskata.bernoullego@gmail.com>
| | Date: Sat Jul 5 23:11:40 2014 +0200
| |
| | Issue 3978: Merge alignment cleanup
| |
| | Make DynamicTexts, TextScripts and many other grobs use aligned_on_parent,
| | which makes their behaviour consitent, predictable and thus more user-friendly.
| |
| * commit [rd6604b0444dfea8c1ad571273c78222ad17ccd2d]
| | Author: Janek Warchoł <lemniskata.bernoullego@gmail.com>
| | Date: Fri Jul 4 22:42:08 2014 +0200
| |
| | define-grobs.scm: reorder properties alphabetically
| |
| * commit [r6f3f8f0fceed3b318c2572337e7dca83e4a11f6c]
| | Author: Janek Warchoł <lemniskata.bernoullego@gmail.com>
| | Date: Mon Jun 23 23:30:49 2014 +0200
| |
| | TextScript, CombineTextScript: use aligned_on_parent
| |
| | I think this makes more sense than just using "self-alignment".
| | It makes these grobs consistent with Lyrics and Dynamics, and
| | allows users to override their alignment more predictably.
| |
| | Expected changes in output: TextScripts and CombineTextScripts
| | will be aligned slightly different when the user sets self-alignment-X
| | explicitly (parent notehead will be included in alignment, as in Lyrics).
| | Default placement should remain the same.
| |
| | I'm also changing one regtest, just to make sure that despite slightly
| | changed alignment the markups will stick out far enough to the left.
| |
| * commit [r1d765020f867995879c761f2c9351c9dc74f1df8]
| | Author: Janek Warchoł <lemniskata.bernoullego@gmail.com>
| | Date: Mon Jun 23 23:04:26 2014 +0200
| |
| | Replace XY-offset closures with aligned_on_parent where possible
| |
| | It doesn't make sense to specify placement using multiple added
| | callbacks or closures when we can use aligned_on_parent. What's
| | more, aligned_on_parent should expose more consistent interface
| | for the users - affected grobs should now behave similarly to
| | LyricTexts and DynamicTexts. For example, this
| |
| | \relative c'' \context Voice {
| | \compressFullBarRests
| | \override MultiMeasureRestNumber.self-alignment-X = #RIGHT
| | [r1]*100
| | }
| |
| | will result in right edge of the MultiMeasureRestNumber being
| | aligned on the right edge of the MultiMeasureRest.
| |
| | Affected grobs:
| | - AccidentalSuggestion
| | - ClefModifier
| | - DoublePercentRepeatCounter
| | - Fingering
| | - GridLine
| | - MultiMeasureRestNumber
| | - MultiMeasureRestText
| | - PercentRepeatCounter
| | - StemTremolo
| |
| | Expected changes in output: none.
| |
| * commit [r09412c25cce3fdffce7cefe491cb3849ece0ae3e]
|/ Author: Janek Warchoł <lemniskata.bernoullego@gmail.com>
| Date: Sat Mar 30 00:11:28 2013 +0100
|
| Clean up DynamicText horizontal alignment.
|
| Until now, DynamicText alignment was messy, as there were 3 different
| callbacks involved - some of them interacting in a confusing way:
| - in define-grobs.scm, X-offset property was initialized to
| ly:self-alignment-interface::x-aligned-on-self,
| - Dynamic_engraver called set_center_parent on every DynamicText
| attached to a note, so that half a NoteHead width was always added
| to its X-offset (producing confusing results for example when
| user requested dynamics to be left-aligned),
| - DynamicTexts living in a Dynamics context used a completely
| different offset callback, which aligned them on NoteColumns.
|
| Since aligned_on_parent is now able to correctly align grobs
| with PaperColumn parents (issue 3254), we can use it for all
| DynamicTexts and have a single interface for the job.
|
| Expected changes in output: DynamicTexts in Dynamics context
| aligned to suspended noteheads may be placed up to 1/4 NoteHead
| width further to the right. This shouldn't be a problem.
|
Labels: -Patch-push Fixed_2_19_10
Status: Fixed
Originally posted by: fedel...@gmail.com
(No comment was entered for this change.)
Status: Verified
Originally posted by: dak@gnu.org
Please see issue 4036 for a proposed revert of the merge commit listed in issue #15.
Originally posted by: k-ohara5...@oco.net
Shucks. This change makes sense, but the documentation uses the old behavior.
<http://lilypond.org/doc/v2.19/Documentation/notation/aligning-objects#using-the-self_002dalignment_002dinterface>
<http://www.lilypond.org/doc/v2.19/Documentation/learning/fixing-overlapping-notation#the-self_002dalignment_002dx-property>
and maybe more. Search self-alignment-X in /Documentation/ and the LSR
This change would need the docs update, and preferably a convert-ly rule or message.
Originally posted by: janek.li...@gmail.com
(No comment was entered for this change.)
Labels: Janek_alignment_changes