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

[dak]

Reviewers: Graham Percival, J_lowe,


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 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/fretted-strings.itely
File Documentation/notation/fretted-strings.itely (left):

http://codereview.appspot.com/6098050/diff/2001/Documentation/notation/fretted-strings.itely#oldcode103
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/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 } {
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/fretted-strings.itely#newcode527
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.

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.

Please review this at http://codereview.appspot.com/6098050/

Affected files:
  M Documentation/changes.tely
  M Documentation/notation/fretted-strings.itely
  M lily/dispatcher.cc
  M lily/include/dispatcher.hh
  M lily/rhythmic-music-iterator.cc


Index: Documentation/changes.tely
diff --git a/Documentation/changes.tely b/Documentation/changes.tely
index  
e57bbe7562eba77fe817a45b9d492552fcc550a8..c1bda08952f5d4b495c1138712360c74a50c7b9e  
100644
--- a/Documentation/changes.tely
+++ b/Documentation/changes.tely
@@ -167,6 +167,11 @@ some events of the original chord, he can run the  
repeat chord
 replacement function @code{\chordRepeats} manually.

 @item
+Another consequence is that string numbers and right hand fingerings on
+single notes appear in normal notation without having to be placed
+inside of chord brackets.
+
address@hidden
 Scheme expressions inside of embedded Lilypond (@address@hidden@address@hidden)
 are now executed in lexical closure of the surrounding Scheme code.
 @code{$} is no longer special in embedded Lilypond.  It can be used
Index: Documentation/notation/fretted-strings.itely
diff --git a/Documentation/notation/fretted-strings.itely  
b/Documentation/notation/fretted-strings.itely
index  
e8deaafb8c9402363d8e4f9195a39416d2d4c82d..ef004f3e2cf032f75d849a5f240c4279d254f1e2  
100644
--- a/Documentation/notation/fretted-strings.itely
+++ b/Documentation/notation/fretted-strings.itely
@@ -97,15 +97,11 @@ Notation Reference:
 @cindex fingering vs. string numbers

 The string on which a note should be played may be indicated by
-appending @address@hidden to a note inside a chord construct
address@hidden<>}.
-
address@hidden numbers @strong{must} be defined inside a chord
-construct even if there is only a single note.}
+appending @address@hidden to a note.

 @lilypond[verbatim,quote,relative=0]
 \clef "treble_8"
-<c\5>4 <e\4> <g\3>2
+c4\5 e\4 g2\3
 <c,\5 e\4 g\3>1
 @end lilypond

@@ -115,8 +111,8 @@ in the code:

 @lilypond[verbatim,quote,relative=1]
 \clef "treble_8"
-<g\3-0>2
-<g-0\3>
+g2\3-0
+g-0\3
 @end lilypond

 @snippets
@@ -224,16 +220,16 @@ symbols = {

 By default pitches are assigned to the lowest playing position on the
 fret-board (first position).  Open strings are automatically preferred.
-If you would like a certain pitch to be played on a specific string
-you can add a string number indication to the pitch name.  If you
-define pitch names and string numbers without a chord construct
-(@code{<>}) the string number indications do not appear in traditional
-notation.  It is much more comfortable to define the playing position
-by using the value of @code{minimumFret}.  The default value for
-minimumFret is 0.
+If you would like a certain pitch to be played on a specific string you
+can add a string number indication to the pitch name.  If you don't want
+to have string number indications appear in traditional notation, you
+can override the respective stencil.  Usually it will be more
+comfortable to define the playing position by using the value of
address@hidden  The default value for minimumFret is 0.


 @lilypond[quote,ragged-right,verbatim]
+\layout { \override Voice.StringNumber #'stencil = ##f }
 \new StaffGroup <<
    \new Staff \relative c {
      \clef "treble_8"
@@ -374,6 +370,7 @@ Harmonic indications can be added to tablature notation  
as sounding
 pitches:

 @lilypond[verbatim,quote]
+\layout { \override Voice.StringNumber #'stencil = ##f }
 firstHarmonic = {
   d'4\4\harmonic
   g'4\3\harmonic
@@ -521,14 +518,13 @@ written.

 @lilypond[quote,ragged-right,verbatim]
 <<
-  \new Staff {
+  \new Voice \with { \override StringNumber #'stencil = ##f } {
     \clef "bass_8"
     \relative c, {
       c4 d e f
     }
   }
-  \new TabStaff {
-    \set TabStaff.stringTunings = #bass-tuning
+  \new TabStaff \with { stringTunings = #bass-tuning } {
     \relative c, {
       c4 d e f
     }
@@ -1576,19 +1572,18 @@ with non-monotonic tunings.
 @funindex rightHandFinger
 @funindex \rightHandFinger

-Right-hand fingerings @var{p-i-m-a} must be entered within a
-chord construct @code{<>} for them to be printed in the score,
-even when applied to a single note.
+Right-hand fingerings @var{p-i-m-a} must be entered using
address@hidden followed by a number.

 @warning{There @strong{must} be a hyphen before
 @address@hidden and a space before the closing @code{>}.}

 @lilypond[quote,verbatim,relative=0]
 \clef "treble_8"
-<c-\rightHandFinger #1 >4
-<e-\rightHandFinger #2 >
-<g-\rightHandFinger #3 >
-<c-\rightHandFinger #4 >
+c4-\rightHandFinger #1
+e-\rightHandFinger #2
+g-\rightHandFinger #3
+c-\rightHandFinger #4
 <c,-\rightHandFinger #1 e-\rightHandFinger #2
  g-\rightHandFinger #3 c-\rightHandFinger #4 >1
 @end lilypond
Index: lily/dispatcher.cc
diff --git a/lily/dispatcher.cc b/lily/dispatcher.cc
index  
94519021abe43ccb4def37ce25fa595e3dd676a9..4db7c93a1555a66a9cffc6772ea9ac349a263379  
100644
--- a/lily/dispatcher.cc
+++ b/lily/dispatcher.cc
@@ -173,6 +173,27 @@ Dispatcher::dispatch (SCM sev)
 #endif
 }

+bool
+Dispatcher::is_listened (Stream_event *ev)
+{
+  SCM class_symbol = ev->get_property ("class");
+  if (!scm_is_symbol (class_symbol))
+    {
+      warning (_ ("Event class should be a symbol"));
+      return false;
+    }
+
+  for (SCM cl = scm_call_1 (ly_lily_module_constant  
("ly:make-event-class"), class_symbol);
+       scm_is_pair (cl); cl = scm_cdr (cl))
+    {
+      SCM list = scm_hashq_ref (listeners_, scm_car (cl), SCM_EOL);
+      if (scm_is_pair (list))
+        return true;
+    }
+  return false;
+}
+
+
 void
 Dispatcher::broadcast (Stream_event *ev)
 {
Index: lily/include/dispatcher.hh
diff --git a/lily/include/dispatcher.hh b/lily/include/dispatcher.hh
index  
d812b27c63ccf73021db1e714db672999b5f1083..b11167121bb93ebf2b1a83f8c3ec7f0349bdbccf  
100644
--- a/lily/include/dispatcher.hh
+++ b/lily/include/dispatcher.hh
@@ -39,6 +39,7 @@ class Dispatcher
 public:
   Dispatcher ();
   void broadcast (Stream_event *ev);
+  bool is_listened (Stream_event *ev);
   void add_listener (Listener, SCM event_class);
   void remove_listener (Listener, SCM event_class);
   void register_as_listener (Dispatcher *dist);
Index: lily/rhythmic-music-iterator.cc
diff --git a/lily/rhythmic-music-iterator.cc  
b/lily/rhythmic-music-iterator.cc
index  
1246e3854272ec622ef10e1766a9bb1024e2be96..427436a12f00ba96c0c96ac438056cf36dfa1b49  
100644
--- a/lily/rhythmic-music-iterator.cc
+++ b/lily/rhythmic-music-iterator.cc
@@ -59,17 +59,12 @@ Rhythmic_music_iterator::process (Moment m)
           SCM unlistened = SCM_EOL;
           for (; scm_is_pair (arts); arts = scm_cdr (arts))
             {
-              if (scm_is_true
-                  (scm_call_2
-                   (ly_lily_module_constant ("any"),
-                    ly_lily_module_constant ("ly:is-listened-event-class"),
-                    scm_call_1
-                    (ly_lily_module_constant ("ly:make-event-class"),
-                     unsmob_stream_event (scm_car (arts))
-                     ->get_property ("class")))))
-                listened = scm_cons (scm_car (arts), listened);
+             SCM art = scm_car (arts);
+
+              if (c->event_source ()->is_listened (unsmob_stream_event  
(art)))
+                listened = scm_cons (art, listened);
               else
-                unlistened = scm_cons (scm_car (arts), unlistened);
+                unlistened = scm_cons (art, unlistened);
             }
           ev->set_property ("articulations", scm_reverse_x (unlistened,  
SCM_EOL));
           c->event_source ()->broadcast (ev);