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

Issue 6098050: Let rhythmic-engraver make its articulation-or-event decision based on current listeners (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 11 months ago by dak
Modified:
11 years, 10 months ago
CC:
lilypond-devel_gnu.org
Base URL:
http://git.savannah.gnu.org/gitweb/?p=lilypond.git/trunk/
Visibility:
Public.

Description

Let rhythmic-engraver make its articulation-or-event decision based on current listeners This removes the dependency of the rhythmic engraver on a static list of unlistened event classes and thus is part of work on issue 2449. As one effect, string numbers on isolated notes in Voice contexts are now typeset by default since the string numbers have no listener in Voice contexts even though they would have one in TabVoice.

Patch Set 1 #

Patch Set 2 : Adapt documentation to behavior #

Total comments: 8

Patch Set 3 : Rewrite changes.tely, some code formatting changes. #

Patch Set 4 : Stop Fingering_engraver from sabotaging \rightHandFinger #

Patch Set 5 : Document string/finger number order peculiarity #

Unified diffs Side-by-side diffs Delta from patch set Stats (+77 lines, -69 lines) Patch
M Documentation/changes.tely View 1 2 1 chunk +19 lines, -20 lines 0 comments Download
M Documentation/notation/fretted-strings.itely View 1 2 3 4 5 chunks +29 lines, -26 lines 0 comments Download
M lily/dispatcher.cc View 1 chunk +21 lines, -0 lines 0 comments Download
M lily/fingering-engraver.cc View 1 2 3 2 chunks +0 lines, -12 lines 0 comments Download
M lily/include/dispatcher.hh View 1 chunk +1 line, -0 lines 0 comments Download
M lily/rhythmic-music-iterator.cc View 1 chunk +5 lines, -10 lines 0 comments Download
M scm/define-event-classes.scm View 1 2 3 1 chunk +2 lines, -1 line 0 comments Download

Messages

Total messages: 3
Graham Percival
LGTM http://codereview.appspot.com/6098050/diff/2001/Documentation/changes.tely File Documentation/changes.tely (right): http://codereview.appspot.com/6098050/diff/2001/Documentation/changes.tely#newcode170 Documentation/changes.tely:170: Another consequence is that string numbers and right ...
11 years, 11 months ago (2012-04-23 03:43:13 UTC) #1
pkx166h
http://codereview.appspot.com/6098050/diff/2001/Documentation/notation/fretted-strings.itely File Documentation/notation/fretted-strings.itely (right): http://codereview.appspot.com/6098050/diff/2001/Documentation/notation/fretted-strings.itely#newcode521 Documentation/notation/fretted-strings.itely:521: \new Voice \with { \override StringNumber #'stencil = ##f ...
11 years, 11 months ago (2012-04-23 05:36:32 UTC) #2
dak
11 years, 11 months ago (2012-04-23 06:51:04 UTC) #3
http://codereview.appspot.com/6098050/diff/2001/Documentation/changes.tely
File Documentation/changes.tely (right):

http://codereview.appspot.com/6098050/diff/2001/Documentation/changes.tely#ne...
Documentation/changes.tely:170: Another consequence is that string numbers and
right hand fingerings on
On 2012/04/23 03:43:13, Graham Percival wrote:
> IMO each @item should be self-contained, and multi-paragraph items are the way
> to go if there's multiple implications of a single change.  Could this (and
the
> previous @item) just be additional paragraphs (i.e. remove the @item).

Can do.  Problem is that this change, programmatically a side effect, is rather
important to the user on its own.  If you want to have the items more
self-contained, they can be either written completely independently, or the
EventChord thing can end with "The next two items are also consequences of this
change."

It's sort of a "you will likely consider this a nuisance when writing your own
code, but there were good reasons for it" reasoning.  Maybe the changes file is
not the place for defending the decisions we (TM) made.

Anyway, I think this is important enough for the user-visible consequences to
warrant individual items.  Should I mention the relation in the lead-up, or just
drop it?

http://codereview.appspot.com/6098050/diff/2001/Documentation/notation/frette...
File Documentation/notation/fretted-strings.itely (left):

http://codereview.appspot.com/6098050/diff/2001/Documentation/notation/frette...
Documentation/notation/fretted-strings.itely:103: @warning{String numbers
@strong{must} be defined inside a chord
On 2012/04/23 03:43:13, Graham Percival wrote:
> awesome change.

Actually, an unexpected side effect of turning the event class hierarchy away
from being a compile-time constant.

http://codereview.appspot.com/6098050/diff/2001/Documentation/notation/frette...
File Documentation/notation/fretted-strings.itely (right):

http://codereview.appspot.com/6098050/diff/2001/Documentation/notation/frette...
Documentation/notation/fretted-strings.itely:521: \new Voice \with { \override
StringNumber #'stencil = ##f } {
On 2012/04/23 03:43:13, Graham Percival wrote:
> our vague almost-certainly-unwritten guidelines on lilypond formatting would
> suggest that the \override should be on a newline, but I can't be bothered to
> complain about this.

Well, this is not a music override but a context modification.  I'll check a bit
around for style.

http://codereview.appspot.com/6098050/diff/2001/Documentation/notation/frette...
Documentation/notation/fretted-strings.itely:527: \new TabStaff \with {
stringTunings = #bass-tuning } {
This would have to be formatted differently as well if I decided to change the
above.
Sign in to reply to this message.

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