Review Request: Add ability to read, store and save page borders in text documents.

[Inge Wallin-3]

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

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@...
https://mail.kde.org/mailman/listinfo/koffice-devel

[Inge Wallin-3]

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

(Updated 2009-10-19 06:46:03.990152)


Review request for KOffice.


Changes
-------

Fixed a FIXME


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 (updated)
-----

  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@...
https://mail.kde.org/mailman/listinfo/koffice-devel

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



trunk/koffice/libs/odf/KoBorder.h
<http://reviewboard.kde.org/r/1890/#comment1990>

    clone is not really Qt API; please create a copy constructor and assignment method instead.



trunk/koffice/libs/odf/KoBorder.h
<http://reviewboard.kde.org/r/1890/#comment1991>

    s/form/from



trunk/koffice/libs/odf/KoBorder.h
<http://reviewboard.kde.org/r/1890/#comment1992>

    param context refers to a not existing parameter



trunk/koffice/libs/odf/KoBorder.h
<http://reviewboard.kde.org/r/1890/#comment1993>

    param element probably refers to 'style'



trunk/koffice/libs/odf/KoBorder.h
<http://reviewboard.kde.org/r/1890/#comment1994>

    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.



trunk/koffice/libs/odf/KoBorder.cpp
<http://reviewboard.kde.org/r/1890/#comment1995>

    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?



trunk/koffice/libs/odf/KoBorder.cpp
<http://reviewboard.kde.org/r/1890/#comment1996>

    the const is irrelevant please remove.



trunk/koffice/libs/odf/KoBorder.cpp
<http://reviewboard.kde.org/r/1890/#comment1997>

    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() ;)



trunk/koffice/libs/odf/KoBorder.cpp
<http://reviewboard.kde.org/r/1890/#comment1998>

    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).



trunk/koffice/libs/odf/KoBorder.cpp
<http://reviewboard.kde.org/r/1890/#comment1999>

    Please remove the extra braces around the whole right side of the argument.



trunk/koffice/libs/odf/KoBorder.cpp
<http://reviewboard.kde.org/r/1890/#comment2000>

    Please move the curly brace to be on the same line as the if()



trunk/koffice/libs/odf/KoBorder.cpp
<http://reviewboard.kde.org/r/1890/#comment2001>

    Please move the curly brace to be at the same line as the if()


- Thomas


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@...
https://mail.kde.org/mailman/listinfo/koffice-devel

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



trunk/koffice/libs/odf/KoBorder.h
<http://reviewboard.kde.org/r/1890/#comment2002>

    trailing whitespace



trunk/koffice/libs/odf/KoBorder.h
<http://reviewboard.kde.org/r/1890/#comment2003>

    trailing whitespace


- Thomas


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@...
https://mail.kde.org/mailman/listinfo/koffice-devel

[Inge Wallin-3]

> 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@...
https://mail.kde.org/mailman/listinfo/koffice-devel

[Inge Wallin-3]

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

(Updated 2009-10-20 07:29:43.694683)


Review request for KOffice.


Changes
-------

New version of the diff which uses QSharedData


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 (updated)
-----

  trunk/koffice/libs/widgets/KoPageLayout.h 1037806
  trunk/koffice/libs/widgets/KoPageLayout.cpp 1037806
  trunk/koffice/libs/odf/KoBorder.cpp PRE-CREATION
  trunk/koffice/libs/odf/CMakeLists.txt 1037806
  trunk/koffice/libs/odf/KoBorder.h PRE-CREATION

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@...
https://mail.kde.org/mailman/listinfo/koffice-devel

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



trunk/koffice/libs/odf/KoBorder.cpp
<http://reviewboard.kde.org/r/1890/#comment2037>

    trailing space



trunk/koffice/libs/odf/KoBorder.cpp
<http://reviewboard.kde.org/r/1890/#comment2028>

    this constructor is not needed



trunk/koffice/libs/odf/KoBorder.cpp
<http://reviewboard.kde.org/r/1890/#comment2029>

    just copy the d, not create a new one and a line later replace it.



trunk/koffice/libs/odf/KoBorder.cpp
<http://reviewboard.kde.org/r/1890/#comment2030>

    please add a check if the d pointer is identical too.



trunk/koffice/libs/odf/KoBorder.cpp
<http://reviewboard.kde.org/r/1890/#comment2031>

    I don't think this solution scales very well, if ODF doesn't support it we have to either;
    a) choose to support it and push it into ODF
    b) choose to not support it and just store it as an anonymous feature we just read and write out.
    c) ignore it.
   
    Adding a lot of styles and hinting we want to add more for MSDoc is not something that scales.



trunk/koffice/libs/odf/KoBorder.cpp
<http://reviewboard.kde.org/r/1890/#comment2032>

    Please don't parse each item 4 times, store it in an intermediate variable.



trunk/koffice/libs/odf/KoBorder.cpp
<http://reviewboard.kde.org/r/1890/#comment2033>

    Please place the curly brace one line up.



trunk/koffice/libs/odf/KoBorder.cpp
<http://reviewboard.kde.org/r/1890/#comment2034>

    Please place the curly brace one line up.



trunk/koffice/libs/widgets/KoPageLayout.h
<http://reviewboard.kde.org/r/1890/#comment2035>

    Why did you write this? How is the operator= different from the auto generated one for a struct?



trunk/koffice/libs/widgets/KoPageLayout.cpp
<http://reviewboard.kde.org/r/1890/#comment2036>

    trailing space


- Thomas


On 2009-10-20 07:29:43, Inge Wallin wrote:

>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1890/
> -----------------------------------------------------------
>
> (Updated 2009-10-20 07:29:43)
>
>
> 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/widgets/KoPageLayout.h 1037806
>   trunk/koffice/libs/widgets/KoPageLayout.cpp 1037806
>   trunk/koffice/libs/odf/KoBorder.cpp PRE-CREATION
>   trunk/koffice/libs/odf/CMakeLists.txt 1037806
>   trunk/koffice/libs/odf/KoBorder.h PRE-CREATION
>
> 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@...
https://mail.kde.org/mailman/listinfo/koffice-devel

[Inge Wallin-3]

> On 2009-10-19 08:44:39, Thomas Zander wrote:
> > trunk/koffice/libs/odf/KoBorder.h, line 41
> > <http://reviewboard.kde.org/r/1890/diff/2/?file=12939#file12939line41>
> >
> >     trailing whitespace

Fixed


> On 2009-10-19 08:44:39, Thomas Zander wrote:
> > trunk/koffice/libs/odf/KoBorder.h, line 46
> > <http://reviewboard.kde.org/r/1890/diff/2/?file=12939#file12939line46>
> >
> >     trailing whitespace

fixed


- Inge


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


On 2009-10-20 07:29:43, Inge Wallin wrote:

>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1890/
> -----------------------------------------------------------
>
> (Updated 2009-10-20 07:29:43)
>
>
> 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/widgets/KoPageLayout.h 1037806
>   trunk/koffice/libs/widgets/KoPageLayout.cpp 1037806
>   trunk/koffice/libs/odf/KoBorder.cpp PRE-CREATION
>   trunk/koffice/libs/odf/CMakeLists.txt 1037806
>   trunk/koffice/libs/odf/KoBorder.h PRE-CREATION
>
> 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@...
https://mail.kde.org/mailman/listinfo/koffice-devel

[Inge Wallin-3]

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

(Updated 2009-10-20 12:49:31.980312)


Review request for KOffice.


Changes
-------

New version of the diff.


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 (updated)
-----

  trunk/koffice/libs/widgets/KoPageLayout.h 1037806
  trunk/koffice/libs/widgets/KoPageLayout.cpp 1037806
  trunk/koffice/libs/odf/KoBorder.cpp PRE-CREATION
  trunk/koffice/libs/odf/CMakeLists.txt 1037806
  trunk/koffice/libs/odf/KoBorder.h PRE-CREATION

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@...
https://mail.kde.org/mailman/listinfo/koffice-devel

[Inge Wallin-3]

> On 2009-10-20 09:36:18, Thomas Zander wrote:
> > trunk/koffice/libs/odf/KoBorder.cpp, line 37
> > <http://reviewboard.kde.org/r/1890/diff/3/?file=13030#file13030line37>
> >
> >     this constructor is not needed

It is, because when I didn't have it I got a compilation error in some other file.  Try it.


> On 2009-10-20 09:36:18, Thomas Zander wrote:
> > trunk/koffice/libs/odf/KoBorder.cpp, line 77
> > <http://reviewboard.kde.org/r/1890/diff/3/?file=13030#file13030line77>
> >
> >     just copy the d, not create a new one and a line later replace it.

Of course.  duh!


> On 2009-10-20 09:36:18, Thomas Zander wrote:
> > trunk/koffice/libs/odf/KoBorder.cpp, line 513
> > <http://reviewboard.kde.org/r/1890/diff/3/?file=13030#file13030line513>
> >
> >     Please don't parse each item 4 times, store it in an intermediate variable.

Copied from your own code, but I fixed it here.  I suggest you do the same in ParagraphStyle :-)


> On 2009-10-20 09:36:18, Thomas Zander wrote:
> > trunk/koffice/libs/odf/KoBorder.cpp, line 694
> > <http://reviewboard.kde.org/r/1890/diff/3/?file=13030#file13030line694>
> >
> >     Please place the curly brace one line up.

That will be more difficult to read, but ok


> On 2009-10-20 09:36:18, Thomas Zander wrote:
> > trunk/koffice/libs/widgets/KoPageLayout.h, line 62
> > <http://reviewboard.kde.org/r/1890/diff/3/?file=13031#file13031line62>
> >
> >     Why did you write this? How is the operator= different from the auto generated one for a struct?

Good question, but without it it doesn't compile.


> On 2009-10-20 09:36:18, Thomas Zander wrote:
> > trunk/koffice/libs/widgets/KoPageLayout.cpp, line 44
> > <http://reviewboard.kde.org/r/1890/diff/3/?file=13032#file13032line44>
> >
> >     trailing space

Aren't we getting into the too-picky state here?

> On 2009-10-20 09:36:18, Thomas Zander wrote:
> > trunk/koffice/libs/odf/KoBorder.cpp, line 227
> > <http://reviewboard.kde.org/r/1890/diff/3/?file=13030#file13030line227>
> >
> >     I don't think this solution scales very well, if ODF doesn't support it we have to either;
> >     a) choose to support it and push it into ODF
> >     b) choose to not support it and just store it as an anonymous feature we just read and write out.
> >     c) ignore it.
> >    
> >     Adding a lot of styles and hinting we want to add more for MSDoc is not something that scales.

This code is copied from libs/kotext/styles/KoParagraphStyle.h (with your name on those lines) and looks exactly the same there.  Let's find a common solution and implement it everywhere.   For that matter, it would be nice to have borders handled only in one place.


- Inge


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


On 2009-10-20 12:49:31, Inge Wallin wrote:

>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1890/
> -----------------------------------------------------------
>
> (Updated 2009-10-20 12:49:31)
>
>
> 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/widgets/KoPageLayout.h 1037806
>   trunk/koffice/libs/widgets/KoPageLayout.cpp 1037806
>   trunk/koffice/libs/odf/KoBorder.cpp PRE-CREATION
>   trunk/koffice/libs/odf/CMakeLists.txt 1037806
>   trunk/koffice/libs/odf/KoBorder.h PRE-CREATION
>
> 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@...
https://mail.kde.org/mailman/listinfo/koffice-devel

[Inge Wallin-3]

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

(Updated 2009-10-20 14:03:52.092823)


Review request for KOffice.


Changes
-------

Removed an unnecessary copy constructor.  Strange, I added it because there was a compile error.  Probably some other problem that was the real issue.


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 (updated)
-----

  trunk/koffice/libs/odf/CMakeLists.txt 1037806
  trunk/koffice/libs/odf/KoBorder.h PRE-CREATION
  trunk/koffice/libs/odf/KoBorder.cpp PRE-CREATION
  trunk/koffice/libs/widgets/KoPageLayout.h 1037806
  trunk/koffice/libs/widgets/KoPageLayout.cpp 1037806

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@...
https://mail.kde.org/mailman/listinfo/koffice-devel

[Inge Wallin-3]

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

(Updated 2009-10-20 16:54:49.538619)


Review request for KOffice.


Changes
-------

New version.  This time it removes two functions from KoParagraphStyle and moves users of KoParagraphStyle::BorderStyle to KoBorder::BorderStyle (which is the same).  This makes the patch a little bigger than before.


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 (updated)
-----

  trunk/koffice/libs/kotext/CMakeLists.txt 1037806
  trunk/koffice/libs/kotext/KoTextBlockBorderData.cpp 1037806
  trunk/koffice/libs/kotext/styles/KoParagraphStyle.h 1037806
  trunk/koffice/libs/kotext/styles/KoParagraphStyle.cpp 1037806
  trunk/koffice/libs/odf/CMakeLists.txt 1037806
  trunk/koffice/libs/odf/KoBorder.h PRE-CREATION
  trunk/koffice/libs/odf/KoBorder.cpp PRE-CREATION
  trunk/koffice/libs/widgets/KoPageLayout.h 1037806
  trunk/koffice/libs/widgets/KoPageLayout.cpp 1037806
  trunk/koffice/plugins/textshape/tests/TestDocumentLayout.cpp 1037806

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@...
https://mail.kde.org/mailman/listinfo/koffice-devel

[Jaroslaw Staniek-3]

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


Optimization: I propose to replace each QString("....") with QLatin1String("....").


- Jaroslaw


On 2009-10-20 16:54:49, Inge Wallin wrote:

>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1890/
> -----------------------------------------------------------
>
> (Updated 2009-10-20 16:54:49)
>
>
> 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/kotext/CMakeLists.txt 1037806
>   trunk/koffice/libs/kotext/KoTextBlockBorderData.cpp 1037806
>   trunk/koffice/libs/kotext/styles/KoParagraphStyle.h 1037806
>   trunk/koffice/libs/kotext/styles/KoParagraphStyle.cpp 1037806
>   trunk/koffice/libs/odf/CMakeLists.txt 1037806
>   trunk/koffice/libs/odf/KoBorder.h PRE-CREATION
>   trunk/koffice/libs/odf/KoBorder.cpp PRE-CREATION
>   trunk/koffice/libs/widgets/KoPageLayout.h 1037806
>   trunk/koffice/libs/widgets/KoPageLayout.cpp 1037806
>   trunk/koffice/plugins/textshape/tests/TestDocumentLayout.cpp 1037806
>
> 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@...
https://mail.kde.org/mailman/listinfo/koffice-devel

On Tuesday 20. October 2009 15.12.12 Inge Wallin wrote:

> > On 2009-10-20 09:36:18, Thomas Zander wrote:
> > > trunk/koffice/libs/odf/KoBorder.cpp, line 513
> > > <http://reviewboard.kde.org/r/1890/diff/3/?file=13030#file13030line51
> > >3>
> > >
> > >     Please don't parse each item 4 times, store it in an intermediate
> > > variable.
>
> Copied from your own code, but I fixed it here.  I suggest you do the
> same in ParagraphStyle :-)

Your patch 6 doesn't show this fix.

[]
>> >     Please place the curly brace one line up.
>
>That will be more difficult to read, but ok

This doesn't show in your latest patch.

>> On 2009-10-20 09:36:18, Thomas Zander wrote:
>> > trunk/koffice/libs/widgets/KoPageLayout.h, line 62
>> >
<http://reviewboard.kde.org/r/1890/diff/3/?file=13031#file13031line62>
>> >
>> >     Why did you write this? How is the operator= different from the
>> > auto generated one for a struct?
>
>Good question, but without it it doesn't compile.

That doesn't make it right  :)
Better look for the real reason. [looks] Ah, you forgot a const in the copy
constructor of KoBorder.

--
Thomas Zander
_______________________________________________
koffice-devel mailing list
koffice-devel@...
https://mail.kde.org/mailman/listinfo/koffice-devel

[Inge Wallin-3]

On Tuesday 20 October 2009 22:08:58 Thomas Zander wrote:

> On Tuesday 20. October 2009 15.12.12 Inge Wallin wrote:
> > > On 2009-10-20 09:36:18, Thomas Zander wrote:
> > > > trunk/koffice/libs/odf/KoBorder.cpp, line 513
> > > > <http://reviewboard.kde.org/r/1890/diff/3/?file=13030#file13030line51
> > > >3>
> > > >
> > > >     Please don't parse each item 4 times, store it in an intermediate
> > > > variable.
> >
> > Copied from your own code, but I fixed it here.  I suggest you do the
> > same in ParagraphStyle :-)
>
> Your patch 6 doesn't show this fix.

Yes it does.  The code now uses QString::split instead of parsing.  Can you
find a spot where it doesn't, then I will change that too, but everywhere I
looked it's fixed.

> >> >     Please place the curly brace one line up.
> >
> >That will be more difficult to read, but ok
>
> This doesn't show in your latest patch.

Well, it did, but there was another place where the same happened.  Now fixed
in both places.
 

> >> On 2009-10-20 09:36:18, Thomas Zander wrote:
> >> > trunk/koffice/libs/widgets/KoPageLayout.h, line 62
>
> <http://reviewboard.kde.org/r/1890/diff/3/?file=13031#file13031line62>
>
> >> >     Why did you write this? How is the operator= different from the
> >> > auto generated one for a struct?
> >
> >Good question, but without it it doesn't compile.
>
> That doesn't make it right  :)
> Better look for the real reason. [looks] Ah, you forgot a const in the copy
> constructor of KoBorder.

unbelievable!  well, fixed now.
_______________________________________________
koffice-devel mailing list
koffice-devel@...
https://mail.kde.org/mailman/listinfo/koffice-devel

[Inge Wallin-3]

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

(Updated 2009-10-21 00:47:01.524106)


Review request for KOffice.


Changes
-------

Hopefully final version


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 (updated)
-----

  trunk/koffice/libs/kotext/CMakeLists.txt 1037806
  trunk/koffice/libs/kotext/KoTextBlockBorderData.cpp 1037806
  trunk/koffice/libs/kotext/styles/KoParagraphStyle.h 1037806
  trunk/koffice/libs/kotext/styles/KoParagraphStyle.cpp 1037806
  trunk/koffice/libs/odf/CMakeLists.txt 1037806
  trunk/koffice/libs/odf/KoBorder.h PRE-CREATION
  trunk/koffice/libs/odf/KoBorder.cpp PRE-CREATION
  trunk/koffice/libs/widgets/KoPageLayout.h 1037806
  trunk/koffice/libs/widgets/KoPageLayout.cpp 1037806
  trunk/koffice/plugins/textshape/tests/TestDocumentLayout.cpp 1037806

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@...
https://mail.kde.org/mailman/listinfo/koffice-devel

[Cyrille Berger]

On Wednesday 21 October 2009, Inge Wallin wrote:
> 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.
The painting would definitely qualify as new feature. And would have to wait
for reopening of trunk. So make sure to have seperate patches :)

--
Cyrille Berger
_______________________________________________
koffice-devel mailing list
koffice-devel@...
https://mail.kde.org/mailman/listinfo/koffice-devel

On Wednesday 21. October 2009 02.47.36 Inge Wallin wrote:

> > > > >     Please don't parse each item 4 times, store it in an
> > > > > intermediate variable.
> > >
> > > Copied from your own code, but I fixed it here.  I suggest you do the
> > > same in ParagraphStyle :-)
> >
> > Your patch 6 doesn't show this fix.
>
> Yes it does.  The code now uses QString::split instead of parsing.  Can
> you find a spot where it doesn't, then I will change that too, but
> everywhere I looked it's fixed.

Hehe, confusion :)
I don't care too much about the splitting of the string, what I tried to
address here is that each string part is parsed multiple times using the
same call (KoUnit::parseSomething()), which is unneeded, once is enough.

--
Thomas Zander
_______________________________________________
koffice-devel mailing list
koffice-devel@...
https://mail.kde.org/mailman/listinfo/koffice-devel

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



trunk/koffice/libs/kotext/CMakeLists.txt
<http://reviewboard.kde.org/r/1890/#comment2054>

    These two changes are not needed.



trunk/koffice/libs/odf/KoBorder.h
<http://reviewboard.kde.org/r/1890/#comment2055>

    Don't use more than one consecutive space



trunk/koffice/libs/odf/KoBorder.cpp
<http://reviewboard.kde.org/r/1890/#comment2056>

    curly brace is misplaced



trunk/koffice/libs/widgets/KoPageLayout.h
<http://reviewboard.kde.org/r/1890/#comment2057>

    these constructors should be removed


- Thomas


On 2009-10-21 00:47:01, Inge Wallin wrote:

>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1890/
> -----------------------------------------------------------
>
> (Updated 2009-10-21 00:47:01)
>
>
> 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/kotext/CMakeLists.txt 1037806
>   trunk/koffice/libs/kotext/KoTextBlockBorderData.cpp 1037806
>   trunk/koffice/libs/kotext/styles/KoParagraphStyle.h 1037806
>   trunk/koffice/libs/kotext/styles/KoParagraphStyle.cpp 1037806
>   trunk/koffice/libs/odf/CMakeLists.txt 1037806
>   trunk/koffice/libs/odf/KoBorder.h PRE-CREATION
>   trunk/koffice/libs/odf/KoBorder.cpp PRE-CREATION
>   trunk/koffice/libs/widgets/KoPageLayout.h 1037806
>   trunk/koffice/libs/widgets/KoPageLayout.cpp 1037806
>   trunk/koffice/plugins/textshape/tests/TestDocumentLayout.cpp 1037806
>
> 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@...
https://mail.kde.org/mailman/listinfo/koffice-devel

[Inge Wallin-3]

> On 2009-10-21 07:53:40, Thomas Zander wrote:
> > trunk/koffice/libs/kotext/CMakeLists.txt, line 85
> > <http://reviewboard.kde.org/r/1890/diff/7/?file=13059#file13059line85>
> >
> >     These two changes are not needed.

Fixed


> On 2009-10-21 07:53:40, Thomas Zander wrote:
> > trunk/koffice/libs/odf/KoBorder.h, line 148
> > <http://reviewboard.kde.org/r/1890/diff/7/?file=13064#file13064line148>
> >
> >     Don't use more than one consecutive space

That makes it easier to read, and is not part of the coding standard.


> On 2009-10-21 07:53:40, Thomas Zander wrote:
> > trunk/koffice/libs/odf/KoBorder.cpp, line 687
> > <http://reviewboard.kde.org/r/1890/diff/7/?file=13065#file13065line687>
> >
> >     curly brace is misplaced

I changed it and the code is now more difficult to read.


- Inge


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


On 2009-10-21 00:47:01, Inge Wallin wrote:

>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1890/
> -----------------------------------------------------------
>
> (Updated 2009-10-21 00:47:01)
>
>
> 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/kotext/CMakeLists.txt 1037806
>   trunk/koffice/libs/kotext/KoTextBlockBorderData.cpp 1037806
>   trunk/koffice/libs/kotext/styles/KoParagraphStyle.h 1037806
>   trunk/koffice/libs/kotext/styles/KoParagraphStyle.cpp 1037806
>   trunk/koffice/libs/odf/CMakeLists.txt 1037806
>   trunk/koffice/libs/odf/KoBorder.h PRE-CREATION
>   trunk/koffice/libs/odf/KoBorder.cpp PRE-CREATION
>   trunk/koffice/libs/widgets/KoPageLayout.h 1037806
>   trunk/koffice/libs/widgets/KoPageLayout.cpp 1037806
>   trunk/koffice/plugins/textshape/tests/TestDocumentLayout.cpp 1037806
>
> 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@...
https://mail.kde.org/mailman/listinfo/koffice-devel