Review Request: Table of content for words

[Thorsten Zachmann]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/100421/#review1048
-----------------------------------------------------------


I have added some stuff I have seen while looking through the patch. I have not tested the patch.


libs/kotext/KoTableOfContentsGeneratorInfo.h
<http://git.reviewboard.kde.org/r/100421/#comment821>

    maybe move the variables to the end of the class to make it easier to find



libs/kotext/KoTableOfContentsGeneratorInfo.h
<http://git.reviewboard.kde.org/r/100421/#comment819>

    To work towards binary compatibility one day the implementation should be moved to the cpp file.
    
    Also I'm not sure that all the classes should be inside one file or if it would be better to split them up.



libs/kotext/KoTableOfContentsGeneratorInfo.h
<http://git.reviewboard.kde.org/r/100421/#comment820>

    please move into initlist of the constructor



libs/kotext/KoTableOfContentsGeneratorInfo.h
<http://git.reviewboard.kde.org/r/100421/#comment822>

    what is the 550 about?



libs/kotext/KoTableOfContentsGeneratorInfo.h
<http://git.reviewboard.kde.org/r/100421/#comment823>

    The opening { should be at the start of the next line 



libs/kotext/KoTableOfContentsGeneratorInfo.h
<http://git.reviewboard.kde.org/r/100421/#comment824>

    Looks like the indention is wrong here



libs/kotext/KoTableOfContentsGeneratorInfo.h
<http://git.reviewboard.kde.org/r/100421/#comment825>

    maybe rename KoTableOfContentsGeneratorInfo to KoTableOfContentsOdfGenerator
    
    Can this be moved to be in the same class that also loads the TOC?



libs/kotext/KoTableOfContentsGeneratorInfo.cpp
<http://git.reviewboard.kde.org/r/100421/#comment826>

    Is that constant needed?



libs/kotext/KoTableOfContentsGeneratorInfo.cpp
<http://git.reviewboard.kde.org/r/100421/#comment827>

    Maybe that should be moved to the destructor of TableOfContent



libs/kotext/KoTableOfContentsGeneratorInfo.cpp
<http://git.reviewboard.kde.org/r/100421/#comment828>

    maybe check of the namespaceURI only once as it used in all if clauses.



libs/kotext/KoTableOfContentsGeneratorInfo.cpp
<http://git.reviewboard.kde.org/r/100421/#comment829>

    maybe also the namespace should be checked.



libs/kotext/KoTableOfContentsGeneratorInfo.cpp
<http://git.reviewboard.kde.org/r/100421/#comment830>

    Maybe save also the QString once and reuse it every item instead of converting it every time again



libs/kotext/KoTableOfContentsGeneratorInfo.cpp
<http://git.reviewboard.kde.org/r/100421/#comment831>

    is the case needed here?



libs/kotext/KoTableOfContentsGeneratorInfo.cpp
<http://git.reviewboard.kde.org/r/100421/#comment832>

    where does the 490pt come from?



libs/kotext/KoTableOfContentsGeneratorInfo.cpp
<http://git.reviewboard.kde.org/r/100421/#comment833>

    can there be only one index-source-style? If so KoXml::namedItemNS can be used to get a specified child item


- Thorsten


On Jan. 24, 2011, 1:44 p.m., Lukáš Tvrdý wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100421/
> -----------------------------------------------------------
> 
> (Updated Jan. 24, 2011, 1:44 p.m.)
> 
> 
> Review request for Calligra, Thorsten Zachmann and Casper Boemann.
> 
> 
> Summary
> -------
> 
> This patch implements the loading of the part of the ODF that says how should be the 
> table of content generated (table-of-content-source element with children).
> 
> Full loading is done, generation of the links is solved, navigation of the links works.
> 
> There are still some TODO's marked in the code. 
> I think the development can continue in master as the solid ground is done.
> 
> This bug was closed as invalid through the development of this feature:
> https://bugs.kde.org/show_bug.cgi?id=260542
> 
> Work done by Pavol Korinek and Lukas Tvrdy.
> 
> 
> Diffs
> -----
> 
>   libs/kotext/CMakeLists.txt 115966e 
>   libs/kotext/KoTableOfContentsGeneratorInfo.h PRE-CREATION 
>   libs/kotext/KoTableOfContentsGeneratorInfo.cpp PRE-CREATION 
>   libs/kotext/KoText.h 6bfb176 
>   libs/kotext/opendocument/KoTextLoader.h 3aba119 
>   libs/kotext/opendocument/KoTextLoader.cpp e40d888 
>   libs/kotext/opendocument/KoTextWriter.cpp 6567065 
>   plugins/textshape/ToCGenerator.h 84747f1 
>   plugins/textshape/ToCGenerator.cpp 3aa6a10 
> 
> Diff: http://git.reviewboard.kde.org/r/100421/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Lukáš
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/calligra-devel/attachments/20110124/5649e644/attachment.htm>