This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102422/

kwin/composite.cpp (Diff revision 1)
void Toplevel::suspendUnredirect(bool suspend)
	800	// TODO: rename decorationPixmapsResized to something useful
	801	connect(decorationPaintRedirector, SIGNAL(decorationPixmapsResized()), this, SLOT(triggerDecorationRepaint()));

kwin/composite.cpp (Diff revision 1)

void Toplevel::suspendUnredirect(bool suspend)

800

        // TODO: rename decorationPixmapsResized to something useful

801

        connect(decorationPaintRedirector, SIGNAL(decorationPixmapsResized()), this, SLOT(triggerDecorationRepaint()));

*cough* :-)

kwin/decorationpaintredirector.h (Diff revision 1)
		class EffectWindowImpl;
		33	class DecorationPaintRedirector
		34	: public PaintRedirector

please add some documentation to the class

kwin/decorationpaintredirector.h (Diff revision 1)
		class EffectWindowImpl;
		41	bool decorationPixmapRequiresRepaint();
		42	void ensureDecorationPixmapsPainted();
		43
		44	enum CoordinateMode {
		45	DecorationRelative, // Relative to the top left corner of the decoration
		46	WindowRelative // Relative to the top left corner of the window
		47	};
		48
		49	// Decorations <-> Effects
		50	const QPixmap *topDecoPixmap() const {
		51	return &decorationPixmapTop;
		52	}
		53	const QPixmap *leftDecoPixmap() const {
		54	return &decorationPixmapLeft;
		55	}
		56	const QPixmap *bottomDecoPixmap() const {
		57	return &decorationPixmapBottom;
		58	}
		59	const QPixmap *rightDecoPixmap() const {
		60	return &decorationPixmapRight;

could we please drop a bunch of Deco and Pixmaps here? In Client that was needed, but I would like:

bool requiresRepaint() const
void ensurePainted() const

const QPixmap *top() const

and so on.

kwin/decorationpaintredirector.h (Diff revision 1)
		class EffectWindowImpl;
		78	QPixmap decorationPixmapLeft, decorationPixmapRight, decorationPixmapTop, decorationPixmapBottom;
		79	// we (instead of Qt) initialize the Pixmaps, and have to free them

please use consistent naming: m_variableName

kwin/decorationpaintredirector.cpp (Diff revision 1)
		DecorationPaintRedirector::~DecorationPaintRedirector()
		49	kDebug(1212) << "DecorationPaintRedirector killed!!";

please drop the debug statement - not needed.

kwin/decorationpaintredirector.cpp (Diff revision 1)
		DecorationPaintRedirector::~DecorationPaintRedirector()
		231	// if (Client* c = qobject_cast< Client* >(((Scene::Window*)parent())->window())) {
		232	// c->layoutDecorationRects(left, top, right, bottom, mode);
		233	// }

why is here the commented code?

kwin/deleted.cpp (Diff revision 1)
void Deleted::copyToDeleted(Toplevel* c)
		82	#if 0
82	if (!no_border) {	83	if (!no_border) {
83	client->layoutDecorationRects(decoration_left,	84	client->layoutDecorationRects(decoration_left,
84	decoration_top,	85	decoration_top,
85	decoration_right,	86	decoration_right,
86	decoration_bottom,	87	decoration_bottom,
87	Client::WindowRelative);	88	Client::WindowRelative);
88	decorationPixmapLeft = *client->leftDecoPixmap();	89	decorationPixmapLeft = *client->leftDecoPixmap();
89	decorationPixmapRight = *client->rightDecoPixmap();	90	decorationPixmapRight = *client->rightDecoPixmap();
90	decorationPixmapTop = *client->topDecoPixmap();	91	decorationPixmapTop = *client->topDecoPixmap();
91	decorationPixmapBottom = *client->bottomDecoPixmap();	92	decorationPixmapBottom = *client->bottomDecoPixmap();
92	}	93	}

please remove :-)

kwin/scene.cpp (Diff revision 1)
Scene::Window::Window(Toplevel * c)
		422	void Scene::Window::decorationChanged(KDecoration* decoration, NETWinInfo2* info)

I don't like the NETWinInfo2 here at all. I will try to think about how we can get rid off it.

kwin/scene.cpp (Diff revision 1)
Scene::Window::Window(Toplevel * c)
		426	kDebug(1212) << "NEW DECORATIONPAINTREDIRECTOR";

Please remove the debug statement

kwin/scene.cpp (Diff revision 1)
Scene::Window::Window(Toplevel * c)
	428	}
	429	else {

coding style:
if () {
} else {
}

- Martin

On August 24th, 2011, 3:48 p.m., Arthur Arlt wrote:

Review request for kwin.

By Arthur Arlt.

Updated Aug. 24, 2011, 3:48 p.m.

Description

This patch introduces the new class DecorationPaintRedirector. It inherits from PaintRedirector and covers the functionality for decoration handling when compositing is active. This functionality was moved from Client to this new class.
The reason for doing this is to get rid of the Client dependency in class Scene.
This is an initial review request. There are still some Client dependencies left. But it works so far except for changing the decoration in composited mode or removing no border. Doing this does not lead to a crash but the decorations are not updated. Newly added clients are displayed correctly with the new decoration. Should be not that hard to fix it; will be done the next days ;)

Testing

Works for both OpenGL and XRender on my system. However, as told in the description, changing the decoration (in composited mode)does not lead to a crash, but does not update the decorations for existing clients. Feel free to tell me the solution if it is obvious to you.

Diffs

kwin/CMakeLists.txt (bde6626)
kwin/client.h (d9cdddf)
kwin/client.cpp (9d86cc4)
kwin/composite.cpp (2b8ea6f)
kwin/decorationpaintredirector.h (PRE-CREATION)
kwin/decorationpaintredirector.cpp (PRE-CREATION)
kwin/deleted.cpp (f465159)
kwin/paintredirector.h (bb5d67c)
kwin/paintredirector.cpp (7ed4d6f)
kwin/scene.h (6ce39c0)
kwin/scene.cpp (8509400)
kwin/scene_opengl.cpp (cf3eb65)
kwin/scene_xrender.cpp (73dd22f)

View Diff