Re: Issue 4154: Compact Chord Symbols Patch (issue 153160043 by d...@gnu.org)
On Sat, 2014-10-18 at 17:19 +, d...@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 ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 4154: Compact Chord Symbols Patch (issue 153160043 by d...@gnu.org)
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. https://codereview.appspot.com/153160043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 4154: Compact Chord Symbols Patch (issue 153160043 by d...@gnu.org)
On Thu, 2014-10-09 at 11:36 +, d...@gnu.org wrote:
> On 2014/10/09 11:08:16, richard_rshann.plus.com wrote:
> > On Wed, 2014-10-08 at 17:41 +, mailto:d...@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#newcode98
> > > 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
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 4154: Compact Chord Symbols Patch (issue 153160043 by d...@gnu.org)
On 2014/10/09 11:08:16, richard_rshann.plus.com wrote:
On Wed, 2014-10-08 at 17:41 +, mailto:d...@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#newcode98
> 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.
https://codereview.appspot.com/153160043/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 4154: Compact Chord Symbols Patch (issue 153160043 by d...@gnu.org)
On Wed, 2014-10-08 at 17:41 +, d...@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#newcode98
> 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
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Issue 4154: Compact Chord Symbols Patch (issue 153160043 by d...@gnu.org)
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#newcode98
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?
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
