Bug 106590 - copy cell with comments does not copy the comment content
Summary: copy cell with comments does not copy the comment content
Status: RESOLVED WORKSFORME
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Calc (show other bugs)
Version:
(earliest affected)
5.2.6.2 release
Hardware: All Windows (All)
: medium critical
Assignee: Kohei Yoshida
URL:
Whiteboard:
Keywords: regression
: 113693 (view as bug list)
Depends on:
Blocks:
 
Reported: 2017-03-17 11:05 UTC by Winfried Donkers
Modified: 2017-11-07 17:17 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 Winfried Donkers 2017-03-17 11:05:28 UTC
Description:
When copy/pasting a row with cells that have comments, the comment content of these cells is not pasted.

Steps to Reproduce:
1.add comment to a cell and enter some text in the comment
2.select row with that cell
3.copy
4.paste the row
5.open the comment of the pasted cell with comment

Actual Results:  
The comment window is there, but without content.

Expected Results:
Comment window with content.


Reproducible: Always

User Profile Reset: No

Additional Info:
-When the comment content has been edited, the problem does not occur.
-When the document is saved, the pasted cell with empty comment is saved as a cell without comment.


User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Firefox/52.0
Comment 1 Winfried Donkers 2017-03-17 11:10:17 UTC
Problem does not occur with version 5.2.5.1

problem does not occur when copy/pasting cell instead of row.
Comment 2 Xisco Faulí 2017-03-17 11:37:29 UTC
I can't reproduce it in

Version: 5.4.0.0.alpha0+
Build ID: d3b5bd4a07a619db6bee1c39c32280ac3c620532
CPU threads: 4; OS: Linux 4.8; UI render: default; VCL: gtk3; 
Locale: ca-ES (ca_ES.UTF-8); Calc: group
Comment 3 Markus Mohrhard 2017-03-17 12:04:26 UTC
If that is really a regression in 5.2.6 can someone please bisect/bibisect it?
Comment 4 Xisco Faulí 2017-03-17 12:12:44 UTC
I can't reproduce it in

Versión: 5.2.6.2
Id. de compilación: a3100ed2409ebf1c212f5048fbe377c281438fdc
Subprocesos de CPU: 1; SO: Windows 6.1; Repr. de IU: predeterminado; 
Configuración regional: es-ES (es_ES); Calc: group

is OpenGL enabled? any change if you reset your profile ?
Comment 5 Winfried Donkers 2017-03-17 12:29:12 UTC
(In reply to Xisco Faulí from comment #4)
> I can't reproduce it in
> 
> Versión: 5.2.6.2
> Id. de compilación: a3100ed2409ebf1c212f5048fbe377c281438fdc
> Subprocesos de CPU: 1; SO: Windows 6.1; Repr. de IU: predeterminado; 
> Configuración regional: es-ES (es_ES); Calc: group
> 
> is OpenGL enabled? any change if you reset your profile ?

OpenCL was enabled.
After disabling, I can no longer reproduce the problem.
Thank you for the pointer!
Comment 6 Winfried Donkers 2017-03-17 12:41:07 UTC
(In reply to Winfried Donkers from comment #5)
> > is OpenGL enabled? any change if you reset your profile ?
> 
> OpenCL was enabled.
> After disabling, I can no longer reproduce the problem.
> Thank you for the pointer!

To be accurate:
OpenCL was not enabled, but 'allow use of software interpreter (even when OpenCl is not available)' was enabled.
This I disabled and made the problem disappear on the machines I checked so far.

All machines are Windows7Pro.
Comment 7 Markus Mohrhard 2017-03-17 15:15:35 UTC
(In reply to Winfried Donkers from comment #6)
> (In reply to Winfried Donkers from comment #5)
> > > is OpenGL enabled? any change if you reset your profile ?
> > 
> > OpenCL was enabled.
> > After disabling, I can no longer reproduce the problem.
> > Thank you for the pointer!
> 
> To be accurate:
> OpenCL was not enabled, but 'allow use of software interpreter (even when
> OpenCl is not available)' was enabled.
> This I disabled and made the problem disappear on the machines I checked so
> far.
> 
> All machines are Windows7Pro.

If you can reproduce it with the software interpreter we still have a serious bug. Ideally that setting should not affect anything that is not a formula. Can you please check again with the setting turned on whether you can reproduce the issue?
Comment 8 Winfried Donkers 2017-03-18 09:10:04 UTC
(In reply to Markus Mohrhard from comment #7)
> If you can reproduce it with the software interpreter we still have a
> serious bug. Ideally that setting should not affect anything that is not a
> formula. Can you please check again with the setting turned on whether you
> can reproduce the issue?

Will check on Monday with versions 5.2.5 and 5.2.6, 32 and 64 bit and software interpreter enabled and disabled (all on Windows).
Comment 9 Winfried Donkers 2017-03-20 11:42:14 UTC
I did some tests and came to the conclusion that
-the option 'allow use of software interpreter (even when OpenCl is not available)' has no effect on the problem;
-the problem does not occur with version 5.2.5.1 (Windows7 nor Windows 10);
-the problem does occur with version 5.2.6.2, both 32 and 64 bit, on Windows 7 and Windows 10.

Steps to reproduce:
1. create a new Calc Document;
2. select cell A1, enter a and add comment with text in it;
3. select cell A1;
4. copy;
5. paste in A5. Both cell content and comment content are copied;
6. select cell A1 and change the text colour (or cell background colour);
7. seelct cell A1;
8. copy;
9. paste in A10. Cell content is copied, but comment content is not.

The changes made in step 6 will probably not be that relevant, I just used these changes to test and reproduce the problem.

I don't have the versions mentioned above here on Linux machines, nor do I have have master on a Linux machine in the office. I will try if I can reproduce on master (Linux) at home.

I don't have experience with bi(bi)secting and rather leave that to someone else.
Comment 10 Xisco Faulí 2017-03-20 18:50:35 UTC
@Winfried, I can reproduce your step in

Versión: 5.2.6.2
Id. de compilación: a3100ed2409ebf1c212f5048fbe377c281438fdc
Subprocesos de CPU: 1; SO: Windows 6.1; Repr. de IU: predeterminado; 
Configuración regional: es-ES (es_ES); Calc: group

but not in

Version: 5.4.0.0.alpha0+
Build ID: 4ba483beccc99d336d0e0bec47b5fd6823b16c16
CPU threads: 4; OS: Linux 4.8; UI render: default; VCL: gtk2; 
Locale: ca-ES (ca_ES.UTF-8); Calc: group

nor

Versión: 5.3.1.1
Id. de compilación: 72fee18f394a980128dc111963f2eefb05998eeb
Subpr. de CPU: 1; SO: Windows 6.1; Repr. de IU: predet.; Motor de trazado: HarfBuzz; 
Configuración regional: es-ES (es_ES); Calc: group
Comment 11 Winfried Donkers 2017-03-21 07:35:38 UTC
(In reply to Xisco Faulí from comment #10)

Ok, I set the status to NEW.
As it leads to data loss in a 'still' release version (5.2.6) where it didn't in the previous version (5.2.5) I keep the regression keyword and critical importance.

Unfortunately, I see no quick way to bi(bi)sect myself; hopefully someone else can do that and pinpoint the commit(s) where it started going wrong.
Comment 12 Kohei Yoshida 2017-03-29 00:30:14 UTC
I'm looking into this now.
Comment 13 Kohei Yoshida 2017-03-29 01:47:10 UTC
This commit

https://cgit.freedesktop.org/libreoffice/core/commit/?h=libreoffice-5-2&id=4d3b4a8ad12767c846b48c3a424e6bb0fa7af924

has introduced this. *But*, if you look closely at the change Eike's made, he left this comment:

        /* XXX this is only a workaround to prevent a crash, the actual note
         * content is lost, only a standard empty note caption will be pasted.
         * TODO: come up with a solution. */

So, it appears that this new behavior is a temporary compromise in order to prevent crash as reported in Bug 104967.

The question is, why do we not see this problem on the master branch?
Comment 14 Kohei Yoshida 2017-03-29 02:11:39 UTC
On the master branch, Eike has already put in place a more correct solution, which is probably why this problem doesn't happen there. The change he's made is quite big though, so I'm not sure if it's worth backporting it to the already-close-to-EOL 5.2 branch.
Comment 15 Winfried Donkers 2017-03-29 05:46:04 UTC
(In reply to Kohei Yoshida from comment #14)
> On the master branch, Eike has already put in place a more correct solution,
> which is probably why this problem doesn't happen there. The change he's
> made is quite big though, so I'm not sure if it's worth backporting it to
> the already-close-to-EOL 5.2 branch.

@Kohei: First of all, thank you for looking into this. The bug is/was important to the company I work for as the copying of cells with comments is common practise here. 
Now that I know the cause and the impact of the correct solution, I can live with the 'buggy' state of versions 5.2 till EOL of the 5.2 branch. We'll simply stick to 5.2.6 till 5.3.3 seems stable enough to us.
_But_ I think a note in the release notes for 5.2.7 saying that this is a known and chosen behaviour. After all, the behaviour does lead to data loss.
Comment 16 Winfried Donkers 2017-03-29 05:52:23 UTC
(In reply to Kohei Yoshida from comment #13)
> This commit
> 
> https://cgit.freedesktop.org/libreoffice/core/commit/?h=libreoffice-5-
> 2&id=4d3b4a8ad12767c846b48c3a424e6bb0fa7af924
> 
> has introduced this. *But*, if you look closely at the change Eike's made,
> he left this comment:
> 
>         /* XXX this is only a workaround to prevent a crash, the actual note
>          * content is lost, only a standard empty note caption will be
> pasted.
>          * TODO: come up with a solution. */
> 
> So, it appears that this new behavior is a temporary compromise in order to
> prevent crash as reported in Bug 104967.
> 
> The question is, why do we not see this problem on the master branch?

AFAICS, this patch is in the destructor of ScDocument. But in case of the behaviour as described in comment 9 there is no closing of documents, all is done within the same open document.
I must admit that I am not familiar with the code in this area of Calc, so it may be possible that temporary instances of ScDocument are used.

BTW, what is the commit ID for the more correct solution applied in master?
Comment 17 Kohei Yoshida 2017-03-31 22:32:07 UTC
(In reply to Winfried Donkers from comment #16)

> AFAICS, this patch is in the destructor of ScDocument. But in case of the
> behaviour as described in comment 9 there is no closing of documents, all is
> done within the same open document.
> I must admit that I am not familiar with the code in this area of Calc, so
> it may be possible that temporary instances of ScDocument are used.

Yes.  ScDocument instances are used to handle both undo and clipboard handling, and this one hits one such case since it's related to clipboard handling.

> BTW, what is the commit ID for the more correct solution applied in master?

It spans across multiple commits, and it's not easy for me to pinpoint exactly which commits. That said, you can take a look at the code around ScPostIt::ForgetCaption() and particularly the introductions of ScCaptionPtr class to properly handle the SdrCaptionObj pointers.  That custom pointer wrapper is a pretty big addition, and backporting that would be a bit of a risk IMO.

Kohei
Comment 18 Winfried Donkers 2017-04-01 17:00:22 UTC
a(In reply to Kohei Yoshida from comment #17)
> It spans across multiple commits, and it's not easy for me to pinpoint
> exactly which commits. That said, you can take a look at the code around
> ScPostIt::ForgetCaption() and particularly the introductions of ScCaptionPtr
> class to properly handle the SdrCaptionObj pointers.  That custom pointer
> wrapper is a pretty big addition, and backporting that would be a bit of a
> risk IMO.
 
I'll set the bug report to WORKSfORME, I don't think the effort of backporting of the new ScCaptionPtr so near to %.2's EOL is worth it.
Thank you for pinpointing the cause and your explanations!
Comment 19 Eike Rathke 2017-04-12 10:33:37 UTC
If this is really related to https://cgit.freedesktop.org/libreoffice/core/commit/?h=libreoffice-5-2&id=4d3b4a8ad12767c846b48c3a424e6bb0fa7af924 (which I doubt, as closing the original document is not involved in the given scenario and the change is triggered only in the dtor), a follow-up commit for bug 104967 was pushed to 5-3 that preserves the comment content after the clipboard's source document was closed, http://cgit.freedesktop.org/libreoffice/core/commit/?id=6c6528828c5ca1da6a1ccc98d1aa5b42d84152a4&h=libreoffice-5-3
If someone wants to try that on 5-2?

The currently ongoing work on ScCaptionPtr should be unrelated and I'd never backport that.
Comment 20 Winfried Donkers 2017-04-12 13:31:30 UTC
(In reply to Eike Rathke from comment #19)
> If this is really related to
> https://cgit.freedesktop.org/libreoffice/core/commit/?h=libreoffice-5-
> 2&id=4d3b4a8ad12767c846b48c3a424e6bb0fa7af924 (which I doubt, as closing the
> original document is not involved in the given scenario and the change is
> triggered only in the dtor), a follow-up commit for bug 104967 was pushed to
> 5-3 that preserves the comment content after the clipboard's source document
> was closed,
> http://cgit.freedesktop.org/libreoffice/core/commit/
> ?id=6c6528828c5ca1da6a1ccc98d1aa5b42d84152a4&h=libreoffice-5-3
> If someone wants to try that on 5-2?
> 
> The currently ongoing work on ScCaptionPtr should be unrelated and I'd never
> backport that.

Given that the freeze date for the last 5.2 release, 5.2.7, is this week, you are not convinced that the commit will do the trick and I don't have the time to work on it at my regular pace before the 5.2.7 branch is frozen, I won't try it.
(Also I can't see yet either why  https://cgit.freedesktop.org/libreoffice/core/commit/?h=libreoffice-5-2&id=4d3b4a8ad12767c846b48c3a424e6bb0fa7af924 causes the problem, which means I would be working 'blind'.)
Comment 21 Eike Rathke 2017-04-13 08:24:24 UTC
Fwiw, I can't even reproduce the problem in 5.2.6 on Linux.
Comment in pasted row is shown and saved.
Comment 22 Winfried Donkers 2017-04-13 09:11:40 UTC
(In reply to Eike Rathke from comment #21)
> Fwiw, I can't even reproduce the problem in 5.2.6 on Linux.
> Comment in pasted row is shown and saved.

I have set the O/S field to Windows, as it only has been reproduced on Windows (and version 5.2.6).
Comment 23 Kohei Yoshida 2017-04-18 20:59:04 UTC
I was able to reproduce it on Linux though, FWIW.
Comment 24 Timur 2017-11-07 17:17:57 UTC
*** Bug 113693 has been marked as a duplicate of this bug. ***