Issue 127897 - Possible access to unintended macro in "openoffice/main/cui/source/tabpages/textattr.cxx" line 247
Summary: Possible access to unintended macro in "openoffice/main/cui/source/tabpages/t...
Status: UNCONFIRMED
Alias: None
Product: General
Classification: Code
Component: code (show other issues)
Version: 4.2.0-dev
Hardware: All All
: P5 (lowest) Normal (vote)
Target Milestone: ---
Assignee: AOO issues mailing list
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-09-17 15:00 UTC by Petru-Florin Mihancea
Modified: 2021-01-13 19:36 UTC (History)
3 users (show)

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


Attachments

Note You need to log in before you can comment on or make changes to this issue.
Description Petru-Florin Mihancea 2018-09-17 15:00:02 UTC
While experimenting with a CodeSonar plugin we develop, we noticed a
potential issue in file "openoffice/main/cui/source/tabpages/textattr.cxx" line 247 function SvxTextAttrPage::Reset



// autogrowsize 
if ( rAttrs.GetItemState( SDRATTR_TEXT_AUTOGROWSIZE ) != SFX_ITEM_DONTCARE )
{
    aTsbAutoGrowSize.SetState( ( ( const SdrTextAutoGrowHeightItem& )rAttrs.Get( SDRATTR_TEXT_AUTOGROWHEIGHT ) ). //HERE
                              GetValue() ? STATE_CHECK : STATE_NOCHECK );
    aTsbAutoGrowSize.EnableTriState( sal_False );
}

Shouldn't SDRATTR_TEXT_AUTOGROWSIZE be used instead of SDRATTR_TEXT_AUTOGROWHEIGHT? 

Thanks,
Petru-Florin Mihancea
Comment 1 Arrigo Marchiori 2021-01-11 18:45:57 UTC
Hello Petru-Florin,

thank you for your report.

Apparently this is not a bug.
Or at least, it's a ``known feature'' around which some code is designed.

File main/svx/source/sdr/properties/customshapeproperties.cxx at line 47:

47   // change TextFrame flag when bResizeShapeToFitText changes (which is mapped
48   // on the item SDRATTR_TEXT_AUTOGROWHEIGHT for custom shapes, argh)
49  rObj.bTextFrame = 0 != static_cast< const SdrTextAutoGrowHeightItem& >(GetObjectItemSet().Get(SDRATTR_TEXT_AUTOGROWHEIGHT)).GetValue();

That "argh" has been there at least since 2013...
Comment 2 Peter 2021-01-12 07:21:31 UTC
Hi Arrigo,

This Property is only valid for Custom Shapes. How does this work for Regular shapes? I find this Code a bit disturbing.

Please note that we have a pending PR for this: 
https://github.com/apache/openoffice/pull/32

I am not really satisfied with the situation. Argh is a good Comment.
If this is Correct we should at least add a comment in openoffice/main/cui/source/tabpages/textattr.cxx too.

All the best
Peter
Comment 3 Arrigo Marchiori 2021-01-12 18:08:51 UTC
(In reply to Peter from comment #2)
> Hi Arrigo,
> 
> This Property is only valid for Custom Shapes. How does this work for
> Regular shapes? I find this Code a bit disturbing.

Me too.

Looking at openoffice/main/cui/source/tabpages/textattr.cxx, in method SvxTextAttrPage::Construct(), the aTsbAutoGrowSize TriStateBox is enabled if bAutoGrowSizeEnabled is true; and it is true only in case of custom shapes. In those cases, the aTsbAutoGrowHeight TriStateBox is surely disabled.

At lines 728-730 aTsbAutoGrowHeight could be enabled, but the condition is always AND-ed with the variable bAutoGrowHeightEnabled set in the Construct() method above.

In other words, the "Auto-grow size" and "Auto-grow height" boxes are never enabled at the same time.

The reported "strange" mapping (i.e. aTsbAutoGrowSize set based on SDRATTR_TEXT_AUTOGROWHEIGHT) is exactly reversed at lines 424-428: an  SdrTextAutoGrowHeightItem is set based on the value of aTsbAutoGrowSize.

Petru-Florin may take this file as a good test case for his CodeSonar plugin: two inversions in the same file should have been both reported! ;-)

Apart from jokes, I believe that the second one is harder to spot because it involves data types and variable names instead of constants. At least, it took me quite some time to notice it!

[...]
> I am not really satisfied with the situation. Argh is a good Comment.
> If this is Correct we should at least add a comment in
> openoffice/main/cui/source/tabpages/textattr.cxx too.

I agree with you. Shall we close this bug and continue on the pull request?
Comment 4 Peter 2021-01-12 20:30:03 UTC
I would not close the Issue just yet. But we can continue on Github. That is fine.
Whereever we finalize this topic, I just add a short note on the other medium.
I think that will help us when we look at this and have no clue anymore what happend.
Comment 5 Arrigo Marchiori 2021-01-13 19:25:14 UTC
(In reply to Peter from comment #4)
> I would not close the Issue just yet. But we can continue on Github. That is
> fine.

If we continue here, we have the bug's original poster who can contribute.

If we continue on the PR, we have the PR issuer who can contribute.

As we started the discussion here, I would like best to try deciding here whether it is a bug or not. Then, once we are finished here, the discussion on the GitHub PR could be about the actual resolution (for example, what wording to use in comments etc.)
Comment 6 Arrigo Marchiori 2021-01-13 19:36:20 UTC
To add up more information to my thesis (that this is not a bug), I searched OpenGrok and found that:

 - SDRATTR_TEXT_AUTOGROWHEIGHT
   http://opengrok.openoffice.org/search?project=trunk&full=&defs=&refs=SDRATTR_TEXT_AUTOGROWHEIGHT&path=&hist=&type=&si=defs
   appears in 10 C++ source files, including WW8 documents filters.
   The corresponding property type, SdrTextAutoGrowHeightItem is used in many more files, some of which call its GetValue() method.
   http://opengrok.openoffice.org/search?project=trunk&full=&defs=&refs=SdrTextAutoGrowHeightItem&path=&hist=&type=&si=refs

 - SDRATTR_TEXT_AUTOGROWSIZE
   http://opengrok.openoffice.org/search?project=trunk&full=&defs=&refs=SDRATTR_TEXT_AUTOGROWSIZE&path=&hist=&type=&si=refs
   appears in 3 C++ source files, and it only seems as a property that is passed back and forth with dialogs.
   Even its property type, SdrTextAutoGrowSizeItem, has its GetValue() method never called.
   http://opengrok.openoffice.org/search?project=trunk&full=&defs=&refs=SdrTextAutoGrowSizeItem&path=&hist=&type=&si=refs

If there is a bug here, it is that the "auto-grow size" property is set but never used. The "argh" comment confirms this, i.e. that those shapes for which the property is important, map it on another value.