Menu

#3978 Patch: cleanup alignments of various grobs (follow-up to 3254 and 3962)

Verified
nobody
Defect
2014-08-09
2014-06-28
Anonymous
No

Originally created by: *anonymous

Originally created by: janek.li...@gmail.com
Originally owned by: janek.li...@gmail.com

rebase on master, don't include aligned_on_parent refactoring

http://codereview.appspot.com/105410046

Discussion

  • Google Importer

    Google Importer - 2014-06-28

    Originally posted by: janek.li...@gmail.com

    (No comment was entered for this change.)

    Owner: janek.li...@gmail.com

     
  • Google Importer

    Google Importer - 2014-06-28

    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

     
  • Google Importer

    Google Importer - 2014-06-28

    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

     
  • Google Importer

    Google Importer - 2014-06-28

    Originally posted by: janek.li...@gmail.com

    (No comment was entered for this change.)

    Blocking: lilypond:3979

     
  • Google Importer

    Google Importer - 2014-06-29

    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

     
  • Google Importer

    Google Importer - 2014-06-29

    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

     
  • Google Importer

    Google Importer - 2014-06-29

    Originally posted by: janek.li...@gmail.com

    rebase on master, slightly adjust one regtest

    http://codereview.appspot.com/105410046

    Labels: -Patch-review Patch-new

     
  • Google Importer

    Google Importer - 2014-06-30

    Originally posted by: pkx1...@gmail.com

    Passes make, make check and a full make doc - reg test diff attached

    Labels: -Patch-new Patch-review

     
  • Google Importer

    Google Importer - 2014-06-30

    Originally posted by: janek.li...@gmail.com

    yes, the change is exactly as expected.

     
  • Google Importer

    Google Importer - 2014-06-30

    Originally posted by: janek.li...@gmail.com

    (No comment was entered for this change.)

    Blocking: lilypond:2245

     
  • Google Importer

    Google Importer - 2014-07-02

    Originally posted by: pkx1...@gmail.com

    Patch on countdown for July 5th 2014

    Labels: -Patch-review Patch-countdown

     
  • Google Importer

    Google Importer - 2014-07-04

    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

     
  • Google Importer

    Google Importer - 2014-07-04

    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

     
  • Google Importer

    Google Importer - 2014-07-05

    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

     
  • Google Importer

    Google Importer - 2014-07-05

    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

     
  • Google Importer

    Google Importer - 2014-07-18

    Originally posted by: fedel...@gmail.com

    (No comment was entered for this change.)

    Status: Verified

     
  • Google Importer

    Google Importer - 2014-07-28

    Originally posted by: dak@gnu.org

    Please see issue 4036 for a proposed revert of the merge commit listed in issue #15.

     
  • Google Importer

    Google Importer - 2014-08-09

    Originally posted by: janek.li...@gmail.com

    (No comment was entered for this change.)

    Labels: Janek_alignment_changes

     
MongoDB Logo MongoDB