Review Request 122278: Additional changes in section handling
Inge Wallin
inge at lysator.liu.se
Thu Jan 29 13:11:39 GMT 2015
On Thursday, January 29, 2015 12:57:35 Denis Kuplyakov wrote:
> > On Янв. 27, 2015, 7:34 п.п., Camilla Boemann wrote:
> > > libs/kotext/KoSectionManager.cpp, line 120
> > > <https://git.reviewboard.kde.org/r/122278/diff/1/?file=345390#file345390
> > > line120>> >
> > > 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>
> > Elvis Stansvik wrote:
> > 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>
> > Denis Kuplyakov wrote:
> > I have tried to use mutable with no luck. I doesn't understand how can
> > I properly apply it in my case: isValidNewName() calls sectionNames()
> > of its private part, and it calls update() public method. It is
> > absolutely unlogic to make update() const function. And also two
> > subsequent calls of isValidNewName() could give you different
> > results, even if KoSectionManager isn't changed. You can create a
> > section between them without referencing anywhere KoSectionManager
> > (creating KoSection with QTextCursor, will get document associated
> > KoSectionManager by pointer, and update() of it will be called and
> > that will be done regardless of you local entity of KoSectionManager
> > is const or not (which is pointed)). And KoSectionManager isn't
> > supposed to be part of some other class, as I think. It is created
> > dynamically and only links to it are stored.>
> > Camilla Boemann wrote:
> > ah yes, you are right it can't be applied to this case. Hmmm why do
> > sections even have names, and why do they then have to be unique -
> > surely we are not exposing some odf attribute?
19.835.14 <text:section>
The text:name attribute specifies a unique name for a section.
The text:name attribute is usable with the following element: <text:section>
5.4.
The text:name attribute has the data type string 18.2.
(Note the beautiful typo in the heading where it says text:section instead of
text:name. :) )
It does not say that it has to have a name, but if it has, it has to be
unique. It does also not say that it does *not* have to have a name, so it's
unclear whether the name is obligatory or optional.
> As I remember ODF documents say, that Section element has a Name that should
> be unique.
> > On Янв. 27, 2015, 7:34 п.п., Camilla Boemann wrote:
> > > libs/kotext/commands/DeleteCommand.cpp, line 321
> > > <https://git.reviewboard.kde.org/r/122278/diff/1/?file=345394#file345394
> > > line321>> >
> > > yikes
> >
> > Denis Kuplyakov wrote:
> > QVariant before was used like
> > static_cast<KoSection*>(v.value<void*>()). And this structure only
> > for local use, doesn't look dangerous for me.>
> > Camilla Boemann wrote:
> > well why do we have both KoSection and KoSectionEnd and not just say
> > KoSectionDelimiter that handles both. Just wondering - not saying it
> > has to be like that
> This is entirely different entities: KoSection has many things like own
> style, name and so on (and I can't say that delimiter could have style).
> And KoSectionEnd is a simple helper class that links to corresponding
> KoSection.
>
>
> - Denis
>
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/122278/#review74840
> -----------------------------------------------------------
>
> On Янв. 28, 2015, 8:21 п.п., Denis Kuplyakov wrote:
> > -----------------------------------------------------------
> > This is an automatically generated e-mail. To reply, visit:
> > https://git.reviewboard.kde.org/r/122278/
> > -----------------------------------------------------------
> >
> > (Updated Янв. 28, 2015, 8:21 п.п.)
> >
> >
> > 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
More information about the calligra-devel
mailing list