Bug 141619 - Undoing of Replace All operation leaves a "dirty" spreadsheet, force recalculation needed
Summary: Undoing of Replace All operation leaves a "dirty" spreadsheet, force recalcul...
Status: NEW
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Calc (show other bugs)
Version:
(earliest affected)
4.2.0.4 release
Hardware: All All
: medium normal
Assignee: Not Assigned
URL:
Whiteboard:
Keywords: bibisected, bisected, regression
: 134620 (view as bug list)
Depends on:
Blocks: Undo-Redo
  Show dependency treegraph
 
Reported: 2021-04-11 05:54 UTC by Liaison to zh-CN User Community
Modified: 2024-02-26 10:52 UTC (History)
5 users (show)

See Also:
Crash report or crash signature:


Attachments
Sample file (58.86 KB, application/vnd.oasis.opendocument.spreadsheet)
2021-04-11 05:54 UTC, Liaison to zh-CN User Community
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Liaison to zh-CN User Community 2021-04-11 05:54:21 UTC
Created attachment 171096 [details]
Sample file

This is a bug report translated and forwarded form a Chinese LO user chat group.

Description:
When "Replace All" is done for a certain column, and then immediately "Undo", some cells containing a formula related to the replaced cells are not properly undone.  The displayed result for these cells are still using the replacement string, not the original string.  A forced recalculation is needed to restore the whole spreadsheet to the state before "Replace All".

Steps to Reproduce:

1. Open the attached sample ODS file.  There are some Chinese text in them but they are not involved in testing, so not displaying them correctly (encoding or font issues) are not critical.  Make sure menu Data > Calculate > AutoCalculate is enabled;

2. Click on the column header "A", the whole column is selected;

3. Open "Find and Replace" dialog by either menu Edit > Find and Replace... or shortcut Ctrl+H.  Type "A004" in Find field, "ABCD" in Replace field, check "Current selection only" if it's not already enabled, leave all other options (the ones under Find field, as well as all others under "Other options") unchecked;

4. Press "Replace All" button, if search results dialog is enabled, it should pop up and tell you 270 cells are replaced.  Close this search result dialog, then also close search and replace dialog;

5. Now all the cells in column A should show "ABCD", either directly replaced by step above or as a result of a formula calculation.  You can also see that data in column B and column O also changed;

6. Undo the "Replace All" operation, either by menu Edit > Undo: Replace or shortcut Ctrl+Z;

7. Observe that while cell A2 is correctly redone back to "A004", cells A3, B2, O2 and many others still contain "ABCD";

8. Force re-calculation of the whole sheet by either menu Data > Calculate > Recalculate Hard or shortcut Ctrl+Shift+F9, now all cells show "A004" like the original state.

Expected Result:
Undo operation reverts all changes caused by Replace All, no forced re-calculation needed.

Additional Information:

I reproduced this bug in (slightly old) 7.2 daily build, 7.1.1, and various versions back to 5.2.7 (the oldest I have installed):

Version: 7.2.0.0.alpha0+ (x64) / LibreOffice Community
Build ID: 722ec600e85cca2e94e82e69f8d13773061172b9
CPU threads: 2; OS: Windows 10.0 Build 19041; UI render: Skia/Raster; VCL: win
Locale: zh-CN (zh_CN); UI: zh-CN
Calc: threaded
---
Version: 7.1.1.2 (x64) / LibreOffice Community
Build ID: fe0b08f4af1bacafe4c7ecc87ce55bb426164676
CPU threads: 2; OS: Windows 10.0 Build 19041; UI render: default; VCL: win
Locale: zh-CN (zh_CN); UI: zh-CN
Calc: threaded
---
Version: 5.2.7.2 (x64)
Build ID: 2b7f1e640c46ceb28adf43ee075a6e8b8439ed10
CPU Threads: 2; OS Version: Windows 6.19; UI Render: default; 
Locale: zh-CN (zh_CN); Calc: group

The original reporter also claims to reproduce on Deepin Linux (exact version information not yet available).

The exact cells that are not properly reverted by undo seems to change a little each time I test, maybe due to version differences, or just variations between different tests.  However cell A3 and B2, and all the other initially visible affected cells (by "affected", I mean containing a formula that concerns cells with the replaced "A004" string) are always wrong.  Other cells that are not initially visible and need scrolling to see may be correctly reverted to "A004".
Comment 1 Liaison to zh-CN User Community 2021-04-11 06:12:54 UTC
> The original reporter also claims to reproduce on Deepin Linux (exact
> version information not yet available).
Got it:
Version: 7.1.1.2 / LibreOffice Community
Build ID: fe0b08f4af1bacafe4c7ecc87ce55bb426164676
CPU threads: 4; OS: Linux 5.4; UI render: default; VCL: gtk3
Locale: zh-CN (zh_CN.UTF-8); UI: zh-CN
Calc: threaded
Comment 2 Hanna Yefremova 2021-08-03 08:36:43 UTC
repro in 
Version: 7.3.0.0.alpha0+ (x64) / LibreOffice Community
Build ID: 7c38362dbe1922c9825dffb463072030948d406b
CPU threads: 4; OS: Windows 10.0 Build 19042; UI render: default; VCL: win
Locale: en-US (en_US); UI: en-US
Calc: threaded
The bug is reproduced except for the O column. Nothing changed in the O column.
Comment 3 Timur 2022-09-28 11:35:59 UTC
Looking only 1st column: no repro 4.1, repro 4.2. So regression.
Comment 4 Aron Budea 2022-10-23 03:48:03 UTC
Bibisected to the following range, which is a single commit in repo bibisect-42max.

https://cgit.freedesktop.org/libreoffice/core/log/?qt=range&q=ac84ffb3c90bb5788608eadf2177f587021daaad..4c99a427ee4adaeddb2682c192384bad21d9d09b
Comment 5 Kevin Suo 2024-02-19 03:15:06 UTC
I still reproduce this on latest master:

Version: 24.8.0.0.alpha0+ (X86_64) / LibreOffice Community
Build ID: 95e6f942b3fa5c6f3e5473ac474a4702ab815502
CPU threads: 4; OS: Linux 6.5; UI render: default; VCL: gtk3
Locale: zh-CN (zh_CN.UTF-8); UI: zh-CN
Calc: threaded
Comment 6 Kevin Suo 2024-02-19 05:44:24 UTC
The bibisected range are all commits made by Kohei Yoshida.
Adding Kohei to see also: would you please take a look?

I am setting up the build environment for this range, but the most suspicious one would be commit 66d3f24334e69e1655e83520950c59a0bda095a3.
Comment 7 Kevin Suo 2024-02-22 03:24:52 UTC
Many commits in the bibsected range does fails to build. Now I managed to:
1. Setup a Fedora 19;
2. Created a new branch based on commit 4347e3b15f10784b482544bd6324d3fcd4f0146c (Adjusted the patch against mdds 0.9.0.). This commit also failes to build due to the use of "return NULL" in "sc/source/core/data/column2.cxx" and a wrong placement of "}" after the "return", so I fixed that mannually to let the new branch build.
3. In the new branch, I revert 66d3f24334e69e1655e83520950c59a0bda095a3 (Make sure to set the cloned formula cells dirty during undo / redo.). The build is success, but this is still a "bad" commit.

So the revised bibisected range is https://cgit.freedesktop.org/libreoffice/core/log/?qt=range&q=ac84ffb3c90bb5788608eadf2177f587021daaad..66d3f24334e69e1655e83520950c59a0bda095a3
Comment 8 Kevin Suo 2024-02-23 04:53:37 UTC
Bug introduced by the any of the following 3 commits related to the switch to use mdds for cell storage.

1. commit c008dc483f8c6840803983e7e351cec6fdd32070
Author: Kohei Yoshida <kohei.yoshida@gmail.com>
Date:   Fri May 24 11:52:18 2013 -0400

    Switch to using multi_type_vector for cell storage.

2. commit 3b0c069c9a157c4cd9ec5636c776115af6d9664f
Author: Kohei Yoshida <kohei.yoshida@gmail.com>
Date:   Wed Jun 19 11:54:12 2013 -0400

    Don't forget to return true if we are successful.

3. commit e3b91687590f08438b5a5d4eec72e634b11a8589
Author: Kohei Yoshida <kohei.yoshida@gmail.com>
Date:   Wed Jun 19 16:48:32 2013 -0400

    Fix the horizontal cell iterator.

(Note that #2 is to fix the hang when open a calc document, and #3 is to fix a crach when open a calc document, so the most possible first bad commit is #1. However, #3 may also possible to cause the bug.)

==========

Below I describe how I identified the 3 commits, in case someone is interested:

1. Set up a old Fedora 19 VM.
Branch off based on:
commit ac84ffb3c90bb5788608eadf2177f587021daaad (Remove unnecessary debug outputs that would slow down perf tests., 2013-05-22)

I use this as the base for the new branch because it is the last known commit which can pass the build before commit c7bdee8dbd1cf260a8513a0d31b36f90daa70f1c which added "#include <mdds/multi_type_vector_custom_func3.hpp> but this header file does not exists in mdds 0.8.1 (this header is added in 0.9.0).

2. cherry-pick the mdds 0.9.0 commits:
$ git cherry-pick -x 878f46727d8bcf1f75d056d9270ef3e2fe0b9d88
$ git cherry-pick -x bb7d5ce2a8bd1dca51eb627aa2df811541053969
$ git cherry-pick -x 4347e3b15f10784b482544bd6324d3fcd4f0146c

and adjust the following file (see mdds 0.9.0 API Incompatibility Note)

sc/inc/mtvelements.hxx:

-typedef mdds::mtv::custom_block_func1<sc::element_type_broadcaster, sc::custom_broadcaster_block> BCBlkFunc;
+typedef mdds::mtv::custom_block_func1<sc::custom_broadcaster_block> BCBlkFunc;

-typedef mdds::mtv::custom_block_func1<sc::element_type_celltextattr, sc::custom_celltextattr_block> CTAttrFunc;
+typedef mdds::mtv::custom_block_func1<sc::custom_celltextattr_block> CTAttrFunc;

sc/source/core/tool/scmatrix.cxx:
-        static void delete_block(mdds::mtv::base_element_block* p)
+        static void delete_block(const mdds::mtv::base_element_block* p)

Build successful. Tested and bug does not exists.

3. Cherry-pick the first commit in the bibisect range: c7bdee8dbd1cf260a8513a0d31b36f90daa70f1c Define block types for string, edit text and formula cell elements.
(need to fix a confict due to step 2)

4. Commit the following changes to make the above commit buildable:
sc/inc/mtvelements.hxx
 // Cell container
 typedef mdds::mtv::custom_block_func3<
-    sc::element_type_string, sc::string_block,
-    sc::element_type_edittext, sc::edittext_block,
-    sc::element_type_formula, sc::formula_block> CellFunc;
+    sc::string_block,
+    sc::edittext_block,
+    sc::formula_block> CellFunc;

Build successful. Bug does not exists.

5. cherry-pick 77ec47356025de4e46f48f94629f896349b0a8e5 Reduce dependency on mtvelements.hxx header.
build successful. Bug does not exists.

6. Cherry-pick e9c5eb60d53204261c7937108bd53e86e46fc2f3 Re-org the headers a bit. In column?.cxx, column.hxx must be the first.
Building sucessful. Bug does not exists.

7. cherry-pick 75dec25730c88bdb8eb5e2a3f92689460fa89d29 Add new cell container to ScColumn.
No need to build and test because this change does not change any behaviour.

8. Cherry-pick c008dc483f8c6840803983e7e351cec6fdd32070 Switch to using multi_type_vector for cell storage.
(need to resolve conflicts)
Building sucessful, but hang when opening the test file and crash when create empty calc document, so we need next steps.

9. Cherry-pick these two commits which are for the fix of the hang and the crash: 3b0c069c9a157c4cd9ec5636c776115af6d9664f Don't forget to return true if we are successful. e3b91687590f08438b5a5d4eec72e634b11a8589 Fix the horizontal cell iterator.

Build successful. But exists!

So: e3b91687590f08438b5a5d4eec72e634b11a8589 Fix the horizontal cell iterator. 3b0c069c9a157c4cd9ec5636c776115af6d9664f Don't forget to return true if we are successful. c008dc483f8c6840803983e7e351cec6fdd32070 Switch to using multi_type_vector for cell storage.
Comment 9 Kevin Suo 2024-02-23 04:54:39 UTC
> Build successful. But exists!
Typo. Should be "Build successful. Bug exists!"
Comment 10 Kevin Suo 2024-02-26 10:49:03 UTC
*** Bug 134620 has been marked as a duplicate of this bug. ***
Comment 11 Kevin Suo 2024-02-26 10:52:44 UTC
Bug 134620 is bisected to the same commit as this one. They are the same bug behaviour, thus is marked as duplicate of this one.

Also, bug 134620 comment 0 contains more simple steps to reproduce. E.g.:
A1: 5; 
B1: =A1/100; 
Find and replace: replace all 5 to 100. B1 correctly shows "1".
Undo. B1 failed to update the formula result.