lilypond-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

reorganize self_alignment_interface - part 1 (issue 7768043)


From: janek . lilypond
Subject: reorganize self_alignment_interface - part 1 (issue 7768043)
Date: Tue, 12 Mar 2013 22:09:11 +0000

Reviewers: MikeSol, joeneeman, lemzwerg,

Message:
Please review.
You'll find an illustration in the issue tracker:
http://code.google.com/p/lilypond/issues/detail?id=3239#c2

Description:
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

Please review this at https://codereview.appspot.com/7768043/

Affected files:
  M lily/include/self-alignment-interface.hh
  M lily/self-alignment-interface.cc


Index: lily/include/self-alignment-interface.hh
diff --git a/lily/include/self-alignment-interface.hh b/lily/include/self-alignment-interface.hh index dd69f8663000e1a2aaf9d3e985451fbedce01b9e..23e24420a87be79f31566f7e36c86bfb42f9359a 100644
--- a/lily/include/self-alignment-interface.hh
+++ b/lily/include/self-alignment-interface.hh
@@ -27,9 +27,8 @@ struct Self_alignment_interface
 {
   DECLARE_GROB_INTERFACE ();

- static SCM aligned_on_self (Grob *me, Axis a, bool pure, int start, int end);
   static SCM centered_on_object (Grob *me, Axis a);
-  static SCM aligned_on_parent (Grob *me, Axis a);
+ static SCM aligned_on_parent (Grob *me, Grob *him, Axis a, bool pure, int start, int end);
   static void set_center_parent (Grob *me, Axis a);
   static void set_align_self (Grob *me, Axis a);

Index: lily/self-alignment-interface.cc
diff --git a/lily/self-alignment-interface.cc b/lily/self-alignment-interface.cc index 59adfc3e6ca04d878349f6e3425843c703694c45..93fb3d2fd26b5579de2971b1153f438aaca25f1c 100644
--- a/lily/self-alignment-interface.cc
+++ b/lily/self-alignment-interface.cc
@@ -32,37 +32,21 @@ MAKE_SCHEME_CALLBACK (Self_alignment_interface, y_aligned_on_self, 1);
 SCM
 Self_alignment_interface::y_aligned_on_self (SCM element)
 {
-  return aligned_on_self (unsmob_grob (element), Y_AXIS, false, 0, 0);
+ return aligned_on_parent (unsmob_grob (element), NULL, Y_AXIS, false, 0, 0);
 }

 MAKE_SCHEME_CALLBACK (Self_alignment_interface, x_aligned_on_self, 1);
 SCM
 Self_alignment_interface::x_aligned_on_self (SCM element)
 {
-  return aligned_on_self (unsmob_grob (element), X_AXIS, false, 0, 0);
+ return aligned_on_parent (unsmob_grob (element), NULL, X_AXIS, false, 0, 0);
 }

 MAKE_SCHEME_CALLBACK (Self_alignment_interface, pure_y_aligned_on_self, 3);
 SCM
Self_alignment_interface::pure_y_aligned_on_self (SCM smob, SCM start, SCM end)
 {
- return aligned_on_self (unsmob_grob (smob), Y_AXIS, true, robust_scm2int (start, 0), robust_scm2int (end, INT_MAX));
-}
-
-SCM
-Self_alignment_interface::aligned_on_self (Grob *me, Axis a, bool pure, int start, int end)
-{
-  SCM sym = (a == X_AXIS) ? ly_symbol2scm ("self-alignment-X")
-            : ly_symbol2scm ("self-alignment-Y");
-
-  SCM align (me->internal_get_property (sym));
-  if (scm_is_number (align))
-    {
-      Interval ext (me->maybe_pure_extent (me, a, pure, start, end));
-      if (!ext.is_empty ())
- return scm_from_double (- ext.linear_combination (scm_to_double (align)));
-    }
-  return scm_from_double (0.0);
+ return aligned_on_parent (unsmob_grob (smob), NULL, Y_AXIS, true, robust_scm2int (start, 0), robust_scm2int (end, INT_MAX));
 }

 SCM
@@ -117,45 +101,54 @@ MAKE_SCHEME_CALLBACK (Self_alignment_interface, aligned_on_x_parent, 1);
 SCM
 Self_alignment_interface::aligned_on_x_parent (SCM smob)
 {
-  return aligned_on_parent (unsmob_grob (smob), X_AXIS);
+  Grob *me = unsmob_grob (smob);
+ return aligned_on_parent (me, me->get_parent (X_AXIS), X_AXIS, false, 0, 0);
 }

 MAKE_SCHEME_CALLBACK (Self_alignment_interface, aligned_on_y_parent, 1);
 SCM
 Self_alignment_interface::aligned_on_y_parent (SCM smob)
 {
-  return aligned_on_parent (unsmob_grob (smob), Y_AXIS);
+  Grob *me = unsmob_grob (smob);
+ return aligned_on_parent (me, me->get_parent (Y_AXIS), Y_AXIS, false, 0, 0);
 }

+// TODO: what should be the name of this new general alignment method? calc_offset?
 SCM
-Self_alignment_interface::aligned_on_parent (Grob *me, Axis a)
+Self_alignment_interface::aligned_on_parent (Grob *me, Grob *him, Axis a, bool pure, int start, int end)
 {
-  Grob *him = me->get_parent (a);
-  if (Paper_column::has_interface (him))
-    return scm_from_double (0.0);
-
-  Interval he = him->extent (him, a);
+  Real offset = 0.0;

-  SCM sym = (a == X_AXIS) ? ly_symbol2scm ("self-alignment-X")
-            : ly_symbol2scm ("self-alignment-Y");
-  SCM align_prop (me->internal_get_property (sym));
+  SCM my_align = (a == X_AXIS)
+ ? me->internal_get_property (ly_symbol2scm ("self-alignment-X")) + : me->internal_get_property (ly_symbol2scm ("self-alignment-Y"));

-  if (!scm_is_number (align_prop))
-    return scm_from_int (0);
+  SCM his_align = my_align; //these will be independent in the future,
+  // to allow things like aligning left edge of a grob to the right edge
+ // of its parent. Right now they need to be equal for compatibility's sake.

-  Real x = 0.0;
-  Real align = scm_to_double (align_prop);
-
-  Interval ext (me->extent (me, a));
-  if (ext.is_empty ())
-    programming_error ("cannot align on self: empty element");
-  else
-    x -= ext.linear_combination (align);
+  if (scm_is_number (my_align))
+    {
+      Interval my_ext (me->maybe_pure_extent (me, a, pure, start, end));
+      if (my_ext.is_empty ())
+ programming_error (me->name () + " has empty extent, cannot align.");
+      else
+        offset -= my_ext.linear_combination (scm_to_double (my_align));
+    }

-  if (!he.is_empty ())
-    x += he.linear_combination (align);
+  // him will be null when we want just to align a grob on itself.
+  // we don't take paper columns' extent into account when aligning
+  // because they are weird (TODO: explain in detail).
+ if (him && scm_is_number (his_align) && !Paper_column::has_interface (him))
+    {
+      Interval his_ext (him->maybe_pure_extent (him, a, pure, start, end));
+      if (his_ext.is_empty ())
+ programming_error (him->name () + " has empty extent, cannot align.");
+      else
+        offset += his_ext.linear_combination (scm_to_double (his_align));
+    }

-  return scm_from_double (x);
+  return scm_from_double (offset);
 }

 void





reply via email to

[Prev in Thread] Current Thread [Next in Thread]