Issue 4154: Compact Chord Symbols Patch

Issue 4154: Compact Chord Symbols Patch

[dak, 2014-10-08 17:41:05]

https://codereview.appspot.com/153160043/diff/1/scm/chord-ignatzek-names.scm
File scm/chord-ignatzek-names.scm (right):

https://codereview.appspot.com/153160043/diff/1/scm/chord-ignatzek-names.scm#...
scm/chord-ignatzek-names.scm:98: (vector-ref #("C" "D" "E" "F" "G" "A" "B")
(ly:pitch-notename
This looks like a bad idea.  It does not obey the various chord name languages. 
It does not use the same callbacks.  It is a large duplication of code not
connected with the other code and not using the same options, functionality and
interfaces.

Can't you try to integrate this kind of code better with the existing code, both
regarding the code that is called as well as the naming conventions and
functionality that are available?

[richard_rshann.plus.com, 2014-10-09 11:08:16]

On Wed, 2014-10-08 at 17:41 +0000, dak@gnu.org wrote:
> Reviewers: ,
> 
> 
> https://codereview.appspot.com/153160043/diff/1/scm/chord-ignatzek-names.scm
> File scm/chord-ignatzek-names.scm (right):
> 
>
https://codereview.appspot.com/153160043/diff/1/scm/chord-ignatzek-names.scm#...
> scm/chord-ignatzek-names.scm:98: (vector-ref #("C" "D" "E" "F" "G" "A"
> "B") (ly:pitch-notename
> This looks like a bad idea.  It does not obey the various chord name
> languages.  It does not use the same callbacks.  It is a large
> duplication of code not connected with the other code and not using the
> same options, functionality and interfaces.

When I saw this (on the lilypond-devel mailing list) I thought this was
a comment about the existing code. The code quoted is the existing code,
which I haven't changed.
My code does not generate any chord names, so it can't obey any chord
name languages, it just typesets the elements of the chord name in a
more compact fashion.

Sorry for not replying earlier, but I did think that my patch had
sparked a debate about the quality of the original file, not my patch to
it, which is purely concerned with how the markup is created for the
chord names as generated by the existing code.

Richard


> 
> Can't you try to integrate this kind of code better with the existing
> code, both regarding the code that is called as well as the naming
> conventions and functionality that are available?
> 
> Description:
> Issue 4154: Compact Chord Symbols Patch
> 
> Please review this at https://codereview.appspot.com/153160043/
> 
> Affected files (+100, -26 lines):
>    M scm/chord-ignatzek-names.scm
>    M scm/define-context-properties.scm
> 
> 
> 
> _______________________________________________
> lilypond-devel mailing list
> lilypond-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/lilypond-devel

[dak, 2014-10-09 11:36:31]

On 2014/10/09 11:08:16, richard_rshann.plus.com wrote:
> On Wed, 2014-10-08 at 17:41 +0000, mailto:dak@gnu.org wrote:
> > Reviewers: ,
> > 
> > 
> > https://codereview.appspot.com/153160043/diff/1/scm/chord-ignatzek-names.scm
> > File scm/chord-ignatzek-names.scm (right):
> > 
> >
>
https://codereview.appspot.com/153160043/diff/1/scm/chord-ignatzek-names.scm#...
> > scm/chord-ignatzek-names.scm:98: (vector-ref #("C" "D" "E" "F" "G" "A"
> > "B") (ly:pitch-notename
> > This looks like a bad idea.  It does not obey the various chord name
> > languages.  It does not use the same callbacks.  It is a large
> > duplication of code not connected with the other code and not using the
> > same options, functionality and interfaces.
> 
> When I saw this (on the lilypond-devel mailing list) I thought this was
> a comment about the existing code. The code quoted is the existing code,
> which I haven't changed.

It most certainly isn't the existing code.  It is not even present
anywhere in the same file.  You might have copied parts of it into the
file from some other file, but even git blame -w -C -C -C does not
point to any purported other file as a potential source.

[richard_rshann.plus.com, 2014-10-09 12:02:16]

On Thu, 2014-10-09 at 11:36 +0000, dak@gnu.org wrote:
> On 2014/10/09 11:08:16, richard_rshann.plus.com wrote:
> > On Wed, 2014-10-08 at 17:41 +0000, mailto:dak@gnu.org wrote:
> > > Reviewers: ,
> > >
> > >
> > >
> https://codereview.appspot.com/153160043/diff/1/scm/chord-ignatzek-names.scm
> > > File scm/chord-ignatzek-names.scm (right):
> > >
> > >
> 
>
https://codereview.appspot.com/153160043/diff/1/scm/chord-ignatzek-names.scm#...
> > > scm/chord-ignatzek-names.scm:98: (vector-ref #("C" "D" "E" "F" "G"
> "A"
> > > "B") (ly:pitch-notename
> > > This looks like a bad idea.  It does not obey the various chord name
> > > languages.  It does not use the same callbacks.  It is a large
> > > duplication of code not connected with the other code and not using
> the
> > > same options, functionality and interfaces.
> 
> > When I saw this (on the lilypond-devel mailing list) I thought this
> was
> > a comment about the existing code. The code quoted is the existing
> code,
> > which I haven't changed.
> 
> It most certainly isn't the existing code.

Well, that depends what I meant by the existing code - the specific file
I was modifying calls chordRootNamer which is initialized to
note-name->markup which is in chord-names.scm:62, and that is where the
quoted construct exists in the current lilypond code, in fact I see it
is repeated several times in that file.

Richard

[dak, 2014-10-18 17:19:14]

On 2014/10/09 12:02:16, richard_rshann.plus.com wrote:

> Well, that depends what I meant by the existing code - the specific file
> I was modifying calls chordRootNamer which is initialized to
> note-name->markup which is in chord-names.scm:62, and that is where the
> quoted construct exists in the current lilypond code, in fact I see it
> is repeated several times in that file.

Well, pulling code from a different file and functionality in parts into a
different file where they are basically integrated as flat code into some parts
of the code while other parts still call the other file -- that's a total
maintenance and reading nightmare.  If the duplicated code needs changes or
maintenance, it is quite improbable that someone working on it will find and
change *both* copies as needed.

It would be best to extend the code to be able to do both tasks.  If that is not
reasonably possible, it would be good to factor out common elements but keep the
code doing the non-common parts in the same file.

[richard_rshann.plus.com, 2014-10-18 18:50:07]

On Sat, 2014-10-18 at 17:19 +0000, dak@gnu.org wrote:
> On 2014/10/09 12:02:16, richard_rshann.plus.com wrote:
> 
> > Well, that depends what I meant by the existing code - the specific
> file
> > I was modifying calls chordRootNamer which is initialized to
> > note-name->markup which is in chord-names.scm:62, and that is where
> the
> > quoted construct exists in the current lilypond code, in fact I see it
> > is repeated several times in that file.
> 
> Well, pulling code from a different file and functionality in parts into
> a different file where they are basically integrated as flat code into
> some parts of the code while other parts still call the other file --
> that's a total maintenance and reading nightmare.  If the duplicated
> code needs changes or maintenance, it is quite improbable that someone
> working on it will find and change *both* copies as needed.
as I say, it is not a question of "both", the construct is quite short
and is repeated several times. But, of course, it makes it worse to
repeat it in a different file. And more to the point, a different (if
more direct) mechanism is used for compact symbols than for the other
cases.
> 
> It would be best to extend the code to be able to do both tasks.  If
> that is not reasonably possible, it would be good to factor out common
> elements but keep the code doing the non-common parts in the same file.
> 
> https://codereview.appspot.com/153160043/

The problem with doing the compact symbols in the same file as the
others is that there seems to be no access to the context property that
gives the scaling needed. I asked about this on the mailing list,
	http://lists.gnu.org/archive/html/lilypond-user/2014-09/msg00525.html

 but it seemed this couldn't be done. It is completely unclear to me why
the creation of markup for a chord symbol is spread over several files
in the first place - it seems to result in the loss of the context
information at the point where the root name markup is being created.

Is there a way in which chordRootNamer can take an extra scale argument,
or is there, indeed a way to access this value as the context property
chordCompactScale ? If so, I could easily move that code back in to
chord-names.scm

Richard