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 LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
