Review Request: Added Footnotes/Endnotes in References Tool

Thorsten Zachmann t.zachmann at zagge.de
Thu Sep 8 05:58:19 BST 2011 

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


I have run the cstester with the patch and found some nice improvements e.g. foot/end notes now numbered when they are printed. Unfortunately it also showed some problems.

- missing first paragraph (mostly on page 3)
- footnotes/endnotes starting at 0
- a crash in one document
- footnote position should take the font in the footnote into account

I have uploaded the documents and thumbnails to 

http://www.zagge.de/anchor/endnotes/

there is a info.txt which describes the files and the seen problems.


libs/kotext/KoInlineNote.h
<http://git.reviewboard.kde.org/r/102552/#comment5582>

    Don't add void as parameter here. That is C syntax we don't use in KDE.



libs/kotext/KoInlineNote.h
<http://git.reviewboard.kde.org/r/102552/#comment5583>

    Using a static variable here will not work as the variable will be used for different documents when more then one document is opened. Maybe you can describe a bit for what this variable is needed so that we come up with a solution for this.



libs/kotext/KoInlineNote.cpp
<http://git.reviewboard.kde.org/r/102552/#comment5584>

    Maybe you can remove the count variable and do the renumbering where a new footnote is added to the inline object manager. Then you will be sure it works correctly also when there are more then one document.



libs/kotext/KoInlineNote.cpp
<http://git.reviewboard.kde.org/r/102552/#comment5585>

    please initialize variable with 0. so you can later check if it was set.



libs/kotext/KoInlineNote.cpp
<http://git.reviewboard.kde.org/r/102552/#comment5586>

    If I understand the code here correctly notesConfig can be 0 here in case it is a annotation. You need to make sure that the object is not used in case it is 0.



libs/kotext/KoInlineNote.cpp
<http://git.reviewboard.kde.org/r/102552/#comment5627>

    This highlighting should not be done always. E.g. when a tumbnail of the page is generated or the document is printed there should be no highlighting. Maybe the use of additional formats would be something which can be used here.



libs/kotext/KoInlineNote.cpp
<http://git.reviewboard.kde.org/r/102552/#comment5588>

    Is there a reason to remove this line here. I think it only makes sure that the initial value of the variable is set correctly.



libs/kotext/KoInlineNote.cpp
<http://git.reviewboard.kde.org/r/102552/#comment5587>

    Please initialize the variable with 0.



libs/kotext/KoInlineNote.cpp
<http://git.reviewboard.kde.org/r/102552/#comment5589>

    There should be a space before (



libs/kotext/KoInlineNote.cpp
<http://git.reviewboard.kde.org/r/102552/#comment5590>

    There should be a space after ,



libs/kotext/KoInlineNote.cpp
<http://git.reviewboard.kde.org/r/102552/#comment5592>

    The notes configuration should be saved only once and not with each footnote. Please ping me on IRC if you have a question on how that can be done.



libs/kotext/KoInlineNote.cpp
<http://git.reviewboard.kde.org/r/102552/#comment5591>

    There should be a space after ,



libs/kotext/KoInlineTextObjectManager.h
<http://git.reviewboard.kde.org/r/102552/#comment5594>

    Please rename the function to renumberNotes as that is more clear



libs/kotext/KoInlineTextObjectManager.cpp
<http://git.reviewboard.kde.org/r/102552/#comment5593>

    These two if's can be merged into one.



libs/kotext/KoInlineTextObjectManager.cpp
<http://git.reviewboard.kde.org/r/102552/#comment5595>

    maybe rename i and j to footNoteNumber and endNoteNumber or something similar to make the code more clear.



libs/kotext/KoInlineTextObjectManager.cpp
<http://git.reviewboard.kde.org/r/102552/#comment5596>

    The function says it returns autoNumberedEndNotes but it does not check for autonumbering here.



libs/kotext/KoInlineTextObjectManager.cpp
<http://git.reviewboard.kde.org/r/102552/#comment5597>

    There should be a space before (



libs/kotext/KoTextEditor.h
<http://git.reviewboard.kde.org/r/102552/#comment5599>

    Please change the include to a forward declaration as the header is not needed.



libs/kotext/KoTextEditor.h
<http://git.reviewboard.kde.org/r/102552/#comment5598>

    Please remove this include. No need to include the same file twice.



libs/kotext/KoTextEditor.cpp
<http://git.reviewboard.kde.org/r/102552/#comment5600>

    Please remove the addition space at the end of the line.



libs/odf/KoOdfNumberDefinition.cpp
<http://git.reviewboard.kde.org/r/102552/#comment5605>

    Please move the definition of the variable to the place where it is first used. 



libs/odf/KoOdfNumberDefinition.cpp
<http://git.reviewboard.kde.org/r/102552/#comment5601>

    Please move the definition of the variable to the place where it is first used.



libs/odf/KoOdfNumberDefinition.cpp
<http://git.reviewboard.kde.org/r/102552/#comment5602>

    Please move the definition of the variable to the place where it is first used.



libs/odf/KoOdfNumberDefinition.cpp
<http://git.reviewboard.kde.org/r/102552/#comment5603>

    Please move the definition of the variable to the place where it is first used.
    



libs/odf/KoOdfNumberDefinition.cpp
<http://git.reviewboard.kde.org/r/102552/#comment5604>

    Please move the definition of the variable to the place where it is first used. 



libs/odf/KoOdfNumberDefinition.cpp
<http://git.reviewboard.kde.org/r/102552/#comment5606>

    Please move the definition of the variable to the place where it is first used. 



libs/odf/KoOdfNumberDefinition.cpp
<http://git.reviewboard.kde.org/r/102552/#comment5607>

    The break is not needed



libs/odf/KoOdfStylesReader.cpp
<http://git.reviewboard.kde.org/r/102552/#comment5608>

    Do you want to set the default values only if it was loaded from the document. I think it might make sense to set it also when no configuration was loaded.



libs/textlayout/KoTextLayoutArea.cpp
<http://git.reviewboard.kde.org/r/102552/#comment5609>

    If that code is not needed please remove. If it is still needed please give a comment on why it is commented out at the moment.



libs/textlayout/KoTextLayoutEndNotesArea.cpp
<http://git.reviewboard.kde.org/r/102552/#comment5610>

    What does this magic number 15 do?



libs/textlayout/KoTextLayoutEndNotesArea.cpp
<http://git.reviewboard.kde.org/r/102552/#comment5611>

    Maybe you can somewhere define what all these numbers are for



plugins/textshape/ReferencesTool.h
<http://git.reviewboard.kde.org/r/102552/#comment5612>

    This include is not needed.
    



plugins/textshape/ReferencesTool.h
<http://git.reviewboard.kde.org/r/102552/#comment5613>

    Please change that to a forward declation as the include is not needed here.



plugins/textshape/ReferencesTool.h
<http://git.reviewboard.kde.org/r/102552/#comment5617>

    The variables should be prefixed by m_ 



plugins/textshape/ReferencesTool.cpp
<http://git.reviewboard.kde.org/r/102552/#comment5614>

    In all other places Foot Note is one word.



plugins/textshape/ReferencesTool.cpp
<http://git.reviewboard.kde.org/r/102552/#comment5615>

    In all other places End Note is one word.



plugins/textshape/ReferencesTool.cpp
<http://git.reviewboard.kde.org/r/102552/#comment5616>

    That link does only work on your machine. Please fix ba adding the image to the patch. Maybe with a better name for it.



plugins/textshape/ReferencesTool.cpp
<http://git.reviewboard.kde.org/r/102552/#comment5618>

    Inserting a variable should create an undo/redo command. Is this the case here. This is also true for all other operations changing the document on user input.



plugins/textshape/ReferencesTool.cpp
<http://git.reviewboard.kde.org/r/102552/#comment5619>

    This seems to be a lot like the code in KoInlineNote::loadOdf. Please refactor to reuse the same code to avoid code duplication



plugins/textshape/dialogs/NotesConfigurationDialog.h
<http://git.reviewboard.kde.org/r/102552/#comment5620>

    There should be a space after ,



plugins/textshape/dialogs/NotesConfigurationDialog.h
<http://git.reviewboard.kde.org/r/102552/#comment5621>

    Please remove the void parameter. Should this method be const?



plugins/textshape/dialogs/NotesConfigurationDialog.h
<http://git.reviewboard.kde.org/r/102552/#comment5622>

    there should be no space after ( and before )



plugins/textshape/dialogs/NotesConfigurationDialog.h
<http://git.reviewboard.kde.org/r/102552/#comment5623>

    there should be no space after ( and before )



plugins/textshape/dialogs/NotesConfigurationDialog.cpp
<http://git.reviewboard.kde.org/r/102552/#comment5624>

    If that is not needed please remove



plugins/textshape/dialogs/NotesConfigurationDialog.cpp
<http://git.reviewboard.kde.org/r/102552/#comment5625>

    Please remvoe the commented out code that is not needed


- Thorsten


On Sept. 7, 2011, 5:17 p.m., Brijesh Patel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102552/
> -----------------------------------------------------------
> 
> (Updated Sept. 7, 2011, 5:17 p.m.)
> 
> 
> Review request for Calligra.
> 
> 
> Summary
> -------
> 
> I worked on implementation of Footnotes and Endnotes in Calligra Words as part of my SoK project.
> Footnotes/Endnotes can now be inserted,saved,loaded and also be configured too.
> Can somebody review my work and notify me about bugs/crashes?
> 
> 
> Diffs
> -----
> 
>   libs/kotext/KoInlineNote.h 515a2f3 
>   libs/kotext/KoInlineNote.cpp 89200fd 
>   libs/kotext/KoInlineTextObjectManager.h 44990e9 
>   libs/kotext/KoInlineTextObjectManager.cpp da8d9ca 
>   libs/kotext/KoTextEditor.h ba28354 
>   libs/kotext/KoTextEditor.cpp 4d3e5b18 
>   libs/kotext/opendocument/KoTextLoader.cpp e9d47da 
>   libs/kotext/styles/KoSectionStyle.cpp 2f3a1ad 
>   libs/odf/KoOdfNotesConfiguration.cpp bc26e31 
>   libs/odf/KoOdfNumberDefinition.cpp d35a07f 
>   libs/odf/KoOdfStylesReader.cpp 5a8324e 
>   libs/textlayout/KoTextLayoutArea.h ea7fd8e 
>   libs/textlayout/KoTextLayoutArea.cpp 504de0c 
>   libs/textlayout/KoTextLayoutEndNotesArea.h 7b0c732 
>   libs/textlayout/KoTextLayoutEndNotesArea.cpp bb746db 
>   plugins/textshape/CMakeLists.txt 3df7aa5 
>   plugins/textshape/ReferencesTool.h 40676b2 
>   plugins/textshape/ReferencesTool.cpp 0bbd38d 
>   plugins/textshape/dialogs/NotesConfigurationDialog.h PRE-CREATION 
>   plugins/textshape/dialogs/NotesConfigurationDialog.cpp PRE-CREATION 
>   plugins/textshape/dialogs/NotesConfigurationDialog.ui PRE-CREATION 
>   plugins/textshape/dialogs/SimpleFootEndNotesWidget.h 1af35a8 
>   plugins/textshape/dialogs/SimpleFootEndNotesWidget.cpp 374b266 
>   plugins/textshape/dialogs/SimpleFootEndNotesWidget.ui 77f33e7 
>   plugins/textshape/pics/settings-icon1_1.png PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/102552/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Brijesh
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/calligra-devel/attachments/20110908/9718556a/attachment.htm>

More information about the calligra-devel mailing list