[prev in list] [next in list] [prev in thread] [next in thread]
List: calligra-devel
Subject: Re: Review Request 122238: KoStyleManager independence from kotext
From: "Inge Wallin" <inge () lysator ! liu ! se>
Date: 2015-01-25 10:41:35
Message-ID: 20150125104135.30682.91845 () probe ! kde ! org
[Download RAW message or body]
--===============2491850817741341425==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122238/#review74689
-----------------------------------------------------------
Ship it!
Much much clearer!
I do have some comments below that you might want to look at, and perhaps write a \
little doc. But I see no need for another round of review.
libs/kotext/OdfTextTrackStyles.h
<https://git.reviewboard.kde.org/r/122238/#comment51777>
Some questions:
- Who is supposed to own instance(s?) of this class?
- Where does it fit in the big scheme of things?
- what does 'handle' in 'we handle them all' mean? Relayout? Store changes \
somewhere?
libs/kotext/OdfTextTrackStyles.cpp
<https://git.reviewboard.kde.org/r/122238/#comment51778>
Hmm, do we have more than one KoStyleManager? I thought the one handled all \
styles.
libs/kotext/OdfTextTrackStyles.cpp
<https://git.reviewboard.kde.org/r/122238/#comment51779>
Thank you for this naming. :)
Maybe we can make it a standard within calligra?
libs/kotext/OdfTextTrackStyles.cpp
<https://git.reviewboard.kde.org/r/122238/#comment51780>
From conversation on IRC I learned that one command is supposed to track all \
changes of all styles from one call of the visual style manager.
But this doesn't fit because here we only send one changed style to be stored in \
the command.
Update: I learned later that there is macro that takes care of this, so maybe \
just document that a bit and it's fine.
- Inge Wallin
On Jan. 25, 2015, 1:40 a.m., Camilla Boemann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/122238/
> -----------------------------------------------------------
>
> (Updated Jan. 25, 2015, 1:40 a.m.)
>
>
> Review request for Calligra.
>
>
> Repository: calligra
>
>
> Description
> -------
>
> Make KoStyleManager independent of rest of libs/kotext - making it possible to move \
> it to libs/odf also fixes a bug that stylechanges were never applied to the \
> document
>
> Diffs
> -----
>
> libs/kotext/CMakeLists.txt bfb0b2d
> libs/kotext/KoTextDocument.cpp 07a47dc
> libs/kotext/OdfTextTrackStyles.h PRE-CREATION
> libs/kotext/OdfTextTrackStyles.cpp PRE-CREATION
> libs/kotext/commands/ChangeStylesCommand.h 3762715
> libs/kotext/commands/ChangeStylesCommand.cpp 771caae
> libs/kotext/commands/ChangeStylesMacroCommand.h 340ba12
> libs/kotext/commands/ChangeStylesMacroCommand.cpp 66f5326
> libs/kotext/styles/ChangeFollower.h 5786541
> libs/kotext/styles/ChangeFollower.cpp 7930cfe
> libs/kotext/styles/KoStyleManager.h e1b400c
> libs/kotext/styles/KoStyleManager.cpp 86108d6
> libs/textlayout/KoTextShapeData.cpp c51ec6e
> sheets/Map.cpp a4fcc7c
>
> Diff: https://git.reviewboard.kde.org/r/122238/diff/
>
>
> Testing
> -------
>
> only basic editing and seeing that undo/redo works and that changes are now \
> actually appied to the document
>
> Thanks,
>
> Camilla Boemann
>
>
--===============2491850817741341425==
MIME-Version: 1.0
Content-Type: text/html; charset="utf-8"
Content-Transfer-Encoding: 7bit
<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/122238/">https://git.reviewboard.kde.org/r/122238/</a>
</td>
</tr>
</table>
<br />
<p>Ship it!</p>
<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;">Much \
much clearer! </p> <p style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: inherit;">I do have some comments below that you \
might want to look at, and perhaps write a little doc. But I see no need for another \
round of review.</p></pre> <br />
<div>
<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/122238/diff/3/?file=344657#file344657line34" \
style="color: black; font-weight: bold; text-decoration: \
underline;">libs/kotext/OdfTextTrackStyles.h</a> <span style="font-weight: normal;">
(Diff revision 3)
</span>
</th>
</tr>
</thead>
<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">34</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span \
class="cm"> * OdfTextTrackStyles is used to update a list of qtextdocument \
with</span></pre></td> </tr>
</tbody>
</table>
<div style="margin-left: 2em;">
<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;">Some \
questions:
- Who is supposed to own instance(s?) of this class?
- Where does it fit in the big scheme of things?
- what does 'handle' in 'we handle them all' mean? Relayout? Store changes \
somewhere?</p></pre> </div>
</div>
<br />
<div>
<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/122238/diff/3/?file=344658#file344658line30" \
style="color: black; font-weight: bold; text-decoration: \
underline;">libs/kotext/OdfTextTrackStyles.cpp</a> <span style="font-weight: \
normal;">
(Diff revision 3)
</span>
</th>
</tr>
</thead>
<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">30</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span \
class="n">QMap</span><span class="o"><</span><span class="n">KoStyleManager</span> \
<span class="o">*</span><span class="p">,</span> <span \
class="n">OdfTextTrackStyles</span> <span class="o">*></span> <span \
class="n">OdfTextTrackStyles</span><span class="o">::</span><span \
class="n">instances</span><span class="p">;</span></pre></td> </tr>
</tbody>
</table>
<div style="margin-left: 2em;">
<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;">Hmm, \
do we have more than one KoStyleManager? I thought the one handled all \
styles.</p></pre> </div>
</div>
<br />
<div>
<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/122238/diff/3/?file=344658#file344658line41" \
style="color: black; font-weight: bold; text-decoration: \
underline;">libs/kotext/OdfTextTrackStyles.cpp</a> <span style="font-weight: \
normal;">
(Diff revision 3)
</span>
</th>
</tr>
</thead>
<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">41</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span \
class="kt">void</span> <span class="n">OdfTextTrackStyles</span><span \
class="o">::</span><span class="n">registerDocument</span><span \
class="p">(</span><span class="n">QTextDocument</span> <span class="o">*</span><span \
class="n">qDoc</span><span class="p">)</span></pre></td> </tr>
</tbody>
</table>
<div style="margin-left: 2em;">
<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;">Thank \
you for this naming. :)</p> <p style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: inherit;">Maybe we can make it a standard within \
calligra?</p></pre> </div>
</div>
<br />
<div>
<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/122238/diff/3/?file=344658#file344658line88" \
style="color: black; font-weight: bold; text-decoration: \
underline;">libs/kotext/OdfTextTrackStyles.cpp</a> <span style="font-weight: \
normal;">
(Diff revision 3)
</span>
</th>
</tr>
</thead>
<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">88</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span \
class="kt">void</span> <span class="n">OdfTextTrackStyles</span><span \
class="o">::</span><span class="n">recordStyleChange</span><span \
class="p">(</span><span class="kt">int</span> <span class="n">id</span><span \
class="p">,</span> <span class="k">const</span> <span \
class="n">KoParagraphStyle</span> <span class="o">*</span><span \
class="n">origStyle</span><span class="p">,</span> <span class="k">const</span> <span \
class="n">KoParagraphStyle</span> <span class="o">*</span><span \
class="n">newStyle</span><span class="p">)</span></pre></td> </tr>
</tbody>
</table>
<div style="margin-left: 2em;">
<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;">From \
conversation on IRC I learned that one command is supposed to track all changes of \
all styles from one call of the visual style manager.</p> <p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">But \
this doesn't fit because here we only send one changed style to be stored in the \
command.</p> <p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">Update: I learned later that there is macro that takes \
care of this, so maybe just document that a bit and it's fine.</p></pre> </div>
</div>
<br />
<p>- Inge Wallin</p>
<br />
<p>On January 25th, 2015, 1:40 a.m. CET, Camilla Boemann 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.</div>
<div>By Camilla Boemann.</div>
<p style="color: grey;"><i>Updated Jan. 25, 2015, 1:40 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;">Make KoStyleManager independent of rest of libs/kotext \
- making it possible to move it to libs/odf also fixes a bug that stylechanges were \
never applied to the document</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;">only basic editing and seeing that undo/redo works and \
that changes are now actually appied to the document</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/kotext/CMakeLists.txt <span style="color: grey">(bfb0b2d)</span></li>
<li>libs/kotext/KoTextDocument.cpp <span style="color: grey">(07a47dc)</span></li>
<li>libs/kotext/OdfTextTrackStyles.h <span style="color: \
grey">(PRE-CREATION)</span></li>
<li>libs/kotext/OdfTextTrackStyles.cpp <span style="color: \
grey">(PRE-CREATION)</span></li>
<li>libs/kotext/commands/ChangeStylesCommand.h <span style="color: \
grey">(3762715)</span></li>
<li>libs/kotext/commands/ChangeStylesCommand.cpp <span style="color: \
grey">(771caae)</span></li>
<li>libs/kotext/commands/ChangeStylesMacroCommand.h <span style="color: \
grey">(340ba12)</span></li>
<li>libs/kotext/commands/ChangeStylesMacroCommand.cpp <span style="color: \
grey">(66f5326)</span></li>
<li>libs/kotext/styles/ChangeFollower.h <span style="color: \
grey">(5786541)</span></li>
<li>libs/kotext/styles/ChangeFollower.cpp <span style="color: \
grey">(7930cfe)</span></li>
<li>libs/kotext/styles/KoStyleManager.h <span style="color: \
grey">(e1b400c)</span></li>
<li>libs/kotext/styles/KoStyleManager.cpp <span style="color: \
grey">(86108d6)</span></li>
<li>libs/textlayout/KoTextShapeData.cpp <span style="color: \
grey">(c51ec6e)</span></li>
<li>sheets/Map.cpp <span style="color: grey">(a4fcc7c)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/122238/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
--===============2491850817741341425==--
_______________________________________________
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