Menu
▾
▴
#3239 Patch: rewrite Self_alignment_interface
Duplicate
nobody
None
Enhancement
2014-06-28
2013-03-12
Anonymous
No
Originally created by: *anonymous
Originally created by: janek.li...@gmail.com
Originally owned by: janek.li...@gmail.com
reorganize self_alignment_interface - part 1
Currently self_alignment_interface is quite messy. We have many
methods for calculating offsets of grobs; some methods are
really similar to each other (-> code duplication), for example
aligned_on_self is a "subset" of aligned_on_parent. At the same
time all these methods are not powerful enough: for example,
it's not possible to align left edge of a grob to the right edge
of its parent. Also, take a look at define-grobs.scm: a lot of
grobs have their offsets defined using closures that combine
different callbacks. That's inelegant and difficult to handle
for users.
My ultimate goal is to replace all methods in
self_alignment_interface (i.e. aligned_on_parent,
centered_on_object and aligned_on_self) with one versatile
method, and reduce the amount of callbacks. The idea is to make
more things configurable via properties. For example, there are
now separate callbacks "aligned_on_xy_parent" and
"xy_aligned_on_self" - to choose whether the size of grob's
parent should be taken into account or not, the user has to
override offset callback. I'd prefer to do this via a property -
it's simpler. Also, if the user wants to align a grob on some
special grob, for example align LyricText on a Stem, currently
he has to write his own callback to do this - that's difficult.
I'd like to make it possible to specify the grob to be aligned
to using a property.
In this commit i merge aligned_on_parent with aligned_on_self
and prepare ground for more powerful alignment options by using
two separate alignment values (my_align and his_align) instead
of one. The next step will be merging centered_on_object, but
i'd like some feedback on this before i move on.
This commit is a work in progress, but it should compile and
don't cause any regressions. ( = James, you can test it)
In addition to comments on this code, i kindly ask for help
with explaining strange behaviour of relative_coordinate
http://lists.gnu.org/archive/html/lilypond-devel/2013-03/msg00131.html
and DynamicText alignment:
http://lists.gnu.org/archive/html/lilypond-devel/2013-03/msg00132.html
Related
Issues: #3273
Issues: #3299
Issues: #3692
Issues: #3979
Originally posted by: janek.li...@gmail.com
(No comment was entered for this change.)
Owner: janek.li...@gmail.com
Originally posted by: janek.li...@gmail.com
An image is worth a thousand words, so here's an illustration of independent alignment that will be possible after rewriting is completed.
Originally posted by: pkx1...@gmail.com
Patchy the autobot says: passes make, make test and a full make doc. Reg tests here: https://www.yousendit.com/download/UVJpL0dBQ3RubVZWeHNUQw
Labels: -Patch-new Patch-review
Originally posted by: janek.li...@gmail.com
The results are surprising - i didn't see anything like that when i ran tests myself (probably because i didn't restart from scratch).
I think i know what caused these changes; i'll look closely tomorrow. Anyway, this shouldn't have much impact on the patch itself.
Thanks for testing!
Originally posted by: pkx1...@gmail.com
(No comment was entered for this change.)
Labels: -Patch-review Patch-needs_work
Originally posted by: lemniska...@gmail.com
Apparently vast majority of these regressions are actually an improvement in a way. There was one really wrong thing (song-repetition), but fortunately i'm on track already. I'll post a new version tomorrow or on friday.
Originally posted by: janek.li...@gmail.com
return 0 if him is a PaperColumn to avoid regressions
http://codereview.appspot.com/7768043
Labels: -Patch-needs_work Patch-new
Originally posted by: janek.li...@gmail.com
There should be no changes in regtests graphical output. Programming errors "<Grob> has empty extent, cannot align it." in tablature-full-notation.log are expected.
Originally posted by: pkx1...@gmail.com
Patchy the autobot says: Programming errors reported in reg test: tablature-full-notation.ly - DynamicText has empty extent, cannot align it...
Labels: -Patch-new Patch-needs_work
Originally posted by: janek.li...@gmail.com
Uh, these programming errors are an expected and desired consequence of the patch (of course we can discuss this). Setting to review.
Labels: -Patch-needs_work Patch-review
Originally posted by: pkx1...@gmail.com
Patch on countdown for March 20th - 19:00 GMT
Labels: -Patch-review Patch-countdown
Originally posted by: janek.li...@gmail.com
rewrite. replaces all three methods in self-alignment-interface with one
http://codereview.appspot.com/7768043
Labels: -Patch-countdown Patch-new
Originally posted by: janek.li...@gmail.com
Hi James,
i need your help. I got quite strange regtest results:
- in addition to some new (expected) programming errors ("empty extent cannot be used for alignment"), i got lots of other, strange changes in regtest logs.
- there are hundreds of regtests with changed cell count. I most cases cells decreased (i guess that's good?), but there are some increased ones.
Here's a zip of my results: http://www.sendspace.com/file/o4hfy0
Could you test it yourself and see if you get something similar?
thanks,
Janek
Originally posted by: janek.li...@gmail.com
Ok, i think i know what caused all these cells differencies: i've done make test-baseline with single thread, and make check with multiple threads.
I'll redo tests tomorrow - still, it would be nice to have them double-checked.
Originally posted by: janek.li...@gmail.com
change explanation comment
http://codereview.appspot.com/7768043
Originally posted by: lemniska...@gmail.com
I've redone the tests and everything is ok now - the problem was caused by compiling test-baseline with different amount of CPU threads than check.
Originally posted by: janek.li...@gmail.com
Issue 2613 has been merged into this issue.
Cc: mts...@gmail.com
Originally posted by: janek.li...@gmail.com
(No comment was entered for this change.)
Labels: GSoC-LyricProject
Originally posted by: pkx1...@gmail.com
Pass make, make test and make check and full make doc.
Reg tests are here:
https://www.yousendit.com/download/UVJpQmtXRSt0NiswYjhUQw
Lots of programming errors reported
still don't know why this doesn't mean the patch doesn't 'need work' but as per comment #10 setting to 'review'
Labels: -Patch-new Patch-review
Originally posted by: dak@gnu.org
I agree with James that this patch should not be put on countdown as long as it produces a number of programming errors. Programming errors mean that LilyPond is encountering an unexpected situation. It is not intended to flag problems with user input, and it is not clear to me that we are even dealing with a problem in user input here.
Originally posted by: pkx1...@gmail.com
I *think*, and I am sure Janek will speak for himself, that he is looking for input and feels that putting the tracker on 'needs work' will mean no one will even give it a second look than help resolve the issues we obviously still have here.
Originally posted by: janek.li...@gmail.com
@David: have you looked at the code itself?
This patch doesn't /introduce/ programming errors - it only makes LilyPond report as errors situations that were already there.
In other words, the situations that are now reported as programming errors *were present* before the patch - the difference is that they weren't flagged as errors (and i think they should be).
Anyway, i see that we need to discuss this. I'll post a separate email about this (the question whether there should be programming errors reported here is quite orthogonal to the substance of this patch).
Originally posted by: janek.li...@gmail.com
don't report additional programming errors
http://codereview.appspot.com/7768043
Labels: -Patch-review Patch-new
Originally posted by: janek.li...@gmail.com
After some thinking it's clear to me that i indeed shouldn't have introduced any code that reports programming errors in this patch, but make it a separate issue. Sorry for mixing together things that should be separate. The programming error thing is now issue 3259.
The last version of this patch shouldn't introduce any regtest changes - it can be tested now.
@James: well, it's true that was afraid that i won't get any reviews if this issue was set to 'needs work'. However, as you can see, the problem wansn't in my code being bad, but in me putting two separate things in one patch.
Cc: dak@gnu.org
Originally posted by: pkx1...@gmail.com
Pases Make, make test and a full make doc but am getting
--snip--
input/regression/tablature-full-notation.log
@@ -2,10 +2,54 @@
Renaming input to: `/tmp/lilypond-autobuild/input/regression/tablature-full-notation.ly'
Interpreting music...[8]
Preprocessing graphical objects...
+programming error: cannot align on self: empty element
+continuing, cross fingers
+programming error: cannot align on self: empty element
+continuing, cross fingers
+programming error: cannot align on self: empty element
+continuing, cross fingers
+programming error: cannot align on self: empty element
+continuing, cross fingers
etc..
--snip--
on this one reg test
Labels: -Patch-new Patch-review