Review Request 122278: Additional changes in section handling

Camilla Boemann cbr at boemann.dk
Thu Jan 29 13:35:43 GMT 2015 


> On Jan. 27, 2015, 7:34 p.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
> 
> 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?
> 
> Denis Kuplyakov wrote:
>     As I remember ODF documents say, that Section element has a Name that should be unique.

well yes, but what is stored in an odf file can be just some kind of id we generate on the fly when saving. It's not a given that we have keep it around and more importantly show and allow the user to create new names. I'm just wonderin if naming the sections brings the users anything valuable.


- Camilla


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122278/#review74840
-----------------------------------------------------------


On Jan. 28, 2015, 8:21 p.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 p.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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/calligra-devel/attachments/20150129/4826004e/attachment.htm>

More information about the calligra-devel mailing list