Re: tdf#74702 2/2 — Libreoffice

On 8 Jul 2019, at 9:06 pm, Tomaž Vajngerl <quikee@xxxxxxxxx> 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/ - 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/ - further follows this up by introducing a new function GetReadableFontColor() and overrides this for Printer

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/ - 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/ 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