[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Allows minimum-length to work for end-of-line spanners. (issue 74530
|
From: |
mtsolo |
|
Subject: |
Re: Allows minimum-length to work for end-of-line spanners. (issue 7453046) |
|
Date: |
Sun, 17 Mar 2013 07:10:24 +0000 |
Reviewers: lemzwerg, dak, mike7,
Message:
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.
Description:
Allows minimum-length to work for end-of-line spanners.
Please review this at https://codereview.appspot.com/7453046/
Affected files:
A input/regression/minimum-length-end-line.ly
M lily/simple-spacer.cc
M lily/spanner.cc
Index: input/regression/minimum-length-end-line.ly
diff --git a/input/regression/minimum-length-end-line.ly
b/input/regression/minimum-length-end-line.ly
new file mode 100644
index
0000000000000000000000000000000000000000..ed130586a32c5c1f44f44024e37cccd1a155334d
--- /dev/null
+++ b/input/regression/minimum-length-end-line.ly
@@ -0,0 +1,17 @@
+\version "2.17.14"
+
+\header {
+ texidoc = "Long spanners at the end of the lines stretch measures
+correctly.
+"
+}
+
+{
+ \override TextSpanner.springs-and-rods = #ly:spanner::set-spacing-rods
+ \override TextSpanner.minimum-length = #60
+ \override TextSpanner.to-barline = ##t
+ \repeat unfold 4 a1
+ a1\startTextSpan
+ a1\stopTextSpan\startTextSpan
+ a1\stopTextSpan
+}
Index: lily/simple-spacer.cc
diff --git a/lily/simple-spacer.cc b/lily/simple-spacer.cc
index
61afdb9bcc281004c4b0dbffbabe2fbd757ec4d5..aef524da87579bb2ecb1e087409f0f2da04baa3f
100644
--- a/lily/simple-spacer.cc
+++ b/lily/simple-spacer.cc
@@ -387,6 +387,9 @@ get_column_description (vector<Grob *> const &cols,
vsize col_index, bool line_s
if (cols[j] == other)
description.rods_.push_back (Rod_description (j, scm_to_double
(scm_cdar (s))));
else /* it must end at the LEFT prebroken_piece */
+ /* see Spanner::set_spacing_rods for more comments on how
+ to deal with situations where we don't know if we're
+ ending yet on the left prebroken piece */
description.end_rods_.push_back (Rod_description (j,
scm_to_double (scm_cdar (s))));
}
}
Index: lily/spanner.cc
diff --git a/lily/spanner.cc b/lily/spanner.cc
index
af2ea7137f0d9ab4dbddc9127d6dfc03f549e180..e0e1c18e1d09f6a9ae59afffc78646fa571ea906
100644
--- a/lily/spanner.cc
+++ b/lily/spanner.cc
@@ -385,6 +385,23 @@ Spanner::set_spacing_rods (SCM smob)
r.item_drul_[LEFT] = sp->get_bound (LEFT);
r.item_drul_[RIGHT] = sp->get_bound (RIGHT);
r.add_to_cols ();
+
+ /*
+ We do not know yet if the spanner is going to have a bound that is
+ broken. To account for this uncertainty, we add the rod twice:
+ once for the central column (see above) and once for the left
column
+ (see below). As end_rods_ are never used when rods_ are used and
vice
+ versa, this rod will only be accessed once for each spacing
+ configuraiton before line breaking. Then, as a grob never exists in
+ both unbroken and broken forms after line breaking, only one of
these
+ two rods will be in the column vector used for spacing in
+ simple-spacer.cc get_line_confugration.
+ */
+ if (Item *left_pbp = sp->get_bound (RIGHT)->find_prebroken_piece
(LEFT))
+ {
+ r.item_drul_[RIGHT] = left_pbp;
+ r.add_to_cols ();
+ }
}
return SCM_UNSPECIFIED;