Issue 119607 - [From Symphony] Text font spacing in comments doesn't expand/condense by the expected value
[From Symphony] Text font spacing in comments doesn't expand/condense by the ...
Status: VERIFIED FIXED
Product: Writer
Classification: Application
Component: open-import
3.4.0
PC All
: P3 normal (vote)
: 4.0.0
Assigned To: AOO issues mailing list
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-31 02:11 UTC by xiao ting xiao
Modified: 2012-10-08 06:24 UTC (History)
4 users (show)

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


Attachments
wrong font spacing in commont (11.95 KB, application/vnd.openxmlformats-officedocument.wordprocessingml.document)
2012-05-31 02:11 UTC, xiao ting xiao
no flags Details
screenshot of wrong font spacing in comment (116.48 KB, image/jpeg)
2012-05-31 02:11 UTC, xiao ting xiao
no flags Details
patch for font spacing in comments for docx (4.67 KB, patch)
2012-08-27 11:14 UTC, bjcheny
no flags Details | Diff
patch for font spacing in comments for docx (7.57 KB, patch)
2012-09-03 09:47 UTC, bjcheny
no flags Details | Diff
patch for font spacing in comments for docx (6.77 KB, patch)
2012-09-04 07:59 UTC, bjcheny
zhangjf: review+
Details | Diff

Note You need to log in before you can comment on or make changes to this issue.
Description xiao ting xiao 2012-05-31 02:11:16 UTC
Created attachment 77824 [details]
wrong font spacing in commont

Steps:
1. Open the attached docx files.
2. Check text font spacing in the comments.
 Defect: Their font spacing in sym20 don't expand/condense to the value set in office2010.
Comment 1 xiao ting xiao 2012-05-31 02:11:58 UTC
Created attachment 77825 [details]
screenshot of wrong font spacing in comment
Comment 2 Du Jing 2012-07-11 09:29:16 UTC
confirmed,change its status
Comment 3 bjcheny 2012-08-27 11:14:17 UTC
Created attachment 79175 [details]
patch for font spacing in comments for docx

The font spacing in comments is calculated in different way from that out of comments.

Thus, the patch includes below changes:
1. add related token id in model.xml to ensure comments will be detected
2. add a flag to indicate whether the font spacing is for comments or not.
Comment 4 bjcheny 2012-08-29 08:48:53 UTC
This solution isn't good enough. Working on a better one.

(In reply to comment #3)
> Created attachment 79175 [details]
> patch for font spacing in comments for docx
> 
> The font spacing in comments is calculated in different way from that out of
> comments.
> 
> Thus, the patch includes below changes:
> 1. add related token id in model.xml to ensure comments will be detected
> 2. add a flag to indicate whether the font spacing is for comments or not.
Comment 5 Yolanda Zhang Ying 2012-08-30 02:16:05 UTC
(In reply to comment #4)
> This solution isn't good enough. Working on a better one.
> 
> (In reply to comment #3)
> > Created attachment 79175 [details]
> > patch for font spacing in comments for docx
> > 
> > The font spacing in comments is calculated in different way from that out of
> > comments.
> > 
> > Thus, the patch includes below changes:
> > 1. add related token id in model.xml to ensure comments will be detected
> > 2. add a flag to indicate whether the font spacing is for comments or not.

Yes, aggree.
It's not so good to justify the properties by add the special flag for detect of in-comments or not, because for docx import, the import filter work as state machine, and the state will be chaos if we add more and more special flag.
I think better way is to separate the properties for different use scenario by definition through parent-child relationship in OOXML.
Comment 6 bjcheny 2012-09-03 09:42:06 UTC
Thanks for your comments.
I tried to work out a solution by tracking tokens' parent and change the spacing to use different token from that in document.xml, however, it doesn't work so well in this case.
In model.xml, it will be necessary to re-write the whole comments part since it shares same structure with that in document. In order to use a different token for spacing in comments, I don't think it's worthwhile the effort.

But I do agree with you on that it's bad to add some non-existing token id.
In new patch, I move the flag into DomainMapper_Impl, and set it to true in PushAnnotation and to false in PopAnnotation.

(In reply to comment #5)
> (In reply to comment #4)
> > This solution isn't good enough. Working on a better one.
> > 
> > (In reply to comment #3)
> > > Created attachment 79175 [details]
> > > patch for font spacing in comments for docx
> > > 
> > > The font spacing in comments is calculated in different way from that out of
> > > comments.
> > > 
> > > Thus, the patch includes below changes:
> > > 1. add related token id in model.xml to ensure comments will be detected
> > > 2. add a flag to indicate whether the font spacing is for comments or not.
> 
> Yes, aggree.
> It's not so good to justify the properties by add the special flag for
> detect of in-comments or not, because for docx import, the import filter
> work as state machine, and the state will be chaos if we add more and more
> special flag.
> I think better way is to separate the properties for different use scenario
> by definition through parent-child relationship in OOXML.
Comment 7 bjcheny 2012-09-03 09:47:32 UTC
Created attachment 79280 [details]
patch for font spacing in comments for docx

The font spacing in comments is calculated in different way from that out of comments.

Add related flag to ensure comments will be detected
Comment 8 bjcheny 2012-09-04 07:59:24 UTC
Created attachment 79282 [details]
patch for font spacing in comments for docx

Re-upload new clean patch.
Comment 9 SVN Robot 2012-09-04 08:12:13 UTC
"zhangjf" committed SVN 0 into trunk:
#i119607#, text font spacing in comments doesn't expand/condense by the expec...
Comment 10 zhang jianfang 2012-09-04 08:13:13 UTC
Comment on attachment 79282 [details]
patch for font spacing in comments for docx

The patch works fine, thanks.
Comment 11 zhang jianfang 2012-09-04 08:14:29 UTC
Committed to 3.5 trunk and change to resolved state.
Comment 12 Du Jing 2012-10-08 06:24:22 UTC
verified on the build Aoo3.5_r1387482