Re: Slashed Half-flat and add. scales for Arabic music (issue 317550043 by amir...@hotmail.de)
https://codereview.appspot.com/317550043/diff/20001/ly/arabic.ly File ly/arabic.ly (right): https://codereview.appspot.com/317550043/diff/20001/ly/arabic.ly#newcode42 ly/arabic.ly:42: % Ajam family On 2017/04/25 20:32:19, thomasmorley651 wrote: There seems to be no entry for 'Ajam family'. Or is the 'Bayati family' a subset of the 'Ajam family'? In any case I'd insert a comment to clearify. Well Ajam is the Arabic name for major. I first thought to do something like "ajam = \major" but then decided against it. However, there are other scales in this family, but they are used seldom. I did not want to include ~100 scales now. So basically I left it for completeness and future changes. However, I'll delete the comment https://codereview.appspot.com/317550043/diff/20001/ly/arabic.ly#newcode87 ly/arabic.ly:87: % Nahawand family On 2017/04/25 20:32:19, thomasmorley651 wrote: Same as above. Same as above but this time it is minor https://codereview.appspot.com/317550043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Slashed Half-flat and add. scales for Arabic music (issue 317550043 by amir...@hotmail.de)
https://codereview.appspot.com/317550043/diff/20001/ly/arabic.ly File ly/arabic.ly (right): https://codereview.appspot.com/317550043/diff/20001/ly/arabic.ly#newcode19 ly/arabic.ly:19: \version "2.18.2" On 2017/04/25 20:39:10, dak wrote: On 2017/04/25 20:32:19, thomasmorley651 wrote: > The version should be the version since this works, i.e. "2.19.60" Disagree, but I don't think we have this formalized anywhere. In my opinion it should be the lowest known version for which it _compiles_ (as an indicator of when something has been introduced, it is useless because of convert-ly runs). That way, one can compare against lower versions without unnecessary failure. If you have no idea, using the current version is fine though. You're right. I confused it with our policy for Documentation/snippets/new. So "2.18.2" looks fine. https://codereview.appspot.com/317550043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Slashed Half-flat and add. scales for Arabic music (issue 317550043 by amir...@hotmail.de)
https://codereview.appspot.com/317550043/diff/20001/ly/arabic.ly File ly/arabic.ly (right): https://codereview.appspot.com/317550043/diff/20001/ly/arabic.ly#newcode19 ly/arabic.ly:19: \version "2.18.2" On 2017/04/25 20:32:19, thomasmorley651 wrote: The version should be the version since this works, i.e. "2.19.60" Disagree, but I don't think we have this formalized anywhere. In my opinion it should be the lowest known version for which it _compiles_ (as an indicator of when something has been introduced, it is useless because of convert-ly runs). That way, one can compare against lower versions without unnecessary failure. If you have no idea, using the current version is fine though. https://codereview.appspot.com/317550043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Slashed Half-flat and add. scales for Arabic music (issue 317550043 by amir...@hotmail.de)
Hi Amir, thanks a lot for your patch. I don't know anything about arabic music. So most comments are naggings about formating/indenting issues... Thanks, Harm https://codereview.appspot.com/317550043/diff/20001/ly/arabic.ly File ly/arabic.ly (right): https://codereview.appspot.com/317550043/diff/20001/ly/arabic.ly#newcode19 ly/arabic.ly:19: \version "2.18.2" The version should be the version since this works, i.e. "2.19.60" https://codereview.appspot.com/317550043/diff/20001/ly/arabic.ly#newcode23 ly/arabic.ly:23: % The accidental that lowers by a quarter is however the slashed flat, not the mirrored one lilypond uses by default. Please keep a 80-characters line-width, here and at several instances below. https://codereview.appspot.com/317550043/diff/20001/ly/arabic.ly#newcode42 ly/arabic.ly:42: % Ajam family There seems to be no entry for 'Ajam family'. Or is the 'Bayati family' a subset of the 'Ajam family'? In any case I'd insert a comment to clearify. https://codereview.appspot.com/317550043/diff/20001/ly/arabic.ly#newcode87 ly/arabic.ly:87: % Nahawand family Same as above. https://codereview.appspot.com/317550043/diff/20001/ly/arabic.ly#newcode129 ly/arabic.ly:129: \override Accidental #'glyph-name-alist = \TwentyFourTETglyphs This syntax is outdated. Please use \override Accidental.glyph-name-alist = \TwentyFourTETglyphs here and similar next line. https://codereview.appspot.com/317550043/diff/20001/ly/arabic.ly#newcode130 ly/arabic.ly:130: \override KeySignature #'glyph-name-alist = \TwentyFourTETglyphs Please always use spaces not tabs in .ly and .scm-files. Though, why do you indent it more than the line above at all? Same for next line. https://codereview.appspot.com/317550043/diff/20001/ly/arabic.ly#newcode157 ly/arabic.ly:157: % Amir Czwink: I left this for backward compatibility but it is basically useless... line-width https://codereview.appspot.com/317550043/diff/20001/ly/arabic.ly#newcode158 ly/arabic.ly:158: % The \dwn command is totally impractical and cumbersome, as one has to write the \dwn command in front of any quarter tone, and also it does not work for key signatures. line-width https://codereview.appspot.com/317550043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Slashed Half-flat and add. scales for Arabic music (issue 317550043 by amir...@hotmail.de)
https://codereview.appspot.com/317550043/diff/1/ly/arabic.ly File ly/arabic.ly (right): https://codereview.appspot.com/317550043/diff/1/ly/arabic.ly#newcode88 ly/arabic.ly:88: On 2017/04/25 10:51:47, amir130 wrote: On 2017/04/24 18:09:59, dak wrote: > Whitespace error? > > Maybe check with > > git log --check > > or so? Yes it seems to be a whitespace error. How can I fix this? Do I have to send in a new patch? Fixing existing branches can usually be done reasonably conveniently using git rebase -i (assuming that your "upstream" branch is set properly). This requires a proper setting of your EDITOR variable. You can then basically "follow instructions". Ask if you need more help. https://codereview.appspot.com/317550043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Slashed Half-flat and add. scales for Arabic music (issue 317550043 by amir...@hotmail.de)
Reviewers: dak, https://codereview.appspot.com/317550043/diff/1/ly/arabic.ly File ly/arabic.ly (right): https://codereview.appspot.com/317550043/diff/1/ly/arabic.ly#newcode88 ly/arabic.ly:88: On 2017/04/24 18:09:59, dak wrote: Whitespace error? Maybe check with git log --check or so? Yes it seems to be a whitespace error. How can I fix this? Do I have to send in a new patch? Description: Slashed Half-flat and add. scales for Arabic music -Introduced a glyph list so that the slashed half-flat is used (always) instead of the mirrored one (the latter is never used for Arabic sheet music) -Added two scales (Maqamat) of major importance for Arabic music For arabic music score writing: -Introduced new glyph list, so that by default the slashed half-flat is used instead of the mirrored half-flat, which isn't used in Arabic score writing. This makes the use of the cumbersome \dwn symbol needless and obsolete -Added Hijaz and Hijaz-Kar maqamat Please review this at https://codereview.appspot.com/317550043/ Affected files (+113, -33 lines): M ly/arabic.ly ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Slashed Half-flat and add. scales for Arabic music (issue 317550043 by amir...@hotmail.de)
https://codereview.appspot.com/317550043/diff/1/ly/arabic.ly File ly/arabic.ly (right): https://codereview.appspot.com/317550043/diff/1/ly/arabic.ly#newcode28 ly/arabic.ly:28: Whitespace error? https://codereview.appspot.com/317550043/diff/1/ly/arabic.ly#newcode86 ly/arabic.ly:86: Whitespace error? https://codereview.appspot.com/317550043/diff/1/ly/arabic.ly#newcode88 ly/arabic.ly:88: Whitespace error? Maybe check with git log --check or so? https://codereview.appspot.com/317550043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
