Bug 117189 - Wrong table formatting when redoing the adding of table rows
Summary: Wrong table formatting when redoing the adding of table rows
Status: VERIFIED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Writer (show other bugs)
Version:
(earliest affected)
6.1.0.0.alpha0+
Hardware: All All
: medium normal
Assignee: Jim Raykowski
URL:
Whiteboard: target:6.1.0 target:6.3.0
Keywords: bibisected, bisected
Depends on:
Blocks: Undo-Redo Writer-Tables-Style
  Show dependency treegraph
 
Reported: 2018-04-23 16:36 UTC by Telesto
Modified: 2019-01-29 15:38 UTC (History)
5 users (show)

See Also:
Crash report or crash signature:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Telesto 2018-04-23 16:36:08 UTC
Description:
Wrong table formatting when redoing the adding table rows (formatted with Table styles)

Steps to Reproduce:
1. Insert -> Table
2. Choose Box list blue style (or something else)
3. Insert some rows by holding tab
4. Undo everything
5. Redo everything (every row will be green/blue depending on the style)

Actual Results:  
Formatting is broken

Expected Results:
Formatting should stay the same


Reproducible: Always


User Profile Reset: No



Additional Info:
Version: 6.1.0.0.alpha0+
Build ID: 2ed7c02478968852d7d39c2c4677f2ecf3441bc7
CPU threads: 4; OS: Windows 6.3; UI render: default; 
TinderBox: Win-x86@42, Branch:master, Time: 2018-04-22_01:00:56
Locale: nl-NL (nl_NL); Calc: CL


User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:52.0) Gecko/20100101 Firefox/52.0
Comment 1 Dieter 2018-04-24 12:28:42 UTC
Repro with

Version: 6.1.0.0.alpha0+ (x64)
Build ID: 2325f9ac789cd12e5ecc9d239baf2558e1d678bb
CPU threads: 4; OS: Windows 10.0; UI render: GL; 
TinderBox: Win-x86_64@42, Branch:master, Time: 2018-04-05_00:37:47
Locale: en-US (de_DE); Calc: CL
Comment 2 Xisco Faulí 2018-04-25 07:58:35 UTC
The style in intermediate rows changed after https://cgit.freedesktop.org/libreoffice/core/commit/?id=d1b13f486eacc60c9b71ec9f1b29cde2f4504d4e. Before this commits, all rows had the same style, now they have different styles for odd and even rows.

@Jim, one for you?
Comment 3 Jim Raykowski 2018-04-28 08:32:24 UTC
(In reply to Xisco Faulí from comment #2)
> The style in intermediate rows changed after
> https://cgit.freedesktop.org/libreoffice/core/commit/
> ?id=d1b13f486eacc60c9b71ec9f1b29cde2f4504d4e. Before this commits, all rows
> had the same style, now they have different styles for odd and even rows.

Pre patch this depends on where autoformat table style is selected. From the Insert Table dialog all rows added with tab have the same formatting as the last row. In this case the autoformat table style is only applied to the table when inserted. It is not assigned to the table. If the table style is selected from the Sidebar Styles deck Table Styles then the autoformat table style is assigned to the table and all rows added with tab follow the autoformat Table style. This may result in different styles for odd and even rows.

The patch simply assigns the table style to the table when selected in the Insert Table dialog like when a table style is selected from the Sidebar.

Tables that are assigned an autoformat table style only work as expected if no direct formatting is made. This has always been the case. Direct table formatting does not persist in tables that have been assigned an autoformat table style. Patches [1][2] submitted for bug 115573 attempt to address some of this.

> @Jim, one for you?

looks fun! I will see what I can do :)  

[1] https://gerrit.libreoffice.org/#/c/50612/
[2] https://gerrit.libreoffice.org/#/c/51990/
Comment 4 Jim Raykowski 2018-04-29 06:36:02 UTC
Comment 3 can/should be ignored except for this part

>> @Jim, one for you?

> looks fun! I will see what I can do :)


Here is a patch
https://gerrit.libreoffice.org/#/c/53618/
Comment 5 Jim Raykowski 2018-05-04 06:26:52 UTC
My first crack at this was a disaster. Patch set 3 is a different approach that seems to be better.
Comment 6 Commit Notification 2018-05-21 20:27:30 UTC
Jim Raykowski committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=137c38a1ba01c51c421f695e1558d2c1499c6627

tdf#117189 Fix table InsertRow redo

It will be available in 6.1.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 7 Jim Raykowski 2018-09-07 08:25:27 UTC
The patch that fixed this caused bug 119018 :( 

No worries, looking at the code again, it seems the problem is that the cursor is not positioned in the table when the table is restored with redo. When the table does not have the cursor the code that inserts rows/cols ignores the table style. 

So a new approach to fix this bug is to move the cursor into the table when restored with redo. This approach can be tested manually by removing this patch or using a version before this patch was merged.

Follow these steps to prove:

1. Insert -> Table
2. Choose Box list blue style (or something else)
3. Insert some rows by holding tab
4. Undo everything
5. Redo once so the the table is restored and the cursor is flashing below the table
6. Use the up arrow key to move the cursor into the table
7. Redo everything now that the cursor is in the table

Results: Behaves as expected

I have a new patch that moves the cursor into the table after it is restored with redo. Will send when this patch is reverted. It doesn't cause the bug 119018 problem.
Comment 8 Jim Raykowski 2018-09-07 08:56:25 UTC
I was ready to throw in the towel on this which was the reason for the revert patch but since a solution came much quicker than expected the revert can be skipped.

Here is a patch that removes the old approach and adds the new approach described in comment 7:

https://gerrit.libreoffice.org/#/c/60132/
Comment 9 Telesto 2018-11-29 08:43:27 UTC
@Jim
Please change the status to FIXED. Everything is OK :-)

Version: 6.3.0.0.alpha0+
Build ID: f21d2b48bd68424a96aa6cd5572e368208378291
CPU threads: 4; OS: Windows 6.3; UI render: GL; VCL: win; 
TinderBox: Win-x86@42, Branch:master, Time: 2018-11-27_00:26:54
Locale: en-US (nl_NL); UI-Language: en-US
Calc: CL
Comment 10 Shulammite 2018-11-29 09:35:53 UTC
Bug not reproducible in version.

  Version: 6.3.0.0.alpha0+ (x64)
  Build ID: 0f25a3c36f27fd51453b9a9115f236b83c143684
  CPU threads: 8; OS: Windows 10.0; UI render: GL; VCL: win; 
  TinderBox: Win-x86_64@42, Branch:master, Time: 2018-11-27_20:06:55
  Locale: zh-TW (zh_TW); UI-Language: en-US
  Calc: threaded
Comment 11 Xisco Faulí 2018-11-29 09:44:51 UTC
> https://gerrit.libreoffice.org/#/c/60132/

The patch hasn't been merged yet. Not sure whether it should be closed as RESOLVED FIXED. OTOH, bug 119018 is still open.
Comment 12 Jim Raykowski 2018-11-29 11:40:07 UTC
I don't repro this bug or bug 119018 with version 6.3.0.0.alpha0+
I see the initial patch for bug 119018 was never reverted and the second attempt was never merged. Mystery to me.
Comment 13 Buovjaga 2018-12-01 19:45:14 UTC
(In reply to Jim Raykowski from comment #12)
> I don't repro this bug or bug 119018 with version 6.3.0.0.alpha0+
> I see the initial patch for bug 119018 was never reverted and the second
> attempt was never merged. Mystery to me.

Bibisecting to find the fix might be interesting.
Comment 14 Jim Raykowski 2018-12-01 20:51:51 UTC
(In reply to Buovjaga from comment #13)
> 
> Bibisecting to find the fix might be interesting.

I am interested.
Comment 15 Jim Raykowski 2018-12-01 23:32:42 UTC
Looking at this closer I see nothing has changed. The revert/next stab patch in comment 8 was never merged and bug 119018 can still be reproduced, not sure why I thought it was no longer reproducible as I stated in comment 12.

The revert/next stab patch in comment 8 seems to fix both.

I am hesitant to merge patches. Each time I have merged a patch to master with gerrit a message from Tinder box is generated:

"One of you broke the build of LibreOffice with your commit :-(
Please commit and push a fix ASAP!"

@Buovjaga, sorry for wasting your powers of bibisect on this. I am interested in bibisecting but need to borrow a fast/stable internet connection to be able to get the required files without timing out.
Comment 16 Buovjaga 2018-12-02 08:40:50 UTC
It's ok, I did not try to bibisect yet.

I am not sure how to check if one's own patch broke the build (except just inspecting the failed build log from Jenkins, console output). You can hop on #libreoffice-dev to ask in such situations.
Comment 17 Commit Notification 2018-12-03 20:58:08 UTC
Jim Raykowski committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/+/2eeabd08acda37502e65730843641870fa2eed39%5E%21

tdf#117189 Fix table insert row redo

It will be available in 6.3.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 18 Xisco Faulí 2019-01-14 09:54:36 UTC
A polite ping to Jim Raykowski:
Is this bug fixed? if so, could you please close it as RESOLVED FIXED ? Otherwise, Could you please explain what's missing?
Thanks
Comment 19 Jim Raykowski 2019-01-15 06:02:33 UTC
Seems to be working correctly in

Version: 6.3.0.0.alpha0+
Build ID: 5fce6efe346c38d17b933641422c98c9cfe54069
CPU threads: 4; OS: Linux 4.18; UI render: default; VCL: gtk3; 
Locale: en-US (en_US.UTF-8); UI-Language: en-US
Calc: threaded
Comment 20 BogdanB 2019-01-29 15:38:30 UTC
Indeed fixed on 6.3

Version: 6.3.0.0.alpha0+
Build ID: 9bc10964f0673b64e282ad567d08bf7ebba4df65
CPU threads: 4; OS: Linux 4.15; UI render: default; VCL: gtk3; 
TinderBox: Linux-rpm_deb-x86_64@86-TDF, Branch:master, Time: 2019-01-27_18:08:30
Locale: ro-RO (ro_RO.UTF-8); UI-Language: en-US
Calc: threaded