Bug 31005 - EasyHack: Table Autoformats does not save (and of course apply) all properties (Writer and Calc)
Summary: EasyHack: Table Autoformats does not save (and of course apply) all propertie...
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Writer (show other bugs)
Version:
(earliest affected)
3.3.2 release
Hardware: x86 (IA32) Windows (All)
: high major
Assignee: Muhammad Haggag
URL:
Whiteboard: target:3.6.0 unitTestNotes:40
Keywords: easyHack
Depends on:
Blocks:
 
Reported: 2010-10-20 08:00 UTC by pz1
Modified: 2023-10-08 16:45 UTC (History)
8 users (show)

See Also:
Crash report or crash signature:


Attachments
Proposed patch. (66.44 KB, patch)
2012-04-22 12:24 UTC, Muhammad Haggag
Details

Note You need to log in before you can comment on or make changes to this bug.
Description pz1 2010-10-20 08:00:11 UTC
This bug has already been filled as Issue 94630 (http://qa.openoffice.org/issues/show_bug.cgi?id=94630) on Oct 4, 2008. Though IMHO this is a serious problem. The OO-organisation has not paid attention to this bug so far. Autoformats for tables are important, because as a result of lacking table styles in Writer, the Autoformat is the only solution to create and reproduce quality corporate styles for tables in Writer.
The bug was first reported in OO 2.4.1, and persists in Libo 3.3 Beta 2.
Comment 1 Muthu 2010-10-20 13:15:33 UTC
Setting it as enhancement so that it can be tracked better...
Comment 2 manj_k 2010-10-24 02:42:27 UTC
CC
Comment 3 pz1 2010-12-06 03:14:54 UTC
(In reply to comment #1)
> Setting it as enhancement so that it can be tracked better...
I do not understand. A bug is not an enhancement imho.
pz1
Comment 4 pz1 2011-01-13 05:03:40 UTC
Still unfixed in: LibreOffice 3.3.0 OOO330m19 (Build:5)tag libreoffice-3.3.0.3
Comment 5 pz1 2011-02-11 04:27:16 UTC
Still not fixed in: LibreOffice 3.3.1 OOO330m19 (Build:5)tag libreoffice-3.3.1.1
Comment 6 Kohei Yoshida 2011-04-18 12:34:31 UTC
Proposing it as an EasyHack.  Patches are welcome.
Comment 7 Kohei Yoshida 2011-04-18 12:38:30 UTC
I'll give this to Cedric meanwhile.

Cedric, please check if this can be an easy hack.  If not, feel free to remove the tag.
Comment 9 pz1 2011-06-14 03:19:44 UTC
For Easyhackers I am available as a tester!
Comment 10 LeMoyne Castle 2011-06-16 20:04:08 UTC
Set back to major - loss of formatting shown as DEFECT in OOo issue tracker.
Comment 11 Julien Nabet 2011-09-11 06:13:02 UTC
updated links from cor, comment 8 (after one git repository conversion) :
 http://opengrok.libreoffice.org/xref/core/sw/inc/tblafmt.hxx
and
 http://opengrok.libreoffice.org/xref/core/sw/source/ui/table/tautofmt.cxx
Comment 12 pz1 2011-10-20 12:19:02 UTC
Thanks to those who looked into this problem. Apparently this is not going to be an easy hack. Maybe it is a misconception to look on a table in Writer as if it were a range of cells in a spreadsheet?
Comment 13 Muhammad Haggag 2012-02-09 08:45:59 UTC
Investigating.
Comment 14 Muhammad Haggag 2012-02-09 13:01:02 UTC
I've been investigating why cell spacing isn't saved/loaded correctly. Here's the story so far.

Summary:
========
The loss of individual spacing values happens on save to ~/.libreoffice/autotbl.fmt. For autotbl.fmt versions 3.1 and 4.0, the saving code writes the smallest spacing value to file, and that value is used for all 4 (left, top, right, and bottom). For versions 5.0+, the 4 spacing values are stored and loaded. However, we're currently using the 4.0 format for autoformat, so we lose data.

Details:
========
The format saving happens when you close the AutoFormat dialog.

* The entry point in the code is SwTableAutoFmt::Save (http://opengrok.libreoffice.org/xref/core/sw/source/core/doc/tblafmt.cxx#858).

* The loop in SwTableAutoFmt::Save calls SwBoxAutoFmt::Save (http://opengrok.libreoffice.org/xref/core/sw/source/core/doc/tblafmt.cxx#450).

* SwBoxAutoFmt::Save stores the various properties, one by one. The cell spacing is stored in SwBoxAutoFmt::aBox. The saving code for that is SvxBoxItem::Store (http://opengrok.libreoffice.org/xref/core/editeng/source/items/frmitems.cxx#2139). It's called with a format version of 4.0:
       aBox.Store( rStream, aBox.GetVersion(SOFFICE_FILEFORMAT_40) );

* SvxBoxItem::GetVersion reports an item version of 0 when given SOFFICE_FILEFORMAT_40 (would've reported BOX_4DISTS_VERSION for FILEFORMAT_50+).

* SvxBoxItem::Store checks the item version (0) against BOX_4DISTS_VERSION, and decides not to store the 4 cell spacing values.

Next Steps:
===========
The ideal solution is to move the saving code to use a newer format version. It seems that the existing loading code can handle the new version just fine. The version handling code is a bit messy, so I need to investigate it a bit more to understand if increasing the file version will have any repercussions.

I will also investigate other properties that were reported broken to see if the same solution would fix them.
Comment 15 Muhammad Haggag 2012-02-14 08:28:24 UTC
Updating the version of autotbl.fmt only fixes spacing. Most other properties aren't saved at all, it seems. Detailed breakdown follows. I'm currently trying to figure out how much work saving these would be. If it's not feasible to save them all, I will at least try to save the most common/important ones (e.g. manual table and column formatting is probably low priority, but break behavior and borders are important).

Table: None of these are saved
    . Width
        - Value
        - Relative (CheckBox)
    . Spacing
        - Left, Right, Above, Below (4 values)
    . Alignment
        - Automatic, Left, From left, Right, Center, Manual
    . Properties
        - Text Direction: Superordinate, LTR, RTL

Text Flow: None of these are saved
    . Break
        - Page/Column
        - Before/After
        - (If Page) With Page Style:
            - Style (enum)
            - Page number
    . Allow table to split
        - Allow row to break
    . Keep with next para
    . Repeat heading
        - Number of rows
    . Text Direction: Superordinate, LTR, RTL
    . Vertical Alignment: Top, Centered, Bottom

Columns: None of these are saved
    . Adapt table width
    . Adjust columns proportionally
    . Column widths (N columns)

Borders: None of these are saved except spacing
    . Line arrangement
        - Border style
        - User-defined
    . Line
        - Style
        - Width
        - Color
    . Spacing to contents
        - Left, Right, Top, Bottom
        - Synchronize
    . Shadow style
        - Position
        - Distance
        - Color
    . Properties
        - Merge adjacent line styles

Background: These are saved
    . As
        - Color or Graphic
    . For
        - Cell, Row, or Table
    . Background Color
Comment 16 Michael Meeks 2012-02-14 09:36:33 UTC
I -think- you're likely to find that these ::Save methods are obsolete, and should be removed anyway ;-) - I -think- that they're vestiges of the old binary file format - which should (in theory) live only in the binfilter/ nowadays.

I imagine that the ODF import/export for this is done in a far more complicated way via UNO interfaces, and the code is split out and most likely lives in the 'xmloff' module somewhere - though chunks of it are still in sw/source/filter/xml/

Does that help ? :-) ( I assume you're targetting ODF ? ).

Without code pointers, this didn't make a terribly easy hack IMHO ;-)
Comment 17 Muhammad Haggag 2012-02-14 10:00:01 UTC
(In reply to comment #16)
> I -think- you're likely to find that these ::Save methods are obsolete, and should be removed anyway ;-) - I -think- that they're vestiges of the old binary
> file format - which should (in theory) live only in the binfilter/ nowadays.
> 
> I imagine that the ODF import/export for this is done in a far more complicated way via UNO interfaces, and the code is split out and most likely lives in the
> 'xmloff' module somewhere - though chunks of it are still in sw/source/filter/xml/
> 
> Does that help ? :-) ( I assume you're targetting ODF ? ).
> 
> Without code pointers, this didn't make a terribly easy hack IMHO ;-)

These Save methods are still invoked (I stepped through gdb), but they're not for the documents. The saved "Table AutoFormats" go into a file of their own, ~/.libreoffice/autotbl.fmt, and it's versioned separately. Judging from the version history for auto formats, people were just adding properties haphazardly or on demand. I'd like to fix it once and for all if possible.

I agree it's not an EasyHack, but I'm already in, and it seems within reach :)
Comment 18 Michael Meeks 2012-02-15 05:53:51 UTC
Oh !

> These Save methods are still invoked (I stepped through gdb), but they're
> not for the documents. The saved "Table AutoFormats" go into a file of
> their own, ~/.libreoffice/autotbl.fmt, and it's versioned separately.
> Judging from the version history for auto formats, people were just
> adding properties haphazardly or on demand. I'd like to fix it once
> and for all if possible.

Silly me :-) you're doing a great job. Clearly it'd be nicer to use an XML format (if we could), then perhaps, later we could shove it into the ODT files if people want to embed them, but ... anyhow - well done for getting on top of it.
Comment 19 Muhammad Haggag 2012-02-17 14:36:07 UTC
Progress update: I finally figured out how to get table-level properties to be saved in AutoFormat (I currently have "Text Flow->Break" working). It's a bit hackish in that it doesn't integrate with undo/redo, but it's a good start. I'm currently looking into enabling undo/redo, as well as adding the remaining properties.

Next update (and perhaps a preliminary patch) expected around Tuesday.
Comment 20 Muhammad Haggag 2012-03-04 10:19:23 UTC
Update: Text flow options (from the "Text Flow" tab in table properties) work, and I'm working on the options from the "Borders" tab (currently only shadow and spacing work). I hit a snag with some table properties which delayed the fix for a while, but it's sorted out now, so things should go smoother.
Comment 21 Muhammad Haggag 2012-04-22 12:24:42 UTC
Created attachment 60462 [details]
Proposed patch.

Added patch. This patch is mainly targeted at Writer autoformats, although some improvements apply to Calc.

There are some known issues with this patch that I'll probably open individual bugs for and address one by one, unless I get strong feedback in favor of fixing them with this.

I'll shoot an email to the dev mailing list later tonight or tomorrow, asking for review. It's highly likely there'll be more work on the patch before it's accepted, since it's a bit messy due to the sharing of autoformats between Calc and Writer.

I've also discovered a bunch of bugs while working on this, and I'll be reporting them in the next couple of days.
Comment 22 pz1 2012-04-22 23:34:02 UTC
Great! Many thanks for all your efforts in this apparantly not_so _easy_hack. I am looking forward to testing it.
Comment 23 Muhammad Haggag 2012-04-25 05:58:03 UTC
Review thread on mailing list: http://nabble.documentfoundation.org/PATCH-fdo-31005-Table-Autoformats-does-not-save-apply-all-properties-td3936451.html
(No replies at the time of this comment)
Comment 24 Not Assigned 2012-05-03 02:23:22 UTC
Muhammad Haggag committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=002127460ad4b18fd66723e93c3d308ffc7207b1

fdo#31005 Writer: Undo support for "Repeat Heading" in autoformat application
Comment 25 Not Assigned 2012-05-03 02:23:50 UTC
Muhammad Haggag committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=7e8c0bd73ee59ff3041e55268c77203373962e51

fdo#31005 Table Autoformats do not save/apply all properties
Comment 26 Korrawit Pruegsanusak 2012-06-03 00:27:16 UTC
(In reply to comment #22)
> Great! Many thanks for all your efforts in this apparantly not_so _easy_hack. I
> am looking forward to testing it.

@pz1, have you tested this? can we mark this bug as fixed?

If you haven't tested yet, please grab a daily build from http://dev-builds.libreoffice.org/daily/ and post result here. Thanks!
Comment 27 pz1 2012-06-03 00:57:21 UTC
I have tested it the day after it was committed in a 3.6 alpha for Windows. I concluded that it does now save one table autoformat properly. And I am 99% sure it could be marked as solved. I had one more rigorous test in mind however. I do have a big 1000+ page document where I use autoformats applied by a complex macro. 

Unfortunately the windows release has a problem with JRE not being recognised. That has been reported as bug 48946 with importance HIGH/BLOCKER. That has not yet been resolved to date. There also was a possibly related macro bug that has recently been solved. I have not yet had the time to test after that fix. I'll do so in one or two days
Comment 28 pz1 2012-06-04 01:23:08 UTC
I got this morning's Alpha1 for Windows. I run on XP.

I still have a problem. Reproduce:
* Create table in new document and place cursor in table
* Do via menus -Table/add/
* Fill-in 'name' and OK and once more OK
I get screen indicating crash telling me:
"Due to an unexpected error, LOdev crashed. All the files you were working on will now be saved. The next time LOdev is launched, your files will be recovered automatically."

I do not remember this error appearing during testing in alpha0 releases back in april/may.
Comment 29 Korrawit Pruegsanusak 2012-06-04 01:38:34 UTC
Thanks for testing, but is this crash related to your 1000+ page document testing?
If not, could you please file a new bug and post the bug number here?

The point is:
* Bug 48946 is fixed, and a build this morning (UTC, June 4) should include the fix.
* Could you please test with your 1000+ page document if the crash isn't related?

Thanks again!
Comment 30 pz1 2012-06-04 01:48:54 UTC
(In reply to comment #29)
> Thanks for testing, but is this crash related to your 1000+ page document
> testing?
> If not, could you please file a new bug and post the bug number here?
> 
> The point is:
> * Bug 48946 is fixed, and a build this morning (UTC, June 4) should include the
> fix.
> * Could you please test with your 1000+ page document if the crash isn't
> related?
> 
> Thanks again!

If you re-read my post you will see that it crashes on an empty document in which I have just created an empty table.
Next thing I did is assign the default format of that table to a named autoformat.
There it crashes. I did reboot my machine twice, and it happened again and again.
Nothing to do with my 1000+ page document.
(The macros are working again which I tested successfully on the first chapter of 66 pages)

So no need for opening a new issue. The problem is in saving the autoformat itself!
Comment 31 Korrawit Pruegsanusak 2012-06-04 02:17:20 UTC
Sorry for that.

Confirmed the crash on Windows XP
* official alpha 1
* tinderbox W2008R2@16-minimal_build, pull time 2012-05-28 04:23:11, git core: b5f066e64a49aa007e04dc57dc0bbd0857ca0e2f

No crash on
* official 3.5.3
* tinderbox Win-x86@6-fast, pull time 2012-04-24 22:15:22, git core: 52d90ce040234cb35fad03bdd12e201bfa3a8634

Could you please contribute more information about "this morning build" like I did here? Thanks again.
Comment 32 pz1 2012-06-04 02:29:06 UTC
(In reply to comment #31)

> Could you please contribute more information about "this morning build" like I
> did here? Thanks again.

The about box says:
version 3.6.0alpha1+ (Build ID: 793f240)
Comment 33 pz1 2012-06-09 23:58:24 UTC
(In reply to comment #28)
> I got this morning's Alpha1 for Windows. I run on XP.
> 
> I still have a problem. Reproduce:
> * Create table in new document and place cursor in table
> * Do via menus -Table/add/
> * Fill-in 'name' and OK and once more OK
> I get screen indicating crash telling me:
> "Due to an unexpected error, LOdev crashed. All the files you were working on
> will now be saved. The next time LOdev is launched, your files will be
> recovered automatically."
> 
> I do not remember this error appearing during testing in alpha0 releases back
> in april/may.

I guess this problem is related with bug https://bugs.freedesktop.org/show_bug.cgi?id=50896
Comment 34 pz1 2012-06-12 01:51:45 UTC
The newly introduced bug 50896 may have been solved in the meantime, I can not yet test the result, because the last nighly build of the Windows version is from 8th of June. That is from before the bug was fixed I assume. (The poster of the fix did not properly report in that bug report, so I can not tell for sure when it really took place)
Comment 35 Muhammad Haggag 2012-06-12 01:58:10 UTC
The fix was committed at 2012-06-11 13:19:37, so you should grab a build from the 12th or later to be sure :)
Comment 36 pz1 2012-06-13 05:45:28 UTC
I tested in Version 3.6.0beta1 (Build ID: 1f1cdd8), which I found this afternoon. This version still crashes on applying an Autoformat to a table, so I assume this won't be fixed before beta2 if at all. So I give up checking to see if it is fixed, until I get notification from this page.
Comment 37 pz1 2012-06-28 00:44:18 UTC
It works OK in Version 3.6.0.0.beta2 (Build ID: f010139)
As far as I am concerned, this case is closed.
Thanks, Pieter
Comment 38 Michael Stahl (allotropia) 2012-06-29 14:51:06 UTC
this is fixed as far as i can remember.
Comment 39 Robinson Tryon (qubit) 2015-12-18 09:55:54 UTC
Migrating Whiteboard tags to Keywords: (EasyHack)
[NinjaEdit]
Comment 40 Buovjaga 2023-10-08 16:45:29 UTC
Notes for unit test writers:

Nothing to revert here, you would be testing new functionality.

The only remaining Writer Autoformat property to test is Break. The others were added in e0bafa78e3ad0df397d78cd65ad19bd5b07dc5f2

For Calc the tests seem to be missing, see test/source/sheet/tableautoformat.cxx