> On Jan. 24, 2011, 3:16 p.m., Thorsten Zachmann wrote: > > I have added some stuff I have seen while looking through the patch. I have > > not tested the patch.
I'm going to fix all the issues I did not comment to.. Otherwise I objected somehow. Thank you for the review! > On Jan. 24, 2011, 3:16 p.m., Thorsten Zachmann wrote: > > libs/kotext/KoTableOfContentsGeneratorInfo.cpp, line 112 > > <http://git.reviewboard.kde.org/r/100421/diff/2/?file=7443#file7443line112> > > > > where does the 490pt come from? ...from the experience of the previous developer? Probably found by trial&error. It is default position, if the ODF tag does not tell the position of the tab stop position. Maybe we should compute the new value in the ToCGenerator as the shape->width, but I'm not sure. It is not possible to compute during the loading. > On Jan. 24, 2011, 3:16 p.m., Thorsten Zachmann wrote: > > libs/kotext/KoTableOfContentsGeneratorInfo.h, line 308 > > <http://git.reviewboard.kde.org/r/100421/diff/2/?file=7442#file7442line308> > > > > maybe rename KoTableOfContentsGeneratorInfo to > > KoTableOfContentsOdfGenerator > > > > Can this be moved to be in the same class that also loads the TOC? KoTableOfContentsOdfGenerator sounds like the Generator itself, but this is just a data structure that holds info(rmation) about how the toc should be generated. I don't think the rename is needed here. This class loads the TOC (void loadOdf(..)). It has to be visible to the ToCGenerator which is doing the rendering into QTextDocument and to the loading class which passes the document to it (KoTextLoader) and also to the saving class (KoTextWriter) which reads from it. It differs from the other loading classes in that way that it does not render the QTextDocument when loading the data (only index-body is done that way (a.k.a. snapshot), it loads the information that is used to generate the TOC. > On Jan. 24, 2011, 3:16 p.m., Thorsten Zachmann wrote: > > libs/kotext/KoTableOfContentsGeneratorInfo.h, line 59 > > <http://git.reviewboard.kde.org/r/100421/diff/2/?file=7442#file7442line59> > > > > 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. Ok, I can move the implementation to the cpp file, but I don't think the classes has to be splitted. They are very small and they inherit from one base class. If somebody will want to reuse them, he can do the job, but for now I think it is easier to maintain them in one class. > On Jan. 24, 2011, 3:16 p.m., Thorsten Zachmann wrote: > > libs/kotext/KoTableOfContentsGeneratorInfo.h, line 162 > > <http://git.reviewboard.kde.org/r/100421/diff/2/?file=7442#file7442line162> > > > > what is the 550 about? Bulgarian constant, I will fix it. > On Jan. 24, 2011, 3:16 p.m., Thorsten Zachmann wrote: > > libs/kotext/KoTableOfContentsGeneratorInfo.cpp, line 132 > > <http://git.reviewboard.kde.org/r/100421/diff/2/?file=7443#file7443line132> > > > > can there be only one index-source-style? If so KoXml::namedItemNS can > > be used to get a specified child item Sorry,I don't get it here. Can you rephrase? - Lukáš ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/100421/#review1048 ----------------------------------------------------------- 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áš > >
_______________________________________________ calligra-devel mailing list calligra-devel@kde.org https://mail.kde.org/mailman/listinfo/calligra-devel
