[prev in list] [next in list] [prev in thread] [next in thread]
List: koffice-devel
Subject: Re: Review Request: bug fix: "Picture inserted as background fill not
From: "Thomas Zander" <zander () kde ! org>
Date: 2010-04-01 12:48:00
Message-ID: 20100401124800.12728.44747 () localhost
[Download RAW message or body]
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/3444/#review4837
-----------------------------------------------------------
Cool new feature, thanks!
Some comments about the patch follow;
trunk/koffice/kword/part/KWPageStyle.cpp
<http://reviewboard.kde.org/r/3444/#comment4318>
propBkgImg is a bit too terse, please use longer variable names like \
propBackgroundImage
trunk/koffice/kword/part/KWPageStyle.cpp
<http://reviewboard.kde.org/r/3444/#comment4319>
The method loadOdfPageBackgroundImage() on a KoPatternBackground is a bit odd to \
add to public API, we already have two loading methods there.
As there is only one place where this code is called from I suggest to not add \
this new method on KoPatternBackground at all but instead move the contents of that \
method to here, inside the KWPageStyle loading code.
trunk/koffice/kword/part/KWPageStyle.cpp
<http://reviewboard.kde.org/r/3444/#comment4324>
Re-using the d->background sounds wrong.
As far as I can see we can have
a) a full-page background (i.e. a red page)
b) a background for the header frames.
c) a background for the footer frames.
So I think we need 3 variables, and not reuse the one we have now.
trunk/koffice/kword/part/KWPageStyle.cpp
<http://reviewboard.kde.org/r/3444/#comment4320>
Why did you add this check? This makes the usecase (3 lines lower) of a \
transparent color not work anymore.
Also, please don't use NULL, thats a C not a C++ keyword. In C++ we just use 0
trunk/koffice/libs/flake/KoPatternBackground.h
<http://reviewboard.kde.org/r/3444/#comment4321>
As mentioned above, the addition of this method can be avoided, so lets not add \
it but move the contents to KWord
trunk/koffice/libs/flake/KoPatternBackground.cpp
<http://reviewboard.kde.org/r/3444/#comment4322>
this comment has typos :)
'possible'
'attributes'
- Thomas
On 2010-03-30 07:18:37, Matus Hanzes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3444/
> -----------------------------------------------------------
>
> (Updated 2010-03-30 07:18:37)
>
>
> Review request for KOffice.
>
>
> Summary
> -------
>
> This fix adds support for page background pictures in kword
>
> loadOdf function changed to loadOdfPageBackgroundImage.
>
> Comments are welcome.
>
>
> Diffs
> -----
>
> trunk/koffice/kword/part/KWOdfLoader.cpp 1108939
> trunk/koffice/kword/part/KWPageStyle.h 1108939
> trunk/koffice/kword/part/KWPageStyle.cpp 1108939
> trunk/koffice/libs/flake/KoPatternBackground.h 1108939
> trunk/koffice/libs/flake/KoPatternBackground.cpp 1108939
>
> Diff: http://reviewboard.kde.org/r/3444/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Matus
>
>
_______________________________________________
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