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 > > > 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 > > > 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 > > > Please use KoImageData instead of imagePath. > > > > trunk/koffice/kword/part/KWPageStyle.h > > > 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 > > > 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 > > > 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 > > > this should be 2 methods that return an KoImageData > > > > trunk/koffice/kword/part/KWPageStyle.cpp > > > 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 > > > Please move this out of the header file to the Private class. > > > > trunk/koffice/libs/flake/KoFrameBorder.h > > > Is this needed? > > > > trunk/koffice/libs/flake/KoFrameBorder.cpp > > > Following the coding style; the case and the switch should be on the > same column. > > > > trunk/koffice/libs/flake/KoFrameBorder.cpp > > > Following the coding style; the case and the switch should be on the > same column. > > > > trunk/koffice/libs/flake/KoFrameBorder.cpp > > > Following the coding style; the case and the switch should be on the > same column. > > > > trunk/koffice/libs/flake/KoFrameBorder.cpp > > > Following the coding style; the case and the switch should be on the > same column. > > > > trunk/koffice/libs/flake/KoFrameBorder.cpp > > > Following the coding style; the case and the switch should be on the > same column. > > > > trunk/koffice/libs/flake/KoFrameBorder.cpp > > > Following the coding style; the case and the switch should be on the > same column. > > > > trunk/koffice/libs/flake/KoFrameBorder.cpp > > > Following the coding style; the case and the switch should be on the > same column. > > > > trunk/koffice/libs/flake/KoFrameBorder.cpp > > > Following the coding style; the case and the switch should be on the > same column. > > > > trunk/koffice/libs/flake/KoFrameBorder.cpp > > > Following the coding style; the case and the switch should be on the > same column. > > > > trunk/koffice/libs/flake/KoFrameBorder.cpp > > > Following the coding style; the case and the switch should be on the > same column. > > > > trunk/koffice/libs/flake/KoFrameBorder.cpp > > > setting the color deletes the user set colors, this looks like the > wrong solution. > > > > trunk/koffice/libs/flake/KoFrameBorder.cpp > > > follow coding style guide; you should have a space after the if and > before the brace. > > > > trunk/koffice/libs/flake/KoFrameBorder.cpp > > > follow coding style guide; you should have a space after the if and > before the brace. > > > > trunk/koffice/libs/flake/KoFrameBorder.cpp > > > if there is no line, would be be faster to just return? > > > > trunk/koffice/libs/flake/KoFrameBorder.cpp > > > I'd say this 'FIXME' is rather important ;) > Can you fix it? > > > > trunk/koffice/libs/flake/KoFrameBorder.cpp > > > If the border style is none, I think the insets should be zero. > > > > trunk/koffice/libs/flake/KoFrameBorder.cpp > > > 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 > > > 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 > > > watch for trailing spaces. > > > > trunk/koffice/libs/widgets/KoPageLayout.h > > > 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 > > > 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 > > > 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