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

Issue 4536068: Adds longas, maximas and non-standard tweaks to MultiMeasureRest (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years, 1 month ago by Bertrand Bordage
Modified:
7 years, 10 months ago
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

Adds longas, maximas and non-standard tweaks to MultiMeasureRest

Patch Set 1 #

Total comments: 2

Patch Set 2 : Nitpicks #

Total comments: 1

Patch Set 3 : 'measure_duration_log' moved #

Total comments: 3

Patch Set 4 : Neil's changes #

Total comments: 4

Patch Set 5 : another bunch of nitpicks #

Patch Set 6 : Another static_cast removed. #

Total comments: 11

Patch Set 7 : Improve recent MultiMeasureRest changes. #

Total comments: 1

Patch Set 8 : Reverting dynamic_cast deletion. #

Patch Set 9 : Creation of round-exceptions. #

Patch Set 10 : Newbie mistake fixed. #

Total comments: 3

Patch Set 11 : Applies Pal's changes & fixes the "hole" issue in church_rest. #

Total comments: 2

Patch Set 12 : Better church rests calculation and update following fixcc.py run. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+106 lines, -66 lines) Patch
M input/regression/multi-measure-rest-tweaks.ly View 1 2 3 4 5 6 7 8 9 10 1 chunk +16 lines, -5 lines 0 comments Download
M lily/multi-measure-rest.cc View 1 2 3 4 5 6 7 8 9 10 11 6 chunks +83 lines, -59 lines 0 comments Download
M scm/define-grob-properties.scm View 1 2 3 4 5 6 7 8 9 10 2 chunks +5 lines, -1 line 0 comments Download
M scm/define-grobs.scm View 1 2 3 4 5 6 7 8 9 10 2 chunks +2 lines, -1 line 0 comments Download

Messages

Total messages: 40
Carl
This looks generally good to me. I'm concerned about the name "duration-log-list". I've commented more ...
8 years, 1 month ago (2011-05-18 13:52:19 UTC) #1
Bertrand Bordage
Just one regtest change : in multi-measure-rest.ly, the two longas of the third measure are ...
8 years, 1 month ago (2011-05-18 13:54:43 UTC) #2
Bertrand Bordage
> This name is nice and generic, which is good. Bit it has no information ...
8 years, 1 month ago (2011-05-18 16:10:12 UTC) #3
benko.pal
http://codereview.appspot.com/4536068/diff/1/scm/define-grobs.scm File scm/define-grobs.scm (right): http://codereview.appspot.com/4536068/diff/1/scm/define-grobs.scm#newcode1305 scm/define-grobs.scm:1305: (duration-log-list . (0 -1 -2 -3)) On 2011/05/18 13:54:43, ...
8 years, 1 month ago (2011-05-18 19:48:30 UTC) #4
Bertrand Bordage
I applied it to the more recent git HEAD without any problem. By maxima, I ...
8 years, 1 month ago (2011-05-18 20:05:12 UTC) #5
Bertrand Bordage
Sorry, I misread your first sentence. Forget the first line of my previous post.
8 years, 1 month ago (2011-05-18 20:08:00 UTC) #6
benko.pal
> By maxima, I mean the glyph "rests.M3". > I chose to use maximas because ...
8 years, 1 month ago (2011-05-19 15:43:06 UTC) #7
Bertrand Bordage
Nitpicks done : "duration-log-list" changed for "usable-duration-logs" (Thanks Carl !) Whitespaces and tabs removed. Bertrand
8 years, 1 month ago (2011-05-26 21:04:30 UTC) #8
Carl
The code looks fine in general, but I question two of the properties that have ...
8 years ago (2011-05-30 02:07:40 UTC) #9
Bertrand Bordage
longest-church-rest is the longest rest displayed in church rests. One may want to only print ...
8 years ago (2011-05-30 10:56:01 UTC) #10
Bertrand Bordage
Carl's suggestions done ! Bertrand
8 years ago (2011-06-02 18:14:54 UTC) #11
Neil Puttock
http://codereview.appspot.com/4536068/diff/10008/lily/multi-measure-rest-engraver.cc File lily/multi-measure-rest-engraver.cc (right): http://codereview.appspot.com/4536068/diff/10008/lily/multi-measure-rest-engraver.cc#newcode232 lily/multi-measure-rest-engraver.cc:232: last_rest_->set_property ("measure-length", sml); This duplicates the setting for the ...
8 years ago (2011-06-02 18:44:21 UTC) #12
Bertrand Bordage
Thanks, 'tis done.
8 years ago (2011-06-02 19:58:31 UTC) #13
Neil Puttock
LGTM. http://codereview.appspot.com/4536068/diff/17008/input/regression/multi-measure-rest-standard.ly File input/regression/multi-measure-rest-standard.ly (right): http://codereview.appspot.com/4536068/diff/17008/input/regression/multi-measure-rest-standard.ly#newcode20 input/regression/multi-measure-rest-standard.ly:20: } newline http://codereview.appspot.com/4536068/diff/17008/input/regression/multi-measure-rest-tweaks.ly File input/regression/multi-measure-rest-tweaks.ly (right): http://codereview.appspot.com/4536068/diff/17008/input/regression/multi-measure-rest-tweaks.ly#newcode14 input/regression/multi-measure-rest-tweaks.ly:14: ...
8 years ago (2011-06-04 16:01:14 UTC) #14
Bertrand Bordage
Thanks again, Neil ! I applied these.
8 years ago (2011-06-06 10:13:52 UTC) #15
Graham Percival (old account)
Please send me the final version with git format-patch origin so that I can push ...
8 years ago (2011-06-08 19:09:03 UTC) #16
Neil Puttock
On 6 June 2011 11:13, <bordage.bertrand@gmail.com> wrote: > Thanks again, Neil ! > I applied ...
8 years ago (2011-06-12 14:41:40 UTC) #17
Bertrand Bordage
Yes, thanks. 'Tis done. Bertrand
8 years ago (2011-06-13 20:02:44 UTC) #18
Graham Percival (old account)
thanks, pushed.
8 years ago (2011-06-14 19:10:30 UTC) #19
dak
I would suggest reverting this patch as "needs work" for now. http://codereview.appspot.com/4536068/diff/30001/lily/multi-measure-rest.cc File lily/multi-measure-rest.cc (right): ...
7 years, 11 months ago (2011-07-26 13:08:09 UTC) #20
Bertrand Bordage
Okay, I will fix these before the end of the day. Thanks for taking time ...
7 years, 11 months ago (2011-07-26 13:20:45 UTC) #21
dak
http://codereview.appspot.com/4536068/diff/30001/lily/multi-measure-rest.cc File lily/multi-measure-rest.cc (right): http://codereview.appspot.com/4536068/diff/30001/lily/multi-measure-rest.cc#newcode254 lily/multi-measure-rest.cc:254: Stencil r (musfont->find_by_name ("rests." + to_string (k))); On 2011/07/26 ...
7 years, 11 months ago (2011-07-26 16:30:43 UTC) #22
Bertrand Bordage
> http://codereview.appspot.com/4536068/diff/30001/lily/multi-measure-rest.cc#newcode130 > lily/multi-measure-rest.cc:130: double duration_log = -log2 > (ml.Rational::to_double ()); > log2 is a ...
7 years, 11 months ago (2011-07-26 22:47:53 UTC) #23
Keith
On 2011/07/26 22:47:53, Bertrand Bordage wrote: > > > > For example, what to do ...
7 years, 11 months ago (2011-07-27 03:51:32 UTC) #24
Keith
http://codereview.appspot.com/4536068/diff/30001/lily/multi-measure-rest.cc File lily/multi-measure-rest.cc (right): http://codereview.appspot.com/4536068/diff/30001/lily/multi-measure-rest.cc#newcode131 lily/multi-measure-rest.cc:131: int measure_duration_log = int (ceil (duration_log)); To me, having ...
7 years, 11 months ago (2011-07-27 04:13:47 UTC) #25
Bertrand Bordage
This is really dirty, but here's an update that does what you want (I hope). ...
7 years, 11 months ago (2011-07-27 16:52:28 UTC) #26
dak
http://codereview.appspot.com/4536068/diff/30001/lily/multi-measure-rest.cc File lily/multi-measure-rest.cc (right): http://codereview.appspot.com/4536068/diff/30001/lily/multi-measure-rest.cc#newcode131 lily/multi-measure-rest.cc:131: int measure_duration_log = int (ceil (duration_log)); On 2011/07/27 04:13:48, ...
7 years, 11 months ago (2011-07-27 17:25:58 UTC) #27
MikeSol
http://codereview.appspot.com/4536068/diff/30001/lily/multi-measure-rest.cc File lily/multi-measure-rest.cc (right): http://codereview.appspot.com/4536068/diff/30001/lily/multi-measure-rest.cc#newcode131 lily/multi-measure-rest.cc:131: int measure_duration_log = int (ceil (duration_log)); On 2011/07/27 17:25:59, ...
7 years, 11 months ago (2011-07-27 17:49:01 UTC) #28
Keith
Works good for me. On 2011/07/27 17:49:01, MikeSol wrote: > Why not just an array ...
7 years, 11 months ago (2011-07-28 02:53:14 UTC) #29
Bertrand Bordage
> http://codereview.appspot.com/4536068/diff/37002/lily/multi-measure-rest.cc#newcode241 > lily/multi-measure-rest.cc:241: length = (2 << -i) / 2; > The division by ...
7 years, 11 months ago (2011-07-28 12:24:09 UTC) #30
benko.pal
> To be honest, I never understood well how this bitset operator works. adds trailing ...
7 years, 11 months ago (2011-07-28 12:32:12 UTC) #31
Bertrand Bordage
> > I should have write "2 << (-i + 1)" instead of "/2"... > ...
7 years, 11 months ago (2011-07-28 12:41:58 UTC) #32
benko.pal
hi Bertrand, I started at the patch but it's quite difficult for now, I hope ...
7 years, 11 months ago (2011-07-28 12:46:53 UTC) #33
Bertrand Bordage
It doesn't need to be ordered. It can have holes, but there's a small issue ...
7 years, 11 months ago (2011-07-28 13:01:06 UTC) #34
benko.pal
hi Bertrand, the patch is correct, AFAICS; see some minor improvements below. minor concerns (which ...
7 years, 11 months ago (2011-07-31 09:16:02 UTC) #35
Bertrand Bordage
Hi Pál (besides, are you Pál Benkő the chess master?) Thanks for this nice review. ...
7 years, 11 months ago (2011-07-31 15:35:46 UTC) #36
pkx166h
Passes Make and there is a reg test difference which looks ok. I created a ...
7 years, 11 months ago (2011-07-31 20:10:30 UTC) #37
pkx166h
On 2011/07/31 20:10:30, J_lowe wrote: > Passes Make and there is a reg test difference ...
7 years, 11 months ago (2011-07-31 22:08:33 UTC) #38
benko.pal
hi Bertrand, (I'm not a grand-master, :( ) good that you caught the problem with ...
7 years, 11 months ago (2011-08-01 11:31:41 UTC) #39
Bertrand Bordage
7 years, 10 months ago (2011-08-04 15:20:16 UTC) #40
Hi,

I think I found a proper way to calculate church rests.
I also updated so that it applies on the latest git HEAD.

Bertrand.
Sign in to reply to this message.

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