[prev in list] [next in list] [prev in thread] [next in thread]
List: calligra-devel
Subject: Re: Review Request 120733: Pass more data between layout and RootAreaProvider
From: "Pierre Ducroquet" <pinaraf () pinaraf ! info>
Date: 2014-10-23 17:23:53
Message-ID: 20141023172353.18179.14119 () probe ! kde ! org
[Download RAW message or body]
--===============4385057474751354890==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8bit
> On Oct. 23, 2014, 7:28 a.m., Camilla Boemann wrote:
> > I like the idea of the constraints, and how you fill it
> >
> > but i am not so sure about the doLayout() testing and acting on the constraints. \
> > I feel this belong in the provider if at all possible
> > And encountering a different set of constraints shouldn't have to mean we release \
> > all root areas after. So releaseAllAfter should only be called when we are at the \
> > end of the layout process, otherwise the provider should just give us the page we \
> > need, and we should just keep on checking if we end where we ended before and if \
> > we get back in sync we shouldn't have to relayout everything (which \
> > releasAllAfter would mean we had to do and just think if we were editng on page 1 \
> > of a 800 page document)
> > i am not saying it's possible but i would surely like it better like that
I understand your feeling about releasing all root areas after. This was the shortest \
path to the fix, and obviously it has to be improved. But I think inserting a new \
root area in the middle of the root area list may introduce serious issues with \
advanced page layout. I'll think about this tonight and look at what is possible, but \
when you link page styles together (I'm not sure we already have that feature, but \
that's on my todo list), then you end up inserting a lot of pages and at the end \
release most pages. It's a matter of calculating (or more likely deciding) which \
costs less.
> On Oct. 23, 2014, 7:28 a.m., Camilla Boemann wrote:
> > words/part/KWRootAreaProvider.cpp, line 168
> > <https://git.reviewboard.kde.org/r/120733/diff/1/?file=321689#file321689line168>
> >
> > i guess we should insert a page instead of appending (thus avoiding the need to \
> > release all after) if it mismatches the next available page in the current list \
> > of pages
> > possibly this could be even smarter
I partially agree, I'll try that way nevertheless, but I fear it's going to have much \
more side effects.
> On Oct. 23, 2014, 7:28 a.m., Camilla Boemann wrote:
> > libs/textlayout/KoTextDocumentLayout.cpp, line 757
> > <https://git.reviewboard.kde.org/r/120733/diff/1/?file=321682#file321682line757>
> >
> > this part i don't like being in the library - i think it should rest upon the \
> > provider
I agree, I don't like it either. I just didn't want to alter the rootAreaProvider \
interface too much, but I'll have to go that way for your other issue so it won't be \
too much trouble.
- Pierre
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120733/#review68957
-----------------------------------------------------------
On Oct. 23, 2014, 7:15 a.m., Pierre Ducroquet wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120733/
> -----------------------------------------------------------
>
> (Updated Oct. 23, 2014, 7:15 a.m.)
>
>
> Review request for Calligra and Camilla Boemann.
>
>
> Repository: calligra
>
>
> Description
> -------
>
> This patch refactors a bit the KoTextLayoutRootAreaProvider.
> It extends the provideNext call by adding a constraints structure.
> This structure contains so far the masterPageName and the page
> number of the requested new rootArea.
> This way, the provider don't have to sneak in layout specific data.
> It also adds a way for KoTextLayoutRootAreaProvider to help the
> layout and fix inserting a new page (by adding lines to a paragraph
> for instance) before a page break changing the page style.
>
> Some more explanations regarding this bug :
> - Page 1, style AAA
> * page break, requesting style BBB
> - Page 2, style BBB
>
> If you add more data to the page 1, then dynamically the layout should
> do :
> - Page 1, style AAA
> - Page 2, style AAA
> * page break, requesting style BBB
> - Page 3, style BBB
>
> But the layout did recycle the page 2, leading to :
> - Page 1, style AAA
> - Page 2, style BBB
> * page break, requesting style BBB
> - Page 3, style BBB
>
> Reloading the document leads to a correct layout.
>
>
> Diffs
> -----
>
> libs/textlayout/KoTextDocumentLayout.cpp 805f11d1c4aaf90a1a2a654053889e06800193de
> libs/textlayout/KoTextLayoutRootAreaProvider.h \
> 23b91bc519761eef2861fc6fc4546294271389cc \
> libs/textlayout/tests/MockRootAreaProvider.h \
> 23056384fb55918ae360ad062d5dfd13ee7c4837 \
> libs/textlayout/tests/MockRootAreaProvider.cpp \
> 0402528998c26cfb586f297abe7c879459adda50 \
> plugins/textshape/SimpleRootAreaProvider.h afada40af1ea3341a820dc0d275a2db5bbb5a3f8 \
> plugins/textshape/SimpleRootAreaProvider.cpp \
> 42df2ecd30aa744780642618556a7013a9ff27da words/part/KWRootAreaProvider.h \
> 8fb9b5e379a0561fceb7ba32935084df46ce6fd9 words/part/KWRootAreaProvider.cpp \
> 62b6fb6fba4dea8d9f68b0b3c1afb7324c50d502
> Diff: https://git.reviewboard.kde.org/r/120733/diff/
>
>
> Testing
> -------
>
> A simple document with page styles (changing the background color to help speed up \
> the development) now doesn't exhibit the wrong layout behaviour.
>
> Thanks,
>
> Pierre Ducroquet
>
>
--===============4385057474751354890==
MIME-Version: 1.0
Content-Type: text/html; charset="utf-8"
Content-Transfer-Encoding: 8bit
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 \
solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;"> \
<tr> <td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/120733/">https://git.reviewboard.kde.org/r/120733/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On October 23rd, 2014, 7:28 a.m. UTC, <b>Camilla \
Boemann</b> wrote:</p> <blockquote style="margin-left: 1em; border-left: 2px solid \
#d0d0d0; padding-left: 10px;"> <pre style="white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">I like the idea of the constraints, and how you fill \
it</p> <p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">but i am not so sure about the doLayout() testing and \
acting on the constraints. I feel this belong in the provider if at all possible</p> \
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">And encountering a different set of constraints \
shouldn't have to mean we release all root areas after. So releaseAllAfter should \
only be called when we are at the end of the layout process, otherwise the provider \
should just give us the page we need, and we should just keep on checking if we end \
where we ended before and if we get back in sync we shouldn't have to relayout \
everything (which releasAllAfter would mean we had to do and just think if we were \
editng on page 1 of a 800 page document)</p> <p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">i am not saying it's \
possible but i would surely like it better like that</p></pre> </blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I \
understand your feeling about releasing all root areas after. This was the shortest \
path to the fix, and obviously it has to be improved. But I think inserting a new \
root area in the middle of the root area list may introduce serious issues with \
advanced page layout. I'll think about this tonight and look at what is possible, but \
when you link page styles together (I'm not sure we already have that feature, but \
that's on my todo list), then you end up inserting a lot of pages and at the end \
release most pages. It's a matter of calculating (or more likely deciding) which \
costs less.</p></pre> <br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On October 23rd, 2014, 7:28 a.m. UTC, <b>Camilla \
Boemann</b> wrote:</p> <blockquote style="margin-left: 1em; border-left: 2px solid \
#d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; \
border-collapse: collapse; margin: 2px padding: 2px;"> <thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; \
font-size: 9pt; padding: 4px 8px; text-align: left;"> <a \
href="https://git.reviewboard.kde.org/r/120733/diff/1/?file=321682#file321682line757" \
style="color: black; font-weight: bold; text-decoration: \
underline;">libs/textlayout/KoTextDocumentLayout.cpp</a> <span style="font-weight: \
normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">void \
KoTextDocumentLayout::layout()</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th> <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td> <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">757</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="k">if</span> <span class="p">((</span><span class="n">rootArea</span><span \
class="o">-></span><span class="n">page</span><span class="p">()</span> <span \
class="o">&&</span></pre></td> </tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">this \
part i don't like being in the library - i think it should rest upon the \
provider</p></pre> </blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">I agree, I don't like it either. I just didn't want to alter the \
rootAreaProvider interface too much, but I'll have to go that way for your other \
issue so it won't be too much trouble.</p></pre> <br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On October 23rd, 2014, 7:28 a.m. UTC, <b>Camilla \
Boemann</b> wrote:</p> <blockquote style="margin-left: 1em; border-left: 2px solid \
#d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; \
border-collapse: collapse; margin: 2px padding: 2px;"> <thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; \
font-size: 9pt; padding: 4px 8px; text-align: left;"> <a \
href="https://git.reviewboard.kde.org/r/120733/diff/1/?file=321689#file321689line168" \
style="color: black; font-weight: bold; text-decoration: \
underline;">words/part/KWRootAreaProvider.cpp</a> <span style="font-weight: \
normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; \
">KoTextLayoutRootArea* KWRootAreaProvider::provideNext(KoTextDocumentLayout \
*documentLayout)</pre></td> <td colspan="2"><pre style="font-size: 8pt; line-height: \
140%; margin: 0; ">KoTextLayoutRootArea* \
KWRootAreaProvider::provideNext(KoTextDocumentLayout* documentLayout, const \
RootAreaConstraint& constraints)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2">168</font></th> <td bgcolor="#fdfebc" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="n"><span class="hl">QString</span></span><span class="hl"> </span><span \
class="n">masterPageName</span><span class="p">;</span></pre></td> <th \
bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">168</font></th> <td bgcolor="#fdfebc" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> \
<span class="n"><span class="hl">KWPage</span></span><span class="hl"> </span><span \
class="n"><span class="hl">page</span></span><span class="hl"> </span><span \
class="o"><span class="hl">=</span></span><span class="hl"> </span><span \
class="n"><span class="hl">kwdoc</span></span><span class="o"><span \
class="hl">-></span></span><span class="n"><span \
class="hl">appendPage</span></span><span class="p"><span \
class="hl">(</span></span><span class="n"><span \
class="hl">constraints</span></span><span class="p"><span \
class="hl">.</span></span><span class="n">masterPageName</span><span class="p"><span \
class="hl">)</span>;</span></pre></td> </tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">i \
guess we should insert a page instead of appending (thus avoiding the need to release \
all after) if it mismatches the next available page in the current list of pages</p> \
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">possibly this could be even smarter</p></pre> \
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">I partially agree, I'll try that way nevertheless, but I fear it's going to \
have much more side effects.</p></pre> <br />
<p>- Pierre</p>
<br />
<p>On October 23rd, 2014, 7:15 a.m. UTC, Pierre Ducroquet wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: \
1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; \
-webkit-border-radius: 6px;"> <tr>
<td>
<div>Review request for Calligra and Camilla Boemann.</div>
<div>By Pierre Ducroquet.</div>
<p style="color: grey;"><i>Updated Oct. 23, 2014, 7:15 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
calligra
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" \
style="border: 1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">This patch refactors a bit the \
KoTextLayoutRootAreaProvider. It extends the provideNext call by adding a constraints \
structure. This structure contains so far the masterPageName and the page
number of the requested new rootArea.
This way, the provider don't have to sneak in layout specific data.
It also adds a way for KoTextLayoutRootAreaProvider to help the
layout and fix inserting a new page (by adding lines to a paragraph
for instance) before a page break changing the page style.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">Some more explanations regarding this \
bug :
- Page 1, style AAA
* page break, requesting style BBB
- Page 2, style BBB</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">If you add more data to the page 1, then dynamically \
the layout should do :
- Page 1, style AAA
- Page 2, style AAA
* page break, requesting style BBB
- Page 3, style BBB</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">But the layout did recycle the page 2, \
leading to :
- Page 1, style AAA
- Page 2, style BBB
* page break, requesting style BBB
- Page 3, style BBB</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">Reloading the document leads to a correct \
layout.</p></pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">A simple document with page styles (changing the \
background color to help speed up the development) now doesn't exhibit the wrong \
layout behaviour.</p></pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>libs/textlayout/KoTextDocumentLayout.cpp <span style="color: \
grey">(805f11d1c4aaf90a1a2a654053889e06800193de)</span></li>
<li>libs/textlayout/KoTextLayoutRootAreaProvider.h <span style="color: \
grey">(23b91bc519761eef2861fc6fc4546294271389cc)</span></li>
<li>libs/textlayout/tests/MockRootAreaProvider.h <span style="color: \
grey">(23056384fb55918ae360ad062d5dfd13ee7c4837)</span></li>
<li>libs/textlayout/tests/MockRootAreaProvider.cpp <span style="color: \
grey">(0402528998c26cfb586f297abe7c879459adda50)</span></li>
<li>plugins/textshape/SimpleRootAreaProvider.h <span style="color: \
grey">(afada40af1ea3341a820dc0d275a2db5bbb5a3f8)</span></li>
<li>plugins/textshape/SimpleRootAreaProvider.cpp <span style="color: \
grey">(42df2ecd30aa744780642618556a7013a9ff27da)</span></li>
<li>words/part/KWRootAreaProvider.h <span style="color: \
grey">(8fb9b5e379a0561fceb7ba32935084df46ce6fd9)</span></li>
<li>words/part/KWRootAreaProvider.cpp <span style="color: \
grey">(62b6fb6fba4dea8d9f68b0b3c1afb7324c50d502)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/120733/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
--===============4385057474751354890==--
_______________________________________________
calligra-devel mailing list
calligra-devel@kde.org
https://mail.kde.org/mailman/listinfo/calligra-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic