Doc: Update ../ly/Bagpipe.ly

Doc: Update ../ly/Bagpipe.ly

Issue 3605

Reported by Julia Meihoefer and Oliver Briede :

... corrected bagpipe.ly version 2.19.
All changes are commented with the
introducttion "@JO".

Our corrections are based on research we did for
Julia's Bachelor Thesis in computer engineering
'Identification, notation and reproduction of
the Great Highland Bagpipe sound' in August 2014.

Sources are:

1. Our own long year piping experience with teachers
from Germany, Canada and Scotland.

2. Different literature from the College of
Piping (Scotland) and McGillivray Piping (Canada)

3. Personal support from the College of Piping and
recommendations from James MacHattie, Director of
Education, College of Piping and Celtic Performing
Arts of Canada

[dak, 2015-06-11 12:19:36]

https://codereview.appspot.com/241270043/diff/1/ly/bagpipe.ly
File ly/bagpipe.ly (left):

https://codereview.appspot.com/241270043/diff/1/ly/bagpipe.ly#oldcode22
ly/bagpipe.ly:22: (c-flat . ,(ly:make-pitch 1 0 FLAT))
Why are you removing the definitions c-flat, f-flat, g-flat?

https://codereview.appspot.com/241270043/diff/1/ly/bagpipe.ly
File ly/bagpipe.ly (right):

https://codereview.appspot.com/241270043/diff/1/ly/bagpipe.ly#newcode14
ly/bagpipe.ly:14: \version "2.19.0"
Rewind the version?  Why?  Well, it does have the advantage that the next run of
update-with-convert-ly run will remove obsolete parser/location arguments (see
later).  But it seems saner to add the file in the proper version.

https://codereview.appspot.com/241270043/diff/1/ly/bagpipe.ly#newcode94
ly/bagpipe.ly:94: stemspace = #(define-music-function (parser location extent)
(pair?) #{
Please remove the parser and location arguments here.

https://codereview.appspot.com/241270043/diff/1/ly/bagpipe.ly#newcode97
ly/bagpipe.ly:97: pgrace = #(define-music-function (parser location notes)
(ly:music?) #{
And here.

[pkx166h, 2015-06-12 06:09:09]

Oliver, I wonder if you could take a look at the comments by one of our
developers about the recent patch you supplied to us for Bagpipe.ly and answer
his questions.

Regards

James

[Dan Eble, 2015-06-12 12:25:33]

https://codereview.appspot.com/241270043/diff/1/ly/bagpipe.ly
File ly/bagpipe.ly (right):

https://codereview.appspot.com/241270043/diff/1/ly/bagpipe.ly#newcode150
ly/bagpipe.ly:150: % @JO: We would skip the previous comment line. It is
irritating, because some triple strikes
But this is not the triple-strike section.  Is the comment true or false?  If it
is true, it taught me something I didn't know, so please leave it and try not to
let it bother you.

https://codereview.appspot.com/241270043/diff/1/ly/bagpipe.ly#newcode157
ly/bagpipe.ly:157: wshaked = { \pgrace { g32[ d e d c] } }	% @JO: added missing
light shake
It is obvious that this has been added.  The comment is unnecessary.

Please also remove every line of wrong code that you have merely commented out. 
It harms readability and offers no benefit.  One can always find the old version
in the source control history if one wants.

https://codereview.appspot.com/241270043/diff/1/ly/bagpipe.ly#newcode297
ly/bagpipe.ly:297: %dgrip = { \pgrace { G32[ b G] } } % @JO: wrong name. This is
b grip
So if someone has used \dgrip, they would probably appreciate it if convert-ly
converted it to \bgrip for them.  (There may be other things; I have not
reviewed every line.)

[pkx, 2015-06-12 12:32:57]

Hello Oliver,

On 12/06/15 13:25, Oliver Briede wrote:
> Hi James, dak
> we have not at all rewinded the version. When we started our work on the 
> bagpipe.ly, the latest version in your github was 2.19.0. In contrary to all 
> others working on the file (obviously) we extensively commented all our
changes. 
> I propose you just merge these changes into the file and forward the questions

> to the people who implemented them.
> The wrong notation in the bagpipe.ly v2.17 messed up Julias bachelor thesis
and 
> it took a very long time and effort to research and verify the correct
notation. 
> We have done our work. It's up to you to merge it in or leave it.
> Best regards,
> Oliver & Julia

The problem is that these are not just additions, there are some
removals as well as some confusions/clarifications on the differences
between what was there and what you have provided us.

I am merely facilitating the patch, I have no experience with bagpipe.ly
music.

Hence while I understand that you think the work has been done, there
are questions that some other developers (and possible bagpipe players)
have because (for example) we have deletions from the file. There are
also some other concerns again that you would have to at least take a
look at and answer because I have no idea about Bagpipe music. We need
the input from you to qualify the changes that were submitted.


https://codereview.appspot.com/241270043/

James


> *Gesendet:* Freitag, 12. Juni 2015 um 08:09 Uhr
> *Von:* pkx166h@gmail.com
> *An:* dak@gnu.org
> *Cc:* lilypond-devel@gnu.org, o.briede@web.de,
reply@codereview-hr.appspotmail.com
> *Betreff:* Re: Doc: Update ../ly/Bagpipe.ly (issue 241270043 by
pkx166h@gmail.com)
> Reviewers: dak,
>
> Message:
> Oliver, I wonder if you could take a look at the comments by one of our
> developers about the recent patch you supplied to us for Bagpipe.ly and
> answer his questions.
>
> Regards
>
> James
>
> Description:
> Doc: Update ../ly/Bagpipe.ly
>
> Issue 3605
>
> Reported by Julia Meihoefer and Oliver Briede :
>
> ... corrected bagpipe.ly version 2.19.
> All changes are commented with the
> introducttion "@JO".
>
> Our corrections are based on research we did for
> Julia's Bachelor Thesis in computer engineering
> 'Identification, notation and reproduction of
> the Great Highland Bagpipe sound' in August 2014.
>
> Sources are:
>
> 1. Our own long year piping experience with teachers
> from Germany, Canada and Scotland.
>
> 2. Different literature from the College of
> Piping (Scotland) and McGillivray Piping (Canada)
>
> 3. Personal support from the College of Piping and
> recommendations from James MacHattie, Director of
> Education, College of Piping and Celtic Performing
> Arts of Canada
>
> Please review this at https://codereview.appspot.com/241270043/
>
> Affected files (+90, -45 lines):
> M ly/bagpipe.ly
>

[dak, 2015-06-12 13:16:07]

pkx166h@gmail.com writes:

> Reviewers: dak,
>
> Message:
> Oliver, I wonder if you could take a look at the comments by one of our
> developers about the recent patch you supplied to us for Bagpipe.ly and
> answer his questions.

I've prepared a patch based on the difference to version 2.19.0.  This
should likely be used instead of what we currently have in Rietveld.

[dak, 2015-06-12 13:20:45]

On 2015/06/12 13:16:07, dak wrote:
> mailto:pkx166h@gmail.com writes:
> 
> > Reviewers: dak,
> >
> > Message:
> > Oliver, I wonder if you could take a look at the comments by one of our
> > developers about the recent patch you supplied to us for Bagpipe.ly and
> > answer his questions.
> 
> I've prepared a patch based on the difference to version 2.19.0.  This
> should likely be used instead of what we currently have in Rietveld.
> 

Well, Rietveld stripped the attachment.  I'm putting it up at the Google issue
tracker in issue 3605.

[pkx166h, 2015-06-14 08:53:35]

With David's merge

[pkx166h, 2015-06-28 11:26:01]

Clarifications by Dan Eble

[pkx166h, 2015-07-05 09:29:55]

author	Dan Eble <nine.fierce.ballads@gmail.com>	
	Mon, 22 Jun 2015 14:58:27 +0000 (10:58 -0400)
committer	James Lowe <pkx166h@gmail.com>	
	Sun, 5 Jul 2015 09:24:40 +0000 (10:24 +0100)
commit	8ea1f8c75bc77d59bad3fe7f9e84f5a8a82e8c9b