Bug 88398 - grouped formulas lose listeners when split, recalculation broken
Summary: grouped formulas lose listeners when split, recalculation broken
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Calc (show other bugs)
Version:
(earliest affected)
4.4.0.2 rc
Hardware: x86-64 (AMD64) All
: highest critical
Assignee: Eike Rathke
URL:
Whiteboard: target:4.5.0 target:4.4.1 target:4.4.0
Keywords: regression
Depends on:
Blocks: mab4.4
  Show dependency treegraph
 
Reported: 2015-01-14 12:31 UTC by Olav Seyfarth
Modified: 2015-01-23 19:15 UTC (History)
3 users (show)

See Also:
Crash report or crash signature:


Attachments
Sample containing formulas to demonstrate issue (19.02 KB, application/vnd.oasis.opendocument.spreadsheet)
2015-01-14 12:31 UTC, Olav Seyfarth
Details
What it looks like on my (W8.1/64) system (14.85 KB, image/png)
2015-01-14 12:32 UTC, Olav Seyfarth
Details
simple reproducer with instructions (8.86 KB, application/vnd.oasis.opendocument.spreadsheet)
2015-01-15 11:21 UTC, Eike Rathke
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Olav Seyfarth 2015-01-14 12:31:05 UTC
Created attachment 112212 [details]
Sample containing formulas to demonstrate issue

To address the cell in the previous row / same column, I use the formula =INDIREKT(ADRESSE(ZEILE()-1;SPALTE())) since using e.g. =A14 results in "#REF!"-error when deleting a line where a cell is referenced (which I do frequently).

But at least in 4.4.0 RC 1+2, if I change the cell content, the cell in the next line is NOT updated. Only after closing and reopening the document, changes are visible in lower cells.

Attached is an example sheet. To test my issue, please open it and enter another date in A16. After pressing ENTER, A17(,A18,...) should display the same date just entered. On my system, it does not. A17,... remain set to the value they had when opening the document.

Is that intended behaviour? I found no settings to change ... so I consider this a (serious?) bug.
Comment 1 Olav Seyfarth 2015-01-14 12:32:08 UTC
Created attachment 112214 [details]
What it looks like on my (W8.1/64) system
Comment 2 raal 2015-01-14 19:32:55 UTC
I can confirm with Version: 4.4.1.0.0+
Build ID: b460414bcd1dfd2f260696e306c49c56585c6464
TinderBox: Linux-rpm_deb-x86_64@46-TDF, Branch:libreoffice-4-4, Time: 2015-01-11_05:32:52

No problem in 4.3.5, win7 -> regression
Comment 3 Eike Rathke 2015-01-14 20:44:56 UTC
Inspecting.

Btw, a less expensive (calculation performance wise as no back and forth string conversion is involved) way to address the previous cell of a column is =OFFSET(A15,-1,0) in A15, =OFFSET(A16,-1,0) in A16, ... this can be copied/filled down as well. This apparently also doesn't suffer from the bug reported.
Comment 4 Eike Rathke 2015-01-14 20:57:26 UTC
My bad, OFFSET does exhibit the same bug (which actually is helpful), but nevertheless would be the better expression.
Comment 5 Olav Seyfarth 2015-01-14 22:18:04 UTC
Thanks for the hint to quicker code and for working on the issue.
Comment 6 Olav Seyfarth 2015-01-14 22:54:46 UTC
While replacing "=INDIREKT(ADRESSE(ZEILE()-1;SPALTE()))" with "=VERSCHIEBUNG(A15;-1;0)" I noticed that if the cell that I enter e.g. "=VERSCHIEBUNG(A15;-1;0)" into was formatted as text ("@") before entering the formula, then the formula is literally displayed as cell value instead of the vulue of the cell above.
Comment 7 Eike Rathke 2015-01-15 10:47:29 UTC
(In reply to Olav Seyfarth from comment #6)
> was formatted as text ("@") before entering
> the formula, then the formula is literally displayed as cell value

That's a feature.
Comment 8 Eike Rathke 2015-01-15 11:21:03 UTC
Created attachment 112286 [details]
simple reproducer with instructions
Comment 9 Commit Notification 2015-01-15 16:28:41 UTC
Eike Rathke committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=98a940b60c666792127c58aee989a87820fb7294

fdo#88398 disable grouped listeners for now

It will be available in 4.5.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 10 Commit Notification 2015-01-15 16:28:45 UTC
Eike Rathke committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=1a2b8b2358fe5954a5c3960c9a48467de3dc5ba8

add unit test for fdo#88398

It will be available in 4.5.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 11 Eike Rathke 2015-01-15 16:57:16 UTC
Pending review
https://gerrit.libreoffice.org/13936 for 4-4
https://gerrit.libreoffice.org/13937 for 4-4-0

Note that this only disables the new group listeners for now to fix the nasty bug, a follow-up should implement the missing parts. I'll give code pointers and findings here later.
Comment 12 Eike Rathke 2015-01-15 20:34:53 UTC
The cause was that during unsharing the formula group the grouped area listeners were discarded with mxGroup->endAllGroupListening(), as necessary, but not re-established for the newly formed two groups (or single cells, depending on length of group and split position). Here the area happened to be BCA_LISTEN_ALWAYS because of INDIRECT() being used in the originally reported document, but the same scenario is hit with ordinary areas as can be seen with the simple reproducer document I attached.

In this case that happened in ScColumn::DetachFormulaCell() called via ScColumn::GetPositionToInsert() and could quite easily be fixed by letting sc::SharedFormulaUtil::unshareFormulaCell() return two ScFormulaCellGroupRef that could be used to re-establish listeners via SharedFormulaUtil::startListeningAsGroup()

However, there is a bunch of other places that call sc::SharedFormulaUtil::unshareFormulaCell() or sc::SharedFormulaUtil::splitFormulaCellGroup() or sc::SharedFormulaUtil::unshareFormulaCells() or sc::SharedFormulaUtil::splitFormulaCellGroups(), also in cell mass handlers, that would either discard the group area listeners or leave the listeners unadjusted. Some of them tricky and where re-establishing the listeners after each single split would not be desirable as a resulting group could get split again in the same loop. All of them need to be inspected and adapted before the group area listeners can be reactivated. That is more work than probably can be reliably done until 4.4.0.3 on Monday, which is why I disabled the group area listeners for now.
Comment 13 Kohei Yoshida 2015-01-16 01:49:51 UTC
I've come up with a correct fix for this, and with it, we don't have to disable the entire group area listeners.  I'll push my fix shortly.
Comment 14 Commit Notification 2015-01-16 01:55:16 UTC
Kohei Yoshida committed a patch related to this issue.
It has been pushed to "master":

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

fdo#88398: Re-enable formula group listeners.

It will be available in 4.5.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 15 Commit Notification 2015-01-16 01:55:20 UTC
Kohei Yoshida committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=4c93c341be1425401112eed3581e8b8a6308880d

fdo#88398: Handle group listeners correctly when splitting formula group.

It will be available in 4.5.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 16 Commit Notification 2015-01-16 18:14:06 UTC
Kohei Yoshida committed a patch related to this issue.
It has been pushed to "libreoffice-4-4":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=6240e598f29ee5e95c56e5885f539ce470d9afd6&h=libreoffice-4-4

fdo#88398: Handle group listeners correctly when splitting formula group.

It will be available in 4.4.1.

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 17 Commit Notification 2015-01-22 05:23:03 UTC
Kohei Yoshida committed a patch related to this issue.
It has been pushed to "libreoffice-4-4-0":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=f3722ccdafbfd36f5debeba886567c1adaded324&h=libreoffice-4-4-0

fdo#88398: Handle group listeners correctly when splitting formula group.

It will be available in 4.4.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 18 Olav Seyfarth 2015-01-23 19:15:03 UTC
Thank you so much. Just tested RC3 - it fixes the issue I reported.