Issue 7453046: Allows minimum-length to work for end-of-line spanners.

dak

https://codereview.appspot.com/7453046/diff/1/input/regression/minimum-length-end-line.ly File input/regression/minimum-length-end-line.ly (right): https://codereview.appspot.com/7453046/diff/1/input/regression/minimum-length-end-line.ly#newcode10 input/regression/minimum-length-end-line.ly:10: \override TextSpanner.springs-and-rods = #ly:spanner::set-spacing-rods Why is this override needed ...

11 years, 1 month ago (2013-03-08 13:10:57 UTC) #2

mike7

On 8 mars 2013, at 14:10, dak@gnu.org wrote: > > https://codereview.appspot.com/7453046/diff/1/input/regression/minimum-length-end-line.ly > File input/regression/minimum-length-end-line.ly (right): ...

11 years, 1 month ago (2013-03-09 07:18:50 UTC) #3

dak

On 2013/03/09 07:18:50, mike7 wrote: > On 8 mars 2013, at 14:10, mailto:dak@gnu.org wrote: > ...

11 years, 1 month ago (2013-03-09 08:51:55 UTC) #4

mike7

On 8 mars 2013, at 14:10, dak@gnu.org wrote: > > https://codereview.appspot.com/7453046/diff/1/input/regression/minimum-length-end-line.ly > File input/regression/minimum-length-end-line.ly (right): ...

11 years, 1 month ago (2013-03-09 09:28:12 UTC) #5

mike7

On 9 mars 2013, at 09:51, dak@gnu.org wrote: > On 2013/03/09 07:18:50, mike7 wrote: >> ...

11 years, 1 month ago (2013-03-10 00:32:43 UTC) #6

On 9 mars 2013, at 09:51, dak@gnu.org wrote:

> On 2013/03/09 07:18:50, mike7 wrote:
>> On 8 mars 2013, at 14:10, mailto:dak@gnu.org wrote:
> 
>> >
>> >
> 
>
https://codereview.appspot.com/7453046/diff/1/input/regression/minimum-length...
>> > File input/regression/minimum-length-end-line.ly (right):
>> >
>> >
> 
>
https://codereview.appspot.com/7453046/diff/1/input/regression/minimum-length...
>> > input/regression/minimum-length-end-line.ly:10: \override
>> > TextSpanner.springs-and-rods = #ly:spanner::set-spacing-rods
>> > Why is this override needed for the regtest?  The other overrides
> are
>> > obvious user-accessible overrides for triggering the tested
>> > functionality.
>> >
>> > But should _this_ override not be the default?
>> >
>> > https://codereview.appspot.com/7453046/
> 
>> Perhaps open a tracker issue for this?
>> The question is not only valid for text spanners but also hairpins,
> glissandos,
>> etc.
> 
> Last time I looked, this issue purportedly "Allows minimum-length to
> work for end-of-line spanners."  And according to the regtest, it does
> not do the job.  Without additional messing with springs-and-rods it
> does not allow minimum-length to work for end-of-line-spanners.
> 

The definition of "allow" in the New Oxford American Dictionary is "give the
necessary time or opportunity for."  This patch gives spanners the opportunity
to have minimum length work at the end of the line.

Additional messing with springs and rods is because minimum-length is currently
implemented by four different interfaces (lyric-hyphen, multi-measure-rest,
ottava-bracket and spanner) and is also looked up in lyric-extender in a way
that does not correspond to its docstring.  So, certain uses of minimum length
require the additional override whereas others don't.  I do not think this is
ideal, which is why I proposed a few weeks ago standardizing property names
across interfaces.  It seems like the issues you are raising above and below
have less to do with this patch and more to do with the multiple implementation
of minimum-length and the use of the springs-and-rods property.

> The only thing more frustrating than missing functionality is
> purportedly available functionality that needs non-user-comprehensible
> trickery to actually work.

I do not have a problem with the current need to set the springs-and-rods
separately.
If you do, please file an issue then to fix this as well as the following
snippets in the Documentation:
-)
http://lilypond.org/doc/v2.17/Documentation/notation/expressive-marks-as-line...
-) http://lilypond.org/doc/v2.17/Documentation/notation/spanners.html,
specifically "For some layout objects, the minimum-length property becomes
effective only if the set-spacing-rodsprocedure is called explicitly. To do
this, the springs-and-rods property should be set to
ly:spanner::set-spacing-rods. For example, the minimum length of a glissando has
no effect unless the springs-and-rodsproperty is set."

> So it is not clear that using this functionality would not
> break other things elsewhere.
> 

I'm positive it would because of the way that minimum-length is multiply
defined.  That is why this patch is intended for the "some layout objects"
discussed above like the TextSpanner.

I agree that the multiple use of the minimum-length property should be changed,
but this seems like the business of another patch.

If the regtest is bothering you that much, I can just eliminate it from this
patch.  It won't change the better functionality of this patch and will just
stop shedding light on the issue I'm discussing above.  But that does not seem
like a good solution, nor does setting springs-and-rods with a default
property.

Cheers,
MS

Sign in to reply to this message.

dak

On 2013/03/10 00:32:43, mike7 wrote: > >> > Why is this override needed for the ...

11 years, 1 month ago (2013-03-11 10:18:59 UTC) #7

On 2013/03/10 00:32:43, mike7 wrote:

> >> > Why is this override needed for the regtest?  The other overrides
> > are
> >> > obvious user-accessible overrides for triggering the tested
> >> > functionality.
> >> >
> >> > But should _this_ override not be the default?
> >> >
> >> > https://codereview.appspot.com/7453046/
> > 
> >> Perhaps open a tracker issue for this?
> >> The question is not only valid for text spanners but also hairpins,
> > glissandos,
> >> etc.
> > 
> > Last time I looked, this issue purportedly "Allows minimum-length to
> > work for end-of-line spanners."  And according to the regtest, it does
> > not do the job.  Without additional messing with springs-and-rods it
> > does not allow minimum-length to work for end-of-line-spanners.
> > 
> 
[...]

> Additional messing with springs and rods is because minimum-length
> is currently implemented by four different interfaces (lyric-hyphen,
> multi-measure-rest, ottava-bracket and spanner) and is also looked
> up in lyric-extender in a way that does not correspond to its
> docstring.  So, certain uses of minimum length require the
> additional override whereas others don't.  I do not think this is
> ideal, which is why I proposed a few weeks ago standardizing
> property names across interfaces.  It seems like the issues you are
> raising above and below have less to do with this patch and more to
> do with the multiple implementation of minimum-length and the use of
> the springs-and-rods property.

This is a typical case of "Somebody Else's Problem".  If there is a
party and you place a chair in the worst possible place, like
somewhere in a doorway, people will walk around it, squeeze themselves
through, get annoyed again and again.

That's our current state.  Now someone wants to do a polonaise and
since the chair is in the way, he proposes putting a plank across it.

Now we are moving from ridiculous to absurd, and it becomes harder to
do the right thing.

Yes, I am fully aware that you are not responsible for the chair in
the way of your polonaise.

Bit it is time to move it aside rather than taking it into account in
our planning and make it even harder to get into a proper state.  In
particular since your coding style requires a lot of reverse
engineering in order to figure out the hidden assumptions and
dependencies.  So that makes it doubly desirable to not add further
dependencies on broken behavior but first clean up the foundations.

Yes, that's not a problem caused by your patch, but the consequences
are acerbated by it.

> > The only thing more frustrating than missing functionality is
> > purportedly available functionality that needs
> > non-user-comprehensible trickery to actually work.
> 
> I do not have a problem with the current need to set the
> springs-and-rods separately.

Of course you don't, and nobody keeps you from applying this sort of
patch to your own private code base without doing the necessary
cleanup before.

But you are not the metric for what goes into LilyPond's own
repository and what not.  The functionality that goes into LilyPond
has to work for the average user encountering that problem space.  The
solutions have to have a meaningful correspondence in complexity to
the problem and not require "don't think about it" steps.

> I'm positive it would because of the way that minimum-length is
> multiply defined.  That is why this patch is intended for the "some
> layout objects" discussed above like the TextSpanner.  I agree that
> the multiple use of the minimum-length property should be changed,
> but this seems like the business of another patch.

Sure.  But that patch will be harder to do once this patch _relies_ on
the broken behavior, and people write code _relying_ on this patch.

> If the regtest is bothering you that much, I can just eliminate it
> from this patch.

There is no point in hiding the symptoms of a problem away.  That only
makes things even harder in future.

Sign in to reply to this message.

MikeSol

On 2013/03/11 10:18:59, dak wrote: > On 2013/03/10 00:32:43, mike7 wrote: > > > >> ...

11 years, 1 month ago (2013-03-17 07:10:23 UTC) #8

On 2013/03/11 10:18:59, dak wrote:
> On 2013/03/10 00:32:43, mike7 wrote:
> 
> > >> > Why is this override needed for the regtest?  The other overrides
> > > are
> > >> > obvious user-accessible overrides for triggering the tested
> > >> > functionality.
> > >> >
> > >> > But should _this_ override not be the default?
> > >> >
> > >> > https://codereview.appspot.com/7453046/
> > > 
> > >> Perhaps open a tracker issue for this?
> > >> The question is not only valid for text spanners but also hairpins,
> > > glissandos,
> > >> etc.
> > > 
> > > Last time I looked, this issue purportedly "Allows minimum-length to
> > > work for end-of-line spanners."  And according to the regtest, it does
> > > not do the job.  Without additional messing with springs-and-rods it
> > > does not allow minimum-length to work for end-of-line-spanners.
> > > 
> > 
> [...]
> 
> > Additional messing with springs and rods is because minimum-length
> > is currently implemented by four different interfaces (lyric-hyphen,
> > multi-measure-rest, ottava-bracket and spanner) and is also looked
> > up in lyric-extender in a way that does not correspond to its
> > docstring.  So, certain uses of minimum length require the
> > additional override whereas others don't.  I do not think this is
> > ideal, which is why I proposed a few weeks ago standardizing
> > property names across interfaces.  It seems like the issues you are
> > raising above and below have less to do with this patch and more to
> > do with the multiple implementation of minimum-length and the use of
> > the springs-and-rods property.
> 
> This is a typical case of "Somebody Else's Problem".  If there is a
> party and you place a chair in the worst possible place, like
> somewhere in a doorway, people will walk around it, squeeze themselves
> through, get annoyed again and again.
> 
> That's our current state.  Now someone wants to do a polonaise and
> since the chair is in the way, he proposes putting a plank across it.
> 
> Now we are moving from ridiculous to absurd, and it becomes harder to
> do the right thing.
> 
> Yes, I am fully aware that you are not responsible for the chair in
> the way of your polonaise.
> 
> Bit it is time to move it aside rather than taking it into account in
> our planning and make it even harder to get into a proper state.  In
> particular since your coding style requires a lot of reverse
> engineering in order to figure out the hidden assumptions and
> dependencies.  So that makes it doubly desirable to not add further
> dependencies on broken behavior but first clean up the foundations.
> 
> Yes, that's not a problem caused by your patch, but the consequences
> are acerbated by it.
> 
> > > The only thing more frustrating than missing functionality is
> > > purportedly available functionality that needs
> > > non-user-comprehensible trickery to actually work.
> > 
> > I do not have a problem with the current need to set the
> > springs-and-rods separately.
> 
> Of course you don't, and nobody keeps you from applying this sort of
> patch to your own private code base without doing the necessary
> cleanup before.
> 
> But you are not the metric for what goes into LilyPond's own
> repository and what not.  The functionality that goes into LilyPond
> has to work for the average user encountering that problem space.  The
> solutions have to have a meaningful correspondence in complexity to
> the problem and not require "don't think about it" steps.
> 
> > I'm positive it would because of the way that minimum-length is
> > multiply defined.  That is why this patch is intended for the "some
> > layout objects" discussed above like the TextSpanner.  I agree that
> > the multiple use of the minimum-length property should be changed,
> > but this seems like the business of another patch.
> 
> Sure.  But that patch will be harder to do once this patch _relies_ on
> the broken behavior, and people write code _relying_ on this patch.
> 
> > If the regtest is bothering you that much, I can just eliminate it
> > from this patch.
> 
> There is no point in hiding the symptoms of a problem away.  That only
> makes things even harder in future.

I don't think this is a problem blocking the current patch.  You are right that
the current multiple functions of minimum-length are problematic.  I'm not
arguing that this is someone else's problem - I am arguing that this (like all
bugs) is the community's problem.  Yes, the regtest that I've proposed uses an
override that corresponds to that used in the Documentation which is, as you
have pointed out, a suboptimal way of doing things.  It may take months or years
to sort out the multiple naming of properties.  This shouldn't block this patch
from being pushed.

So when you say "But this patch will be harder to do once this _relies_ on the
broken behavior.", all this patch will rely on is an override that you are
arguing should be the default.

This patch fixes behavior that would exist even if the springs-and-rods override
became the default.

Imagine it this way.  Patients are taking a suboptimal drug for a disease.  With
R&D, a company could invent a better one in a few years.  Mike INC. proposes a
supplement that cures a side effect in both the current and future drugs.  You
are making the argument that, before approving the supplement, we should wait
until the future drug comes out because the supplement will invariably make the
current drug less painful and thus more widespread, making the switch more
difficult down the line.  I don't find this argument convincing.  I'm curious
what others have to say.

Sign in to reply to this message.

dak

On 2013/03/17 07:10:23, MikeSol wrote: > On 2013/03/11 10:18:59, dak wrote: > > > > ...

11 years, 1 month ago (2013-03-17 09:19:54 UTC) #9

On 2013/03/17 07:10:23, MikeSol wrote:
> On 2013/03/11 10:18:59, dak wrote:
> > 
> > There is no point in hiding the symptoms of a problem away.  That only
> > makes things even harder in future.
> 
> I don't think this is a problem blocking the current patch.

It is a problem making the current patch ill-fitting and ill-advised.

> You are right that the current multiple functions of minimum-length
> are problematic.  I'm not arguing that this is someone else's
> problem - I am arguing that this (like all bugs) is the community's
> problem.

Since you don't care for fixing this yourself, you _are_ arguing that
this is someone else's problem.

> It may take months or years to sort out the multiple naming of
> properties.

It will if people just heap on new "features" that can't be made to
work consistently and blame "the community" for the lack of solid
groundwork.

> This shouldn't block this patch from being pushed.

I disagree.  There is no mythical beast called "community" that
magically fixes things.  Code gets fixed by people working on that
area who are leaving the code base in a better state than they found
them.  But that's not your attitude.  Your attitude is that if the
code base is bad, that is the perfect excuse to make it worse.

You want to achieve a certain thing in an area that can't be done
cleanly due to design errors.  And you draw the "community" card for
who should be doing it.  But who should do the groundwork for your
features if not you?  Who is most intimate with the area you are
working on?  If you feel fine heaping inconsistent and incomplete
features on for the sake of getting your own work done, you are free
to do so in a branch.

I have pointed out completely undocumented code from you in the past,
like with partial elliptic stencils.  You have not bothered adding a
single line of documentation yet, and as one consequence people are
unable to use it for improving the woodwind diagrams.

You don't fix your own work after it has been committed, so why would
you fix inconsistencies afterwards that you felt ok to ignore in the
first place?  Who is going to fix them?  "the community".  Please try
acting as a part of "the community" instead of piling work on for "the
community" that "somebody else (TM)" will at some time solve.

Sign in to reply to this message.

mike7

On 17 mars 2013, at 10:19, dak@gnu.org wrote: > You don't fix your own work ...

11 years, 1 month ago (2013-03-17 09:49:52 UTC) #10

janek

I'll risk joining the discussion. I see valid points from both of you. I agree ...

11 years, 1 month ago (2013-04-02 22:34:53 UTC) #12

dak

On 2013/04/02 22:34:53, janek wrote: > I'll risk joining the discussion. > > I see ...

11 years, 1 month ago (2013-04-02 22:57:44 UTC) #13

mike7

On 3 avr. 2013, at 01:57, dak@gnu.org wrote: > On 2013/04/02 22:34:53, janek wrote: >> ...

11 years, 1 month ago (2013-04-03 04:07:58 UTC) #14

On 3 avr. 2013, at 01:57, dak@gnu.org wrote:

> On 2013/04/02 22:34:53, janek wrote:
>> I'll risk joining the discussion.
> 
>> I see valid points from both of you.  I agree that it's better to fix
> a broken
>> design than to patch it with red tape.
> 
> It isn't patched.  minimum-length is used in multiple
> contexts/interfaces, and Mike's patch muddies the situation further.
> 
>> However, if we only accepted code changes that were implementing the
>> Ultimate Solution, i'm afraid that the development process would
>> grind to a halt - Ultimate Solutions are obvioulsly best, but they
>> take much much more time to implement, and usually only experts can
>> write them.
> 
> That's the ultimate excuse to never bother about doing things right.
> 
>> This should not be an excuse for broken code, but i think that we
>> can accept patches that are iterations towards Ultimate Solution.
> 
> This one is an iteration away from a proper solution since it
> increases the inconsistency of minimum-length.
> 
>> As i see it, Mike's patch doesn't make matters worse - it's just a
>> piece of duct tape to make a temporary solution (i.e. current messy
>> code) less broken.
> 
> No, it makes it _more_ broken in order to paper over an annoying
> consequence of the brokenness.
> 
>> I think that we could add a FIXME to it and accept it, because it's
>> not making future rewrite harder (at least it seems so to me).
> 
> How does an increase in the inconsistency of minimum-length _not_ make
> a future rewrite harder?
> 
>> Also, we're trying to make a stable release soon, so this is not a
>> good time to start rewriting big pieces of code.
> 
> It is not a good time shoving in behavior that is not going to survive
> into the next stable release (assuming that this _is_ going to be
> fixed in a sensible manner) either.  The behavior of our stable
> releases should not be erratic but rather increasingly reliable.
> 
> 
> https://codereview.appspot.com/7453046/

The callback for minimum length is broken. This patch fixes a broken callback
that used to work in a previous version of LilyPond. You are objecting to the
patch because you don't like the naming of the property that he callback is
attached to. I think the benefits of fixing this issue (which is a regression
and IMHO should be marked as critical) far outweighs the disadvantages of
continuing to use the bad naming scheme, especially as said scheme is used here
as it is everywhere else in the documentation.

We can talk about how to better name things later, but that should not stop this
regression from being fixed.

Cheers,
MS

Sign in to reply to this message.

janek

Hi all, On Wed, Apr 3, 2013 at 12:57 AM, <dak@gnu.org> wrote: > On 2013/04/02 ...

11 years, 1 month ago (2013-04-03 09:42:32 UTC) #15

Hi all,

On Wed, Apr 3, 2013 at 12:57 AM,  <dak@gnu.org> wrote:
> On 2013/04/02 22:34:53, janek wrote:
>> This should not be an excuse for broken code, but i think that we
>> can accept patches that are iterations towards Ultimate Solution.
>
> This one is an iteration away from a proper solution since it
> increases the inconsistency of minimum-length.
>
>> As i see it, Mike's patch doesn't make matters worse - it's just a
>> piece of duct tape to make a temporary solution (i.e. current messy
>> code) less broken.
>
> No, it makes it _more_ broken in order to paper over an annoying
> consequence of the brokenness.
>
>> I think that we could add a FIXME to it and accept it, because it's
>> not making future rewrite harder (at least it seems so to me).
>
> How does an increase in the inconsistency of minimum-length _not_ make
> a future rewrite harder?
>
>> Also, we're trying to make a stable release soon, so this is not a
>> good time to start rewriting big pieces of code.
>
> It is not a good time shoving in behavior that is not going to survive
> into the next stable release (assuming that this _is_ going to be
> fixed in a sensible manner) either.  The behavior of our stable
> releases should not be erratic but rather increasingly reliable.

Well, i don't insist on sharing my point of view.  I think we can
agree to differ.

>> However, if we only accepted code changes that were implementing the
>> Ultimate Solution, i'm afraid that the development process would
>> grind to a halt - Ultimate Solutions are obvioulsly best, but they
>> take much much more time to implement, and usually only experts can
>> write them.
>
> That's the ultimate excuse to never bother about doing things right.

Hmm.  What about doing something constructive, then?
There is one big "let's do things right" patch that needs review.
Despite a lot of effort spent by its author on writing a detailed
description to attract reviewers, only one person cared to review it:
codereview.appspot.com/7768043

best,
Janek

Sign in to reply to this message.

dak

Janek Warchoł <janek.lilypond@gmail.com> writes: > Hmm. What about doing something constructive, then? > There is ...

11 years, 1 month ago (2013-04-03 10:15:34 UTC) #16

janek

11 years, 1 month ago (2013-04-03 10:51:14 UTC) #17

David,

On Wed, Apr 3, 2013 at 12:15 PM, David Kastrup <dak@gnu.org> wrote:
> Janek Warchoł <janek.lilypond@gmail.com> writes:
>
>> Hmm.  What about doing something constructive, then?
>
>> There is one big "let's do things right" patch that needs review.
>> Despite a lot of effort spent by its author on writing a detailed
>> description to attract reviewers, only one person cared to review it:
>> codereview.appspot.com/7768043
>
> If you bothered to look, you'd have found that in spite of daring to
> take off a week of vacation from a year of LilyPond development,
> I committed about half a dozen fixes of problems and regressions that
> impeded getting to stable release quality while also running a batch of
> Patchy tests.  I apologize if I don't seem to have the time to review
> post-stable release material thoroughly.  There is also the question of
> preparing the EU proposal.
>
> So I apologize if I don't appear to have the time to be constructive.
> It definitely looks like I should call off the stable release for good
> as nobody else wants to get bothered with its consequences and it is not
> like I don't have enough other stuff on my hand.

I apologize for making it look like i blamed you for the situation.
This was not my intention; it's not your fault that issue 3239 was
reviewed by one person only.  You're not responsible for reviewing
everything, and i certainly don't demand that you review something
when you're on vacation (issue 3239 was up for review before your
vacation, but this still doesn't mean that you have any duty to review
it).

I was indeed quite surprised to see you being so active during last
week, to the point that i wasn't sure if i remembered the date of your
vacation correctly.

I also believe that you deserved your vacation, and i'm sorry that you
didn't have more vacations during this year.

My only intention in writing the previous message was to point out
that "doing things right" is hard and that even thoroughly described
patches don't get much reviews, which is discouraging (that's only
stating a fact, i'm not blaming anyone for this).

Janek

PS i'm not sure what you mean by "reviewing thoroughly".  I never
expect anyone to do more than read commit messages and Rietveld diffs
once; that's why i spend so much time on polishing commit messages.
If anything is unclear after this first reading, i expect everyone to
ask me, even if such questions may look silly.

Sign in to reply to this message.

Issue 7453046: Allows minimum-length to work for end-of-line spanners. (Closed)

Description

Patch Set 1 #

Patch Set 2 : Rebase off of current master #

Messages