Re: Allows user to set ChordName text (issue 6496085)

2012-09-12 Thread dak 


http://codereview.appspot.com/6496085/diff/1/lily/chord-name-engraver.cc
File lily/chord-name-engraver.cc (right):

http://codereview.appspot.com/6496085/diff/1/lily/chord-name-engraver.cc#newcode149
lily/chord-name-engraver.cc:149:  ly_is_equal (chord_as_scm,
last_chord_))
On 2012/09/06 09:59:09, MikeSol wrote:

On 2012/09/06 08:50:40, dak wrote:
 If one is doing the chord calculation manually, you can't make the

decision of

 whether a chord changed based on the automatic calculation.  For

better or

 worse, you need to compare the computed chord versions/text.



To respond to your points above, I don't throw away the values above

because

they're used here.  As for the present point, that is an interesting
conundrum...I'll  drum up some logic for that.  That may eliminate the

need for

guarding the values above having to do with chord changes, in which

point the if

else statement will be able to be simplified.


Is there a reason you did none of the above, addressed only the very
first point of my review, ignored all of the rest and pushed?

http://codereview.appspot.com/6496085/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Allows user to set ChordName text (issue 6496085)

2012-09-12 Thread m...@mikesolomon.org 

On 12 sept. 2012, at 14:05, d...@gnu.org wrote:

 
 http://codereview.appspot.com/6496085/diff/1/lily/chord-name-engraver.cc
 File lily/chord-name-engraver.cc (right):
 
 http://codereview.appspot.com/6496085/diff/1/lily/chord-name-engraver.cc#newcode149
 lily/chord-name-engraver.cc:149:  ly_is_equal (chord_as_scm,
 last_chord_))
 On 2012/09/06 09:59:09, MikeSol wrote:
 On 2012/09/06 08:50:40, dak wrote:
  If one is doing the chord calculation manually, you can't make the
 decision of
  whether a chord changed based on the automatic calculation.  For
 better or
  worse, you need to compare the computed chord versions/text.
 
 To respond to your points above, I don't throw away the values above
 because
 they're used here.  As for the present point, that is an interesting
 conundrum...I'll  drum up some logic for that.  That may eliminate the
 need for
 guarding the values above having to do with chord changes, in which
 point the if
 else statement will be able to be simplified.
 
 Is there a reason you did none of the above, addressed only the very
 first point of my review, ignored all of the rest and pushed?
 

There are four points in your previous review and I addressed all of them in 
the code before I pushed to staging.  If it's unclear how any of them are 
addressed, please make reference to a specific one and I can put a comment in 
the code to clear things up.

Cheers,
MS


___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Allows user to set ChordName text (issue 6496085)

2012-09-12 Thread dak 

Yes, I did not check you actually addressed the points of the review
before the countdown ended.  But I see my role as a reviewer, not as a
surveillance team.


http://codereview.appspot.com/6496085/diff/1/lily/chord-name-engraver.cc
File lily/chord-name-engraver.cc (right):

http://codereview.appspot.com/6496085/diff/1/lily/chord-name-engraver.cc#newcode87
lily/chord-name-engraver.cc:87: {
On 2012/09/06 08:50:40, dak wrote:

What kind of contorted logic and guessing game is that?
if (make_markup)
{
[old code ending in setting text]
}



Please don't obfuscate code just to save reindentation.


This comment has not been addressed.

http://codereview.appspot.com/6496085/diff/1/lily/chord-name-engraver.cc#newcode136
lily/chord-name-engraver.cc:136: markup = scm_call_4 (name_proc,
pitches, bass, inversion,
On 2012/09/06 08:50:40, dak wrote:

You do all the calculation and then throw it away?  Where is the

point?

This comment has not been addressed.  If neither rest_event nor
make_markup are set, you calculate pitches, bass, inversion and throw
them away.

http://codereview.appspot.com/6496085/diff/1/lily/chord-name-engraver.cc#newcode149
lily/chord-name-engraver.cc:149:  ly_is_equal (chord_as_scm,
last_chord_))
On 2012/09/06 09:59:09, MikeSol wrote:

On 2012/09/06 08:50:40, dak wrote:
 If one is doing the chord calculation manually, you can't make the

decision of

 whether a chord changed based on the automatic calculation.  For

better or

 worse, you need to compare the computed chord versions/text.



To respond to your points above, I don't throw away the values above

because

they're used here.


I don't see where the current code version would use them.  Can you
point that out to me?

http://codereview.appspot.com/6496085/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Allows user to set ChordName text (issue 6496085)

2012-09-12 Thread m...@mikesolomon.org 
 http://codereview.appspot.com/6496085/diff/1/lily/chord-name-engraver.cc#newcode87
 lily/chord-name-engraver.cc:87: {
 On 2012/09/06 08:50:40, dak wrote:
 What kind of contorted logic and guessing game is that?
 if (make_markup)
 {
[old code ending in setting text]
 }
 
 Please don't obfuscate code just to save reindentation.
 
 This comment has not been addressed.
 

I think I misunderstood your comment - could you show what you mean by 
proposing an alternative (in pseudo-code if you'd like) to the code that's in 
there?

 http://codereview.appspot.com/6496085/diff/1/lily/chord-name-engraver.cc#newcode136
 lily/chord-name-engraver.cc:136: markup = scm_call_4 (name_proc,
 pitches, bass, inversion,
 On 2012/09/06 08:50:40, dak wrote:
 You do all the calculation and then throw it away?  Where is the
 point?
 
 This comment has not been addressed.  If neither rest_event nor
 make_markup are set, you calculate pitches, bass, inversion and throw
 them away.

I'm not sure what you mean - every time these are calculated, these are used on 
line 139.  Unless I am reading the code incorrectly.

 
 http://codereview.appspot.com/6496085/diff/1/lily/chord-name-engraver.cc#newcode149
 lily/chord-name-engraver.cc:149:  ly_is_equal (chord_as_scm,
 last_chord_))
 On 2012/09/06 09:59:09, MikeSol wrote:
 On 2012/09/06 08:50:40, dak wrote:
  If one is doing the chord calculation manually, you can't make the
 decision of
  whether a chord changed based on the automatic calculation.  For
 better or
  worse, you need to compare the computed chord versions/text.
 
 To respond to your points above, I don't throw away the values above
 because
 they're used here.
 
 I don't see where the current code version would use them.  Can you
 point that out to me?
 

They're no longer used as per your suggestion to compare text instead of these 
values.

 http://codereview.appspot.com/6496085/


___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Allows user to set ChordName text (issue 6496085)

2012-09-12 Thread dak 

On 2012/09/12 11:48:45, mike7 wrote:




http://codereview.appspot.com/6496085/diff/1/lily/chord-name-engraver.cc#newcode87

 lily/chord-name-engraver.cc:87: {
 On 2012/09/06 08:50:40, dak wrote:
 What kind of contorted logic and guessing game is that?
 if (make_markup)
 {
[old code ending in setting text]
 }

 Please don't obfuscate code just to save reindentation.

 This comment has not been addressed.




I think I misunderstood your comment


I have a hard time finding an interpretation where doing nothing is
the proper remedy.


- could you show what you mean by proposing
an alternative (in pseudo-code if you'd like) to the code that's in
there?


I did it in pseudo-code above, and you ignored it.  So let's do it in
straight code.

The diff to the original has the form

- if (rest_event_)
+ if (rest_event_  !make_markup) { }
+ else if (rest_event_)
{
   SCM no_chord_markup = get_property (noChordSymbol);
   if (!Text_interface::is_markup (no_chord_markup))
...
} else

when it should rather have the form

  if (rest_event_)
{
-   SCM no_chord_markup = get_property (noChordSymbol);
-   if (!Text_interface::is_markup (no_chord_markup))
...
+ if (make_markup)
+   {
+ SCM no_chord_markup = get_property (noChordSymbol);
+ if (!Text_interface::is_markup (no_chord_markup))
...
+   }
} else




http://codereview.appspot.com/6496085/diff/1/lily/chord-name-engraver.cc#newcode136

 lily/chord-name-engraver.cc:136: markup = scm_call_4 (name_proc,
 pitches, bass, inversion,
 On 2012/09/06 08:50:40, dak wrote:
 You do all the calculation and then throw it away?  Where is the
 point?

 This comment has not been addressed.  If neither rest_event nor
 make_markup are set, you calculate pitches, bass, inversion and

throw

 them away.



I'm not sure what you mean - every time these are calculated, these

are used on

line 139.


Only if make_markup is set.  Otherwise you calculate them and throw
them away.

Why?  Why do all the work in the case where make_markup is not even
set?  Why not skip all that much earlier when make_markup is not set?


http://codereview.appspot.com/6496085/diff/1/lily/chord-name-engraver.cc#newcode149

 lily/chord-name-engraver.cc:149:  ly_is_equal (chord_as_scm,
 last_chord_))
 On 2012/09/06 09:59:09, MikeSol wrote:
 On 2012/09/06 08:50:40, dak wrote:
  If one is doing the chord calculation manually, you can't make

the

 decision of
  whether a chord changed based on the automatic calculation.  For
 better or
  worse, you need to compare the computed chord versions/text.

 To respond to your points above, I don't throw away the values

above

 because
 they're used here.

 I don't see where the current code version would use them.  Can you
 point that out to me?




They're no longer used as per your suggestion to compare text
instead of these values.


And is there a reason the logic of the code is not changed to reflect
the changes of the code?

http://codereview.appspot.com/6496085/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Allows user to set ChordName text (issue 6496085)

2012-09-12 Thread dak 

On 2012/09/12 16:11:05, dak wrote:


when it should rather have the form



   if (rest_event_)
 {
-   SCM no_chord_markup = get_property (noChordSymbol);
-   if (!Text_interface::is_markup (no_chord_markup))
...
+ if (make_markup)
+   {
+ SCM no_chord_markup = get_property (noChordSymbol);
+ if (!Text_interface::is_markup (no_chord_markup))
...
+   }
 } else


Actually, come to think of it you might want to move
if (make_markup)
upwards to surround the whole if (rest_event_), and then you will
arrive at the situation where a markup (and all information required
for it) is only calculated when it will get used, addressing the next
point as well.

That was the intent of the original suggestion.


http://codereview.appspot.com/6496085/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Allows user to set ChordName text (issue 6496085)

2012-09-12 Thread m...@mikesolomon.org 

On 12 sept. 2012, at 19:16, d...@gnu.org wrote:

 On 2012/09/12 16:11:05, dak wrote:
 
 when it should rather have the form
 
   if (rest_event_)
 {
 -   SCM no_chord_markup = get_property (noChordSymbol);
 -   if (!Text_interface::is_markup (no_chord_markup))
 ...
 + if (make_markup)
 +   {
 + SCM no_chord_markup = get_property (noChordSymbol);
 + if (!Text_interface::is_markup (no_chord_markup))
 ...
 +   }
 } else
 
 Actually, come to think of it you might want to move
 if (make_markup)
 upwards to surround the whole if (rest_event_), and then you will
 arrive at the situation where a markup (and all information required
 for it) is only calculated when it will get used, addressing the next
 point as well.
 
 That was the intent of the original suggestion.
 

Excellent - this is the exact type of feedback I needed.  Many thanks.

Cheers,
MS

 
 http://codereview.appspot.com/6496085/


___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Allows user to set ChordName text (issue 6496085)

2012-09-06 Thread dak 


http://codereview.appspot.com/6496085/diff/1/lily/chord-name-engraver.cc
File lily/chord-name-engraver.cc (right):

http://codereview.appspot.com/6496085/diff/1/lily/chord-name-engraver.cc#newcode83
lily/chord-name-engraver.cc:83: || ly_is_procedure
(chord_name_-get_property (text)));
If it is a procedure, shouldn't it be called with the calculated value?

http://codereview.appspot.com/6496085/diff/1/lily/chord-name-engraver.cc#newcode87
lily/chord-name-engraver.cc:87: {
What kind of contorted logic and guessing game is that?
if (make_markup)
{
   [old code ending in setting text]
}

Please don't obfuscate code just to save reindentation.

http://codereview.appspot.com/6496085/diff/1/lily/chord-name-engraver.cc#newcode136
lily/chord-name-engraver.cc:136: markup = scm_call_4 (name_proc,
pitches, bass, inversion,
You do all the calculation and then throw it away?  Where is the point?

http://codereview.appspot.com/6496085/diff/1/lily/chord-name-engraver.cc#newcode149
lily/chord-name-engraver.cc:149:  ly_is_equal (chord_as_scm,
last_chord_))
If one is doing the chord calculation manually, you can't make the
decision of whether a chord changed based on the automatic calculation.
For better or worse, you need to compare the computed chord
versions/text.

http://codereview.appspot.com/6496085/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Allows user to set ChordName text (issue 6496085)

2012-09-06 Thread mtsolo 

Reviewers: dak,


http://codereview.appspot.com/6496085/diff/1/lily/chord-name-engraver.cc
File lily/chord-name-engraver.cc (right):

http://codereview.appspot.com/6496085/diff/1/lily/chord-name-engraver.cc#newcode83
lily/chord-name-engraver.cc:83: || ly_is_procedure
(chord_name_-get_property (text)));
On 2012/09/06 08:50:40, dak wrote:

If it is a procedure, shouldn't it be called with the calculated

value?

Right you are - I should have used get_property_data.  Will fix.

http://codereview.appspot.com/6496085/diff/1/lily/chord-name-engraver.cc#newcode149
lily/chord-name-engraver.cc:149:  ly_is_equal (chord_as_scm,
last_chord_))
On 2012/09/06 08:50:40, dak wrote:

If one is doing the chord calculation manually, you can't make the

decision of

whether a chord changed based on the automatic calculation.  For

better or

worse, you need to compare the computed chord versions/text.


To respond to your points above, I don't throw away the values above
because they're used here.  As for the present point, that is an
interesting conundrum...I'll  drum up some logic for that.  That may
eliminate the need for guarding the values above having to do with chord
changes, in which point the if else statement will be able to be
simplified.

Description:
Allows user to set ChordName text

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

Affected files:
  A input/regression/chord-name-override-text.ly
  M lily/chord-name-engraver.cc


Index: input/regression/chord-name-override-text.ly
diff --git a/input/regression/chord-name-override-text.ly  
b/input/regression/chord-name-override-text.ly

new file mode 100644
index  
..4acd549394eff095065c0b9605473424ef13c649

--- /dev/null
+++ b/input/regression/chord-name-override-text.ly
@@ -0,0 +1,13 @@
+\version 2.17.2
+
+\header {
+  texidoc = Users can override the @code{text} property of
+@code{ChordName}.
+
+}
+
+\new ChordNames \chordmode {
+  a b c:7
+  \once \override ChordName #'text = #foo
+  d
+}
\ No newline at end of file
Index: lily/chord-name-engraver.cc
diff --git a/lily/chord-name-engraver.cc b/lily/chord-name-engraver.cc
index  
eab4d94d075c52ef95f0ae09449d6d9d184585c5..6aad475ebb5a0f61292a1d58c8fe68aafe64c1c6  
100644

--- a/lily/chord-name-engraver.cc
+++ b/lily/chord-name-engraver.cc
@@ -76,7 +76,14 @@ Chord_name_engraver::process_music ()
   SCM inversion = SCM_EOL;
   SCM pitches = SCM_EOL;

-  if (rest_event_)
+  chord_name_ = make_item (ChordName,
+   rest_event_ ? rest_event_-self_scm () :  
notes_[0]-self_scm ());

+
+  bool make_markup = !(Text_interface::is_markup  
(chord_name_-get_property (text))
+   || ly_is_procedure (chord_name_-get_property  
(text)));

+
+  if (rest_event_  !make_markup) { }
+  else if (rest_event_)
 {
   SCM no_chord_markup = get_property (noChordSymbol);
   if (!Text_interface::is_markup (no_chord_markup))
@@ -125,17 +132,17 @@ Chord_name_engraver::process_music ()
   pitches = scm_sort_list (pitches, Pitch::less_p_proc);

   SCM name_proc = get_property (chordNameFunction);
-  markup = scm_call_4 (name_proc, pitches, bass, inversion,
-   context ()-self_scm ());
+  if (make_markup)
+markup = scm_call_4 (name_proc, pitches, bass, inversion,
+ context ()-self_scm ());
 }
   /*
 Ugh.
   */
   SCM chord_as_scm = scm_cons (pitches, scm_cons (bass, inversion));

-  chord_name_ = make_item (ChordName,
-   rest_event_ ? rest_event_-self_scm () :  
notes_[0]-self_scm ());

-  chord_name_-set_property (text, markup);
+  if (make_markup)
+chord_name_-set_property (text, markup);

   SCM chord_changes = get_property (chordChanges);
   if (to_boolean (chord_changes)  scm_is_pair (last_chord_)



___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel