[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Issue 2376: Automatic footnotes on \null markups causes unexpected r
|
From: |
dak |
|
Subject: |
Re: Issue 2376: Automatic footnotes on \null markups causes unexpected results (issue 5755058) |
|
Date: |
Wed, 07 Mar 2012 07:40:58 +0000 |
Reviewers: MikeSol,
http://codereview.appspot.com/5755058/diff/1/lily/page-breaking.cc
File lily/page-breaking.cc (right):
http://codereview.appspot.com/5755058/diff/1/lily/page-breaking.cc#newcode584
lily/page-breaking.cc:584: }
On 2012/03/07 07:30:42, MikeSol wrote:
Just a C++ question - do these lines implicitly call the copy
constructor to
create footnote_stencil so that footnote_stencil is a copy of foot?
I'm not
familiar with this syntax for copying, so I just wanted to be sure
that this is
what these lines were doing.
The first line calls the assignment operator. The second line just
resets the foot pointer to point at footnote_stencil.
It would likely be cleaner to just pass a pointer to footnote_stencil
(which is constructed as a null Stencil as far as I can see) in either
case. Since the special case "foot == 0" would then be ruled out in
advance (do you need the error message? Can add_footnotes_to_footer
deal with a null stencil?) one could in a more C++-like manner pass foot
in by reference instead of by pointer.
I did not want to meddle with the logic of the code: I just made sure
that it copied what was needed. If you say that add_footnotes_to_footer
can deal with getting a null stencil for starters, that's what I would
prefer using. Incidentally, the length of the axes of a null stencil
should turn out to be 0, so this would remove a few special cases as
well.
Description:
Issue 2376: Automatic footnotes on \null markups causes unexpected
results
The page layout routines passed around pointers to stencils rather
indiscriminately and worked on them rather than on stencil copies.
Sort of copy-on-write without the copy. This tries working on actual
copies where appropriate.
Please review this at http://codereview.appspot.com/5755058/
Affected files:
M lily/page-breaking.cc
M lily/page-layout-problem.cc
Index: lily/page-breaking.cc
diff --git a/lily/page-breaking.cc b/lily/page-breaking.cc
index
61a4e82f91d3098baf29e77b800a622749902522..0eee8e90a45f346a84caffeac89925da05390857
100644
--- a/lily/page-breaking.cc
+++ b/lily/page-breaking.cc
@@ -574,14 +574,21 @@ Page_breaking::draw_page (SCM systems, SCM
configuration, int page_num, bool las
p->set_property ("lines", paper_systems);
p->set_property ("configuration", configuration);
+ Stencil footnote_stencil;
+
Stencil *foot = unsmob_stencil (p->get_property ("foot-stencil"));
+ if (foot)
+ {
+ footnote_stencil = *foot;
+ foot = &footnote_stencil;
+ }
SCM footnotes = Page_layout_problem::get_footnotes_from_lines (systems);
Page_layout_problem::add_footnotes_to_footer (footnotes, foot, book_);
if (foot)
- p->set_property ("foot-stencil", foot->smobbed_copy ());
+ p->set_property ("foot-stencil", footnote_stencil.smobbed_copy ());
scm_apply_1 (page_stencil, page, SCM_EOL);
return page;
Index: lily/page-layout-problem.cc
diff --git a/lily/page-layout-problem.cc b/lily/page-layout-problem.cc
index
6320827c767a74b07078d328eb7041d7649f6179..82be7f204fe11223d15050ccebbf01c2d28a2dc9
100644
--- a/lily/page-layout-problem.cc
+++ b/lily/page-layout-problem.cc
@@ -162,7 +162,7 @@ Page_layout_problem::add_footnotes_to_lines (SCM lines,
int counter, Paper_book
by passing its results downstream.
*/
vector<SCM> footnote_number_markups; // Holds the numbering markups.
- vector<Stencil *> footnote_number_stencils; // Holds translated versions
of the stencilized numbering markups.
+ vector<Stencil> footnote_number_stencils; // Holds translated versions
of the stencilized numbering markups.
for (vsize i = 0; i < fn_count; i++)
{
if (fn_grobs[i])
@@ -176,36 +176,39 @@ Page_layout_problem::add_footnotes_to_lines (SCM
lines, int counter, Paper_book
if (!s)
{
programming_error ("Your numbering function needs to return a
stencil.");
- markup = SCM_EOL;
- s = new Stencil (Box (Interval (0, 0), Interval (0, 0)),
SCM_EOL);
+ footnote_number_markups.push_back (SCM_EOL);
+ footnote_number_stencils.push_back (Stencil (Box (Interval (0,
0), Interval (0, 0)), SCM_EOL));
}
- footnote_number_markups.push_back (markup);
- footnote_number_stencils.push_back (s);
+ else
+ {
+ footnote_number_markups.push_back (markup);
+ footnote_number_stencils.push_back (*s);
+ }
counter++;
}
// find the maximum X_AXIS length
Real max_length = -infinity_f;
for (vsize i = 0; i < fn_count; i++)
- max_length = max (max_length, footnote_number_stencils[i]->extent
(X_AXIS).length ());
+ max_length = max (max_length, footnote_number_stencils[i].extent
(X_AXIS).length ());
/*
translate each stencil such that it attains the correct maximum length
and bundle the
footnotes into a scheme object.
*/
- SCM *tail = &numbers;
- SCM *in_text_tail = &in_text_numbers;
for (vsize i = 0; i < fn_count; i++)
{
- *in_text_tail = scm_cons (footnote_number_markups[i], SCM_EOL);
- in_text_tail = SCM_CDRLOC (*in_text_tail);
- footnote_number_stencils[i]->translate_axis ((max_length
- -
footnote_number_stencils[i]->extent (X_AXIS).length ()),
+ in_text_numbers = scm_cons (footnote_number_markups[i],
in_text_numbers);
+ footnote_number_stencils[i].translate_axis ((max_length
+ -
footnote_number_stencils[i].extent (X_AXIS).length ()),
X_AXIS);
- *tail = scm_cons (footnote_number_stencils[i]->smobbed_copy (),
SCM_EOL);
- tail = SCM_CDRLOC (*tail);
+ numbers = scm_cons (footnote_number_stencils[i].smobbed_copy (),
numbers);
}
+
+ in_text_numbers = scm_reverse_x (in_text_numbers, SCM_EOL);
+ numbers = scm_reverse_x (numbers, SCM_EOL);
+
// build the footnotes
for (SCM s = lines; scm_is_pair (s); s = scm_cdr (s))
@@ -236,7 +239,7 @@ Page_layout_problem::add_footnotes_to_lines (SCM lines,
int counter, Paper_book
SCM footnote_stl = Text_interface::interpret_markup
(paper->self_scm (),
props,
footnote_markup);
- Stencil *footnote_stencil = unsmob_stencil (footnote_stl);
+ Stencil footnote_stencil = *unsmob_stencil (footnote_stl);
bool do_numbering = to_boolean (footnote->get_property
("automatically-numbered"));
if (Spanner *orig = dynamic_cast<Spanner *>(footnote))
{
@@ -257,21 +260,21 @@ Page_layout_problem::add_footnotes_to_lines (SCM
lines, int counter, Paper_book
orig->broken_intos_[i]->set_property ("text",
annotation_scm);
}
- Stencil *annotation = unsmob_stencil (scm_car (numbers));
- annotation->translate_axis ((footnote_stencil->extent
(Y_AXIS)[UP]
- + number_raise
- - annotation->extent
(Y_AXIS)[UP]),
+ Stencil annotation = *unsmob_stencil (scm_car (numbers));
+ annotation.translate_axis ((footnote_stencil.extent
(Y_AXIS)[UP]
+ + number_raise
+ - annotation.extent (Y_AXIS)[UP]),
Y_AXIS);
- footnote_stencil->add_at_edge (X_AXIS, LEFT,
*annotation, 0.0);
+ footnote_stencil.add_at_edge (X_AXIS, LEFT, annotation,
0.0);
numbers = scm_cdr (numbers);
in_text_numbers = scm_cdr (in_text_numbers);
}
- if (!footnote_stencil->is_empty ())
+ if (!footnote_stencil.is_empty ())
{
if (to_boolean (footnote->get_property ("footnote")))
- mol.add_at_edge (Y_AXIS, DOWN, *footnote_stencil,
padding);
+ mol.add_at_edge (Y_AXIS, DOWN, footnote_stencil,
padding);
else
- in_note_mol.add_at_edge (Y_AXIS, DOWN,
*footnote_stencil, padding);
+ in_note_mol.add_at_edge (Y_AXIS, DOWN,
footnote_stencil, padding);
}
}
sys->set_property ("in-note-stencil", in_note_mol.smobbed_copy
());
@@ -292,18 +295,18 @@ Page_layout_problem::add_footnotes_to_lines (SCM
lines, int counter, Paper_book
SCM in_text_stencil = Stencil ().smobbed_copy ();
if (do_numbering)
{
- Stencil *annotation = unsmob_stencil (scm_car (numbers));
+ Stencil annotation = *unsmob_stencil (scm_car (numbers));
SCM in_text_annotation = scm_car (in_text_numbers);
in_text_stencil = Text_interface::interpret_markup
(layout,
props,
in_text_annotation);
if (!unsmob_stencil (in_text_stencil))
in_text_stencil = SCM_EOL;
- annotation->translate_axis ((footnote_stencil.extent
(Y_AXIS)[UP]
- + number_raise
- - annotation->extent
(Y_AXIS)[UP]),
- Y_AXIS);
- footnote_stencil.add_at_edge (X_AXIS, LEFT, *annotation,
0.0);
+ annotation.translate_axis ((footnote_stencil.extent
(Y_AXIS)[UP]
+ + number_raise
+ - annotation.extent (Y_AXIS)[UP]),
+ Y_AXIS);
+ footnote_stencil.add_at_edge (X_AXIS, LEFT, annotation,
0.0);
numbers = scm_cdr (numbers);
in_text_numbers = scm_cdr (in_text_numbers);
}
@@ -330,7 +333,7 @@ Page_layout_problem::get_footnote_separator_stencil
(Output_def *paper)
SCM markup = paper->c_variable ("footnote-separator-markup");
if (!Text_interface::is_markup (markup))
- return NULL;
+ return 0;
SCM footnote_stencil = Text_interface::interpret_markup (paper->self_scm
(),
props, markup);
@@ -390,9 +393,17 @@ Page_layout_problem::Page_layout_problem (Paper_book
*pb, SCM page_scm, SCM syst
if (page)
{
+ Stencil foot_stencil;
+
Stencil *head = unsmob_stencil (page->get_property ("head-stencil"));
Stencil *foot = unsmob_stencil (page->get_property ("foot-stencil"));
+ if (foot)
+ {
+ foot_stencil = *foot;
+ foot = &foot_stencil;
+ }
+
if (pb && pb->paper_)
{
SCM footnotes = get_footnotes_from_lines (systems);
- Re: Issue 2376: Automatic footnotes on \null markups causes unexpected results (issue 5755058),
dak <=