Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(42)

Issue 6305115: Issue 1320: Scheme bar line interface (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 years, 3 months ago by marc
Modified:
7 years, 1 month ago
Reviewers:
benko.pal, MikeSol, dak, mail, Keith
CC:
lilypond-devel_gnu.org
Base URL:
http://git.savannah.gnu.org/gitweb/?p=lilypond.git/trunk/
Visibility:
Public.

Description

Issue 1320: Scheme bar line interface This patch moves most of the code from lily/bar-line.cc and lily/span-bar.cc into scm/bar-line.scm After this patch is applied, we will move to the new bar line input handling routines, but it is easier for the revisiting process to split the work in two parts.

Patch Set 1 #

Total comments: 9

Patch Set 2 : First corrections in scm/bar-line.scm #

Patch Set 3 : bar-line::calc-anchor corrected #

Patch Set 4 : Colon stencil rewritten; still faulty #

Patch Set 5 : Colon algorithm works now; some helper functions added #

Patch Set 6 : remove most of c++ code; take recent revert of issue 2533 into account #

Patch Set 7 : make-empty-barline corrected #

Patch Set 8 : handling of 'allow-span-bar is now correct #

Total comments: 5

Patch Set 9 : Corrections in bar-line::calc-anchor; minor nitpicks #

Patch Set 10 : computation of bar-extent and alignment of dashed bar-line corrected #

Patch Set 11 : unneccessary definition in bar-line:calc-bar-extent removed #

Total comments: 7

Patch Set 12 : linewith staff vs. layout #

Patch Set 13 : dashed bar line has now correct x-extent #

Patch Set 14 : forgot to rebase :-( #

Patch Set 15 : Correcting snippet calling ly:bar-line::print #

Patch Set 16 : Adding ly: prefix to callbacks #

Unified diffs Side-by-side diffs Delta from patch set Stats (+731 lines, -844 lines) Patch
M lily/auto-beam-engraver.cc View 1 2 3 4 5 1 chunk +0 lines, -1 line 0 comments Download
M lily/bar-engraver.cc View 1 2 3 4 5 1 chunk +0 lines, -1 line 0 comments Download
M lily/bar-line.cc View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -454 lines 0 comments Download
M lily/clef-engraver.cc View 1 2 3 4 5 1 chunk +0 lines, -1 line 0 comments Download
M lily/cue-clef-engraver.cc View 1 2 3 4 5 1 chunk +0 lines, -1 line 0 comments Download
M lily/custos-engraver.cc View 1 2 3 4 5 1 chunk +0 lines, -1 line 0 comments Download
M lily/grob-scheme.cc View 1 2 3 4 5 6 7 8 1 chunk +13 lines, -0 lines 0 comments Download
M lily/hairpin.cc View 1 2 3 4 5 3 chunks +2 lines, -2 lines 0 comments Download
M lily/include/bar-line.hh View 1 2 3 4 5 1 chunk +0 lines, -11 lines 0 comments Download
D lily/include/span-bar.hh View 1 2 3 4 5 1 chunk +0 lines, -47 lines 0 comments Download
M lily/key-engraver.cc View 1 2 3 4 5 1 chunk +0 lines, -1 line 0 comments Download
M lily/mark-engraver.cc View 1 2 3 4 5 1 chunk +0 lines, -1 line 0 comments Download
D lily/span-bar.cc View 1 2 3 4 5 6 7 8 9 1 chunk +0 lines, -251 lines 0 comments Download
M lily/span-bar-engraver.cc View 1 2 3 4 5 4 chunks +4 lines, -5 lines 0 comments Download
M lily/span-bar-stub-engraver.cc View 1 2 3 4 5 1 chunk +0 lines, -2 lines 0 comments Download
M lily/staff-spacing.cc View 1 2 3 4 5 2 chunks +2 lines, -2 lines 0 comments Download
M lily/vertical-align-engraver.cc View 1 2 3 4 5 1 chunk +0 lines, -1 line 0 comments Download
M lily/volta-engraver.cc View 1 2 3 4 5 1 chunk +0 lines, -1 line 0 comments Download
A scm/bar-line.scm View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +650 lines, -0 lines 0 comments Download
M scm/define-grob-interfaces.scm View 1 2 3 4 5 6 7 8 2 chunks +52 lines, -0 lines 0 comments Download
M scm/lily.scm View 1 chunk +1 line, -0 lines 0 comments Download
M scm/lily-library.scm View 1 2 3 4 5 6 7 8 9 1 chunk +4 lines, -0 lines 0 comments Download
M scm/output-lib.scm View 1 2 3 4 5 1 chunk +1 line, -61 lines 0 comments Download

Messages

Total messages: 33
MikeSol
Good work - two overall things. 1) In Scheme, try to avoid `do'. Use map ...
7 years, 3 months ago (2012-06-20 19:13:01 UTC) #1
benko.pal
http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm File scm/bar-line.scm (right): http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm#newcode83 scm/bar-line.scm:83: (define (make-colon-bar-line grob) I'm afraid this defun doesn't match ...
7 years, 3 months ago (2012-06-20 19:34:58 UTC) #2
marc
Am 20.06.2012 21:13, schrieb mtsolo@gmail.com: > Good work - two overall things. > > 1) ...
7 years, 3 months ago (2012-06-21 06:32:39 UTC) #3
marc
Am 20.06.2012 21:34, schrieb benko.pal@gmail.com: > > http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm > File scm/bar-line.scm (right): > > http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm#newcode83 ...
7 years, 3 months ago (2012-06-21 06:35:55 UTC) #4
marc
Am 20.06.2012 21:13, schrieb mtsolo@gmail.com: > Good work - two overall things. > > [...] ...
7 years, 3 months ago (2012-06-21 07:54:16 UTC) #5
marc
Am 21.06.2012 08:35, schrieb Marc Hohl: > Am 20.06.2012 21:34, schrieb benko.pal@gmail.com: >> >> http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm ...
7 years, 3 months ago (2012-06-21 09:05:47 UTC) #6
benko.pal
hi Marc, >> http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm >> File scm/bar-line.scm (right): >> >> http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm#newcode83 >> scm/bar-line.scm:83: (define (make-colon-bar-line ...
7 years, 2 months ago (2012-06-23 09:06:41 UTC) #7
marc
Am 23.06.2012 11:06, schrieb Benkő Pál: > hi Marc, > >>> http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm >>> File scm/bar-line.scm ...
7 years, 2 months ago (2012-06-23 09:11:59 UTC) #8
marc
Am 23.06.2012 11:11, schrieb Marc Hohl: > Am 23.06.2012 11:06, schrieb Benkő Pál: >> hi ...
7 years, 2 months ago (2012-06-26 19:28:51 UTC) #9
Keith
Wherever there is an empty bar-line, \bar"" , this patch reserves too much space for ...
7 years, 2 months ago (2012-07-16 22:58:04 UTC) #10
marc
Am 17.07.2012 00:58, schrieb k-ohara5a5a@oco.net: > Wherever there is an empty bar-line, \bar"" , this ...
7 years, 2 months ago (2012-07-17 07:43:23 UTC) #11
marc
The regression test span-par-allow-span-bar.ly works now as expected. I added some comments explaining how the ...
7 years, 2 months ago (2012-07-17 19:48:39 UTC) #12
marc
Am 17.07.2012 21:48, schrieb marc@hohlart.de: > The regression test > > span-par-allow-span-bar.ly Should read span-bar-allow-span-bar.ly ...
7 years, 2 months ago (2012-07-20 10:15:41 UTC) #13
dak
On 2012/07/20 10:15:41, marc wrote: > I think the differences concerning MIDI output can safely ...
7 years, 2 months ago (2012-07-20 10:24:08 UTC) #14
mail_philholmes.net
----- Original Message ----- From: "Marc Hohl" <marc@hohlart.de> To: <marc@hohlart.de>; <mtsolo@gmail.com>; <benko.pal@gmail.com>; <k-ohara5a5a@oco.net>; <lilypond-devel@gnu.org>; <reply@codereview-hr.appspotmail.com> ...
7 years, 2 months ago (2012-07-20 10:26:16 UTC) #15
marc
Am 20.07.2012 12:24, schrieb dak@gnu.org: > On 2012/07/20 10:15:41, marc wrote: > >> I think ...
7 years, 2 months ago (2012-07-20 18:17:07 UTC) #16
marc
Am 20.07.2012 12:26, schrieb Phil Holmes: [...] > It might be worth your comparing the ...
7 years, 2 months ago (2012-07-20 18:22:22 UTC) #17
dak
Marc Hohl <marc@hohlart.de> writes: > Am 20.07.2012 12:24, schrieb dak@gnu.org: >> On 2012/07/20 10:15:41, marc ...
7 years, 2 months ago (2012-07-20 18:28:34 UTC) #18
marc
Am 20.07.2012 20:17, schrieb Marc Hohl: > Am 20.07.2012 12:24, schrieb dak@gnu.org: >> On 2012/07/20 ...
7 years, 2 months ago (2012-07-20 18:31:19 UTC) #19
marc
Am 20.07.2012 20:28, schrieb David Kastrup: > Marc Hohl <marc@hohlart.de> writes: > >> Am 20.07.2012 ...
7 years, 2 months ago (2012-07-20 18:44:00 UTC) #20
dak
http://codereview.appspot.com/6305115/diff/30001/scm/bar-line.scm File scm/bar-line.scm (right): http://codereview.appspot.com/6305115/diff/30001/scm/bar-line.scm#newcode649 scm/bar-line.scm:649: Empty line at end of file makes git complain ...
7 years, 2 months ago (2012-07-20 19:03:20 UTC) #21
benko.pal
Marc, please don't throw the whole 2533 issue stuff out; look at the newer version ...
7 years, 2 months ago (2012-07-20 19:15:14 UTC) #22
Keith
Works fine for me, but it will need updated to synchronize with the patches soon-to-be-pushed ...
7 years, 2 months ago (2012-07-20 21:16:01 UTC) #23
marc
Am 20.07.2012 21:15, schrieb Benkő Pál: > Marc, please don't throw the whole 2533 issue ...
7 years, 2 months ago (2012-07-21 08:04:27 UTC) #24
marc
Am 20.07.2012 23:16, schrieb k-ohara5a5a@oco.net: > Works fine for me, but it will need updated ...
7 years, 2 months ago (2012-07-21 10:07:01 UTC) #25
marc
Am 20.07.2012 21:03, schrieb dak@gnu.org: > > http://codereview.appspot.com/6305115/diff/30001/scm/bar-line.scm > File scm/bar-line.scm (right): > > http://codereview.appspot.com/6305115/diff/30001/scm/bar-line.scm#newcode649 ...
7 years, 2 months ago (2012-07-21 10:07:37 UTC) #26
Keith
Looks like LilyPond defines two line-thicknesses, one for staff-line thickness in each StaffSymbol, and one ...
7 years, 1 month ago (2012-07-23 21:45:12 UTC) #27
marc
Am 23.07.2012 23:45, schrieb k-ohara5a5a@oco.net: > Looks like LilyPond defines two line-thicknesses, > one for ...
7 years, 1 month ago (2012-07-24 09:41:33 UTC) #28
benko.pal
Marc, please don't throw the whole 2533 issue stuff out; look at the latest version ...
7 years, 1 month ago (2012-07-26 07:51:56 UTC) #29
marc
Am 26.07.2012 09:51, schrieb benko.pal@gmail.com: > Marc, please don't throw the whole 2533 issue stuff ...
7 years, 1 month ago (2012-07-26 08:26:43 UTC) #30
Keith
On Thu, 26 Jul 2012 00:51:56 -0700, <benko.pal@gmail.com> wrote: > Marc, please don't throw the ...
7 years, 1 month ago (2012-07-26 08:29:14 UTC) #31
marc
Am 26.07.2012 10:31, schrieb Keith OHara: > On Thu, 26 Jul 2012 00:51:56 -0700, <benko.pal@gmail.com> ...
7 years, 1 month ago (2012-07-26 08:36:58 UTC) #32
benko.pal
7 years, 1 month ago (2012-07-26 15:35:57 UTC) #33
2012/7/26 Marc Hohl <marc@hohlart.de>:
> Am 26.07.2012 10:31, schrieb Keith OHara:
>
>> On Thu, 26 Jul 2012 00:51:56 -0700, <benko.pal@gmail.com> wrote:
>>
>>> Marc, please don't throw the whole 2533 issue stuff out; look at the
>>> latest version at
>>> http://codereview.appspot.com/6431044
>>> in particular bar-line.cc and repeat-sign.ly.
>>>
>>> I really don't mind if your patch goes before mine (it would even be
>>> better - why push a change to a file that is to be thrown out by the
>>> next commit), but I do mind that (some of) those cases work right, and
>>> unfortunately I'm far less fluent in Scheme than in C++.
>>>
>>
>> I had suggested that Marc incorporate pending changes to the bar-line code
>> if he is confident in them,
>> but it does seem cleaner make one commit a simple re-implementation of C
>> to Scheme, with the same functionality.
>>
>> He is not really throwing out the issue-2533 stuff, because that stuff is
>> not in the code base at the moment.
>>
>> You will just have to cooperate with Marc or another Scheme programmer, if
>> the re-implementation goes in first.  If it really turns out to be harder
>> for the group to modify the code in Scheme than it was in C, then the
>> reason
>> for Marc's patch would prove to be wrong and we can just go back to C.
>
> I had already implemented Páls work on repeat signs *before* his patch
> was reverted, so reimplementing this in scheme should be straightforward.
>
> ... and wait until you see what will be possible with barlines when part 2
> of issue 1320 is out ;-) Stay tuned!

ok, thanks in advance to everyone!
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b