Fix docx import of wrap 'Square' for groups

[Regina Henschel]

Hi Miklos, hi all,

I have adapted some tests now and added some new ones.

Do you have some time to review the patch?
https://gerrit.libreoffice.org/c/core/+/114193


My patch excludes rotated groups from scaling by
          if (m_pImpl->isXSizeValid())
              aSize.Width = m_pImpl->getXSize();
          if (m_pImpl->isYSizeValis())
              aSize.Height = m_pImpl->getYSize();
	 m_xShape->setSize(aSize);
in
case NS_ooxml::LN_shape:
in GraphicImport::lcl_attribute()
and still allows scaling of unrotated groups like that from 
testGroupShapeRotation in GraphicImport.cxx

But I wonder, why such scaling is needed at all. When the shape is 
inserted by Shape::createAndInsert in oox/source/drawingml/shape.cxx,
then it should have the correct size. I thought GraphicImport in 
writerfilter would only set position, margins and wrap related things as 
needed for Writer. Impress uses the same resulting shape from oox 
without any scaling.

Nevertheless I suggest to use the patch unless someone has concerns.

Kind regards
Regina


Miklos Vajna schrieb am 16.04.2021 um 09:31:
> Hi Regina,
> 
> On Fri, Apr 16, 2021 at 12:42:28AM +0200, Regina Henschel <rb.henschel at t-online.de> wrote:
>> Now I stuck, because some unit tests fail with small differences. But before
>> I put a lot of time into it, to find whether the values in the unit tests
>> are wrong or whether and where my patch is incorrect, I would like, if you
>> have a look at the patch in general.
> 
> The approach looks reasonable to me in general. As I understand it, the
> LO group shape is a simple container of shapes (with a name), while the
> MSO group shape can have its own rotation, scaling, etc -- so their
> document model can be more complex. Based on this, it's expected that
> you may need the "original" rotation of the group shape in
> writerfilter/.
> 
> Regarding the unit tests: I try to always document how a test failed
> without the original fix. So in case you see that e.g. now the test
> fails because of a 98 vs 99 difference and the comment says the old
> value was 42, then this may suggest that only the test was poor and it
> can be adapted.
> 
> You can always manually compare the import result of a particular test
> document with and without your fix: if you see no differences, then that
> also suggests that you are OK to change the test.
> 
> Otherwise it's likely that the test caught some badness and you need to
> improve your patch.
> 
> Hope that helps,
> 
> Miklos
>