[PATCH] fdo#31005: Table Autoformats does not save/apply all properties

classic Classic list List threaded Threaded
9 messages Options
Muhammad Haggag Muhammad Haggag
Reply | Threaded
Open this post in threaded view
|

[PATCH] fdo#31005: Table Autoformats does not save/apply all properties

Hello.

Bug:
====
https://bugs.freedesktop.org/show_bug.cgi?id=31005

Patch:
=====
https://bugs.freedesktop.org/attachment.cgi?id=60462

Patch review:
===========
https://bugs.freedesktop.org/page.cgi?id=splinter.html&bug=31005&attachment=60462

Summary:
=========
Extended the number of properties supported in AutoFormats, mainly for
Writer. Borders, border styles, table shadow, cell spacing, and text
flow options are now part of autoformats.

AutoFormat file format was modified so that Writer can use
writer-specific types in the format without having to expose them to
Calc. Calc treats such data as binary blobs that it can skip over. The
same mechanism allows Calc to create autoformats that can be read by
Writer, even when they're missing the Writer-specific types.

Detailed Changes (copied from patch):
========================
This change expands the number of properties supported by autoformats,
mainly for Writer.
Some improvements affect Calc as well (e.g. border styles are now
preserved for Calc).

Common: boxitem.hxx, frmitems.cxx
* Added a new version for SvxBoxItem serialization that includes border styles.
* Updated SvxBoxItem and SvxBorderLine serialization logic accordingly.

Writer: fmtornt.hxx, attrfrm.cxx
* Added serialization/deserialization logic for SwFmtVertOrient.

Writer: tblafmt.hxx, tblafmt.cxx, ndtbl.cxx
* Updated file version for autotbl.fmt to be SOFFICE_FILEFORMAT_50.
* Autoformats now record the text orientation and vertical alignment
of table cells.
* Autoformats now record the following table-level properties:
    - Break
    - Keep with next paragraph
    - Repeat heading
    - Allow table split across pages
    - Allow rows to break across pages
    - Merge adjacent line styles
    - Table shadow

Writer: UndoTable.hxx, undtbl.cxx
* Undo support for "Repeat Heading" in autoformat application.

Calc: autoform.hxx, autoform.cxx
* Added support for reading/writing writer-specific data as binary blobs.
* Updated file version for autotbl.fmt to be SOFFICE_FILEFORMAT_50.

Known Issues:
============
* The sharing of autotbl.fmt between Calc and Writer is rather
annoying, and leads to a lot of duplicate code. It might make more
sense to split it into two files, but I'm not sure if autoformat
sharing (between Calc and Writer) is an actively used feature.
* Table->Text Flow->"With Page Style" is not applied, even though it's
saved as part of the autoformat. I'll have to open a bug to track
this.
* The newly added properties were not added to AutoFormat preview
(e.g. table shadow doesn't show up as part of autoformat preview, even
if it's included in the autoformat).
* The table properties in the "Table", "Columns", and "Background"
tabs were untouched. "Columns" is for manual table layout, and it's
not clear to me how it can be generalized as part of an autoformat
(i.e. it's highly dependent on the exact number of rows/columns).
"Background" is rather low priority, seeing as that cell background's
already working. I ran out of steam for "Table->Alignment" and
"Table->Spacing". I'll probably follow up with another patch for those
once I work on something other than AutoFormats :)

QA
===
Ideally, I'd like to have someone do some testing to make sure
autoformats are working as expected. I did a lot of ad-hoc testing for
each newly-supported property, as well as old properties. However, one
should not be trusted to test his own code, especially with such big
changes :)

Pieter, who reported the bug, is willing to do some testing provided
he's given a Windows build. He's not a developer, so if there's an
easy way to get him a Windows build with the changes, it'd be great.

Regards,
--Muhammad
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Muhammad Haggag Muhammad Haggag
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] fdo#31005: Table Autoformats does not save/apply all properties

Ping.

On Tue, Apr 24, 2012 at 10:11 PM, Muhammad Haggag <[hidden email]> wrote:

> Hello.
>
> Bug:
> ====
> https://bugs.freedesktop.org/show_bug.cgi?id=31005
>
> Patch:
> =====
> https://bugs.freedesktop.org/attachment.cgi?id=60462
>
> Patch review:
> ===========
> https://bugs.freedesktop.org/page.cgi?id=splinter.html&bug=31005&attachment=60462
>
> Summary:
> =========
> Extended the number of properties supported in AutoFormats, mainly for
> Writer. Borders, border styles, table shadow, cell spacing, and text
> flow options are now part of autoformats.
>
> AutoFormat file format was modified so that Writer can use
> writer-specific types in the format without having to expose them to
> Calc. Calc treats such data as binary blobs that it can skip over. The
> same mechanism allows Calc to create autoformats that can be read by
> Writer, even when they're missing the Writer-specific types.
>
> Detailed Changes (copied from patch):
> ========================
> This change expands the number of properties supported by autoformats,
> mainly for Writer.
> Some improvements affect Calc as well (e.g. border styles are now
> preserved for Calc).
>
> Common: boxitem.hxx, frmitems.cxx
> * Added a new version for SvxBoxItem serialization that includes border styles.
> * Updated SvxBoxItem and SvxBorderLine serialization logic accordingly.
>
> Writer: fmtornt.hxx, attrfrm.cxx
> * Added serialization/deserialization logic for SwFmtVertOrient.
>
> Writer: tblafmt.hxx, tblafmt.cxx, ndtbl.cxx
> * Updated file version for autotbl.fmt to be SOFFICE_FILEFORMAT_50.
> * Autoformats now record the text orientation and vertical alignment
> of table cells.
> * Autoformats now record the following table-level properties:
>    - Break
>    - Keep with next paragraph
>    - Repeat heading
>    - Allow table split across pages
>    - Allow rows to break across pages
>    - Merge adjacent line styles
>    - Table shadow
>
> Writer: UndoTable.hxx, undtbl.cxx
> * Undo support for "Repeat Heading" in autoformat application.
>
> Calc: autoform.hxx, autoform.cxx
> * Added support for reading/writing writer-specific data as binary blobs.
> * Updated file version for autotbl.fmt to be SOFFICE_FILEFORMAT_50.
>
> Known Issues:
> ============
> * The sharing of autotbl.fmt between Calc and Writer is rather
> annoying, and leads to a lot of duplicate code. It might make more
> sense to split it into two files, but I'm not sure if autoformat
> sharing (between Calc and Writer) is an actively used feature.
> * Table->Text Flow->"With Page Style" is not applied, even though it's
> saved as part of the autoformat. I'll have to open a bug to track
> this.
> * The newly added properties were not added to AutoFormat preview
> (e.g. table shadow doesn't show up as part of autoformat preview, even
> if it's included in the autoformat).
> * The table properties in the "Table", "Columns", and "Background"
> tabs were untouched. "Columns" is for manual table layout, and it's
> not clear to me how it can be generalized as part of an autoformat
> (i.e. it's highly dependent on the exact number of rows/columns).
> "Background" is rather low priority, seeing as that cell background's
> already working. I ran out of steam for "Table->Alignment" and
> "Table->Spacing". I'll probably follow up with another patch for those
> once I work on something other than AutoFormats :)
>
> QA
> ===
> Ideally, I'd like to have someone do some testing to make sure
> autoformats are working as expected. I did a lot of ad-hoc testing for
> each newly-supported property, as well as old properties. However, one
> should not be trusted to test his own code, especially with such big
> changes :)
>
> Pieter, who reported the bug, is willing to do some testing provided
> he's given a Windows build. He's not a developer, so if there's an
> easy way to get him a Windows build with the changes, it'd be great.
>
> Regards,
> --Muhammad
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Michael Stahl-2 Michael Stahl-2
Reply | Threaded
Open this post in threaded view
|

[PUSHED] Re: [PATCH] fdo#31005: Table Autoformats does not save/apply all properties

In reply to this post by Muhammad Haggag
On 24/04/12 22:11, Muhammad Haggag wrote:
> Hello.
>
> Bug:
> ====
> https://bugs.freedesktop.org/show_bug.cgi?id=31005
>
> Patch:
> =====
> https://bugs.freedesktop.org/attachment.cgi?id=60462

hi Muhammad,

that's some really nice work from you here!

sorry for taking so long, but i had hoped that somebody else would
perhaps be more familiar with table autoformats and review this rather
substantial patch :)

> Patch review:
> ===========
> https://bugs.freedesktop.org/page.cgi?id=splinter.html&bug=31005&attachment=60462
>
> Summary:
> =========
> Extended the number of properties supported in AutoFormats, mainly for
> Writer. Borders, border styles, table shadow, cell spacing, and text
> flow options are now part of autoformats.
>
> AutoFormat file format was modified so that Writer can use
> writer-specific types in the format without having to expose them to
> Calc. Calc treats such data as binary blobs that it can skip over. The
> same mechanism allows Calc to create autoformats that can be read by
> Writer, even when they're missing the Writer-specific types.
>
> Detailed Changes (copied from patch):
> ========================
> This change expands the number of properties supported by autoformats,
> mainly for Writer.
> Some improvements affect Calc as well (e.g. border styles are now
> preserved for Calc).
>
> Common: boxitem.hxx, frmitems.cxx
> * Added a new version for SvxBoxItem serialization that includes border styles.
> * Updated SvxBoxItem and SvxBorderLine serialization logic accordingly.
>
> Writer: fmtornt.hxx, attrfrm.cxx
> * Added serialization/deserialization logic for SwFmtVertOrient.
>
> Writer: tblafmt.hxx, tblafmt.cxx, ndtbl.cxx
> * Updated file version for autotbl.fmt to be SOFFICE_FILEFORMAT_50.
> * Autoformats now record the text orientation and vertical alignment
> of table cells.
> * Autoformats now record the following table-level properties:
>     - Break
>     - Keep with next paragraph
>     - Repeat heading
>     - Allow table split across pages
>     - Allow rows to break across pages
>     - Merge adjacent line styles
>     - Table shadow
>
> Writer: UndoTable.hxx, undtbl.cxx
> * Undo support for "Repeat Heading" in autoformat application.
>
> Calc: autoform.hxx, autoform.cxx
> * Added support for reading/writing writer-specific data as binary blobs.
> * Updated file version for autotbl.fmt to be SOFFICE_FILEFORMAT_50.
>
> Known Issues:
> ============
> * The sharing of autotbl.fmt between Calc and Writer is rather
> annoying, and leads to a lot of duplicate code. It might make more
> sense to split it into two files, but I'm not sure if autoformat
> sharing (between Calc and Writer) is an actively used feature.
> * Table->Text Flow->"With Page Style" is not applied, even though it's
> saved as part of the autoformat. I'll have to open a bug to track
> this.
> * The newly added properties were not added to AutoFormat preview
> (e.g. table shadow doesn't show up as part of autoformat preview, even
> if it's included in the autoformat).

wonder why that is, perhaps an SfxItemSet somewhere that has the wrong
WhichRange?

> * The table properties in the "Table", "Columns", and "Background"
> tabs were untouched. "Columns" is for manual table layout, and it's
> not clear to me how it can be generalized as part of an autoformat
> (i.e. it's highly dependent on the exact number of rows/columns).

columns are rather tricky anyway because AFAIK they don't really
actually exist in Writer tables...

> "Background" is rather low priority, seeing as that cell background's
> already working. I ran out of steam for "Table->Alignment" and
> "Table->Spacing". I'll probably follow up with another patch for those
> once I work on something other than AutoFormats :)

couldn't find anything obviously wrong with your patch; other than the
email address which was invalid so i've replaced it with your gmail
address, perhaps you have not configured git properly, try "git config
user.email".

> @@ -825,6 +837,9 @@ bool ScAutoFormatData::Load( SvStream& rStream,
const ScAfVersions& rVersions )
>          rStream >> b; bIncludeValueFormat = b;
>          rStream >> b; bIncludeWidthHeight = b;
>
> +        if (nVer >= AUTOFORMAT_DATA_ID_31005)
> +            rStream >> swFields;
> +

> @@ -878,9 +893,12 @@ bool ScAutoFormatData::Save(SvStream& rStream)
>      rStream << ( b = bIncludeValueFormat );
>      rStream << ( b = bIncludeWidthHeight );
>
> +    if (fileVersion >= SOFFICE_FILEFORMAT_50)
> +        rStream << swFields;
> +

seems inconsistent, the second check should also be for
AUTOFORMAT_ID_31005?  no, on further examination actually this is right,
there really are different kinds of versions involved here :-/

i did some trivial tweaks to your patch, to follow our coding standards
added an m_ prefix to all new member variables, which makes things much
easier to read (because with the implicit "this" you never know what
kind of side-effects an assignment has); also i've split out the Undo
thing into a separate commit.

it is rather surprising and sub-optimal that all this autoformat stuff
uses an awful binary format based on direct serialization of
implementation details; this is very brittle and it would be nice
for example, if i read the autotbl.fmt with your patch in an old LO
without the patch, the old version cannot read it at all.
a text-based format (e.g. XML) would be much nicer, in case you want to
do further work in this area :)

also i noticed there is support for loading ancient versions of this
format, as in older than StarOffice5, behind an #ifdef READ_OLDVERS,
which is useless nowadays so i've removed that.

> QA
> ===
> Ideally, I'd like to have someone do some testing to make sure
> autoformats are working as expected. I did a lot of ad-hoc testing for
> each newly-supported property, as well as old properties. However, one
> should not be trusted to test his own code, especially with such big
> changes :)

i've tested it just a tiny bit, and i can now apply dashed borders via
autoformat; of course more thorough testing by QA folks would be
appreciated.

> Pieter, who reported the bug, is willing to do some testing provided
> he's given a Windows build. He's not a developer, so if there's an
> easy way to get him a Windows build with the changes, it'd be great.

the tinderboxes should provide daily builds somewhere...

pushed your patch to master now, thanks again!

regards,
 michael

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Lubos Lunak Lubos Lunak
Reply | Threaded
Open this post in threaded view
|

Re: [PUSHED] Re: [PATCH] fdo#31005: Table Autoformats does not save/apply all properties

On Thursday 03 of May 2012, Michael Stahl wrote:
> On 24/04/12 22:11, Muhammad Haggag wrote:
> > Pieter, who reported the bug, is willing to do some testing provided
> > he's given a Windows build. He's not a developer, so if there's an
> > easy way to get him a Windows build with the changes, it'd be great.
>
> the tinderboxes should provide daily builds somewhere...

 http://dev-builds.libreoffice.org/daily/

--
 Lubos Lunak
 [hidden email]
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Michael Meeks-2 Michael Meeks-2
Reply | Threaded
Open this post in threaded view
|

Re: [PUSHED] Re: [PATCH] fdo#31005: Table Autoformats does not save/apply all properties

In reply to this post by Michael Stahl-2

On Thu, 2012-05-03 at 11:24 +0200, Michael Stahl wrote:
> > Ideally, I'd like to have someone do some testing to make sure
> > autoformats are working as expected. I did a lot of ad-hoc testing for
> > each newly-supported property, as well as old properties. However, one
> > should not be trusted to test his own code, especially with such big
> > changes :)
>
> i've tested it just a tiny bit, and i can now apply dashed borders via
> autoformat; of course more thorough testing by QA folks would be
> appreciated.

        Oh - and one more thing :-) as we inch closer to the feature-freeze,
it'd be great to update the 3.6 features page:
       
        http://wiki.documentfoundation.org/ReleaseNotes/3.6

        For any substantial fixes / improvements.

        Thanks !

                Michael.

--
[hidden email]  <><, Pseudo Engineer, itinerant idiot

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Muhammad Haggag Muhammad Haggag
Reply | Threaded
Open this post in threaded view
|

Re: [PUSHED] Re: [PATCH] fdo#31005: Table Autoformats does not save/apply all properties

>        Oh - and one more thing :-) as we inch closer to the feature-freeze,
> it'd be great to update the 3.6 features page:
>
>        http://wiki.documentfoundation.org/ReleaseNotes/3.6
>
>        For any substantial fixes / improvements.
>

I'll update the release notes once I get confirmation from the bug
reporter. He'll try the next nightly build on Windows and see if
things work as they should.

Regards,
--Muhammad
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Muhammad Haggag Muhammad Haggag
Reply | Threaded
Open this post in threaded view
|

Re: [PUSHED] Re: [PATCH] fdo#31005: Table Autoformats does not save/apply all properties

In reply to this post by Michael Stahl-2
On Thu, May 3, 2012 at 11:24 AM, Michael Stahl <[hidden email]> wrote:
> sorry for taking so long, but i had hoped that somebody else would
> perhaps be more familiar with table autoformats and review this rather
> substantial patch :)

No problem. I'm improving my knowledge of AutoFormats and documenting
it as I go, so hopefully it'll become clearer to anyone who reads the
code.

> couldn't find anything obviously wrong with your patch; other than the
> email address which was invalid so i've replaced it with your gmail
> address, perhaps you have not configured git properly, try "git config
> user.email".

Ah, thanks for pointing it out. I updated my configuration.

> i did some trivial tweaks to your patch, to follow our coding standards
> added an m_ prefix to all new member variables, which makes things much
> easier to read (because with the implicit "this" you never know what
> kind of side-effects an assignment has); also i've split out the Undo
> thing into a separate commit.

Cool. I actually tried to follow the convention that already existed
in the code. I prefer member variable prefixes as well. (I have no
clue what the 'a' prefix means).
Is there an official style guideline somewhere?

> it is rather surprising and sub-optimal that all this autoformat stuff
> uses an awful binary format based on direct serialization of
> implementation details; this is very brittle and it would be nice
> for example, if i read the autotbl.fmt with your patch in an old LO
> without the patch, the old version cannot read it at all.
> a text-based format (e.g. XML) would be much nicer, in case you want to
> do further work in this area :)

Sure, I opened a bug:
"Writer/Calc: Use XML for serializing table autoformats instead of the
current brittle binary format"
https://bugs.freedesktop.org/show_bug.cgi?id=49437

Regards,
--Muhammad
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Michael Stahl-2 Michael Stahl-2
Reply | Threaded
Open this post in threaded view
|

Re: [PUSHED] Re: [PATCH] fdo#31005: Table Autoformats does not save/apply all properties

On 03/05/12 19:15, Muhammad Haggag wrote:
> On Thu, May 3, 2012 at 11:24 AM, Michael Stahl <[hidden email]> wrote:
>> sorry for taking so long, but i had hoped that somebody else would
>> perhaps be more familiar with table autoformats and review this rather
>> substantial patch :)
>
> No problem. I'm improving my knowledge of AutoFormats and documenting
> it as I go, so hopefully it'll become clearer to anyone who reads the
> code.

ah yes, those comments you added are great as well :)

>> i did some trivial tweaks to your patch, to follow our coding standards
>> added an m_ prefix to all new member variables, which makes things much
>> easier to read (because with the implicit "this" you never know what
>> kind of side-effects an assignment has); also i've split out the Undo
>> thing into a separate commit.
>
> Cool. I actually tried to follow the convention that already existed
> in the code. I prefer member variable prefixes as well. (I have no
> clue what the 'a' prefix means).

the "a" prefix probably means "anything for which no more specific
prefix exists", and as such i consider it almost completely useless.

> Is there an official style guideline somewhere?

the Writer team had one back on the OOo wiki:

http://wiki.services.openoffice.org/wiki/Writer/Code_Conventions

seems still relevant, except for some things like "our_" prefix which
i've never seen in actual code, git grep says it's mostly used in
bookmark code so it's entirely obvious who simply made that up and added it.

also this:

http://wiki.services.openoffice.org/wiki/Cpp_Coding_Standards

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Eike Rathke-2 Eike Rathke-2
Reply | Threaded
Open this post in threaded view
|

Re: [PUSHED] Re: [PATCH] fdo#31005: Table Autoformats does not save/apply all properties

Hi Michael,

On Thursday, 2012-05-03 19:54:51 +0200, Michael Stahl wrote:

> > Cool. I actually tried to follow the convention that already existed
> > in the code. I prefer member variable prefixes as well. (I have no
> > clue what the 'a' prefix means).
>
> the "a" prefix probably means "anything for which no more specific
> prefix exists", and as such i consider it almost completely useless.

"a" as "a"n instance, "a"n object ... as opposed to "the" unique
occurrence ... or some such.

  Eike

--
LibreOffice Calc developer. Number formatter stricken i18n transpositionizer.
GnuPG key 0x293C05FD : 997A 4C60 CE41 0149 0DB3  9E96 2F1A D073 293C 05FD

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

attachment0 (205 bytes) Download Attachment