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

Issue 4490045: Bugfix for issue 1630 (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
12 years, 10 months ago by Janek Warchol
Modified:
12 years, 9 months ago
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

Bugfix for issue 1630 Remove duplicate ties for chords autosplit with Completion_heads_engraver and manually tied.

Patch Set 1 #

Total comments: 12

Patch Set 2 : fixing whitespace and changing autosplit remainder #

Total comments: 3

Patch Set 3 : Compact assignment to autosplit-end #

Total comments: 17

Patch Set 4 : fixing style #

Patch Set 5 : more style fixing #

Total comments: 6

Patch Set 6 : indent issues #

Total comments: 2

Patch Set 7 : indent #

Patch Set 8 : more indent! #

Patch Set 9 : even more indent! I don't know what's going on here! #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+68 lines, -77 lines) Patch
M lily/completion-note-heads-engraver.cc View 1 2 3 4 8 chunks +10 lines, -53 lines 0 comments Download
M lily/tie-engraver.cc View 1 2 3 4 5 6 7 8 16 chunks +56 lines, -24 lines 4 comments Download
M scm/define-music-properties.scm View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 48
Janek Warchol
Finally i'm back in Warsaw and i'm uploading Karin's patch. Sorry for the delay :( ...
12 years, 10 months ago (2011-05-07 09:19:21 UTC) #1
Janek Warchol
I've checked that Karin's patch set passes regression tests successfully. cheers, Janek 2011/5/7 <lemniskata.bernoullego@gmail.com> > ...
12 years, 10 months ago (2011-05-07 10:44:33 UTC) #2
Trevor Daniels
LGTM, except for non-standard indentation which should be corrected, although I've tested it only on ...
12 years, 10 months ago (2011-05-08 08:57:17 UTC) #3
Neil Puttock
http://codereview.appspot.com/4490045/diff/1/lily/completion-note-heads-engraver.cc File lily/completion-note-heads-engraver.cc (right): http://codereview.appspot.com/4490045/diff/1/lily/completion-note-heads-engraver.cc#newcode204 lily/completion-note-heads-engraver.cc:204: event->set_property("autosplit-remainder", Moment(left_to_do_ - note_dur.get_length ()).smobbed_copy ()); Wouldn't this work ...
12 years, 10 months ago (2011-05-09 19:20:25 UTC) #4
karin.hoethker
http://codereview.appspot.com/4490045/diff/1/lily/completion-note-heads-engraver.cc File lily/completion-note-heads-engraver.cc (right): http://codereview.appspot.com/4490045/diff/1/lily/completion-note-heads-engraver.cc#newcode204 lily/completion-note-heads-engraver.cc:204: event->set_property("autosplit-remainder", Moment(left_to_do_ - note_dur.get_length ()).smobbed_copy ()); Indeed, using a ...
12 years, 10 months ago (2011-05-09 19:41:31 UTC) #5
benko.pal
2011/5/9 <karin.hoethker@googlemail.com>: > http://codereview.appspot.com/4490045/diff/1/lily/completion-note-heads-engraver.cc > File lily/completion-note-heads-engraver.cc (right): > > http://codereview.appspot.com/4490045/diff/1/lily/completion-note-heads-engraver.cc#newcode204 > lily/completion-note-heads-engraver.cc:204: > event->set_property("autosplit-remainder", ...
12 years, 10 months ago (2011-05-10 10:19:47 UTC) #6
karin.hoethker
You are right, in version 2.13 even shifted tuplets like "c'8 \times 2/3 { c'4 ...
12 years, 10 months ago (2011-05-25 13:25:27 UTC) #7
Graham Percival (old account)
On 2011/05/25 13:25:27, karin.hoethker wrote: > You are right, in version 2.13 even shifted tuplets ...
12 years, 10 months ago (2011-05-26 12:30:26 UTC) #8
karin_hoethker.de
Hi Graham, I sent two patches by mail to Janek yesterday, because I could not ...
12 years, 10 months ago (2011-05-26 15:49:05 UTC) #9
Janek Warchol
New patches from Karin uploaded. I've got a warning while applying patch 0003 (about autosplit-remainder) ...
12 years, 9 months ago (2011-05-27 21:05:17 UTC) #10
benko.pal
http://codereview.appspot.com/4490045/diff/12001/lily/completion-note-heads-engraver.cc File lily/completion-note-heads-engraver.cc (right): http://codereview.appspot.com/4490045/diff/12001/lily/completion-note-heads-engraver.cc#newcode207 lily/completion-note-heads-engraver.cc:207: event->set_property("autosplit-end", ly_bool2scm (false)); event->set_property ("autosplit-end", ly_bool2scm (left_to_do_ - note_dur.get_length ...
12 years, 9 months ago (2011-05-28 16:10:30 UTC) #11
benko.pal
aargh, that's not too readable. what I actually suggest is replacing lines 204-207 of > ...
12 years, 9 months ago (2011-05-28 16:13:43 UTC) #12
Carl
On 2011/05/28 16:13:43, benko.pal wrote: > aargh, that's not too readable. > what I actually ...
12 years, 9 months ago (2011-05-28 17:45:32 UTC) #13
karin.hoethker
http://codereview.appspot.com/4490045/diff/12001/lily/completion-note-heads-engraver.cc File lily/completion-note-heads-engraver.cc (right): http://codereview.appspot.com/4490045/diff/12001/lily/completion-note-heads-engraver.cc#newcode207 lily/completion-note-heads-engraver.cc:207: event->set_property("autosplit-end", ly_bool2scm (false)); The style guide in Lilypond Contributor's ...
12 years, 9 months ago (2011-05-28 20:27:13 UTC) #14
benko.pal
2011/5/28 <Carl.D.Sorensen@gmail.com>: > On 2011/05/28 16:13:43, benko.pal wrote: >> >> aargh, that's not too readable. ...
12 years, 9 months ago (2011-05-29 08:53:30 UTC) #15
Carl
On 2011/05/29 08:53:30, benko.pal wrote: > I must miss something, to me it's still a ...
12 years, 9 months ago (2011-05-29 12:14:18 UTC) #16
karin.hoethker
On 2011/05/29 12:14:18, Carl wrote: > On 2011/05/29 08:53:30, benko.pal wrote: > > I must ...
12 years, 9 months ago (2011-05-29 12:46:48 UTC) #17
Graham Percival (old account)
http://codereview.appspot.com/4490045/diff/12001/lily/completion-note-heads-engraver.cc File lily/completion-note-heads-engraver.cc (right): http://codereview.appspot.com/4490045/diff/12001/lily/completion-note-heads-engraver.cc#newcode207 lily/completion-note-heads-engraver.cc:207: event->set_property("autosplit-end", ly_bool2scm (false)); On 2011/05/28 20:27:13, karin.hoethker wrote: > ...
12 years, 9 months ago (2011-05-29 15:39:49 UTC) #18
Janek Warchol
New patch set from Karin uploaded.
12 years, 9 months ago (2011-05-29 18:36:20 UTC) #19
Carl
LGTM Carl
12 years, 9 months ago (2011-05-30 01:17:28 UTC) #20
benko.pal
On 2011/05/29 18:36:20, Janek Warchol wrote: > New patch set from Karin uploaded. thanks, looks ...
12 years, 9 months ago (2011-05-30 08:17:09 UTC) #21
Graham Percival (old account)
LGTM
12 years, 9 months ago (2011-06-01 13:26:50 UTC) #22
Neil Puttock
LGTM. http://codereview.appspot.com/4490045/diff/20001/lily/completion-note-heads-engraver.cc File lily/completion-note-heads-engraver.cc (right): http://codereview.appspot.com/4490045/diff/20001/lily/completion-note-heads-engraver.cc#newcode204 lily/completion-note-heads-engraver.cc:204: event->set_property("autosplit-end", set_property ( http://codereview.appspot.com/4490045/diff/20001/lily/completion-note-heads-engraver.cc#newcode205 lily/completion-note-heads-engraver.cc:205: ly_bool2scm (left_to_do_ - ...
12 years, 9 months ago (2011-06-01 21:21:25 UTC) #23
karin.hoethker
I have send the patch set to Janek for upload. http://codereview.appspot.com/4490045/diff/20001/lily/tie-engraver.cc File lily/tie-engraver.cc (right): http://codereview.appspot.com/4490045/diff/20001/lily/tie-engraver.cc#newcode163 ...
12 years, 9 months ago (2011-06-13 22:44:37 UTC) #24
Graham Percival (old account)
http://codereview.appspot.com/4490045/diff/20001/lily/tie-engraver.cc File lily/tie-engraver.cc (right): http://codereview.appspot.com/4490045/diff/20001/lily/tie-engraver.cc#newcode163 lily/tie-engraver.cc:163: /* On 2011/06/13 22:44:37, karin.hoethker wrote: > On 2011/06/01 ...
12 years, 9 months ago (2011-06-13 22:51:39 UTC) #25
Janek Warchol
New patch set uploaded, i think all formatting issues are resolved. Should i run the ...
12 years, 9 months ago (2011-06-14 20:28:14 UTC) #26
Graham Percival (old account)
On 2011/06/14 20:28:14, Janek Warchol wrote: > New patch set uploaded, i think all formatting ...
12 years, 9 months ago (2011-06-14 20:34:38 UTC) #27
Janek Warchol
2011/6/14 <percival.music.ca@gmail.com>: > > On 2011/06/14 20:28:14, Janek Warchol wrote: >> New patch set uploaded. ...
12 years, 9 months ago (2011-06-14 21:27:28 UTC) #28
Neil Puttock
http://codereview.appspot.com/4490045/diff/30001/lily/tie-engraver.cc File lily/tie-engraver.cc (right): http://codereview.appspot.com/4490045/diff/30001/lily/tie-engraver.cc#newcode157 lily/tie-engraver.cc:157: maybe should check positions too. looks like tab -> ...
12 years, 9 months ago (2011-06-14 21:36:59 UTC) #29
Neil Puttock
Hi Janek, To save time fixing indentation, if you have emacs installed there's a script ...
12 years, 9 months ago (2011-06-14 21:43:56 UTC) #30
Graham Percival (old account)
http://codereview.appspot.com/4490045/diff/30001/lily/tie-engraver.cc File lily/tie-engraver.cc (right): http://codereview.appspot.com/4490045/diff/30001/lily/tie-engraver.cc#newcode157 lily/tie-engraver.cc:157: maybe should check positions too. On 2011/06/14 21:36:59, Neil ...
12 years, 9 months ago (2011-06-14 21:52:54 UTC) #31
Graham Percival
On Tue, Jun 14, 2011 at 09:43:56PM +0000, n.puttock@gmail.com wrote: > To save time fixing ...
12 years, 9 months ago (2011-06-14 21:57:32 UTC) #32
Neil Puttock
On 14 June 2011 22:57, Graham Percival <graham@percival-music.ca> wrote: > On Tue, Jun 14, 2011 ...
12 years, 9 months ago (2011-06-14 22:01:02 UTC) #33
Janek Warchol
2011/6/14 <percival.music.ca@gmail.com> > > http://codereview.appspot.com/4490045/diff/30001/lily/tie-engraver.cc#newcode157 > lily/tie-engraver.cc:157: maybe should check positions too. > On 2011/06/14 ...
12 years, 9 months ago (2011-06-14 22:19:16 UTC) #34
Graham Percival
On Wed, Jun 15, 2011 at 12:19:01AM +0200, Janek Warchoł wrote: > 2011/6/14 <percival.music.ca@gmail.com> > ...
12 years, 9 months ago (2011-06-14 22:24:21 UTC) #35
Janek Warchol
Thanks to Neil's help, it's done. http://codereview.appspot.com/4490045/diff/30001/lily/tie-engraver.cc File lily/tie-engraver.cc (right): http://codereview.appspot.com/4490045/diff/30001/lily/tie-engraver.cc#newcode157 lily/tie-engraver.cc:157: maybe should check ...
12 years, 9 months ago (2011-06-14 22:48:30 UTC) #36
c_sorensen
On 6/14/11 4:19 PM, "Janek Warchoł" <lemniskata.bernoullego@gmail.com> wrote: > 2011/6/14 <percival.music.ca@gmail.com> >> and Janek: you ...
12 years, 9 months ago (2011-06-14 22:49:43 UTC) #37
Graham Percival (old account)
http://codereview.appspot.com/4490045/diff/25003/lily/tie-engraver.cc File lily/tie-engraver.cc (right): http://codereview.appspot.com/4490045/diff/25003/lily/tie-engraver.cc#newcode189 lily/tie-engraver.cc:189: /* these should be tabs hint: look at the ...
12 years, 9 months ago (2011-06-14 23:07:53 UTC) #38
Janek Warchol
indent indent indent http://codereview.appspot.com/4490045/diff/25003/lily/tie-engraver.cc File lily/tie-engraver.cc (right): http://codereview.appspot.com/4490045/diff/25003/lily/tie-engraver.cc#newcode189 lily/tie-engraver.cc:189: /* On 2011/06/14 23:07:54, Graham Percival ...
12 years, 9 months ago (2011-06-15 08:57:16 UTC) #39
Graham Percival (old account)
I've identified a few more questionable lines, although maybe you should wait for a lilypond ...
12 years, 9 months ago (2011-06-15 09:29:00 UTC) #40
Janek Warchol
2011/6/15 <percival.music.ca@gmail.com>: > I've identified a few more questionable lines, although maybe you should > ...
12 years, 9 months ago (2011-06-15 11:02:22 UTC) #41
karin.hoethker
"All indentation should be done with spaces". This is why I changed some tabs to ...
12 years, 9 months ago (2011-06-15 11:09:22 UTC) #42
Janek Warchol
W dniu 15 czerwca 2011 00:49 użytkownik Carl Sorensen <c_sorensen@byu.edu> napisał: > > On 6/14/11 ...
12 years, 9 months ago (2011-06-15 11:32:04 UTC) #43
Graham Percival
On Wed, Jun 15, 2011 at 01:02:07PM +0200, Janek Warchoł wrote: > 2011/6/15 <percival.music.ca@gmail.com>: > ...
12 years, 9 months ago (2011-06-15 11:47:25 UTC) #44
Janek Warchol
2011/6/15 Graham Percival <graham@percival-music.ca>: > On Wed, Jun 15, 2011 at 01:02:07PM +0200, Janek Warchoł ...
12 years, 9 months ago (2011-06-15 12:05:25 UTC) #45
Graham Percival
On Wed, Jun 15, 2011 at 02:05:10PM +0200, Janek Warchoł wrote: > Patches attached. Rebased ...
12 years, 9 months ago (2011-06-15 12:40:56 UTC) #46
Graham Percival (old account)
I've pushed a modified version of this. Given the confusion about our C++ style, I ...
12 years, 9 months ago (2011-06-15 13:09:06 UTC) #47
Janek Warchol
12 years, 9 months ago (2011-06-15 13:26:16 UTC) #48
2011/6/15  <percival.music.ca@gmail.com>:
> I've pushed a modified version of this.

wOOt!
YEEHAW!
Hooray!

> Given the confusion about our C++ style, I removed a few changes which
> were probably good.  In some places I added an extra (pointless) space;
> in other places I changed a tab character to a space to match the
> existing (broken) indentation, etc.  The goal was to eliminate as many
> changes as possible.
>
> I know that Karin and Janek worked hard on some of those style changes,
> so I'm sorry for removing that work -- but I thought that avoiding ANY
> change which was not strictly necessary would less the chance of anybody
> finding something to complain about.

meh.

thanks for pushing!!
Janek
Sign in to reply to this message.

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