Bug 60306 - EDITING: CONDITIONAL FORMATTING destroyed after copy-paste cell
Summary: EDITING: CONDITIONAL FORMATTING destroyed after copy-paste cell
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Calc (show other bugs)
Version:
(earliest affected)
4.0.0.0.alpha0+ Master
Hardware: Other Windows (All)
: medium critical
Assignee: Not Assigned
URL:
Whiteboard: BSA target:4.1.0 target:4.0.1
Keywords: regression
Depends on:
Blocks: mab4.0
  Show dependency treegraph
 
Reported: 2013-02-05 07:02 UTC by Rainer Bielefeld Retired
Modified: 2013-05-03 06:52 UTC (History)
8 users (show)

See Also:
Crash report or crash signature:


Attachments
Test Kit (71.25 KB, application/x-zip-compressed)
2013-02-05 07:02 UTC, Rainer Bielefeld Retired
Details
CF for cell I28 after copy (17.33 KB, image/jpeg)
2013-02-26 18:36 UTC, Michel Rudelle
Details
CF for cell I28 after copy, then format paintbrush from I27 to I28 (16.93 KB, image/jpeg)
2013-02-26 18:42 UTC, Michel Rudelle
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Rainer Bielefeld Retired 2013-02-05 07:02:39 UTC
Created attachment 74218 [details]
Test Kit

I have a document containing the same kinds of conditional formatting for cells with the same Style tempates. When I copy / paste a cell from soure.ods to target.ods, conditional formatting changes 

Steps how to reproduce [Reproducible] with "LibO  4.0.0.3 rc   -  GERMAN UI / German Locale  [Build ID: 7545bee9c2a0782548772a21bc84a9dcc583b89)]"  {tinderbox: @6, pull time 2013-01-31 11:30(?)} on German WIN7 Home Premium (64bit) with User Profile updated from 4.0.0.2:

0. Download and unzip test kit
1. Open both documents.
2. In source.ods select row 65 by click on row heading
3. <control+c> for copy all row
4. In target document click A28
5. <control+v> for paste
   > Row contents will become pasted
   Expected: I28 with green border (Style "EK_Aktuell"), 
             because price info in T28 is young enough
   Actual: Blue Border.
          

Reason for the damage still is unclear, I observed various damages. sometimes the formulas became destroyed, Sometimes I saw a wrong conditional Style in the formula ("Berechnet" instead of "EK_Aktuell"), and sometimes I see nothing.

The most often problem seems to be, that the visible style in the cell is a different one than should be expected (in my normal documents for that application I most times see Style "Ersparnis" in affected cells.
              
Operating System: Windows 7
Version: 4.0.0.3 rc
Last worked in: 3.6.5.2 release
Comment 1 Rainer Bielefeld Retired 2013-02-05 07:17:15 UTC
I see that already broken  with 
* unzipped  installation of  "LOdev  4.0.0.0.alpha1+   -  ENGLISH UI / German Locale  [Build ID: 76c921de48ee41716b5a500b892945c704da73c)]"  {tinderbox: Win-x86@6, pull time 2012-12-10 09:43:47} on German WIN7 Home Premium (64bit) with own separate User Profile
* unzipped  installation of  "LOdev  4.0.0.0.alpha1+   -  ENGLISH UI / German Locale  [Build ID: af60316514f3ae3d4c475819bf86f2af837171e)]"  {tinderbox: Win-x86@6, pull time 2012-11-23 22:10:31} on German WIN7 Home Premium (64bit) with own separate User Profile

Server-installation of Master "4.0.0.alpha0+  – ENGLISH UI [Build ID: f2e622]" {tinderbox: Win-x86@16, pull time 2012-10-06 09:31:39} on German WIN7 Home Premium (64bit) UserInstallation=$SYSUSERCONFIG/LOdev/3 and older versions crashed when I tried to have opened both documents simultanously, but when I closed source before I opened target I was able to reporduce the problem.

Works fine with 3.6.5.2.
Opening a document with damaged CF from 4.0 with 3.6 will not heal the problem.
Comment 2 Rainer Bielefeld Retired 2013-02-05 07:21:57 UTC
Still a problem  with parallel Dev. Installation of "LODev  4.0.1.0+   -  English UI / German Locale  [Build ID: 9b70bf62e6b5319e282cd3533c90216aabccfe5)]"  {tinderbox: @6, pull time  2013-02-03 09:04:55} on German WIN7 Home Premium (64bit) with newly created user profile ….\LODev\401\
Comment 3 Rainer Bielefeld Retired 2013-02-05 09:02:03 UTC
Also observed in Master in Bug 60311 - EDITING: Copy cells with CONDITIONAL FORMATTING formula with wrong references after paste and other independent Builds, so NEW.

Version changed due to Results in Comment 1

@Markus
Please set Status to ASSIGNED and add yourself to "Assigned To" if you accept this Bug or forward the Bug if it's not your turf
Comment 4 john.pratt 2013-02-05 09:12:48 UTC
@Rainer Bielefeld
I am having problems, but not the same as you have described.

The styles remained identical when I tested, but the conditional formatting condition became broken for cell I28.  This is happening because the original condition:
$Tabelle1.$T22>$Tabelle1.$C$10
does not have the row number as absolute(i.e. 22 can vary), but it runs out of cells when pasted and becomes:
$Tabelle1.$T#REF!>$Tabelle1.$C$10

Presumed that it was working on the basis that the condition originated in row 65 and referenced row 22, so it would always try to reference a row 43 higher than where it is pasted, but that does not seem to be the case either.
When I paste row 65 from the source.ods to row 45 in simpletarget.ods the condition changes to refer to row 25.  This is not 43 rows higher, so I'm not sure what it is doing.

However:
A. Open source.ods
B. Edit cell I65 to say: =IF(G22<>0,SUM(G65)*H65,"").
C. Select row 65.
D. Copy (CTRL+C).
E. Open sampletarget.ods
F. Select row 44.
G. Paste (CTRL+V)
Expected: formula now reads: =IF(G1<>0,SUM(G44)*H44,"")
Actual: correct result - this works

On that basis I would expect the conditional formatting condition to change in the same way, but the conditional formatting condition on cell I65 changes to say: $Tabelle1.$T23>$Tabelle1.$C$10
I'm not sure what logic it used to change the relative reference from cell T22 to cell T23, but it doesn't seem consistent with what happens in steps A-G with formulae.

Is this the cause of your problem?

Tested using:
Version 4.1.0.0.alpha0+ (Build ID: 28b7359e00fec9fd3bfab3c9105cf250c4320a2)
TinderBox: Win-x86@6, Branch:master, Time: 2013-02-02_00:14:06
Comment 5 Rainer Bielefeld Retired 2013-02-05 09:16:31 UTC
That definitively started before 4.0.0.3, so the master-Version was correct
Comment 6 Rainer Bielefeld Retired 2013-02-05 09:58:07 UTC
This one is really tricky

I wanted to attach a target document edited with unzipped  installation of  "LOdev  4.0.0.0.alpha1+   -  ENGLISH UI / German Locale  [Build ID: af60316514f3ae3d4c475819bf86f2af837171e)]"  {tinderbox: Win-x86@6, pull time 2012-11-23 22:10:31} on German WIN7 Home Premium (64bit) with own separate User Profile.
But that one now shows a wrong CF in Dialog already in the source Cell I65: "Berechnet" instead "EK_aktuell" (although view of cell is "EK_aktuell")

So it might be that I observed the same symptoms because of different roots in the various tests with defferent versions, I will have to recheck later.
Comment 7 Michel Rudelle 2013-02-05 12:00:18 UTC
(In reply to comment #4)
> The styles remained identical when I tested, but the conditional formatting
> condition became broken for cell I28.
> [...] and becomes:
> $Tabelle1.$T#REF!>$Tabelle1.$C$10

Hi,
I reproduce exactly this result
Version 4.0.0.3 (Build ID: 7545bee9c2a0782548772a21bc84a9dcc583b89)

(Working fine with 3.6.5.2)
Comment 8 Rainer Bielefeld Retired 2013-02-05 12:25:12 UTC
(In reply to comment #7)
@ Michel Rudelle:
Thank you for review! 
What result? Comment 6 or original report? Please always leave a short description to avoid misunderstanding, and here in this Bug it seems we have lots of effects influnencing, we will have to be very careful to sort out the unrelated ones.
With what Operating System did you test?
Comment 9 Michel Rudelle 2013-02-05 14:00:08 UTC
(In reply to comment #8)
You are right:

> What result?
Using the Test Kit and following the procedure of your original report, I have the same result as reported comment #4: the reference is broken and the condition formula in cell I28 (sample target.ods) is now after pasting:
$Tabelle1.$T#REF!>$Tabelle1.$C$10

> With what Operating System did you test?
Vista
Comment 10 Not Assigned 2013-02-25 07:19:55 UTC
Markus Mohrhard committed a patch related to this issue.
It has been pushed to "master":

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

use URM_COPY when copying cond formats, fdo#60306, fdo#60311



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 Not Assigned 2013-02-26 07:26:33 UTC
Markus Mohrhard committed a patch related to this issue.
It has been pushed to "libreoffice-4-0":

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

use URM_COPY when copying cond formats, fdo#60306, fdo#60311


It will be available in LibreOffice 4.0.2.

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 12 Michael Meeks 2013-02-26 11:14:52 UTC
=> fixed - thanks Markus :-)
Comment 13 Michael Meeks 2013-02-26 11:25:54 UTC
*** Bug 60311 has been marked as a duplicate of this bug. ***
Comment 14 Michael Meeks 2013-02-26 11:37:39 UTC
    Sorry for the thrash: even with the fix I don't see Rainer's nice test kit working - I get a blue border in I28 from the -4-0 branch at: 3b605c98a1e6385211e1f2ab76a1b86202f988cb - or am I mistaking something ?
Comment 15 Rainer Bielefeld Retired 2013-02-26 12:41:13 UTC
(In reply to comment #14):
The problem is that it was not easy to separate the various problems to separate bugs, because in my source documents, from what I created various test documents, often several different bugs affected the same cell, and often It took some time until I learned what the real problem was. I will try to help to sort as soon as I have a build with the latest fixes; currently Win-x86@6/' latest build was 2013-02-23_23.21.01, MinGW similar (and see Bug 61482).
Comment 16 Michel Rudelle 2013-02-26 18:36:05 UTC
Created attachment 75593 [details]
CF for cell I28 after copy

Hi,
Working on the Test Kit with today daily-build:
Version 4.0.2.0+ (Build ID: 01f8d0a1dffce854a66c0f957e81e6df6d361a8)
under Vista-32b

1/ using the procedure of the initial report:
The CF is correctly reproduced (no more broken reference), but the border is still blue
(attachment 1-CF after copy)

2/ on the SampleTarget spreadsheet modified at the previous stage:
using the Format Paintbrush to reproduce the format from I27 to I28,
The border is now green, but, when I look at the CF (with manage), the condition is the same as in point 1, nevertheless, there is a little difference when you look at the style preview
(attachment 2-CF after copy then paintbrush)

I hope this can give you a hint.
Comment 17 Michel Rudelle 2013-02-26 18:42:53 UTC
Created attachment 75595 [details]
CF for cell I28 after copy, then format paintbrush from I27 to I28

I'm sorry, I should not have used the word "attachment" is the previous comment.
I did not know that it would create links (so they are wrong !)
Comment 18 Rainer Bielefeld Retired 2013-02-26 22:02:58 UTC
@Markus:
We have some criss cross here, may be because of the vague summary and because the wrong reference also appears in my sample documents, what (because of that) are also sample for Bug 60311.  My suggestions:

a) I think your fix is for "Bug 60311 - EDITING: Copy cells with CONDITIONAL FORMATTING formula with wrong references after paste". The wrong references have gone after your fix, and that one should be marked as FIXED

b) this one should be limited to the "blue border" problem and get status NEW
Comment 19 Markus Mohrhard 2013-02-27 00:35:56 UTC
(In reply to comment #18)
> @Markus:
> We have some criss cross here, may be because of the vague summary and
> because the wrong reference also appears in my sample documents, what
> (because of that) are also sample for Bug 60311.  My suggestions:
> 
> a) I think your fix is for "Bug 60311 - EDITING: Copy cells with CONDITIONAL
> FORMATTING formula with wrong references after paste". The wrong references
> have gone after your fix, and that one should be marked as FIXED

Might be. It was just unclear to me what you tried to track with this bug.

> 
> b) this one should be limited to the "blue border" problem and get status NEW

Ok. The blue border is another problem. But I think I finally have a clue what is going wrong here. We are copying the ScAttrArray to the new row which contains the old entries for the conditional format keys. These old keys point however to different entries and therefore we have a different conditional format. This could only happen in 4.0 with overlapping conditional formats as we can have different conditional formats now in the same cell.
Before that the requirement that the conditional format in a cell must be unique prevented this bug.
Comment 20 Not Assigned 2013-02-27 01:06:05 UTC
Markus Mohrhard committed a patch related to this issue.
It has been pushed to "master":

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

remove the copied cond format cell attr entries, fdo#60306



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 21 Not Assigned 2013-02-27 09:03:09 UTC
Markus Mohrhard committed a patch related to this issue.
It has been pushed to "libreoffice-4-0-1":

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

use URM_COPY when copying cond formats, fdo#60306, fdo#60311


It will be available already in LibreOffice 4.0.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 22 Petr Mladek 2013-02-27 09:51:34 UTC
I tested the patch from comment #20 (remove the copied cond format cell attr entries) It helped to show the green border around the cell "I28" but it did not really work. I did the following test:

1. I followed instructions from the comment #0

    Result: Green border around the cell "I28" which looks promissing

2. Changed the data in the cell "T28" from "March 2012" to "March 2010"

   Result: It still shown the green border.
   Expected result: It should remove the green border and show red strike over the "I28" cell
                                 because the condition is not longer true


Alternative test is to copy rows 65 and 66 from the source.ods. The result is that both "I28" and "I29" cells have the green border. The expect result is the red strike over the "I29" cell.

Observation: It helped me to safe the "SampleTarget.ods" document as "SampleTarget.new.ods" and 
                        reload.

                        This helped also without the patch in the comment #20.

                        => It seems that the problem is rather in refreshing or recalculating the formatting.


Observation 2: If I had the patch from the comment #20, it helped me to do:

                                 - open "Format/Conditional Formatting/Manage"
                                 - select the range "I28:I29"
                                 - press "Edit"
                                 - press "OK" to close the edit window (no change is needed)
                                 - press "OK" to close the manage window

                              Result: The cell "I29" become the red strike as expected

                              This does not help without the patch in comment #20

                              => the patch in the comment #20 actually makes a difference.
Comment 23 Markus Mohrhard 2013-02-27 10:09:44 UTC
(In reply to comment #22)
> I tested the patch from comment #20 (remove the copied cond format cell attr
> entries) It helped to show the green border around the cell "I28" but it did
> not really work. I did the following test:
> 
> 1. I followed instructions from the comment #0
> 
>     Result: Green border around the cell "I28" which looks promissing
> 
> 2. Changed the data in the cell "T28" from "March 2012" to "March 2010"
> 
>    Result: It still shown the green border.
>    Expected result: It should remove the green border and show red strike
> over the "I28" cell
>                                  because the condition is not longer true
> 
> 
> Alternative test is to copy rows 65 and 66 from the source.ods. The result
> is that both "I28" and "I29" cells have the green border. The expect result
> is the red strike over the "I29" cell.
> 
> Observation: It helped me to safe the "SampleTarget.ods" document as
> "SampleTarget.new.ods" and 
>                         reload.
> 
>                         This helped also without the patch in the comment
> #20.
> 
>                         => It seems that the problem is rather in refreshing
> or recalculating the formatting.
> 
> 
> Observation 2: If I had the patch from the comment #20, it helped me to do:
> 
>                                  - open "Format/Conditional
> Formatting/Manage"
>                                  - select the range "I28:I29"
>                                  - press "Edit"
>                                  - press "OK" to close the edit window (no
> change is needed)
>                                  - press "OK" to close the manage window
> 
>                               Result: The cell "I29" become the red strike
> as expected
> 
>                               This does not help without the patch in
> comment #20
> 
>                               => the patch in the comment #20 actually makes
> a difference.

Hey Petr,

so let me leave some comments about the patch and why saving, closing and then reopening will always fix the problems that I fixed with the patch in Comment 20.

First why save, close reopen fixes the problem: We store the information about conditional format ranges in two places: The ScConditionalFormat::maRange as a ScRangeList and inside of the cell attribute storage for fast look-up. The big task is now to keep the two in sync because for import/export and nearly all update conditional format tasks the ScConditionalFormat::maRange information is the important one. However for rendering we use for performance reasons the one in the cell attribute storage.

So now why does my patch solve the problem (I have to check the problem you mentioned). The cell attribute storage stores as I mentioned the information which conditional format is applied to a cell. This is done by a vector storing an index od the conditional format (in ScConditionalFormatItem with ID ATTR_CONDITIONAL). Now when we copy&paste we copy the cell information from the old document into the paste area of the new document. This cell information also contains the old index which is not the same index as in the new document. Additionally in ScTable::CopyConditionalFormat we add the index of the copy to the the paste area but after the old index. Therefore the conditional format with the old index is evaluated first.
So removing the old conditional format information in the copy&paste case is always correct and has to happen outside of the IDF_ATTRIB case as we want to clean it always. In case of IDF_ATTRIB we also copy the conditional format and add the information into the cell information by ScDocument::AddCondFormatData
Comment 24 Not Assigned 2013-02-27 10:54:55 UTC
Markus Mohrhard committed a patch related to this issue.
It has been pushed to "master":

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

the method is actually called twice, fdo#60306



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 25 Not Assigned 2013-02-27 11:01:11 UTC
Markus Mohrhard committed a patch related to this issue.
It has been pushed to "libreoffice-4-0":

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

remove the copied cond format cell attr entries, fdo#60306


It will be available in LibreOffice 4.0.2.

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 26 Not Assigned 2013-02-27 11:39:46 UTC
Markus Mohrhard committed a patch related to this issue.
It has been pushed to "libreoffice-4-0-1":

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

remove the copied cond format cell attr entries, fdo#60306


It will be available already in LibreOffice 4.0.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 27 Michel Rudelle 2013-02-28 15:00:09 UTC
Hi,
Working on the Test Kit with today daily-build:
Version 4.0.2.0+ (Build ID: 0a12e1278b2ea6a4a668d610ff3c6a23c5fc624)
under Vista-32b

it's ok, the border is now green!
thanks Markus