Review Request 111787: Page Layout improvements for Words

Sun Aug 4 12:21:45 BST 2013

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

Hi Jeremy,

Great to see things happening in the formatting department! I'd love to see this go in eventually.

Camilla told me that she had already gone over some bugs and the user experience with you so we agreed that I will concentrate on coding issues instead. Here is a list of issues that need to be fixed.  I think you should perhaps take a look at our coding standards so that you don't get drowned in nitpicks that are easily avoidable. Other than that I had some naming issues and one place where I might have not understood (the pixel vs point issue).

libs/widgets/KoPageLayoutDialog.h
<http://git.reviewboard.kde.org/r/111787/#comment27359>

    This is very unusual.  Do you have to access the private class of KoPageLayoutDialog directly?  Can't you use the public API?

words/part/dialogs/KWPageSettingsDialog.h
<http://git.reviewboard.kde.org/r/111787/#comment27360>

    We like to have some whitespace to separate the sections.

words/part/dialogs/KWPageSettingsDialog.cpp
<http://git.reviewboard.kde.org/r/111787/#comment27361>

    Our coding standard says that functions start with a brace in column 1 below the signature.

words/part/dialogs/KWPageSettingsDialog.cpp
<http://git.reviewboard.kde.org/r/111787/#comment27362>

    Why add code that is commented out from the beginning?

words/part/dialogs/KWPageSettingsDialog.cpp
<http://git.reviewboard.kde.org/r/111787/#comment27363>

    An empty line is enough as a separator.

words/part/pagetool/KWPageTool.h
<http://git.reviewboard.kde.org/r/111787/#comment27364>

    Enum values should have one value per line, and preferably also a short comment what it represents.

words/part/pagetool/KWPageTool.h
<http://git.reviewboard.kde.org/r/111787/#comment27365>

    Wouldn't a more useful method have (x, y) parameters and you can call this with any input method: mouse, touch, ...?

words/part/pagetool/KWPageTool.h
<http://git.reviewboard.kde.org/r/111787/#comment27366>

    I may be misunderstanding here, but if this about the margin in the document, you should never use pixels for that.  All document coordinates are in real world lengths, and the Calligra standard is to measure it in points (pt).

words/part/pagetool/KWPageTool.h
<http://git.reviewboard.kde.org/r/111787/#comment27367>

    typo?

words/part/pagetool/KWPageTool.h
<http://git.reviewboard.kde.org/r/111787/#comment27368>

    Normally we would call this m_viewHeaderAction since it's actually an action and not a header.  Same with the footer below. 

words/part/pagetool/KWPageTool.cpp
<http://git.reviewboard.kde.org/r/111787/#comment27369>

    why qt includes in several different places?

words/part/pagetool/KWPageTool.cpp
<http://git.reviewboard.kde.org/r/111787/#comment27370>

    coding standard: function start braces in column 1

words/part/pagetool/KWPageTool.cpp
<http://git.reviewboard.kde.org/r/111787/#comment27371>

    coding standards: space after keywords like if, while, for, ...  I will only mention it here but there are many places.

words/part/pagetool/KWPageTool.cpp
<http://git.reviewboard.kde.org/r/111787/#comment27372>

    name typo?

words/part/pagetool/KWPageTool.cpp
<http://git.reviewboard.kde.org/r/111787/#comment27373>

    is "style" the name of the style?  If so, I think it would be clearer if the parameter was called "styleName".  Otherwise you'd think it was a serialization of the style, i.e. the actual style contents.

words/part/pagetool/KWPageTool.cpp
<http://git.reviewboard.kde.org/r/111787/#comment27374>

    Should be "Page style" I think.

words/part/pagetool/SimplePageStyleWidget.h
<http://git.reviewboard.kde.org/r/111787/#comment27375>

    Isn't this your code?  I think you should claim copyright for your own work :)

    Of course if you copied code from another file and not just the header, then you should also keep boemann's copyright notice.

words/part/pagetool/SimplePageStyleWidget.h
<http://git.reviewboard.kde.org/r/111787/#comment27376>

    I don't think you need an empty line between every method.  Just group them in a good way and separate the groups by an empty line.

words/part/pagetool/SimplePageStyleWidget.cpp
<http://git.reviewboard.kde.org/r/111787/#comment27377>

    same as above.

words/part/pagetool/SimplePageStyleWidget.cpp
<http://git.reviewboard.kde.org/r/111787/#comment27378>

    coding standard:

    type Class::Class(parameters)
        : parentclass(params)
        , member1(...)
        , member2(...)
    {
        method code
    }

words/part/pagetool/SimplePageStyleWidget.cpp
<http://git.reviewboard.kde.org/r/111787/#comment27380>

    Why not initialize these in the initialization list above?

words/part/pagetool/SimplePageStyleWidget.cpp
<http://git.reviewboard.kde.org/r/111787/#comment27379>

    "Information" is not countable so the English language doesn't have the word "Informations". :)  I think updateData() is a better name.

- Inge Wallin

On July 29, 2013, 8:07 p.m., Jérémy BOURDIOL wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111787/
> -----------------------------------------------------------
> 
> (Updated July 29, 2013, 8:07 p.m.)
> 
> 
> Review request for Calligra.
> 
> 
> Description
> -------
> 
> Huge feature on page layout started a moment ago.
> 
> PageLayout improvement now allows users to :
> - resize margins directly with the mouse (still in debug)
> - resize page borders directly with the mouse (still in debug)
> - switch from portrait to landscape (and vise-versa) with one movement (vertical drag)
> - add and remove header/footer with a simple double-click in the corresponding half of the page
> - all changes could be see in the page layout dialog (in "page layout" section)
> 
> The feature has still some problems (most of them graphical) in extrem uses, please checkout the branch and give your feelings.
> 
> 
> Diffs
> -----
> 
>   libs/widgets/KoPageLayoutDialog.h 7b0d029 
>   libs/widgets/KoPageLayoutWidget.cpp fd3667b 
>   words/part/CMakeLists.txt 4e2650c 
>   words/part/KWView.h 517cbc1 
>   words/part/KWView.cpp 48265fd 
>   words/part/author/CAuView.cpp 0c21610 
>   words/part/dialogs/KWPageSettingsDialog.h 30a25d3 
>   words/part/dialogs/KWPageSettingsDialog.cpp e046ac6 
>   words/part/pagetool/KWPageTool.h 45ad7d5 
>   words/part/pagetool/KWPageTool.cpp 7935390 
>   words/part/pagetool/SimpleHeaderFooterWidget.h 981d917 
>   words/part/pagetool/SimpleHeaderFooterWidget.cpp 75b4409 
>   words/part/pagetool/SimplePageStyleWidget.h PRE-CREATION 
>   words/part/pagetool/SimplePageStyleWidget.cpp PRE-CREATION 
>   words/part/pagetool/SimplePageStyleWidget.ui PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/111787/diff/
> 
> 
> Testing
> -------
> 
> A lot of manual tests.
> 
> known bug :
> When resizing the page and the cursor is passing over the previous/next page a double textarea appears
> 
> 
> Thanks,
> 
> Jérémy BOURDIOL
> 
>

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