Issue 119446 - [From Symphony]Formula field lost
[From Symphony]Formula field lost
Status: CLOSED FIXED
Product: Writer
Classification: Application
Component: ui
3.4.0
PC All
: P3 major (vote)
: 4.0.0
Assigned To: Oliver-Rainer Wittmann
:
: 119585 (view as issue list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-28 08:45 UTC by Yan Ji
Modified: 2012-10-18 06:37 UTC (History)
4 users (show)

See Also:
Issue Type: DEFECT
Latest Confirmation on: ---
Developer Difficulty: ---


Attachments
sample (31.00 KB, application/msword)
2012-05-28 08:45 UTC, Yan Ji
no flags Details
patch for i119446 (1.07 KB, patch)
2012-07-30 07:28 UTC, zjchen
no flags Details | Diff
sample for field code contains '/' or '.' (21.50 KB, application/msword)
2012-07-30 07:32 UTC, zjchen
no flags Details
revised patch for i119446 (1.09 KB, patch)
2012-08-13 11:23 UTC, zjchen
orw: review-
Details | Diff
attached test case for reference (226.50 KB, application/msword)
2012-08-13 11:27 UTC, zjchen
no flags Details
patch for i119446 (1.15 KB, patch)
2012-08-21 11:27 UTC, zjchen
orw: review+
Details | Diff

Note You need to log in before you can comment on or make changes to this issue.
Description Yan Ji 2012-05-28 08:45:28 UTC
Created attachment 77631 [details]
sample

Build: AOO3.4

Load sample file and compare with MS Office

Defect: The formula field is lost
Comment 1 zjchen 2012-07-30 07:10:16 UTC
The problem is that there is a code change for defect #124725# skipped all the field codes which contain '/' or '.'. This defect number could NOT be tracked in AOO Bugzilla and the change makes regression of field import. See main\sw\source\filter\ww8\ww8par5.cxx Line 989 for more details.

For the field types which could not be depicted in WW8 filter. these could be imported as a plain text or just ignored. Imported formula field as main text will retain the information integrity of document for this issue.
Comment 2 zjchen 2012-07-30 07:13:18 UTC
It seems that roll back part of #124725# change also not impact the field code are not displayed in MS Word. Attached another sample file for my test scenario
The field code in this file will not be displayed after imported by Writer.
Comment 3 zjchen 2012-07-30 07:28:34 UTC
Created attachment 78775 [details]
patch for i119446
Comment 4 zjchen 2012-07-30 07:32:51 UTC
Created attachment 78776 [details]
sample for field code contains '/' or '.'
Comment 5 Oliver-Rainer Wittmann 2012-08-09 08:56:12 UTC
taking over to review the patch
Comment 6 Oliver-Rainer Wittmann 2012-08-09 11:15:11 UTC
#124725# is a number of the internal bug tracking system from Sun Microsystems. 
I assume that the change for #124725# makes sense in certain situations. But the change was too less restrictive. Thus, the test on " ADDIN" was added later. I think that this test has to be generalized. If a "/" or ".", but a space was found before than fix for #124725# should not apply.

@zjchen:
(1) Are you fine with my above suggestion for changing the fix for #124725#?
(2) I did not see any fields or field content in the sample file which you have attached in AOO (with and without your patch), in MS Word 2003 or in MS Word 2010.
Can you help me here?
Comment 7 zjchen 2012-08-09 12:14:26 UTC
@orw: Thanks to your response.

(1)Noticed that the comment of #124725# is "field codes which contain '/' or '.' are not displayed in WinWord", so I assume the change for #124725# is to make the field code which contain '/' or '.' NOT displayed in AOO too, like what you can see in my test case. see my response for your question (2) :). 

ADDIN is widely used in References feature and usually contain lots of  '/' or '.'. Adding exception of ADDIN is to keep the reference integrity of document from my point of view.  Given that there is no function to handle this field in WW8 filter, AOO is still able to parse the reference as plain text and shown correctly.

However, ADDIN is not the ONLY exception case for those field code which contain '/' or '.' also LINK field ,see #i119585. and this issue which is more general.

(2) Press Shortcut key “ALT + F9”when you open the test case in MS WORD, either 2003 or 2010. The field code display will be enable and you would find the hidden field code behind the text. It should be "Test field whose code contains a / {/test file code/}" and "Test field whose code contains a .  {.test file code.}". Press the shortcut key again, the field code display will be disable. the hidden field will be disappeared from main text. 

With and without this patch, the hidden field would not be shown in AOO,so I think the code change of #124725# is obsoleted if my case is what it really want to fix.
Comment 8 zjchen 2012-08-10 04:51:24 UTC
@Oliver 
For (1), searching space position before fix for #124725# make sense, it solve this issue properly.
Comment 9 Oliver-Rainer Wittmann 2012-08-10 06:07:22 UTC
@zjchen:
I want to ask you, if you want to work on a revised patch?
If not, assign this issue back to me.
Comment 10 zjchen 2012-08-10 06:14:54 UTC
@Oliver
Yes, I'm trying to work out a solution for this issue regarding your suggestion
Comment 11 Oliver-Rainer Wittmann 2012-08-13 09:50:09 UTC
Comment on attachment 78775 [details]
patch for i119446

removing request for review flag as zjchen is working on a revised patch
Comment 12 zjchen 2012-08-13 11:23:29 UTC
Created attachment 78918 [details]
revised patch for i119446

output field content as plain text if it is formula field or found space before '.' or '/' in field code.
Comment 13 zjchen 2012-08-13 11:27:37 UTC
Created attachment 78919 [details]
attached test case for reference

import formula field as plain text for case 2 in this sample file.
Comment 14 Oliver-Rainer Wittmann 2012-08-17 08:26:57 UTC
taking over for patch review
Comment 15 Oliver-Rainer Wittmann 2012-08-21 09:49:44 UTC
Comment on attachment 78918 [details]
revised patch for i119446

Thx for the revised patch - I have finished my review.

I have some remarks:
(1) The conditions which are checked are 
- A == "it is not a formula field"
- B == ". found before space"
- C == "/ found before space"
The if statement in the patch is ( A && B || C ).
I think it should be ( A && ( B || C ) )
Otherwise a formula field which contains a / before a space would not be read.

(2) If no space character is found, variable <nSpacePos> will equal to <STRING_NOTFOUND>. Currently, the value of <STRING_NOTFOUND> is 0xFFFF or 0x7FFFFFFF and thus, the checks work as expected. But I think we should explicit check such a situation, because the checks should not rely on certain assumptions of the value of <STRING_NOTFOUND> which could be changed in the future.

@zjchen: Please revise your patch according to my remarks. Check, if my remark (1) is correct or not. Thanks in advance.
Comment 16 Oliver-Rainer Wittmann 2012-08-21 09:51:19 UTC
@zjchen: If you not able to work on a revised patch, please assign this issue back to me.
Comment 17 zjchen 2012-08-21 11:27:33 UTC
Created attachment 79035 [details]
patch for i119446

@orw
  
  Your remarks for (1) is correct. Sorry for my typo mistake, It should be ( A && ( B || C )).
 
  For (2) more safe check is reasonable. So, use field code string length to check in this situation.
Comment 18 Oliver-Rainer Wittmann 2012-08-21 12:20:16 UTC
cool, thx for the fast reply.

taking over for reviewing the patch.
Comment 19 Oliver-Rainer Wittmann 2012-08-21 12:57:54 UTC
Comment on attachment 79035 [details]
patch for i119446

review finished.
patch looks good and solves the problem.
--> I will apply it on trunk
Comment 20 SVN Robot 2012-08-21 13:28:53 UTC
"orw" committed SVN revision 1375536 into trunk:
#119446# - method <SwWW8ImplReader::Read_Field(..)> - correct condition for f...
Comment 21 Oliver-Rainer Wittmann 2012-08-21 13:31:57 UTC
fixed on trunk.
Comment 22 Du Jing 2012-08-28 03:17:35 UTC
verified on the build 1377620
Comment 23 zjchen 2012-09-19 08:34:58 UTC
*** Issue 119585 has been marked as a duplicate of this issue. ***
Comment 24 Shenfeng Liu 2012-10-18 06:37:08 UTC
Update Target Milestone to AOO 3.5.0.