Re: tdf#74702 2/2

On 8 Jul 2019, at 9:06 pm, Tomaž Vajngerl <quikee@gmail.com> wrote:
> Hi,
>
> On 06.07.19 19:59, Adrien Ollier wrote:
> ....
>
> Well IMHO the problem that you even have to think about this is that OutputDevice is a enormous 
> class, and then you have to deal with another even more enormous subclass vcl::Window, which 
> should never be a subclass of OutputDevice in the first place. However the work to change that is 
> quite big and non-trivial. Once that is done I'm sure the need that a user needs to know with 
> what subclass of OutputDevice it deals with will be mostly gone. Until then I'm also comfortable 
> with the status quo and still have the enum and work with conditions for the outside use. From 
> inside the hierarchy of course it is better to use polymorphism. 
>
> Regards, Tomaž

I have to agree with this. In actual fact, I have long thought that OutputDevice is a misnomer 
anyway, because:

1. It’s not a “device”
2. It deals with things other than output - it also handles input!

I like the idea of gradually moving it to RenderContext.

The class definitely needs splitting up, but it’s very difficult to do as so many other classes use 
it and rely on it. 

But getting back to the bug: I have been looking at each area and I’m starting with some 
low-hanging fruit to start with:

https://gerrit.libreoffice.org/#/c/75521/ <https://gerrit.libreoffice.org/#/c/75521/> - changes 
GetBackground().GetColor() to GetBackgroundColor() - turns out that this allows us to remove at 
least one area that removes the need for GetOutDevType in SmTmpDevice::Impl_GetColor()

https://gerrit.libreoffice.org/#/c/75524/ <https://gerrit.libreoffice.org/#/c/75524/> - further 
follows this up by introducing a new function GetReadableFontColor() and overrides this for Printer

https://gerrit.libreoffice.org/#/c/75556/ <https://gerrit.libreoffice.org/#/c/75556/> - moves some 
code into Window::SaveBackground() and creates an OutputDevice::SaveBackground() for all other 
cases. 

https://gerrit.libreoffice.org/#/c/75616/ <https://gerrit.libreoffice.org/#/c/75616/> - makes 
Flush() a virtual function introduced in OutputDevice - it’s a noop as there is no need to do any 
flushes in OutputDevice but if any subclasses need to flush their buffers then it can be 
overridden. 

https://gerrit.libreoffice.org/#/c/75641/ <https://gerrit.libreoffice.org/#/c/75641/> makes 
ExpandPaintClipRegion() and virtual function of OutputDevice

These are all small changes, I’m hoping that they are reasonable!

I’m fairly confident that we can actually start to eliminate GetOutDevType() without resorting to 
dynamic_cast<> hackiness. 

There are a few hairy bits of the code (in particular svx) that are going to be harder to refactor, 
but again, in time I think we can resolve this. 

Ultimately, if we can seperate OutputDevice from Window, VirtualDevice and Printer then I’m pretty 
certain we can make OutputDevice smaller and more focussed, and eventually even get to the Holy 
Grail of making it a true RenderContext. 

Chris