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

Issue 6457049: Set indent based on instrument name (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 8 months ago by PhilEHolmes
Modified:
11 years, 3 months ago
Reviewers:
Keith, bernard, dak, mike7, MikeSol, mail, Graham Percival, email
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

An updated version which takes the instrumentName length from the interval from the stencil. This took me most of the day to work out :-(. Please review the general approach. I get the occasional fail on make test which I will look at tomorrow.

Patch Set 1 #

Patch Set 2 : Updated to calculate text size correctly #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+50 lines, -5 lines) Patch
M lily/include/output-def.hh View 1 1 chunk +1 line, -1 line 0 comments Download
M lily/instrument-name-engraver.cc View 1 4 chunks +29 lines, -0 lines 3 comments Download
M lily/output-def.cc View 1 2 chunks +20 lines, -4 lines 2 comments Download

Messages

Total messages: 18
PhilEHolmes
My first attempt at coding in LilyPond. Treat me gently...
11 years, 8 months ago (2012-07-29 18:20:16 UTC) #1
dak
On 2012/07/29 18:20:16, PhilEHolmes wrote: > My first attempt at coding in LilyPond. Treat me ...
11 years, 8 months ago (2012-07-29 20:16:55 UTC) #2
email_philholmes.net
----- Original Message ----- From: <dak@gnu.org> To: <PhilEHolmes@googlemail.com>; <graham@percival-music.ca> Cc: <lilypond-devel@gnu.org>; <reply@codereview-hr.appspotmail.com> Sent: Sunday, July ...
11 years, 8 months ago (2012-07-29 20:46:28 UTC) #3
PhilEHolmes
Please review.
11 years, 8 months ago (2012-07-30 17:45:20 UTC) #4
Graham Percival
Little nitpicks based on my C++ experience in other projects, with no knowledge whatsoever of ...
11 years, 8 months ago (2012-07-30 17:54:36 UTC) #5
email_philholmes.net
----- Original Message ----- From: <graham@percival-music.ca> To: <PhilEHolmes@googlemail.com>; <dak@gnu.org>; <email@philholmes.net> Cc: <lilypond-devel@gnu.org>; <reply@codereview-hr.appspotmail.com> Sent: Monday, ...
11 years, 8 months ago (2012-07-30 21:14:41 UTC) #6
bernard_marcade.biz
On Mon, Jul 30, 2012 at 10:14:37PM +0100, Phil Holmes wrote: > ----- Original Message ...
11 years, 8 months ago (2012-07-30 22:44:33 UTC) #7
Graham Percival
On Mon, Jul 30, 2012 at 11:44:28PM +0100, Bernard Hurley wrote: > On Mon, Jul ...
11 years, 8 months ago (2012-07-30 23:12:49 UTC) #8
mail_philholmes.net
----- Original Message ----- From: "Graham Percival" <graham@percival-music.ca> To: "Bernard Hurley" <bernard@marcade.biz> Cc: "Phil Holmes" ...
11 years, 8 months ago (2012-07-31 08:44:04 UTC) #9
dak
Graham Percival <graham@percival-music.ca> writes: > On Mon, Jul 30, 2012 at 11:44:28PM +0100, Bernard Hurley ...
11 years, 8 months ago (2012-07-31 10:59:36 UTC) #10
bernard_marcade.biz
On Tue, Jul 31, 2012 at 12:59:16PM +0200, David Kastrup wrote: > Graham Percival <graham@percival-music.ca> ...
11 years, 8 months ago (2012-07-31 12:45:50 UTC) #11
dak
http://codereview.appspot.com/6457049/diff/4001/lily/output-def.cc File lily/output-def.cc (right): http://codereview.appspot.com/6457049/diff/4001/lily/output-def.cc#newcode275 lily/output-def.cc:275: set_inst_name_len (Real long_inst_name_len, Real short_inst_name_len) Correct me if I ...
11 years, 8 months ago (2012-07-31 13:36:30 UTC) #12
mail_philholmes.net
----- Original Message ----- From: <dak@gnu.org> To: <PhilEHolmes@googlemail.com>; <graham@percival-music.ca>; <email@philholmes.net>; <bernard@marcade.biz>; <mail@philholmes.net> Cc: <lilypond-devel@gnu.org>; <reply@codereview-hr.appspotmail.com> ...
11 years, 8 months ago (2012-07-31 14:04:23 UTC) #13
MikeSol
Hey Phil! First and foremost, congrats on this work. I'm thrilled to see you venturing ...
11 years, 8 months ago (2012-08-01 06:45:22 UTC) #14
Keith
On 2012/08/01 06:45:22, MikeSol wrote: > Avoid measuring extents when engraving is happening because they ...
11 years, 8 months ago (2012-08-04 07:28:29 UTC) #15
dak
On 2012/08/04 07:28:29, Keith wrote: > On 2012/08/01 06:45:22, MikeSol wrote: > > > Avoid ...
11 years, 8 months ago (2012-08-04 07:37:21 UTC) #16
Keith
On Sat, 04 Aug 2012 00:37:21 -0700, <dak@gnu.org> wrote: > On 2012/08/04 07:28:29, Keith wrote: ...
11 years, 8 months ago (2012-08-04 08:04:18 UTC) #17
mike7
11 years, 7 months ago (2012-08-04 15:59:19 UTC) #18
On 4 août 2012, at 09:28, k-ohara5a5a@oco.net wrote:

> On 2012/08/01 06:45:22, MikeSol wrote:
> 
>> Avoid measuring extents when engraving is happening because they could
> be
>> dependent on other callbacks which could trigger many layout decisions
> before
>> engraving is finished.
> 
> 
> This is a case where measuring extents early seems appropriate.
> 
> Phil wants the line-indents, thus line-lengths, thus some very early
> layout decisions, to depend on the extents of the instrument names.
> 
> The dependency is reversed in this case from the more usual situations
> where Mike's suggestion holds, such as the extent of a hairpin, which
> depends on the layout.
> 

I agree that the calculation will happen early (definitely before line
breaking), but if someone writes an exotic callback for the stencil function of
instrument name that somehow is linked to other grobs, it should only be called
after engraving is done.

>> Here, what I would do is use the Pointer_group_interface to add two
> grob arrays
> 
> Running through those arrays and re-checking the lengths of the
> instrument names every time we look up the line-indent, would let the
> line-breaker see if those lengths changed for some reason depending on
> the evolving layout.  I do not expect the line-breaker to behave
> sensibly if the indents change like this.

I didn't express myself clearly.  I meant that there should be a finalize method
in the engraver that loops through the vectors and adds them to a grob array via
Pointer_group_interface::add_grob.  finalize is only ever called once for a
given engraver.

> 
>> Properties, in LilyPond, are the best way to stash
>> information.
> 
> Phil, working with properties, or in this case Scheme variables
> associated with layout, does seems necessary.  See set_variable().
> Old scores where we chose \paper {indent = 2.3\cm } to fit the
> instrument names should continue to work.
> If you can figure out enough Scheme to have LilyPond initialize indent
> so that it evaluates a different Scheme variable (say, default-indent)
> and have the engraver updates that variable, then we can have
> automatically-adapting intends by default, and let users override by
> replacing the link to default-indent with their own choice.
> 
> http://codereview.appspot.com/6457049/

I agree that users should be able to override something, but they should be able
to override a property of InstrumentName or System (depending on how things are
implemented).
I'm always wary of Scheme variables because of global scopes and multiple
scores: that has come to bite me in the past.  I generally steer clear of this
approach and try to make things grob-centric (or if necessary prob-centric). 
There are some things that fall outside the scope of grob-related issues (like
the paper bloc) but I think it's best to try to use grobs, callbacks, and
properties as much as possible.  I'm definitely not saying that your solution
won't work, but I think that going down the grob road is more architecturally
sound.  Of course, if Phil has time, he can investigate the plausibility of both
:-)

Cheers,
MS
Sign in to reply to this message.

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