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

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