[prev in list] [next in list] [prev in thread] [next in thread]
List: koffice-devel
Subject: Re: Review Request: Class KoFrameBorder for drawing borders for
From: "Thomas Zander" <zander () kde ! org>
Date: 2010-01-09 14:34:49
Message-ID: 20100109143449.7473.67423 () localhost
[Download RAW message or body]
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/2268/#review3610
-----------------------------------------------------------
trunk/koffice/kword/part/KWPageStyle.h
<http://reviewboard.kde.org/r/2268/#comment2926>
Please split to two methods, following the rest of the class having both a header \
and a footer specific function.
trunk/koffice/kword/part/KWPageStyle.h
<http://reviewboard.kde.org/r/2268/#comment2927>
Please split to two methods, following the rest of the class having both a header \
and a footer specific function.
trunk/koffice/kword/part/KWPageStyle.h
<http://reviewboard.kde.org/r/2268/#comment2928>
Please use KoImageData instead of imagePath.
trunk/koffice/kword/part/KWPageStyle.h
<http://reviewboard.kde.org/r/2268/#comment2929>
Please split to two methods, following the rest of the class having both a header \
and a footer specific function.
trunk/koffice/kword/part/KWPageStyle.h
<http://reviewboard.kde.org/r/2268/#comment2930>
Please split to two methods, following the rest of the class having both a header \
and a footer specific function.
trunk/koffice/kword/part/KWPageStyle.h
<http://reviewboard.kde.org/r/2268/#comment2931>
Please split to two methods, following the rest of the class having both a header \
and a footer specific function.
trunk/koffice/kword/part/KWPageStyle.h
<http://reviewboard.kde.org/r/2268/#comment2932>
this should be 2 methods that return an KoImageData
trunk/koffice/kword/part/KWPageStyle.cpp
<http://reviewboard.kde.org/r/2268/#comment2933>
this addition is wrong, please remove. See the API docs for why its set to zero \
in this constructor.
trunk/koffice/libs/flake/KoFrameBorder.h
<http://reviewboard.kde.org/r/2268/#comment2898>
Please move this out of the header file to the Private class.
trunk/koffice/libs/flake/KoFrameBorder.h
<http://reviewboard.kde.org/r/2268/#comment2899>
Is this needed?
trunk/koffice/libs/flake/KoFrameBorder.cpp
<http://reviewboard.kde.org/r/2268/#comment2900>
Following the coding style; the case and the switch should be on the same column.
trunk/koffice/libs/flake/KoFrameBorder.cpp
<http://reviewboard.kde.org/r/2268/#comment2901>
Following the coding style; the case and the switch should be on the same column.
trunk/koffice/libs/flake/KoFrameBorder.cpp
<http://reviewboard.kde.org/r/2268/#comment2902>
Following the coding style; the case and the switch should be on the same column.
trunk/koffice/libs/flake/KoFrameBorder.cpp
<http://reviewboard.kde.org/r/2268/#comment2903>
Following the coding style; the case and the switch should be on the same column.
trunk/koffice/libs/flake/KoFrameBorder.cpp
<http://reviewboard.kde.org/r/2268/#comment2904>
Following the coding style; the case and the switch should be on the same column.
trunk/koffice/libs/flake/KoFrameBorder.cpp
<http://reviewboard.kde.org/r/2268/#comment2905>
Following the coding style; the case and the switch should be on the same column.
trunk/koffice/libs/flake/KoFrameBorder.cpp
<http://reviewboard.kde.org/r/2268/#comment2906>
Following the coding style; the case and the switch should be on the same column.
trunk/koffice/libs/flake/KoFrameBorder.cpp
<http://reviewboard.kde.org/r/2268/#comment2907>
Following the coding style; the case and the switch should be on the same column.
trunk/koffice/libs/flake/KoFrameBorder.cpp
<http://reviewboard.kde.org/r/2268/#comment2908>
Following the coding style; the case and the switch should be on the same column.
trunk/koffice/libs/flake/KoFrameBorder.cpp
<http://reviewboard.kde.org/r/2268/#comment2909>
Following the coding style; the case and the switch should be on the same column.
trunk/koffice/libs/flake/KoFrameBorder.cpp
<http://reviewboard.kde.org/r/2268/#comment2912>
setting the color deletes the user set colors, this looks like the wrong \
solution.
trunk/koffice/libs/flake/KoFrameBorder.cpp
<http://reviewboard.kde.org/r/2268/#comment2910>
follow coding style guide; you should have a space after the if and before the \
brace.
trunk/koffice/libs/flake/KoFrameBorder.cpp
<http://reviewboard.kde.org/r/2268/#comment2911>
follow coding style guide; you should have a space after the if and before the \
brace.
trunk/koffice/libs/flake/KoFrameBorder.cpp
<http://reviewboard.kde.org/r/2268/#comment2913>
if there is no line, would be be faster to just return?
trunk/koffice/libs/flake/KoFrameBorder.cpp
<http://reviewboard.kde.org/r/2268/#comment2914>
I'd say this 'FIXME' is rather important ;)
Can you fix it?
trunk/koffice/libs/flake/KoFrameBorder.cpp
<http://reviewboard.kde.org/r/2268/#comment2916>
If the border style is none, I think the insets should be zero.
trunk/koffice/libs/flake/KoFrameBorder.cpp
<http://reviewboard.kde.org/r/2268/#comment2915>
Is this correct? Isn't a border-line drawn half on one side and half on the \
other side of the line? Then the insets would be half.
trunk/koffice/libs/flake/KoFrameBorder.cpp
<http://reviewboard.kde.org/r/2268/#comment2917>
I think this will return true in valid usecases.
So this detection is incorrect. Maybe use '-1' as a width?
trunk/koffice/libs/odf/KoBorder.h
<http://reviewboard.kde.org/r/2268/#comment2918>
watch for trailing spaces.
trunk/koffice/libs/widgets/KoPageLayout.h
<http://reviewboard.kde.org/r/2268/#comment2923>
Please don't use abbreviations 'num' is short for number, I guess.
Also please don't use a QString but an enum for the format.
trunk/koffice/libs/widgets/KoPageLayout.h
<http://reviewboard.kde.org/r/2268/#comment2924>
KWord already has a direction for each page, why is it needed in the libs? Are \
there other apps that need it? If it needs to be moved then;
a) make it an enum
b) remove it from KWPageStyle.
trunk/koffice/libs/widgets/KoPageLayout.h
<http://reviewboard.kde.org/r/2268/#comment2925>
This looks like a property that really should be kword specific.
- Thomas
On 2009-12-14 12:43:53, Mani Chandrasekar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/2268/
> -----------------------------------------------------------
>
> (Updated 2009-12-14 12:43:53)
>
>
> Review request for KOffice.
>
>
> Summary
> -------
>
> * Created a new class KoFrameBorder inherited from KoShapeBorderModel to handle \
> borders for Frames. This class stores style information using \
> KoBorder.
> * I have moved Paint function from KWCanvas to KoBorder, which can be used by \
> KoFrameBorder. As it is common for both KWCanvas and \
> KoFrameBorder.
> * Header/Footer styles are now supported. Background color/Image fill, Complex \
> borders (color border and border with different width and styles).
> KoFrameBorder::fillStyle(KoGenStyle &style, KoShapeSavingContext &context)
> How do i store the style information of Complex borders, As it is possible to have \
> different colors for different sides.
> Saving back the header/footer information is still not implemented. The \
> header/footer-style should be saved as a child of the style:page-layout; but using \
> the addChildElement its instead saved as a child of style:page-layout-properties. \
> Any idea how this can be implemented ?
>
> Diffs
> -----
>
> trunk/koffice/kword/part/KWCanvas.h 1058441
> trunk/koffice/kword/part/KWCanvas.cpp 1058441
> trunk/koffice/kword/part/KWOdfLoader.cpp 1058441
> trunk/koffice/kword/part/KWPageStyle.h 1058441
> trunk/koffice/kword/part/KWPageStyle.cpp 1058441
> trunk/koffice/kword/part/KWPageStyle_p.h 1058441
> trunk/koffice/kword/part/frames/KWTextFrameSet.h 1058441
> trunk/koffice/kword/part/frames/KWTextFrameSet.cpp 1058441
> trunk/koffice/libs/flake/CMakeLists.txt 1058441
> trunk/koffice/libs/flake/KoFrameBorder.h PRE-CREATION
> trunk/koffice/libs/flake/KoFrameBorder.cpp PRE-CREATION
> trunk/koffice/libs/odf/KoBorder.h 1058441
> trunk/koffice/libs/odf/KoBorder.cpp 1058441
> trunk/koffice/libs/widgets/KoPageLayout.h 1058441
> trunk/koffice/libs/widgets/KoPageLayout.cpp 1058441
>
> Diff: http://reviewboard.kde.org/r/2268/diff
>
>
> Testing
> -------
>
> I have done testing with few documents and works fine.
>
>
> Thanks,
>
> Mani
>
>
_______________________________________________
koffice-devel mailing list
koffice-devel@kde.org
https://mail.kde.org/mailman/listinfo/koffice-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic