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

Issue 6201068: Allows lyrics to slide under TimeSignature when OctaveEight present. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 10 months ago by MikeSol
Modified:
11 years, 7 months ago
Reviewers:
mike, janek, dak, carl.d.sorensen
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

Allows lyrics to slide under TimeSignature when OctaveEight present.

Patch Set 1 #

Total comments: 5

Patch Set 2 : Fixes indentation #

Unified diffs Side-by-side diffs Delta from patch set Stats (+44 lines, -6 lines) Patch
A input/regression/lyric-octave-eight.ly View 1 chunk +16 lines, -0 lines 0 comments Download
M lily/pure-from-neighbor-engraver.cc View 1 3 chunks +28 lines, -6 lines 0 comments Download

Messages

Total messages: 7
janek
http://codereview.appspot.com/6201068/diff/1/lily/pure-from-neighbor-engraver.cc File lily/pure-from-neighbor-engraver.cc (right): http://codereview.appspot.com/6201068/diff/1/lily/pure-from-neighbor-engraver.cc#newcode56 lily/pure-from-neighbor-engraver.cc:56: in_same_column (Grob *g1, Grob *g2) That's problably the most ...
11 years, 10 months ago (2012-05-10 21:40:48 UTC) #1
Carl
LGTM, but I concur with the line-wrap comment of Janek.
11 years, 10 months ago (2012-05-10 23:48:29 UTC) #2
MikeSol
http://codereview.appspot.com/6201068/diff/1/lily/pure-from-neighbor-engraver.cc File lily/pure-from-neighbor-engraver.cc (right): http://codereview.appspot.com/6201068/diff/1/lily/pure-from-neighbor-engraver.cc#newcode56 lily/pure-from-neighbor-engraver.cc:56: in_same_column (Grob *g1, Grob *g2) On 2012/05/10 21:40:48, janek ...
11 years, 10 months ago (2012-05-11 05:23:27 UTC) #3
dak
http://codereview.appspot.com/6201068/diff/1/lily/pure-from-neighbor-engraver.cc File lily/pure-from-neighbor-engraver.cc (right): http://codereview.appspot.com/6201068/diff/1/lily/pure-from-neighbor-engraver.cc#newcode56 lily/pure-from-neighbor-engraver.cc:56: in_same_column (Grob *g1, Grob *g2) On 2012/05/11 05:23:27, MikeSol ...
11 years, 10 months ago (2012-05-11 07:09:04 UTC) #4
mike_apollinemike.com
On 11 mai 2012, at 09:09, dak@gnu.org wrote: > > http://codereview.appspot.com/6201068/diff/1/lily/pure-from-neighbor-engraver.cc > File lily/pure-from-neighbor-engraver.cc (right): ...
11 years, 10 months ago (2012-05-11 08:02:16 UTC) #5
dak
"mike@apollinemike.com" <mike@apollinemike.com> writes: > On 11 mai 2012, at 09:09, dak@gnu.org wrote: > >> >> ...
11 years, 10 months ago (2012-05-11 08:11:18 UTC) #6
MikeSol
11 years, 10 months ago (2012-05-17 06:05:39 UTC) #7
On 2012/05/11 08:11:18, dak wrote:
> "mike@apollinemike.com" <mailto:mike@apollinemike.com> writes:
> 
> > On 11 mai 2012, at 09:09, mailto:dak@gnu.org wrote:
> >
> >> 
> >>
>
http://codereview.appspot.com/6201068/diff/1/lily/pure-from-neighbor-engraver.cc
> >> File lily/pure-from-neighbor-engraver.cc (right):
> >> 
> >>
>
http://codereview.appspot.com/6201068/diff/1/lily/pure-from-neighbor-engraver...
> >> lily/pure-from-neighbor-engraver.cc:56: in_same_column (Grob *g1, Grob
> >> *g2)
> >> On 2012/05/11 05:23:27, MikeSol wrote:
> >>> On 2012/05/10 21:40:48, janek wrote:
> >>>> That's problably the most stupid question ever, but why this doesn't
> >> it begin
> >>>> with Pure_from_neighbor_engraver:: ?  I don't see it being special.
> >> 
> >>> It doesn't need to be a class method - it'll never be called outside
> >> of this
> >>> file.  It is just a small helper function.
> >> 
> >> Then it should be declared static, right?
> >> 
> >
> > Why would it be part of the class?  It's just a small helper function
> > - architecturally, I don't see it as being part of a class.  There are
> > small helper functions like this all over the code base.
> 
> I did not say it should be declared a class function.  I said that it
> should be declared static since it is not referenced outside of the
> source file.
> 
> -- 
> David Kastrup

Hey David,

I don't completely understand the hows and whys of it, and there are many
functions in the source code that are only used in one file but are not declared
static.  If all of these should be declared static, I'd recommend opening a
tracker issue describing the general problem.

Cheers,
MS
Sign in to reply to this message.

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