[SOLVED] Re: About GraphicDisplayCacheEntry::IsCacheableAsBitmap/GraphicManager::ImplCreateOutput

[Zolnai Tamás]

2015-07-12 13:02 GMT+02:00 Julien Nabet <serval2412 at yahoo.fr>:
> On 12/07/2015 12:47, Zolnai Tamás wrote:
>>
>> Hi Julien,
>>
>> 2015-07-12 0:44 GMT+02:00 julien2412 <serval2412 at yahoo.fr>:
>>>
>>> Hello,
>>>
>>> Giving a try to tdf#47832, I noticed that there were similar comments in
>>> these files:
>>> GraphicDisplayCacheEntry::IsCacheableAsBitmap:
>>>      487 // This function is based on GraphicManager::ImplCreateOutput(),
>>> in
>>> fact it mostly copies
>>>      488 // it, the difference is that this one does not create anything,
>>> it
>>> only checks if
>>>      489 // ImplCreateOutput() would use the optimization of using the
>>> single
>>> bitmap.
>>>      490 // If you do changes here, change the original function too.
>>> see
>>>
>>> http://opengrok.libreoffice.org/xref/core/svtools/source/graphic/grfcache.cxx#487
>>> and GraphicManager::ImplCreateOutput
>>>     1112 // NOTE: If you do changes in this function, check
>>> GraphicDisplayCacheEntry::IsCacheableAsBitmap
>>>     1113 // in grfcache.cxx too.
>>> see
>>>
>>> http://opengrok.libreoffice.org/xref/core/svtools/source/graphic/grfmgr2.cxx#1112
>>>
>>> But MetaActionType::FONT case isn't managed the same way:
>>> In the first, there's just a fallthrough,
>>> in the second one, there's some treatment.
>>>
>>> 1) Should we copy/paste the treatment in the first file?
>>> 2) Should we remove the treatment in the second file?
>>> 3) Should we just tweak the comment?
>>> or simply nothing at all?
>>
>> The code seems good to me.
>> GraphicDisplayCacheEntry::IsCacheableAsBitmap() is checks whether the
>> metafile can be displayed as a single bitmap. In ImplCreateOutput() I
>> see that MetaActionType::FONT is not handled as a bitmap (see
>> nNumBitmaps increment), so it's useless to copy that code.
>> If you check the IsCacheableAsBitmap()'s return value:
>>      return nNumBitmaps == 1 && !bNonBitmapActionEncountered;
>> you can see that non of these variables are affected by
>> MetaActionType::FONT case in ImplCreateOutput().
>> So I think we don't need any changes here.
>>
> Thank you for the explanation Zolnai! :-)
> (You didn't mention about tweaking comment so I suppose it doesn't worth it
> too)


Yeap, that's right. The existing comment seems good enough for me.

Tamás