[prev in list] [next in list] [prev in thread] [next in thread]
List: calligra-devel
Subject: Re: Review Request 122278: Additional changes in section handling
From: "Elvis Stansvik" <elvstone () gmail ! com>
Date: 2015-01-28 20:39:11
Message-ID: 20150128203911.30683.6621 () probe ! kde ! org
[Download RAW message or body]
--===============8910554897592091071==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
> On jan 27, 2015, 7:34 e.m., Camilla Boemann wrote:
> > libs/kotext/KoSectionManager.cpp, line 120
> > <https://git.reviewboard.kde.org/r/122278/diff/1/?file=345390#file345390line120>
> >
> > why no long const ?
>
> Denis Kuplyakov wrote:
> I don't know why, but QHash<T,Q>::find() isn't const. Thats why I can't make this function const and \
> possibleNewName() too as dependant.
> Denis Kuplyakov wrote:
> Ohh, I see constFind there. Why they made it such way? Anyway, will fix this.
>
> Denis Kuplyakov wrote:
> There is a sectionNames() call, that updates internal data, to prevent using of invalidated one. So we \
> can't put const here.
> Friedrich W. H. Kossebau wrote:
> Could the members which hold that data that is only lazily fetched/updated be tagged with `mutable` \
> perhaps? Problem with non-const get-methods is that this escalates up in the call hierarchy, i.e. any \
> other method which uses this method now also needs to be non-const. Which is not nice, because it \
> spoils the usefulness of const, which should protect against unintended data modification. Which lazily \
> updated data surely is not, that's e.g. why there is `mutable`.
> Denis Kuplyakov wrote:
> Can you post me some link, where I can read about what you mean under mutable?
>
> Friedrich W. H. Kossebau wrote:
> Any good physical C++ book, hard to link ;) (no idea about what related ebooks there are)
>
> Hm, perhaps this here gives you a start: http://en.cppreference.com/w/cpp/language/cv
http://en.cppreference.com/w/cpp/language/cv - "mutable - applies to non-static class members of \
non-reference non-const type and specifies that the member does not affect the externally visible state \
of the class. mutable members of const classes are modifiable (Note: the C++ language grammar treats \
mutable as a storage-class-specifier, but it does not affect storage class)". Wikipedia has a slightly \
less technical description (second example): http://en.wikipedia.org/wiki/Immutable_object#C.2B.2B
- Elvis
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122278/#review74840
-----------------------------------------------------------
On jan 28, 2015, 8:21 e.m., Denis Kuplyakov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/122278/
> -----------------------------------------------------------
>
> (Updated jan 28, 2015, 8:21 e.m.)
>
>
> Review request for Calligra, Camilla Boemann and Inge Wallin.
>
>
> Repository: calligra
>
>
> Description
> -------
>
> This all things that I have done after GSoC, but they seems not be in master by now.
>
> 1) Added special functions to retrive startings and endings. Changed all accesses to use this functions \
> from KoSectionUtils. Also extracted some section related functionality to KoSectionUtils.
> 2) Fixed wrong indication of section level.
>
> 3) There were no invalidate send to KoSectionManager after paste to update section level.
>
> 4) Added KoSection and KoSectionEnd to Metatype system
>
> This allowed to reduce number of QVariant casts allwhere
> sections are used. Also fixed some missing usings of
> KoSectionUtils functions.
>
> 5) Deleted unused code somewhere.
>
> 6) KoSectionManager doesn't store QStandardItemModel now, but can generate it on update. Extracted \
> KoSectionManagerPrivate to cpp file back and changed QScopedPointer to usual pointer for MS VC \
> compiler. Now there is a set of all ever registered sections in KoSectionManager, cuz there are \
> sections that store in memory but cannot be deleted due to possible undo.
> 7) Bug with "Incorrect name" is fixed, if you try to rename section with name that existed before.
>
>
> Diffs
> -----
>
> plugins/textshape/dialogs/SimpleInsertWidget.cpp b5500ae
> words/part/dockers/KWDebugWidget.cpp 2aa53c0
> libs/kotext/KoSectionManager.h dc4821f
> libs/kotext/KoSectionManager.cpp df4e535
> libs/kotext/KoSectionUtils.h 1bd5eb3
> libs/kotext/KoSectionUtils.cpp cbcefd5
> libs/kotext/KoTextPaste.cpp 08c6cf8
> libs/kotext/commands/DeleteCommand.cpp 7c69df7
> libs/kotext/commands/NewSectionCommand.cpp 50ba14d
> libs/kotext/opendocument/KoTextLoader.cpp fe4713a
> libs/kotext/opendocument/KoTextWriter_p.cpp 0e4ea11
> libs/kotext/tests/TestKoTextEditor.cpp a4b02c0
> libs/textlayout/KoTextLayoutArea_paint.cpp d6fa3b8
> plugins/textshape/dialogs/SectionFormatDialog.cpp c825fd2
> libs/kotext/KoSection.h 8183958
> libs/kotext/KoSection.cpp a651fd4
> libs/kotext/KoSectionEnd.h 0005851
> libs/kotext/KoSectionEnd.cpp 0d7cf83
>
> Diff: https://git.reviewboard.kde.org/r/122278/diff/
>
>
> Testing
> -------
>
> Have played with sections a bit (deleting, inserting, renaming, undo). And DeleteCommand for sections \
> unittest is passing. Seems to be OK.
>
> Thanks,
>
> Denis Kuplyakov
>
>
--===============8910554897592091071==
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/122278/">https://git.reviewboard.kde.org/r/122278/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On januari 27th, 2015, 7:34 e.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/122278/diff/1/?file=345390#file345390line120" style="color: \
black; font-weight: bold; text-decoration: underline;">libs/kotext/KoSectionManager.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; ">bool \
KoSectionManager::isValidNewName(const QString &name) const</pre></td> <td colspan="2"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; ">bool KoSectionManager::isValidNewName(const \
QString &name)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font \
size="2">85</font></th> <td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; \
margin: 0; "><span class="kt">bool</span> <span class="n">KoSectionManager</span><span \
class="o">::</span><span class="n">isValidNewName</span><span class="p">(</span><span \
class="k">const</span> <span class="n">QString</span> <span class="o">&</span><span \
class="n">name</span><span class="p">)</span><span class="hl"> </span><span class="k"><span \
class="hl">const</span></span></pre></td> <th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; \
border-right: 1px solid #C0C0C0;" align="right"><font size="2">115</font></th> <td bgcolor="#fdfebc" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="kt">bool</span> \
<span class="n">KoSectionManager</span><span class="o">::</span><span \
class="n">isValidNewName</span><span class="p">(</span><span class="k">const</span> <span \
class="n">QString</span> <span class="o">&</span><span class="n">name</span><span \
class="p">)</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;">why no long const ?</p></pre> </blockquote>
<p>On januari 27th, 2015, 7:42 e.m. UTC, <b>Denis Kuplyakov</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 don't know why, but QHash<T,Q>::find() isn't const. Thats why I \
can't make this function const and possibleNewName() too as dependant.</p></pre> </blockquote>
<p>On januari 27th, 2015, 7:43 e.m. UTC, <b>Denis Kuplyakov</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;">Ohh, I see constFind there. Why they made it such way? Anyway, will fix \
this.</p></pre> </blockquote>
<p>On januari 28th, 2015, 8:05 e.m. UTC, <b>Denis Kuplyakov</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;">There is a sectionNames() call, that updates internal data, to prevent \
using of invalidated one. So we can't put const here.</p></pre> </blockquote>
<p>On januari 28th, 2015, 8:22 e.m. UTC, <b>Friedrich W. H. Kossebau</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;">Could the members which hold that data that is only lazily fetched/updated \
be tagged with <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: \
0;line-height: inherit;">mutable</code> perhaps? Problem with non-const get-methods is that this \
escalates up in the call hierarchy, i.e. any other method which uses this method now also needs to be \
non-const. Which is not nice, because it spoils the usefulness of const, which should protect against \
unintended data modification. Which lazily updated data surely is not, that's e.g. why there is <code \
style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: \
inherit;">mutable</code>.</p></pre> </blockquote>
<p>On januari 28th, 2015, 8:25 e.m. UTC, <b>Denis Kuplyakov</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;">Can you post me some link, where I can read about what you mean under \
mutable?</p></pre> </blockquote>
<p>On januari 28th, 2015, 8:38 e.m. UTC, <b>Friedrich W. H. Kossebau</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;">Any good physical C++ book, hard to link ;) (no idea about what related \
ebooks there are)</p> <p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">Hm, perhaps this here gives you a start: \
http://en.cppreference.com/w/cpp/language/cv</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;">http://en.cppreference.com/w/cpp/language/cv - "mutable - \
applies to non-static class members of non-reference non-const type and specifies that the member does \
not affect the externally visible state of the class. mutable members of const classes are modifiable \
(Note: the C++ language grammar treats mutable as a storage-class-specifier, but it does not affect \
storage class)". Wikipedia has a slightly less technical description (second example): \
http://en.wikipedia.org/wiki/Immutable_object#C.2B.2B</p></pre> <br />
<p>- Elvis</p>
<br />
<p>On januari 28th, 2015, 8:21 e.m. UTC, Denis Kuplyakov 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, Camilla Boemann and Inge Wallin.</div>
<div>By Denis Kuplyakov.</div>
<p style="color: grey;"><i>Updated jan 28, 2015, 8:21 e.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 all things that I have done after \
GSoC, but they seems not be in master by now.</p> <p style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: inherit;">1) Added special functions to retrive startings and \
endings. Changed all accesses to use this functions from KoSectionUtils. Also extracted some section \
related functionality to KoSectionUtils.</p> <p style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: inherit;">2) Fixed wrong indication of section level.</p> <p \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">3) There \
were no invalidate send to KoSectionManager after paste to update section level.</p> <p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">4) Added KoSection and \
KoSectionEnd to Metatype system</p> <p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">This allowed to reduce number of QVariant casts allwhere sections are \
used. Also fixed some missing usings of KoSectionUtils functions.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">5) \
Deleted unused code somewhere.</p> <p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">6) KoSectionManager doesn't store QStandardItemModel now, but can generate \
it on update. Extracted KoSectionManagerPrivate to cpp file back and changed QScopedPointer to usual \
pointer for MS VC compiler. Now there is a set of all ever registered sections in KoSectionManager, cuz \
there are sections that store in memory but cannot be deleted due to possible undo.</p> <p \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">7) Bug \
with "Incorrect name" is fixed, if you try to rename section with name that existed before.</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;">Have played with sections a bit (deleting, \
inserting, renaming, undo). And DeleteCommand for sections unittest is passing. Seems to be OK.</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>plugins/textshape/dialogs/SimpleInsertWidget.cpp <span style="color: grey">(b5500ae)</span></li>
<li>words/part/dockers/KWDebugWidget.cpp <span style="color: grey">(2aa53c0)</span></li>
<li>libs/kotext/KoSectionManager.h <span style="color: grey">(dc4821f)</span></li>
<li>libs/kotext/KoSectionManager.cpp <span style="color: grey">(df4e535)</span></li>
<li>libs/kotext/KoSectionUtils.h <span style="color: grey">(1bd5eb3)</span></li>
<li>libs/kotext/KoSectionUtils.cpp <span style="color: grey">(cbcefd5)</span></li>
<li>libs/kotext/KoTextPaste.cpp <span style="color: grey">(08c6cf8)</span></li>
<li>libs/kotext/commands/DeleteCommand.cpp <span style="color: grey">(7c69df7)</span></li>
<li>libs/kotext/commands/NewSectionCommand.cpp <span style="color: grey">(50ba14d)</span></li>
<li>libs/kotext/opendocument/KoTextLoader.cpp <span style="color: grey">(fe4713a)</span></li>
<li>libs/kotext/opendocument/KoTextWriter_p.cpp <span style="color: grey">(0e4ea11)</span></li>
<li>libs/kotext/tests/TestKoTextEditor.cpp <span style="color: grey">(a4b02c0)</span></li>
<li>libs/textlayout/KoTextLayoutArea_paint.cpp <span style="color: grey">(d6fa3b8)</span></li>
<li>plugins/textshape/dialogs/SectionFormatDialog.cpp <span style="color: grey">(c825fd2)</span></li>
<li>libs/kotext/KoSection.h <span style="color: grey">(8183958)</span></li>
<li>libs/kotext/KoSection.cpp <span style="color: grey">(a651fd4)</span></li>
<li>libs/kotext/KoSectionEnd.h <span style="color: grey">(0005851)</span></li>
<li>libs/kotext/KoSectionEnd.cpp <span style="color: grey">(0d7cf83)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/122278/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
--===============8910554897592091071==--
_______________________________________________
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