LilyIssues / Issues / #4036 Patch: Revert "Issue 3978: Merge alignment cleanup"

Google Importer - 2014-07-28

Originally posted by: dak@gnu.org

(No comment was entered for this change.)

Owner: dak@gnu.org

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2014-07-28

Originally posted by: dak@gnu.org

See <URL:https://lists.gnu.org/archive/html/bug-lilypond/2014-07/msg00115.html> and context.

Labels: -Type-Enhancement Type-Defect
Cc: janek.li...@gmail.com

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2014-07-28

Originally posted by: david.na...@gmail.com

The problem is evident in Self_alignment_interface::aligned_on_parent, which returns the value which will be used for 'X-offset.

Running the following example:

\version "2.19.11"

{
\once\override DynamicText.self-alignment-X = #LEFT
c'4\f
\once\override DynamicText.self-alignment-X = #CENTER
c'4\f
\once\override DynamicText.self-alignment-X = #RIGHT
c'4\f
\once\override DynamicText.self-alignment-X = #20
c'4\f
}

I get these values:

self-alignment-X: -1 return: 0
self-alignment-X: 20 return: 0.555194
self-alignment-X: 1 return: 0.0528756
self-alignment-X: 0 return: 0.0264378

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2014-07-28

Originally posted by: pkx1...@gmail.com

Patchy the autobot says: passes 'make', 'make check' and a full 'make doc'.

Labels: -Patch-new Patch-review

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2014-07-28

Originally posted by: janek.li...@gmail.com

WTF?!? Folks, calm down! My changes in alignment are working exactly as expected.

David Nalesnik posted a question about alignment and barely 2 hours later a revert of my work is proposed. I didn't even got a chance to explain myself, and it seems that noone really tried to understand the code in question. In fact, it seems to me that David (K) assumed that I *must have* broken something, i.e. it seems to me that he doesn't trust me at all. Which makes me very sad.

Maybe my alignment patch should be changed, but I believe that reverting it is nonsense (see explanation at http://lists.gnu.org/archive/html/bug-lilypond/2014-07/msg00116.html). However, i'm not going to mark this issue as invalid myself, because that could easily lead to a flamewar. It's up to you to decide.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2014-07-28

Originally posted by: david.na...@gmail.com

I don't think that it should be reverted--not at all. However, the
potential confusion between the old setting of X-offset and the new one
needs to be highlighted at some point, because it leads to behavior users
probably won't expect. (Though this behavior is correct.)

The old behavior relied on X-offset being
ly:self-alignment-interface::x-aligned-self.

Best,
David

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2014-07-28

Originally posted by: dak@gnu.org

"This behavior is correct" can be stated only if you change the _definition_ of the correct behavior. The previous behavior is still documented as being correct.

Cf.
<URL:http://lilypond.org/doc/v2.18/Documentation/notation/aligning-objects#-Self_002daligning-objects-in-both-directions>
with
<URL:http://lilypond.org/doc/v2.19/Documentation/notation/aligning-objects#-Self_002daligning-objects-in-both-directions>

Both effect and description in 2.19 are now bad. This was a sweeping change that will affect a number of tweaks that were done as still suggested in our documentation, in the NR chapter about self-alignment-interface.

And any redefinition of "this behavior is correct" that "leads to behavior users probably won't expect" in a major user-tweakable interface does not sound like a good idea in first approximation.

How can we converge better with documented behavior and user expectations?

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2014-07-28

Originally posted by: dak@gnu.org

Regarding comment #5: the changes in alignment are obviously not working "exactly as expected" since the report on the list was because of unexpected behavior.

The change to the still documented behavior could be traced back to a single commit. Reverting commits introducing a regression is a pretty standard remedy. This commit was not amicable to reverting on its own, so its reversal requires reverting the whole branch.

In order to see whether that is still an option in the current state of master, one needs to run patchy which requires an issue report.

The test run has established that a revert is feasible. Whether it should be done depends on whether a proper cure for the regression can be found in a reasonable time frame. Even when a revert is applied, this does not mean that the original patch series cannot be resubmitted in a fixed form at a later point of time without pressure.

I invested a sizable effort into tracing this regression to its source and trying out various ways of reverting it and testing them for feasibility. Getting a "WTF?!?" for that is not exactly encouraging quality assurance efforts.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2014-07-28

Originally posted by: k-ohara5...@oco.net

The docs linked above start with a painfully complicated qualifier
"The horizontal alignment of an object which supports the self-alignment-interface is controlled by the value of the self-alignment-X property, provided the object’s X-offset property is set to ly:self-alignment-interface::x-aligned-on-self."

Actually, self-alignment-X does has sensible effect if the object's X-offset is set to some other self-alignment-interface property (such as aligned-on-x-parent) but the effect is different from that assumed in those doc examples (as we see after Janek had Fingering align-on-parent).

If we want Fingering to self-align on parent, rather than self-align on-self (which really meant self-align on the center of its parent, due to some tricky code you can see in the revert linked above) the documentation will need some updating. Along with something in onvert-ly.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2014-07-29

Originally posted by: dak@gnu.org

Well, "self-alignment-X does has sensible effect if the object's X-offset is set to some other self-alignment-interface property (such as aligned-on-x-parent) but the effect is different from that assumed in those doc examples (as we see after Janek had Fingering align-on-parent)."

I mean, isn't it pretty clear what we want here? I don't see that we have anything to gain by setting X-offset to something like align-on-parent which interprets self-alignment-X for aligning not the "self" but the parent, and then mushing together a parent-aligner with a self-aligner by using make-simple-closure. Or whatever.

So what we should have is just a single callback for all those aligners, and two properties
parent-alignment-X (which can be ##f for just taking the reference point 0 or a number between #LEFT and #RIGHT for locating the reference point between left and right edge), and self-alignment-X (which can be ##f for just taking the reference point 0 or a number between #LEFT and #RIGHT for locating the reference point between left and right edge).

If we do that, meddling with self-alignment-X will result in a predictable shift of the self-aligned object around the parent reference point, and meddling with parent-alignment-X will change the parent reference point.

Yes, we'll have one more property lookup for alignment. But it should be worth the trouble because we then have exactly one meaning for self-alignment-X, and we just have a single callback for X-offset when we design aligning to the parent.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2014-07-29

Originally posted by: david.na...@gmail.com

This seems eminently sensible to me.

At some point, I imagine we would also have a pair of alignment settings
for the Y-axis, too.

I'm curious what the opinion is on allowing values outside the range of -1
to 1. I know that Janek considers these a misuse, but I've used such
values for self-alignment-X for "moving things around," and I'm certainly
not alone.

If only values between -1 and 1 produced a useful result, and X-offset were
used to move things about, it seems natural to me that an override to
X-offset would respect the alignment settings. (If they didn't, such
behavior could be created by applying \offset X-offset, but this is hardly
obvious.)

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2014-07-29

Originally posted by: janek.li...@gmail.com

I'll think about this, but right now i'm too upset to say anything reasonable and not start a flamewar. Please give me several days to calm down.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2014-07-30

Originally posted by: pkx1...@gmail.com

I'm going to leave this 'revert' on review as it seems to me there are for/against reverting, so another few days won't hurt. If this tracker is not updated or the label changed then I'll keep pushing it through the patch process.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2014-08-01

Originally posted by: janek.li...@gmail.com

Okay, i calmed down. Here's the plan:
issue 4022 implements using separate properties for independent parent- and self-alignment, like suggested by David in comment #10. Having that should allow us to get back the old behavior while taking advantage of the cleanup I did with issue 3978 - for example, we can let DynamicTexts have

parent-alignment-X = #CENTER
self-alignment-X = #CENTER

and, since these properties will be separate, overriding self-alignment-X would have the same effect as it had before my patch for issue 3978. Additionally, old documentation should also remain valid.

After getting back the old behavior we may discuss whether we want to change default parent-alignments for some grobs - but that would be a separate issue.

What do you think?

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2014-08-02

Originally posted by: janek.li...@gmail.com

FWIW, the latest patchset for issue 4022 (patchset 11) restores old behaviour in all grobs (assuming i didn't forget any).

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2014-08-04

Originally posted by: pkx1...@gmail.com

Patch on countdown of August 8th 2014

Labels: -Patch-review Patch-countdown

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2014-08-05

Originally posted by: dak@gnu.org

This is currently being tackled by several other issues. Putting the Patch on waiting. If the underlying issues are addressed, the issue is probably going to be Invalid.

Labels: -Patch-countdown Patch-waiting

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2014-08-09

Originally posted by: janek.li...@gmail.com

Ok, I have pushed the code solving issue 4022 as

d7b067b Grob alignment: get back 2.19.9 behaviour
0c4f221 Issue 4022: Allow specifying different alignment for grob and its parent

David, please check whether you agree that all problems are fixed, and close this issue if everything's ok. Thanks!

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Google Importer - 2014-10-06

Originally posted by: dak@gnu.org

I've finally checked the examples listed here (and the regenerated documentation). I don't see any of of the changes that were flagged as problematic any more. So even if further problems occur, I expect them to be better to be addressed individually rather than by a sweeping revert.

Marking this issue as invalid.

Labels: -Patch-waiting
Status: Invalid

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Patch: Revert "Issue 3978: Merge alignment cleanup"

Issue Tracker for LilyPond

Searches

Help

#4036 Patch: Revert "Issue 3978: Merge alignment cleanup"

Discussion