Fixing endless warnings on GTK 3.14

[Colomban Wendling]

Hi,

GTK 3.14 (current development version) will (likely) make Scintilla emit

Gdk-WARNING **: gdk_cairo_create called from outside a paint. Make sure to call gdk_window_begin_paint_region before calling gdk_cairo_create!

Yaykes. See
https://mail.gnome.org/archives/gtk-devel-list/2014-June/msg00010.html

The good news is, it may not be so hard to fix actually.

The reason we see these warning is because GTK implementation of
SurfaceImpl::Init(WindowID) creates a drawing context (hence calling
gdk_cairo_create()) unconditionally, even for surfaces only used for
measurements (like the one used in Editor::LocationFromPosition() --
method indirectly called on mouse movements).

So, what I suggest would be to delay the creation of the actual drawing
context (and the call to gdk_cairo_create()) until we actually want to
draw something on the surface.

Attached is a (somewhat ugly) patch doing this, which seems to get rid
of all these new warnings we had (or at least, that I could trigger).
It should be completely safe in any situations anyway, as it only delays
the creation of this internal context, it doesn't remove it.

If the concept is acceptable but patch is too ugly, I'm willing to
improve the actual implementation if you want, just tell me.

Regards,
Colomban

[Neil Hodgson]

Post by Colomban Wendling
So, what I suggest would be to delay the creation of the actual drawing
context (and the call to gdk_cairo_create()) until we actually want to
draw something on the surface.

That still means that there will probably be warnings when creating off-screen pixmaps since that uses the context from a window based surface.

Post by Colomban Wendling
Attached is a (somewhat ugly) patch doing this,

The attachment didn't arrive.

Neil

[Colomban Wendling]

Post by Neil Hodgson

Post by Colomban Wendling
So, what I suggest would be to delay the creation of the actual
drawing context (and the call to gdk_cairo_create()) until we
actually want to draw something on the surface.

That still means that there will probably be warnings when creating
off-screen pixmaps since that uses the context from a window based
surface.

In which situation does this happen? Also, I'd think we could/should
use something like gdk_window_create_similar_surface() in such
situation, shouldn't we?

Post by Neil Hodgson

Post by Colomban Wendling
Attached is a (somewhat ugly) patch doing this,

The attachment didn't arrive.

Is it here now?

Regards,
Colomban

[Neil Hodgson]

Post by Colomban Wendling

Post by Neil Hodgson
That still means that there will probably be warnings when creating
off-screen pixmaps since that uses the context from a window based
surface.

In which situation does this happen? Also, I'd think we could/should
use something like gdk_window_create_similar_surface() in such
situation, shouldn't we?

While gdk_window_create_similar_surface is used inside InitPixMap, the context from the surface argument is used in the pango_cairo_update_context call that you added in change set 4322.

https://sourceforge.net/p/scintilla/code/ci/74c71632dd1afa726b0f1608d13413e0864da9b0/

Actually it looks like all calls to InitPixMap are when painting so will be OK. I thought that the pattern pixmaps were used for printing but they are not.

Since non-painting surfaces should never be drawn on, then maybe the right thing to do is just fail if they ever are. Possibly just have a NULL context when the Surface was created from just a window. Then assert(context) at the start of all drawing methods or if (!context) return.

Scintilla should only be calling text measurement methods on a Surface created from just a window.

Post by Colomban Wendling
Is it here now?

Yes.

Neil

[Colomban Wendling]

Post by Neil Hodgson

Post by Colomban Wendling

Post by Neil Hodgson
That still means that there will probably be warnings when
creating off-screen pixmaps since that uses the context from a
window based surface.

In which situation does this happen? Also, I'd think we
could/should use something like gdk_window_create_similar_surface()
in such situation, shouldn't we?

While gdk_window_create_similar_surface is used inside InitPixMap,
the context from the surface argument is used in the
pango_cairo_update_context call that you added in change set 4322.
https://sourceforge.net/p/scintilla/code/ci/74c71632dd1afa726b0f1608d13413e0864da9b0/

This can probably be changed to use the context crated for the new
surface (e.g. moving it after the cairo_create(psurf)), it would
probably be better, although probably won't change a thing as the
surface is similar enough.

The point of the above-linked change was in case the surface was not the
widget's one (like when printing), since the Pango context is created by
gtk_widget_create_pango_context() and then setup for the widget, not
necessarily for the passed-in surface.

But anyway in both cases it uses a passed-in surface, it doesn't itself
call gdk_caiuro_create(), so should be just fine. I tested it quickly
and apparently it results in identically-looking prints.

Post by Neil Hodgson
Actually it looks like all calls to InitPixMap are when painting so
will be OK. I thought that the pattern pixmaps were used for printing
but they are not.
Since non-painting surfaces should never be drawn on, then maybe the
right thing to do is just fail if they ever are. Possibly just have a
NULL context when the Surface was created from just a window. Then
assert(context) at the start of all drawing methods or if (!context)
return.
Scintilla should only be calling text measurement methods on a
Surface created from just a window.

Yes, apparently we properly always pass the cairo context when we
actually draw, even in ScintillaGTK::ExposeTextThis() we manually call
gdk_cairo_create() and pass that context, so we should be all good.

Attached is a patch that simply leaves the surface and Cairo context
NULL, and that asserts it's never used in such case. I'll run it and
see how it goes, but quick testing seems to show it's indeed not a problem.

Also, many surface methods actually check whether the context is
non-null and only do anything in that case, so maybe instead of
PLATFORM_ASSERT()s we should simply add similar checks where they are
missing?

Regards,
Colomban

[Colomban Wendling]

Post by Neil Hodgson
[...]
Scintilla should only be calling text measurement methods on a
Surface created from just a window.

They reverted it [1], so this is not so important to fix this anymore.
However, they still consider it a bad thing to do, and is unsupported at
least on Wayland (actually, it's rather only really supported on X11
apparently).

And anyway if indeed the Cairo context is never used, I think it should
be better not to create it. Maybe however Surface::Init() should rather
be given an extra measurementOnly parameter or something so it's clear
in which case it is used for measurement purposes only and the
implementation can easily perform any non-drawing optimization it wants,
without having to make a guess?

Regards,
Colomban


[1] https://mail.gnome.org/archives/gtk-devel-list/2014-June/msg00041.html

[Neil Hodgson]

Post by Colomban Wendling
Attached is a patch that simply leaves the surface and Cairo context
NULL, and that asserts it's never used in such case. I'll run it and
see how it goes, but quick testing seems to show it's indeed not a problem.

Since the context is not being created, createdGC should be left (or set) false. Otherwise, at destruction, cairo_destroy is called on the context which doesn't fail on testing but the documentation doesn't explicitly say that cairo_destroy(NULL) is OK.

Post by Colomban Wendling
And anyway if indeed the Cairo context is never used, I think it should
be better not to create it. Maybe however Surface::Init() should rather
be given an extra measurementOnly parameter or something so it's clear
in which case it is used for measurement purposes only and the
implementation can easily perform any non-drawing optimization it wants,
without having to make a guess?

Adding a parameter (or method) to the platform interface means every platform implementation has to be updated.

You can trust the cross-platform code to never try and draw directly on a window since its not possible on some platforms. If drawing on a window outside paint events is ever useful (possibly scrolling or interaction with a 3D API) then the platform layer will be in control of the actions.

Neil

[Neil Hodgson]

Committed Columban's patch as
https://sourceforge.net/p/scintilla/code/ci/ce16b76af6652df879c21883d657ac6749889bc0/
Set createdGC false with commit
https://sourceforge.net/p/scintilla/code/ci/a90e4219593b2c6c8ed5237bb5a593ff4ad0445d/

BTW, the first Surface::Init used for measurement surfaces originally had no argument - the WindowID was added for Pango.

Neil

[Colomban Wendling]

Post by Neil Hodgson

Post by Colomban Wendling
Attached is a patch that simply leaves the surface and Cairo
context NULL, and that asserts it's never used in such case. I'll
run it and see how it goes, but quick testing seems to show it's
indeed not a problem.

Since the context is not being created, createdGC should be left (or
set) false. Otherwise, at destruction, cairo_destroy is called on the
context which doesn't fail on testing but the documentation doesn't
explicitly say that cairo_destroy(NULL) is OK.

Good catch, I forgot that when doing this patch, sorry -- and since it
didn't blow up in my face I didn't notice.

Post by Neil Hodgson

Post by Colomban Wendling
And anyway if indeed the Cairo context is never used, I think it
should be better not to create it. Maybe however Surface::Init()
should rather be given an extra measurementOnly parameter or
something so it's clear in which case it is used for measurement
purposes only and the implementation can easily perform any
non-drawing optimization it wants, without having to make a guess?

Adding a parameter (or method) to the platform interface means every
platform implementation has to be updated.

OK, so I guess it indeed isn't worth it.

Post by Neil Hodgson
You can trust the cross-platform code to never try and draw directly
on a window since its not possible on some platforms. If drawing on a
window outside paint events is ever useful (possibly scrolling or
interaction with a 3D API) then the platform layer will be in control
of the actions.

OK, that's definitely good as indeed more and more platforms don't like
getting their draw model being abused.

Regards,
Colomban