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

Issue 28134: Fix spacing for accidentals in tied chords (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 3 months ago by csnyder
Modified:
9 years, 7 months ago
Reviewers:
joeneeman
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

Fix spacing for accidentals in tied chords Previously, accidentals that weren't printed because they were tied still took up space in the placement of accidentals in chords. This is noted as issue # 612 in the bug tracker. This patch changes the accidental spacing code to calculate spacing for two sets of accidentals: those that will be printed mid-line, and those that will be printed after a line break, each with the correct spacing for the particular situation.

Patch Set 1 #

Total comments: 11
Unified diffs Side-by-side diffs Delta from patch set Stats (+211 lines, -88 lines) Patch
A input/regression/accidental-tied-chords.ly View 1 chunk +22 lines, -0 lines 0 comments Download
M lily/accidental.cc View 5 chunks +21 lines, -3 lines 1 comment Download
M lily/accidental-engraver.cc View 9 chunks +50 lines, -25 lines 1 comment Download
M lily/accidental-placement.cc View 7 chunks +78 lines, -46 lines 6 comments Download
M lily/include/accidental-placement.hh View 1 chunk +5 lines, -4 lines 0 comments Download
M lily/include/note-head.hh View 1 chunk +1 line, -0 lines 0 comments Download
M lily/ledger-line-spanner.cc View 2 chunks +2 lines, -2 lines 1 comment Download
M lily/new-fingering-engraver.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M lily/note-column.cc View 1 chunk +1 line, -1 line 0 comments Download
M lily/note-head.cc View 3 chunks +18 lines, -1 line 0 comments Download
M lily/tie-formatting-problem.cc View 1 chunk +1 line, -1 line 0 comments Download
M scm/define-grob-properties.scm View 2 chunks +9 lines, -3 lines 2 comments Download
M scm/define-grobs.scm View 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 2
csnyder
This patch implements Joe's suggested approach for correcting the spacing of tied chords with accidentals. ...
10 years, 3 months ago (2009-03-28 16:43:33 UTC) #1
joeneeman
10 years, 3 months ago (2009-03-29 03:42:01 UTC) #2
http://codereview.appspot.com/28134/diff/1/3
File lily/accidental-engraver.cc (right):

http://codereview.appspot.com/28134/diff/1/3#newcode431
Line 431: Grob *newa = 0;
Please use a more descriptive name. Also, add a comment regarding why you need
two copies of the accidental.

http://codereview.appspot.com/28134/diff/1/4
File lily/accidental-placement.cc (right):

http://codereview.appspot.com/28134/diff/1/4#newcode30
Line 30: Accidental_placement::add_accidental (Grob *me, Grob *a, Grob *newa)
Please use more descriptive parameter names.

http://codereview.appspot.com/28134/diff/1/4#newcode71
Line 71: if (break_reminder)
slightly cleaner, perhaps, to do
SCM accidental_sym = ly_symbol2scm(break_reminder ?
"accidental-break-reminder-grobs" : "accidental_grobs");

and then
accs = me->get_object(accidental_sym);
...
me->set_object(accidental_sym, accs);

http://codereview.appspot.com/28134/diff/1/4#newcode97
Line 97: ret.push_back(a);
space before (

http://codereview.appspot.com/28134/diff/1/4#newcode433
Line 433: return;
Don't need the return here

http://codereview.appspot.com/28134/diff/1/4#newcode436
Line 436: void Accidental_placement::filter_tied_accidentals (Grob * me)
Rather than coping every accidental and then filtering out some, have you
considered delaying the copying until calc_positioning_done and only copying the
accidentals you care about? This also has the advantage of hiding more stuff in
accidental-placement instead of modifying accidental-engraver.

http://codereview.appspot.com/28134/diff/1/4#newcode460
Line 460: "accidental-break-reminder-grobs "
Only properties go here, not objects.

http://codereview.appspot.com/28134/diff/1/5
File lily/accidental.cc (right):

http://codereview.appspot.com/28134/diff/1/5#newcode181
Line 181: }
I'm a little confused about this. Depending on whether a group of accidentals is
at the beginning of a line or not, you want either all of the break-reminder
accidentals or all of the non-break-reminder accidentals to suicide. It's not
clear to me that this is achieved by the above code.

http://codereview.appspot.com/28134/diff/1/8
File lily/ledger-line-spanner.cc (right):

http://codereview.appspot.com/28134/diff/1/8#newcode237
Line 237: if (Grob *g = unsmob_grob (me->get_property ("accidental-grob")))
get_object (and subsequently, too)

http://codereview.appspot.com/28134/diff/1/13
File scm/define-grob-properties.scm (right):

http://codereview.appspot.com/28134/diff/1/13#newcode835
Line 835: @var{groblist})} entries. Includes accidentals that will be printed if
at the beginning of the line.")
"Consists of" is more accurate than "Includes", I think.

http://codereview.appspot.com/28134/diff/1/13#newcode841
Line 841: @var{groblist})} entries. Does not include tied accidentals that won't
be printed mid-line.")
Can you rephrase to avoid the double-negative?
Sign in to reply to this message.

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