[REVIEW][3-5] Re: [[REVIEW][3-5-2] fdo#47717, fdo#45562 sw: yet more border painting regressions

[Michael Stahl]

On 26/03/12 11:42, Cedric Bosdonnat wrote:
> Hi Michael,
> 
> On Fri, 2012-03-23 at 17:44 +0100, Michael Stahl wrote:
>> On 20/03/12 12:19, Cedric Bosdonnat wrote:
>>> On Fri, 2012-03-16 at 23:29 +0100, Michael Stahl wrote:
>>>> this fix introduces a new array to store the borders and paints them
>>>> after the subsidiary lines are done, effectively on top of the
>>>> subsidiary lines.
>>>>
>>>> http://cgit.freedesktop.org/libreoffice/core/commit/?id=804d0a896731629397c5328c13c04a45bc55f459
>>>
>>> Thanks for the patch. I apologize as I should have done that a lot
>>> earlier. I cherry-picked and pushed it to -3-5.
>>
>> unfortunately it turns out that the patch introduced a regression,
>> fdo#47717, which is hopefully fixed with this one (as is fdo#45562,
>> which is about borders vs. hellish drawing objects), so please consider
>> it for libreoffice-3-5-2:
>>
>> http://cgit.freedesktop.org/libreoffice/core/commit/?id=1024c172a5bfb3d85a86fcf7a046aa2b03950edd
>>
>> because up until a week ago my knowledge of Writer's drawing code was
>> precisely zero, it would be a good idea to test this a bit.  in case
>> something is still wrong i'd strongly consider reverting the original
>> commit (0f0896c26fb260d1bbf31d7a886df3f61837f0f2).
> 
> The patch fixes the mentioned bugs, but I still found a buggy case. See
> this document:
> http://people.freedesktop.org/~cbosdo/frame-to-back.odt

thanks, i think i've fixed this problem now (and attached your bugdoc,
slightly enhanced, to fdo#45562 as well), so please consider this for
libreoffice-3-5, which basically results in painting the borders in
almost exactly the same places as the old OOo code, and thus looks safe
to me wrt. layering:

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

there is also some old border painting code in SwFlyFrmFmt::MakeGraphic,
which looks unnecessary to me because the borders are painted by
SwFlyFrm::Paint already; the patch also happens to fix the following
obscure regression introduced by my earlier patches:

1. create new writer document
2. Insert->Frame
3. OK
4. Edit->Image Map
5. crash

> IMHO, the best effort we could do on that border / subsidiary lines
> painting is to avoid buffering any line / subline. I'm still unsure if
> it's possible at all without having weird corner cases. I still believe
> in "each objects paints itself completely in one shot" idea, but that
> would mean to remove all the subsidiary lines code and consider those as
> the normal lines.
> 
> Any thought on that? I we go in that direction, I'ld revert the patch as
> you mention in 3.5.2 and target that work for 3.6: it's still a bit
> risky and will need heavy testing.

i don't believe this is possible, as there is code that combines
adjacent borders if there is a sufficiently small gap between them (see
SwLineRect::MakeUnion), which is now mostly unused since
0f0896c26fb260d1bbf31d7a886df3f61837f0f2, and results in regressions
such as fdo#38215, about which i'll send another mail.