'Re: Review Request: Add ability to read,'

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       koffice-devel
Subject:    Re: Review Request: Add ability to read,
From:       "Inge Wallin" <inge () lysator ! liu ! se>
Date:       2009-10-19 9:55:21
Message-ID: 20091019095521.28086.86588 () localhost
[Download RAW message or body]

> On 2009-10-19 08:43:27, Thomas Zander wrote:
> > trunk/koffice/libs/odf/KoBorder.cpp, line 334
> > <http://reviewboard.kde.org/r/1890/diff/2/?file=12940#file12940line334>
> > 
> > The 'fixme' is funny above, you worry about iterating over the same string 3 \
> > times and then parse the string 4 times using KoUnit::parseValue() ;)

This is actually copied from your own code that handles paragraph borders.  :-)

> On 2009-10-19 08:43:27, Thomas Zander wrote:
> > trunk/koffice/libs/odf/KoBorder.h, line 71
> > <http://reviewboard.kde.org/r/1890/diff/2/?file=12939#file12939line71>
> > 
> > clone is not really Qt API; please create a copy constructor and assignment \
> > method instead.

fixed

> On 2009-10-19 08:43:27, Thomas Zander wrote:
> > trunk/koffice/libs/odf/KoBorder.h, line 122
> > <http://reviewboard.kde.org/r/1890/diff/2/?file=12939#file12939line122>
> > 
> > s/form/from

fixed

> On 2009-10-19 08:43:27, Thomas Zander wrote:
> > trunk/koffice/libs/odf/KoBorder.h, line 124
> > <http://reviewboard.kde.org/r/1890/diff/2/?file=12939#file12939line124>
> > 
> > param context refers to a not existing parameter

fixed

> On 2009-10-19 08:43:27, Thomas Zander wrote:
> > trunk/koffice/libs/odf/KoBorder.h, line 125
> > <http://reviewboard.kde.org/r/1890/diff/2/?file=12939#file12939line125>
> > 
> > param element probably refers to 'style'

fixed

> On 2009-10-19 08:43:27, Thomas Zander wrote:
> > trunk/koffice/libs/odf/KoBorder.cpp, line 66
> > <http://reviewboard.kde.org/r/1890/diff/2/?file=12940#file12940line66>
> > 
> > why is it not ODF compatible? The enum in the header should be clearer on that.
> > What does it mean to have parsing of this property when its not odf compatible?

fixed

> On 2009-10-19 08:43:27, Thomas Zander wrote:
> > trunk/koffice/libs/odf/KoBorder.cpp, line 79
> > <http://reviewboard.kde.org/r/1890/diff/2/?file=12940#file12940line79>
> > 
> > the const is irrelevant please remove.

fixed

> On 2009-10-19 08:43:27, Thomas Zander wrote:
> > trunk/koffice/libs/odf/KoBorder.cpp, line 353
> > <http://reviewboard.kde.org/r/1890/diff/2/?file=12940#file12940line353>
> > 
> > I'm wondering if that 1.0 default should not be 0.0 instead... Does ODF say \
> > anything there?  If there is no border specified, I expect it to not be there \
> > (css does that IIRC).

I'm not sure, I don't think that odf says anything about it.

> On 2009-10-19 08:43:27, Thomas Zander wrote:
> > trunk/koffice/libs/odf/KoBorder.cpp, line 447
> > <http://reviewboard.kde.org/r/1890/diff/2/?file=12940#file12940line447>
> > 
> > Please remove the extra braces around the whole right side of the argument.

There is no brace.  The parenthesis is to make emacs autoindent correctly.

> On 2009-10-19 08:43:27, Thomas Zander wrote:
> > trunk/koffice/libs/odf/KoBorder.cpp, line 469
> > <http://reviewboard.kde.org/r/1890/diff/2/?file=12940#file12940line469>
> > 
> > Please move the curly brace to be on the same line as the if()

I can't find anything like this.  Are you sure it's not just a linebreak issue in \
your browser?

> On 2009-10-19 08:43:27, Thomas Zander wrote:
> > trunk/koffice/libs/odf/KoBorder.h, line 134
> > <http://reviewboard.kde.org/r/1890/diff/2/?file=12939#file12939line134>
> > 
> > As this class is exported it can not have any private members that might change \
> > one day. Please use a private class and I think you want that private class to \
> > inherit QSharedData.

This was actually meant to be a struct to begin with.  Any thoughts about the \
class-with-d-pointer vs. struct question?

- Inge

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

On 2009-10-19 06:46:03, Inge Wallin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1890/
> -----------------------------------------------------------
> 
> (Updated 2009-10-19 06:46:03)
> 
> 
> Review request for KOffice.
> 
> 
> Summary
> -------
> 
> This patch adds the ability to read, store and save page borders in text documents.
> 
> So far it does not paint the borders, because I couldn't understand the paint code \
> of KWord, but I hope to get some help with that today. 
> 
> Diffs
> -----
> 
> trunk/koffice/libs/odf/CMakeLists.txt 1037437 
> trunk/koffice/libs/odf/KoBorder.h PRE-CREATION 
> trunk/koffice/libs/odf/KoBorder.cpp PRE-CREATION 
> trunk/koffice/libs/widgets/KoPageLayout.h 1037437 
> trunk/koffice/libs/widgets/KoPageLayout.cpp 1037437 
> 
> Diff: http://reviewboard.kde.org/r/1890/diff
> 
> 
> Testing
> -------
> 
> I have tested with loading and saving a document that contains page borders created \
> in OOo. 
> 
> Thanks,
> 
> Inge
> 
> 

_______________________________________________
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] 