[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-29 14:14:41
Message-ID: 201001291514.42026.zander () kde ! org
[Download RAW message or body]
Seems this patch hasn't been updated since the latest review; Mani?
On Saturday 9. January 2010 15.34.49 Thomas Zander wrote:
> -----------------------------------------------------------
> 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
>
--
Thomas Zander
_______________________________________________
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