Unity

Merge lp://staging/~renatofilho/unity/unity-lp876017 into lp://staging/unity

unity-lp876017
Merge into trunk

Proposed by Renato Araujo Oliveira Filho on 2012-08-20

Status:

Merged

Approved by:

Renato Araujo Oliveira Filho on 2012-08-31

Approved revision:

no longer in the source branch.

Merged at revision:

2650

Proposed branch:

lp://staging/~renatofilho/unity/unity-lp876017

Merge into:

lp://staging/unity

Diff against target:

729 lines (+507/-31)

6 files modified

CMakeLists.txt (+1/-2)
plugins/unityshell/CMakeLists.txt (+1/-1)
plugins/unityshell/src/unityshell.cpp (+428/-0)
plugins/unityshell/src/unityshell.h (+39/-1)
unity-shared/PanelStyle.cpp (+37/-27)
unity-shared/PanelStyle.h (+1/-0)

To merge this branch:

bzr merge lp://staging/~renatofilho/unity/unity-lp876017

Medium

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Sam Spilsbury (community)		2012-08-20	Approve on 2012-08-30
Review via email: mp+120450@code.staging.launchpad.net

Commit message

UnityWindow now implements ScaleWindowInterface.
Implemented support to close window during the scale plugin.
Fake windows decoration rendering using panel code as base.

Description of the change

UnityWindow now implements ScaleWindowInterface.
Implemented support to close window during the scale plugin.
Fake windows decoration rendering using panel code as base.

Revision history for this message

Sam Spilsbury (smspillaz) wrote on 2012-08-21:

Download full text (3.7 KiB)

Right approach, some pointers:

76 +// Based on Scale plugin code
77 +CompWindow* UnityScreen::checkForWindowAt (int x, int y)
78 +{
79 + int x1, y1, x2, y2;
80 + CompWindowList::reverse_iterator rit = screen->windows ().rbegin ();
81 +
82 + for (; rit != screen->windows ().rend (); ++rit)
83 + {
84 + CompWindow *w = *rit;
85 + SCALE_WINDOW (w);
86 +
87 + ScalePosition pos = sw->getCurrentPosition ();
88 + if (sw->hasSlot ())
89 + {
90 + x1 = w->x () - w->input ().left * pos.scale;
91 + y1 = w->y () - w->input ().top * pos.scale;
92 + x2 = w->x () + (w->width () + w->input ().right) * pos.scale;
93 + y2 = w->y () + (w->height () + w->input ().bottom) * pos.scale;
94 + x1 += pos.x ();
95 + y1 += pos.y ();
96 + x2 += pos.x ();
97 + y2 += pos.y ();
98 + if (x1 <= x && y1 <= y && x2 > x && y2 > y)
99 + return w;
100 + }
101 + }
102 + return NULL;
103 +}

Is this necessary?

I think you can just overload scaleSelectWindow to determine what the currently hovered window is. From there you will need to damage and re-render both the new and old window. Have a look at the scaleaddon plugin.

151 + gWindow->geometry ().reset ();
152 + if (width && height)
153 + gWindow->glAddGeometry (ml, iconReg, iconReg);
154 +
155 + if (gWindow->geometry ().vCount)
156 + {
157 + GLFragment::Attrib fragment (attrib);
158 + GLMatrix wTransform (transform);
159 +
160 + wTransform.translate (x, y, 0.0f);
161 +
162 + glPushMatrix ();
163 + glLoadMatrixf (wTransform.getMatrix ());
164 + gWindow->glDrawTexture (icon, fragment, mask);
165 + glPopMatrix ();
166 + }

You will need to put this inside of #if USE_MODERN_COMPIZ_GL ifdefs. Sorry :( Here's how to do it:

#ifdef USE_MODERN_COMPIZ_GL
gWindow->vertexBuffer ()->begin ();
#else
151 + gWindow->geometry ().reset ();
#endif
152 + if (width && height)
153 + gWindow->glAddGeometry (ml, iconReg, iconReg);
154 +
#ifdef USE_MODERN_COMPIZ_GL
gWindow->vertexBuffer ().end ();
if (gWindow->vertexBuffer ().countVertices ())
#else
155 + if (gWindow->geometry ().vCount)
#endif
156 + {
#ifdef USE_MODERN_COMPIZ_GL
157 + GLFragment::Attrib fragment (attrib);
#endif
158 + GLMatrix wTransform (transform);
159 +
160 + wTransform.translate (x, y, 0.0f);
161 +
#ifdef USE_MODERN_COMPIZ_GL
gWindow->glDrawTexture (icon, wTransform, attrib, mask);
#else
162 + glPushMatrix ();
163 + glLoadMatrixf (wTransform.getMatrix ());
164 + gWindow->glDrawTexture (icon, fragment, mask);
165 + glPopMatrix ();
#endif
166 + }

172 + // BG
173 + glColor3f (0.0f, 0.0f, 0.0f);
174 + glRectf (x, y2, x2, y);

Preferably use client side buffers for this, eg

#ifndef USE_MODERN_COMPIZ_GL
172 + // BG
173 + glColor3f (0.0f, 0.0f, 0.0f);
174 + glRectf (x, y2, x2, y);
#else
GLVertexBuffer *vertexBuffer = GLVertexBuffer::streamingBuffer ();
vertexBuffer->begin (GL_TRIANGLE_STRIP)
const GLfloat vertices[] =
{
    x, y2, 0.0f,
    x, y, 0.0f,
    x2, y, 0.0f,
    x2, y2, 0.0f
};

vertexBuffer->addVertices (4, vertices);
vertexBuffer->color4f (0.0f, 0.0f, 0.0f, 1.0f);
vertexBuffer->end ();
vertexBuffer->render (transform, attrib);

209 + x + CLOSE_ICON_SPACE, iconY,
210 + maxWidth , maxHeight);

Make this a constant

190 + if (!sWindow->hasSlot() || // animation finished
191 + ...

Right approach, some pointers:

76	+// Based on Scale plugin code
77	+CompWindow* UnityScreen::checkForWindowAt (int x, int y)
78	+{
79	+ int x1, y1, x2, y2;
80	+ CompWindowList::reverse_iterator rit = screen->windows ().rbegin ();
81	+
82	+ for (; rit != screen->windows ().rend (); ++rit)
83	+ {
84	+ CompWindow *w = *rit;
85	+ SCALE_WINDOW (w);
86	+
87	+ ScalePosition pos = sw->getCurrentPosition ();
88	+ if (sw->hasSlot ())
89	+ {
90	+ x1 = w->x () - w->input ().left * pos.scale;
91	+ y1 = w->y () - w->input ().top * pos.scale;
92	+ x2 = w->x () + (w->width () + w->input ().right) * pos.scale;
93	+ y2 = w->y () + (w->height () + w->input ().bottom) * pos.scale;
94	+ x1 += pos.x ();
95	+ y1 += pos.y ();
96	+ x2 += pos.x ();
97	+ y2 += pos.y ();
98	+ if (x1 <= x && y1 <= y && x2 > x && y2 > y)
99	+ return w;
100	+ }
101	+ }
102	+ return NULL;
103	+}

Is this necessary?

151	+ gWindow->geometry ().reset ();
152	+ if (width && height)
153	+ gWindow->glAddGeometry (ml, iconReg, iconReg);
154	+
155	+ if (gWindow->geometry ().vCount)
156	+ {
157	+ GLFragment::Attrib fragment (attrib);
158	+ GLMatrix wTransform (transform);
159	+
160	+ wTransform.translate (x, y, 0.0f);
161	+
162	+ glPushMatrix ();
163	+ glLoadMatrixf (wTransform.getMatrix ());
164	+ gWindow->glDrawTexture (icon, fragment, mask);
165	+ glPopMatrix ();
166	+ }

You will need to put this inside of #if USE_MODERN_COMPIZ_GL ifdefs. Sorry :( Here's how to do it:

#ifdef USE_MODERN_COMPIZ_GL
gWindow->vertexBuffer ()->begin ();
#else
151	+ gWindow->geometry ().reset ();
#endif
152	+ if (width && height)
153	+ gWindow->glAddGeometry (ml, iconReg, iconReg);
154	+
#ifdef USE_MODERN_COMPIZ_GL
gWindow->vertexBuffer ().end ();
if (gWindow->vertexBuffer ().countVertices ())
#else
155	+ if (gWindow->geometry ().vCount)
#endif
156	+ {
#ifdef USE_MODERN_COMPIZ_GL
157	+ GLFragment::Attrib fragment (attrib);
#endif
158	+ GLMatrix wTransform (transform);
159	+
160	+ wTransform.translate (x, y, 0.0f);
161	+
#ifdef USE_MODERN_COMPIZ_GL
gWindow->glDrawTexture (icon, wTransform, attrib, mask);
#else
162	+ glPushMatrix ();
163	+ glLoadMatrixf (wTransform.getMatrix ());
164	+ gWindow->glDrawTexture (icon, fragment, mask);
165	+ glPopMatrix ();
#endif
166	+ }

172	+ // BG
173	+ glColor3f (0.0f, 0.0f, 0.0f);
174	+ glRectf (x, y2, x2, y);

Preferably use client side buffers for this, eg

#ifndef USE_MODERN_COMPIZ_GL
172	+ // BG
173	+ glColor3f (0.0f, 0.0f, 0.0f);
174	+ glRectf (x, y2, x2, y);
#else
GLVertexBuffer *vertexBuffer = GLVertexBuffer::streamingBuffer ();
vertexBuffer->begin (GL_TRIANGLE_STRIP)
const GLfloat vertices[] =
{
    x, y2, 0.0f,
    x, y, 0.0f,
    x2, y, 0.0f,
    x2, y2, 0.0f
};

vertexBuffer->addVertices (4, vertices);
vertexBuffer->color4f (0.0f, 0.0f, 0.0f, 1.0f);
vertexBuffer->end ();
vertexBuffer->render (transform, attrib);

209	+ x + CLOSE_ICON_SPACE, iconY,
210	+ maxWidth , maxHeight);

Make this a constant

190	+ if (!sWindow->hasSlot() || // animation finished
191	+ attrib.opacity != 65535) // no focus
192	+ return;
193	+
194	+ ScalePosition pos = sWindow->getCurrentPosition ();
195	+ int maxHeight, maxWidth;
196	+ int width = window->width () * pos.scale;
197	+ float x = pos.x () + window->x ();
198	+ float y = pos.y () + window->y ();
199	+ float iconY = y + ((SCALE_WINDOW_TITLE_SIZE - CLOSE_ICON_SIZE) / 2.0);
200	+
201	+ maxHeight = maxWidth = 0;

That needs to be under test.

Also avoid the 65535, use OPAQUE

121	+ CompString name (PKGDATADIR"/close_dash.png");

Is that the correct asset?

review: Needs Fixing

Revision history for this message

Renato Araujo Oliveira Filho (renatofilho) wrote on 2012-08-21:

> I think you can just overload scaleSelectWindow to determine what the
> currently hovered window is. From there you will need to damage and re-render
> both the new and old window. Have a look at the scaleaddon plugin.
> #ifdef USE_MODERN_COMPIZ_GL
> gWindow->vertexBuffer ()->begin ();
> #else
> 151 + gWindow->geometry ().reset ();
> #endif
> 152 + if (width && height)
> 153 + gWindow->glAddGeometry (ml, iconReg, iconReg);
> 154 +
> #ifdef USE_MODERN_COMPIZ_GL
> gWindow->vertexBuffer ().end ();
> if (gWindow->vertexBuffer ().countVertices ())
> #else
> 155 + if (gWindow->geometry ().vCount)
> #endif
> 156 + {
> #ifdef USE_MODERN_COMPIZ_GL
> 157 + GLFragment::Attrib fragment (attrib);
> #endif
> 158 + GLMatrix wTransform (transform);
> 159 +
> 160 + wTransform.translate (x, y, 0.0f);
> 161 +
> #ifdef USE_MODERN_COMPIZ_GL
> gWindow->glDrawTexture (icon, wTransform, attrib, mask);
> #else
> 162 + glPushMatrix ();
> 163 + glLoadMatrixf (wTransform.getMatrix ());
> 164 + gWindow->glDrawTexture (icon, fragment, mask);
> 165 + glPopMatrix ();
> #endif
> 166 + }
Fixed on rev: 2577
I have implemented the "scaleSelectWindow" based on scaleaddon plugin, but I did not understand why I need this code. (The implementation is working well without this)

>
> 172 + // BG
> 173 + glColor3f (0.0f, 0.0f, 0.0f);
> 174 + glRectf (x, y2, x2, y);
>
> Preferably use client side buffers for this
Fixed on rev: 2574
Thanks for the example code.

> Make this a constant
Fixed on rev: 2575

>
> Also avoid the 65535, use OPAQUE
Fixed on rev: 2576

>
> 121 + CompString name (PKGDATADIR"/close_dash.png");
>
> Is that the correct asset?
Based on designer docs, yes this is the correct icon.

>
> That needs to be under test.
Any suggestion or example how to test it?

Revision history for this message

Renato Araujo Oliveira Filho (renatofilho) wrote on 2012-08-22:

> > That needs to be under test.
My suggestion to make the code testable is split unityshell.(h/cpp) file into different files (UnityWindow/UnityWindowPrivate, UntiyScree/UnityScreenPrivate and UnityShell), with this we can instantiate only the private class inside of the tests and call/test the "private" functions.

What do you think?

Revision history for this message

Sam Spilsbury (smspillaz) wrote on 2012-08-24:

> > I think you can just overload scaleSelectWindow to determine what the
> > currently hovered window is. From there you will need to damage and re-
> render
> > both the new and old window. Have a look at the scaleaddon plugin.
> > #ifdef USE_MODERN_COMPIZ_GL
> > gWindow->vertexBuffer ()->begin ();
> > #else
> > 151 + gWindow->geometry ().reset ();
> > #endif
> > 152 + if (width && height)
> > 153 + gWindow->glAddGeometry (ml, iconReg, iconReg);
> > 154 +
> > #ifdef USE_MODERN_COMPIZ_GL
> > gWindow->vertexBuffer ().end ();
> > if (gWindow->vertexBuffer ().countVertices ())
> > #else
> > 155 + if (gWindow->geometry ().vCount)
> > #endif
> > 156 + {
> > #ifdef USE_MODERN_COMPIZ_GL
> > 157 + GLFragment::Attrib fragment (attrib);
> > #endif
> > 158 + GLMatrix wTransform (transform);
> > 159 +
> > 160 + wTransform.translate (x, y, 0.0f);
> > 161 +
> > #ifdef USE_MODERN_COMPIZ_GL
> > gWindow->glDrawTexture (icon, wTransform, attrib, mask);
> > #else
> > 162 + glPushMatrix ();
> > 163 + glLoadMatrixf (wTransform.getMatrix ());
> > 164 + gWindow->glDrawTexture (icon, fragment, mask);
> > 165 + glPopMatrix ();
> > #endif
> > 166 + }
> Fixed on rev: 2577
> I have implemented the "scaleSelectWindow" based on scaleaddon plugin, but I
> did not understand why I need this code. (The implementation is working well
> without this)

Hmm, what do you mean by this? If you're saying that you don't need either I guess you can remove them both? Its just that overriding scaleSelectWindow is the preferred way to finding out the currently highlighted window as opposed to reimplementing checkForWindowAt.

>
> >
> > 172 + // BG
> > 173 + glColor3f (0.0f, 0.0f, 0.0f);
> > 174 + glRectf (x, y2, x2, y);
> >
> > Preferably use client side buffers for this
> Fixed on rev: 2574
> Thanks for the example code.
>
> > Make this a constant
> Fixed on rev: 2575
>
> >
> > Also avoid the 65535, use OPAQUE
> Fixed on rev: 2576

Thanks.

>
> >
> > 121 + CompString name (PKGDATADIR"/close_dash.png");
> >
> > Is that the correct asset?
> Based on designer docs, yes this is the correct icon.

>
> >
> > That needs to be under test.
> Any suggestion or example how to test it?

My suggestion to make the code testable is split unityshell.(h/cpp) file into different files (UnityWindow/UnityWindowPrivate, UntiyScree/UnityScreenPrivate and UnityShell), with this we can instantiate only the private class inside of the tests and call/test the "private" functions.

What do you think?

^ This suggestion is the right way to do it :)

> > I think you can just overload scaleSelectWindow to determine what the
> > currently hovered window is. From there you will need to damage and re-
> render
> > both the new and old window. Have a look at the scaleaddon plugin.
> > #ifdef USE_MODERN_COMPIZ_GL
> > gWindow->vertexBuffer ()->begin ();
> > #else
> > 151     + gWindow->geometry ().reset ();
> > #endif
> > 152     + if (width && height)
> > 153     + gWindow->glAddGeometry (ml, iconReg, iconReg);
> > 154     +
> > #ifdef USE_MODERN_COMPIZ_GL
> > gWindow->vertexBuffer ().end ();
> > if (gWindow->vertexBuffer ().countVertices ())
> > #else
> > 155     + if (gWindow->geometry ().vCount)
> > #endif
> > 156     + {
> > #ifdef USE_MODERN_COMPIZ_GL
> > 157     + GLFragment::Attrib fragment (attrib);
> > #endif
> > 158     + GLMatrix wTransform (transform);
> > 159     +
> > 160     + wTransform.translate (x, y, 0.0f);
> > 161     +
> > #ifdef USE_MODERN_COMPIZ_GL
> > gWindow->glDrawTexture (icon, wTransform, attrib, mask);
> > #else
> > 162     + glPushMatrix ();
> > 163     + glLoadMatrixf (wTransform.getMatrix ());
> > 164     + gWindow->glDrawTexture (icon, fragment, mask);
> > 165     + glPopMatrix ();
> > #endif
> > 166     + }
> Fixed on rev: 2577
> I have implemented the "scaleSelectWindow" based on scaleaddon plugin, but I
> did not understand why I need this code. (The implementation is working well
> without this)

> 
> >
> > 172     + // BG
> > 173     + glColor3f (0.0f, 0.0f, 0.0f);
> > 174     + glRectf (x, y2, x2, y);
> >
> > Preferably use client side buffers for this
> Fixed on rev: 2574
> Thanks for the example code.
> 
> > Make this a constant
> Fixed on rev: 2575
> 
> >
> > Also avoid the 65535, use OPAQUE
> Fixed on rev: 2576

Thanks.

> 
> >
> > 121     + CompString name (PKGDATADIR"/close_dash.png");
> >
> > Is that the correct asset?
> Based on designer docs, yes this is the correct icon.

> 
> >
> > That needs to be under test.
> Any suggestion or example how to test it?

What do you think?

^ This suggestion is the right way to do it :)

Revision history for this message

Renato Araujo Oliveira Filho (renatofilho) wrote on 2012-08-24:

> Hmm, what do you mean by this? If you're saying that you don't need either I
> guess you can remove them both? Its just that overriding scaleSelectWindow is
> the preferred way to finding out the currently highlighted window as opposed
> to reimplementing checkForWindowAt.

I have implemented "scaleSelectWindow" but I did not use all the code you suggested, what I did was:

+void UnityWindow::scaleSelectWindow ()
210 +{
211 + UnityScreen* us = UnityScreen::get(screen);
212 +
213 + if (us->highlighted_window_ != window->id ())
214 + {
215 + CompositeWindow *cWindow = CompositeWindow::get (window);
216 + if (cWindow)
217 + cWindow->addDamage ();
218 +
219 + cWindow = 0;
220 + CompWindow *old_window = screen->findWindow (us->highlighted_window_);
221 + if (old_window)
222 + cWindow = CompositeWindow::get (old_window);
223 +
224 + if (cWindow)
225 + cWindow->addDamage ();
226 +
227 + us->highlighted_window_ = window->id ();
228 + }
229 +
230 + ScaleWindow *sWindow = ScaleWindow::get (window);
231 + if (sWindow)
232 + sWindow->scaleSelectWindow ();

Revision history for this message

Renato Araujo Oliveira Filho (renatofilho) wrote on 2012-08-27:

> > >
> > > That needs to be under test.
> > Any suggestion or example how to test it?
>
> My suggestion to make the code testable is split unityshell.(h/cpp) file into
> different files (UnityWindow/UnityWindowPrivate, UntiyScree/UnityScreenPrivate
> and UnityShell), with this we can instantiate only the private class inside of
> the tests and call/test the "private" functions.
>
> What do you think?
>
> ^ This suggestion is the right way to do it :)

This will be a big change in the code, because of that me and Olivier think this is not the best moment to do it, since this is our last week working on Unity bug fixes, and the code is already in feature freeze.

Revision history for this message

Sam Spilsbury (smspillaz) wrote on 2012-08-29:

This looks good. I don't know how much of this is testable, but I agree at this point its probably better in than out. I'll put my +1 here and hopefully someone can give a second opinion in the next few hours.

review: Approve

Revision history for this message

Sam Spilsbury (smspillaz) on 2012-08-30:

review: Approve

Revision history for this message

Unity Merger (unity-merger) wrote on 2012-08-31:

No commit message specified.

Revision history for this message

Unity Merger (unity-merger) wrote on 2012-08-31:

The Jenkins job https://jenkins.qa.ubuntu.com/job/automerge-unity/1164/console reported an error when processing this lp:~renatofilho/unity/unity-lp876017 branch.
Not merging it.

Revision history for this message

Sam Spilsbury (smspillaz) wrote on 2012-08-31:

Hey renato.

You need to add xrender to the pkgconfig deps in the top level cmakelists file.

Sent from Samsung Mobile

Unity Merger <email address hidden> wrote:

The Jenkins job https://jenkins.qa.ubuntu.com/job/automerge-unity/1164/console reported an error when processing this lp:~renatofilho/unity/unity-lp876017 branch.
Not merging it.
--
https://code.launchpad.net/~renatofilho/unity/unity-lp876017/+merge/120450
You are reviewing the proposed merge of lp:~renatofilho/unity/unity-lp876017 into lp:unity.

Revision history for this message

Unity Merger (unity-merger) wrote on 2012-08-31:

There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.

Revision history for this message

Marco Trevisan (Treviño) (3v1n0) wrote on 2012-08-31:

64 + WindowCairoContext ()

When you have some time, could you please update this code to match the unity coding stiles (i.e. no spaces between methods and brackets)

Instead of GetTextProperty and GetTextProperty why not just using gdk_x11_get_xatom_by_name?
Also, these functions should actually be in WindowManager class (implemented by PluginAdapterCompiz).

441 + if (window_header_style_)
442 + g_object_unref (window_header_style_);

You should have used glib::Object for this, and try to get rid of some manual deletion using smart pointers.

Revision history for this message

Renato Araujo Oliveira Filho (renatofilho) wrote on 2012-08-31:

@Trevino
Fixes on: https://code.launchpad.net/~renatofilho/unity/unity-lp876017-fixes/+merge/122357