Bug Hunting Session
Bug 117145 - Crop Outline does not match Crop Area
Summary: Crop Outline does not match Crop Area
Status: VERIFIED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Writer (show other bugs)
Version:
(earliest affected)
6.1.0.0.alpha0+
Hardware: All All
: medium normal
Assignee: Armin Le Grand
URL:
Whiteboard: target:6.1.0
Keywords: bibisected, bisected, regression
Depends on:
Blocks: AW080-Regressions
  Show dependency treegraph
 
Reported: 2018-04-21 15:58 UTC by Luke
Modified: 2018-05-04 08:43 UTC (History)
6 users (show)

See Also:
Crash report or crash signature:


Attachments
Screenshot of problem (119.87 KB, image/png)
2018-04-21 16:00 UTC, Luke
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Luke 2018-04-21 15:58:35 UTC
The crop outline is broken in the Writer module of recent builds of LibreOffice.

Steps to reproduce:
1. Open a Sample document with a cropped image such as attachment 106084 [details] 
2. Right click -> Crop
3. Attempt to crop image by dragging one of the crop handles

Expected Results
1. Crop Area is inside of image

Actual Results 
1. Crop Area is outside image area


Version: 6.1.0.0.alpha0+
Build ID: 94acabe8d0cb555e76635c4bceeb48a14bd16a2b
BAD


Version: 6.1.0.0.alpha0+
Build ID: ffc49bd2fba97659919d9232ae42ca675c1aa9d0
GOOD
Comment 1 Luke 2018-04-21 16:00:57 UTC
Created attachment 141525 [details]
Screenshot of problem
Comment 2 Jacques Guilleron 2018-04-22 09:56:46 UTC
Hi Luke,
I reproduce with
LO 6.1.0.0.alpha0+ Build ID: f80029445e2b558f0d0e0a25c2c1bbcbe5254120
CPU threads: 2; OS: Windows 6.1; UI render: default; 
TinderBox: Win-x86@42, Branch:master, Time: 2018-04-13_23:07:11
Locale: fr-FR (fr_FR); Calc: CL
But not with
LO 6.1.0.0.alpha0+ Build ID: dd4f1b1bd31daf080dc0420524712dc244e539b5
CPU threads: 2; OS: Windows 6.1; UI render: default; 
TinderBox: Win-x86@42, Branch:master, Time: 2018-03-20_23:26:38
Locale: en-ZA (fr_FR); Calc: CL
Can you provide the dates of the quoted versions?
Thank you.
Comment 3 Xisco Faulí 2018-04-23 15:20:32 UTC
Regression introduced by:

author	Armin Le Grand <Armin.Le.Grand@cib.de (CIB)>	2018-03-01 15:54:32 +0100
committer	Armin Le Grand <Armin.Le.Grand@cib.de>	2018-04-07 00:28:30 +0200
commit	dfefe448c41921f2f1e54d3f69b8b9e89031d055 (patch)
tree	1aace31054b5740e2faffcbc5de66a791be27f7d
parent	eba4d5b2b76cefde90cb3d6638c736f435023a45 (diff)
SOSAW080: Added first bunch of basic changes to helpers

Bisected with: bibisect-linux64-6.1
Comment 4 Armin Le Grand 2018-04-30 18:25:51 UTC
Taking a look...
Problem is that SwVirtFlyDrawObj::getFullDragClone() in Writer uses SdrTextObj::TRSetBaseGeometry at the cloned interation object, and that SdrTextObj::TRSetBaseGeometry tries to 'force' metric to pool metric. The pool metric befor the change that all SdrObjects have a SdrModel was fetched from GlobalDrawObjectItemPool and thus was 100thmm, while now gets correctly fetched from Writer's DrawItemPool and is Twips.
This shows that it did not work before and would have been wronf if it ever would have worked.
It also shows that 'someone' implies that input data to ::TRSetBaseGeometry is *always' 100thmm - this was never defined nor intended.
Thus the question is - why is this 'force' done here? Will need to find that out. In *some* circumstances someone seems to have used this (and the wrong imply) to fix/change/do something...
Comment 5 Armin Le Grand 2018-04-30 18:30:40 UTC
iT#s from myself, see 40c61e9ad1c7f. It refers to #83259#, hopefully will find out more.
Comment 6 Armin Le Grand 2018-04-30 18:35:10 UTC
That issue is not the correct one in bugzilla (see https://bz.apache.org/ooo/show_bug.cgi?id=83259), thinking about other old issue sources...
Comment 7 Luke 2018-04-30 23:14:11 UTC
Jan Holesovsky,
It looks like in https://cgit.freedesktop.org/libreoffice/core/commit/?id=8c4a1663f5d93380268365d35a5581d8065df897 
you implemented this feature. Any thoughts on this issue?
Comment 8 Armin Le Grand 2018-05-02 10:54:46 UTC
@luke: Reason already identified, no need for Jan to do anything.
Thinking aqbout how to solve this. There is a quick-fix possible, but solving the general problem would be better, esp. for future chanegs...
Comment 9 Armin Le Grand 2018-05-02 10:58:27 UTC
Quick note: GetObjectMapUnit is exactly used in TR.*etBaseGeometry, nowhere else.
Comment 10 Jan Holesovsky 2018-05-02 15:21:15 UTC
(In reply to Luke from comment #7)
> Jan Holesovsky,
> It looks like in
> https://cgit.freedesktop.org/libreoffice/core/commit/
> ?id=8c4a1663f5d93380268365d35a5581d8065df897 
> you implemented this feature. Any thoughts on this issue?

You are crediting the wrong guy - Philippe Jung has implemented the feature (thank you, Philippe!), I've only reviewed his patch :-)
Comment 11 Armin Le Grand 2018-05-02 16:10:50 UTC
After some thinking I have the following decisions:
- Adaptions to MapMode should not be supported by SdrModel/SdrObject level at all. It is not designed to do so, else e.g. all geometry/Rect setting/getting methods would also have to do that. Getting the SnapRect and getting the Transformatoin on that level *must not* return different values depending on MapMode decisions.
- MapMode comes into play in UNO API implementation. The UNO API is designed to completely work in 100ThMM, independent from the Application. Thus this implementation already uses quite some conversions. This is where MapMode conversions belong.

This means to:
- Remove all MapMode conversions from ::TRSetBaseGeometry and ::TRGetBaseGeometry, this are the wrong places
- Adapt SvxShape UNO API implementations where these are used to do the correct conversions
- Take a look at all other usages and evtl. apply if needed

Will no tbe easy, but will help in the future when we cleanup/go more to a clean homogen matrix solution.
Comment 12 Armin Le Grand 2018-05-02 16:27:49 UTC
Added adapted 1st version to gerrit, will see the tests running.
Comment 13 Commit Notification 2018-05-02 22:28:20 UTC
Armin Le Grand committed a patch related to this issue.
It has been pushed to "master":

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

tdf#117145 Cleanup MapMode handling between SdrModel/UNO API

It will be available in 6.1.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 14 Luke 2018-05-04 04:00:04 UTC
Verified FIXED in Version: 6.1.0.0.alpha1+ (x64)
Build ID: 55fcb23ea5caa509d8254910c581d234f7ec9199

Thanks for taking care of this Armin!