Nux

Merge lp://qastaging/~smspillaz/nux/nux.fix_1091589.1 into lp://qastaging/nux

nux.fix_1091589.1
Merge into trunk

Proposed by Sam Spilsbury on 2013-05-26

Status:

Superseded

Proposed branch:

lp://qastaging/~smspillaz/nux/nux.fix_1091589.1

Merge into:

lp://qastaging/nux

Diff against target:

1795 lines (+1136/-125)

16 files modified

Nux/BaseWindow.cpp (+41/-0)
Nux/BaseWindow.h (+16/-0)
Nux/View.cpp (+7/-4)
Nux/WindowCompositor.cpp (+222/-53)
Nux/WindowCompositor.h (+18/-3)
Nux/WindowThread.cpp (+110/-8)
Nux/WindowThread.h (+50/-1)
NuxGraphics/GLDeviceFrameBufferObject.cpp (+20/-5)
NuxGraphics/GpuDevice.cpp (+7/-1)
NuxGraphics/GpuDeviceTexture.cpp (+7/-1)
NuxGraphics/IOpenGLFrameBufferObject.cpp (+7/-1)
configure.ac (+2/-3)
debian/changelog (+6/-0)
tests/gtest-nux-windowcompositor.cpp (+110/-44)
tests/gtest-nux-windowthread.cpp (+496/-0)
tests/gtest-nuxgraphics-texture.cpp (+17/-1)

To merge this branch:

bzr merge lp://qastaging/~smspillaz/nux/nux.fix_1091589.1

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot (community)	continuous-integration	2013-05-26	Approve on 2013-07-22
Brandon Schaefer (community)			Approve on 2013-06-11
Stephen M. Webb (community)		2013-05-26	Needs Fixing on 2013-05-29
Review via email: mp+165772@code.qastaging.launchpad.net

This proposal supersedes a proposal from 2013-02-10.

This proposal has been superseded by a proposal from 2013-10-30.

Commit message

Allow embedded clients to specify which windows should be redrawn.

Added some new API to nux to allow embedded clients (eg, compiz) to specify which BaseWindows should be redrawn on a frame. This is done by passing a list of rectangles to nux which represent the redraw region, and nux will mark all intersecting windows as needing re-presenting to the screen. This means that we aren't re-presenting every window to the screen on every frame (bad for performance, and also doesn't work correctly in the buffer-swap case).

Also added API to allow nux to tell the difference between draw and read framebuffers when it plays around with the framebuffer object binding. This is necessary in case the embedded client expects the read framebuffer binding to remain defined even after nux has changed the draw framebuffer binding.

(LP: #1091589)

Description of the change

Allow embedded clients to specify which windows should be redrawn.

(LP: #1091589)

Both APIs are fully tested.

Update: test results here: http://www.ucc.asn.au/~smspillaz/phoronix-test-suite/composite.xml

I was careful to modify phoronix-test-suite to run tests in windowed mode only, and those were the only three tests I was able to get to run in windowed mode. Of particular interest was the fact that the unigine demos had a 5x performance improvement, probably because the driver was spending less time filling redundant pixels from the compositor. In other areas we had a roughly 5FPS boost.

Compiz framerate graph:

http://www.ucc.asn.au/~smspillaz/compiz-perf-graph.png

You'll see that especially on the last test, it drops off quite a bit on the non buffer_age case, and is generally speaking lower across the board.

Revision history for this message

Andrea Azzarone (azzar1) wrote on 2013-03-26: Posted in a previous version of this proposal

39 + void BaseWindow::WasPresentedInEmbeddedMode()
40 + {
41 + _present_in_embedded_mode = false;
42 + _last_presented_geometry_in_embedded_mode = GetAbsoluteGeometry();
43 + }

Can we try to find a better name for this function? WasPresented* suggests me that the function should return a bool value.

45 + nux::Geometry const& BaseWindow::LastPresentedGeometryInEmbeddedMode()
46 + {
47 + return _last_presented_geometry_in_embedded_mode;
48 + }
49 +
50 + bool BaseWindow::AllowPresentationInEmbeddedMode()
51 + {
52 + return _present_in_embedded_mode;
53 + }

Can these methods be const?

+ for (WindowList::iterator it = _view_window_list.begin();
145 + it != _view_window_list.end();
146 + ++it)
147 + {
148 + if (it->IsValid())
149 + func (*it);
150 + }

Why not a range-based for loop? :)

206 + void WindowCompositor::SetReferenceFramebuffer(unsigned int draw_fbo_object,
207 + unsigned int read_fbo_object,
208 + Geometry fbo_geometry)

Geometry const& fbo... ?

374 + WindowCompositor::WeakBaseWindowPtr ptr;
375 + window_compositor_->OnAllBaseWindows(std::bind(AssignWeakBaseWindowMatchingRaw, _1, bw, &ptr));

It took me a while to understand what this code was for, maybe we can make it more readable.

406 + for (std::vector<WindowCompositor::WeakBaseWindowPtr>::iterator it =
407 + m_presentation_list_embedded.begin();
408 + it != m_presentation_list_embedded.end();
409 + ++it)
410 + {

For range-based loop... :)

Btw logic looks good to me.

Revision history for this message

Sam Spilsbury (smspillaz) wrote on 2013-03-26: Posted in a previous version of this proposal

Agree with all points. Will fix later today
On 26/03/2013 8:50 AM, "Andrea Azzarone" <email address hidden> wrote:

> 39 + void BaseWindow::WasPresentedInEmbeddedMode()
> 40 + {
> 41 + _present_in_embedded_mode = false;
> 42 + _last_presented_geometry_in_embedded_mode =
> GetAbsoluteGeometry();
> 43 + }
>
> Can we try to find a better name for this function? WasPresented* suggests
> me that the function should return a bool value.
>
> 45 + nux::Geometry const&
> BaseWindow::LastPresentedGeometryInEmbeddedMode()
> 46 + {
> 47 + return _last_presented_geometry_in_embedded_mode;
> 48 + }
> 49 +
> 50 + bool BaseWindow::AllowPresentationInEmbeddedMode()
> 51 + {
> 52 + return _present_in_embedded_mode;
> 53 + }
>
> Can these methods be const?
>
> + for (WindowList::iterator it = _view_window_list.begin();
> 145 + it != _view_window_list.end();
> 146 + ++it)
> 147 + {
> 148 + if (it->IsValid())
> 149 + func (*it);
> 150 + }
>
> Why not a range-based for loop? :)
>
> 206 + void WindowCompositor::SetReferenceFramebuffer(unsigned int
> draw_fbo_object,
> 207 + unsigned int
> read_fbo_object,
> 208 + Geometry
> fbo_geometry)
>
> Geometry const& fbo... ?
>
> 374 + WindowCompositor::WeakBaseWindowPtr ptr;
> 375 +
> window_compositor_->OnAllBaseWindows(std::bind(AssignWeakBaseWindowMatchingRaw,
> _1, bw, &ptr));
>
> It took me a while to understand what this code was for, maybe we can make
> it more readable.
>
> 406 + for
> (std::vector<WindowCompositor::WeakBaseWindowPtr>::iterator it =
> 407 + m_presentation_list_embedded.begin();
> 408 + it != m_presentation_list_embedded.end();
> 409 + ++it)
> 410 + {
>
> For range-based loop... :)
>
> Btw logic looks good to me.
> --
> https://code.launchpad.net/~smspillaz/nux/nux.fix_1091589/+merge/147543
> You are the owner of lp:~smspillaz/nux/nux.fix_1091589.
>

Agree with all points. Will fix later today
On 26/03/2013 8:50 AM, "Andrea Azzarone" <azzaronea@gmail.com> wrote:

> 39      +  void BaseWindow::WasPresentedInEmbeddedMode()
> 40      +  {
> 41      +    _present_in_embedded_mode = false;
> 42      +    _last_presented_geometry_in_embedded_mode =
> GetAbsoluteGeometry();
> 43      +  }
>
> Can we try to find a better name for this function? WasPresented* suggests
> me that the function should return a bool value.
>
> 45      +  nux::Geometry const&
> BaseWindow::LastPresentedGeometryInEmbeddedMode()
> 46      +  {
> 47      +    return _last_presented_geometry_in_embedded_mode;
> 48      +  }
> 49      +
> 50      +  bool BaseWindow::AllowPresentationInEmbeddedMode()
> 51      +  {
> 52      +    return _present_in_embedded_mode;
> 53      +  }
>
> Can these methods be const?
>
>         +    for (WindowList::iterator it = _view_window_list.begin();
> 145     +         it != _view_window_list.end();
> 146     +         ++it)
> 147     +    {
> 148     +      if (it->IsValid())
> 149     +        func (*it);
> 150     +    }
>
> Why not a range-based for loop? :)
>
> 206     +  void WindowCompositor::SetReferenceFramebuffer(unsigned int
> draw_fbo_object,
> 207     +                                                 unsigned int
> read_fbo_object,
> 208     +                                                 Geometry
> fbo_geometry)
>
> Geometry const& fbo... ?
>
> 374     +    WindowCompositor::WeakBaseWindowPtr ptr;
> 375     +
>  window_compositor_->OnAllBaseWindows(std::bind(AssignWeakBaseWindowMatchingRaw,
> _1, bw, &ptr));
>
> It took me a while to understand what this code was for, maybe we can make
> it more readable.
>
> 406     +    for
> (std::vector<WindowCompositor::WeakBaseWindowPtr>::iterator it =
> 407     +         m_presentation_list_embedded.begin();
> 408     +         it != m_presentation_list_embedded.end();
> 409     +         ++it)
> 410     +    {
>
> For range-based loop... :)
>
> Btw logic looks good to me.
> --
> https://code.launchpad.net/~smspillaz/nux/nux.fix_1091589/+merge/147543
> You are the owner of lp:~smspillaz/nux/nux.fix_1091589.
>

Revision history for this message

Sam Spilsbury (smspillaz) wrote on 2013-03-26: Posted in a previous version of this proposal

Just the only points warranting some discussion:

On Tue, Mar 26, 2013 at 8:50 AM, Andrea Azzarone <email address hidden> wrote:
> 39 + void BaseWindow::WasPresentedInEmbeddedMode()
> 40 + {
> 41 + _present_in_embedded_mode = false;
> 42 + _last_presented_geometry_in_embedded_mode = GetAbsoluteGeometry();
> 43 + }
>
> Can we try to find a better name for this function? WasPresented* suggests me that the function should return a bool value.

Yeah, I remember when I read through this code later I was suprised it didn't.

How about OnPresentedInEmbeddedMode (); ?

>
> 374 + WindowCompositor::WeakBaseWindowPtr ptr;
> 375 + window_compositor_->OnAllBaseWindows(std::bind(AssignWeakBaseWindowMatchingRaw, _1, bw, &ptr));
>
> It took me a while to understand what this code was for, maybe we can make it more readable.
>
>

Yeah, it could get a bit like that for a codebase that's not really
designed around callbacks. It might actually make sense to wrap this
up in a helper function:

nux::WindowCompositor::WeakBaseWindowPtr const&
nux::WindowCompositor::FindWeakBaseWindowPtrForRawPtr (nux::BaseWindow
*);

Then the helper function could look something like:

window_compositor_->OnAllBaseWindows(std::bind(AssignWeakBaseWindowPtrIfMatchesRawPtr,
_1, bw, &ptr));

Its basically a glorified:

nux::WindowCompositor::WeakBaseWindowPtr weak;

for (nux::WeakBaseWindowPtr const& ptr : weak_base_window)
    if (ptr.get () == raw)
    {
        weak = ptr;
        break;
    }

Having it this way would have been preferable, but then it would have
meant exposing the internals of WindowCompositor to BaseWindow, which
was something I wanted to avoid (nux already exposes too many
internals to clients).

What do you think?

--
Sam Spilsbury

Revision history for this message

Andrea Azzarone (azzar1) wrote on 2013-03-26: Posted in a previous version of this proposal

> Just the only points warranting some discussion:
>
> On Tue, Mar 26, 2013 at 8:50 AM, Andrea Azzarone <email address hidden> wrote:
> > 39 + void BaseWindow::WasPresentedInEmbeddedMode()
> > 40 + {
> > 41 + _present_in_embedded_mode = false;
> > 42 + _last_presented_geometry_in_embedded_mode =
> GetAbsoluteGeometry();
> > 43 + }
> >
> > Can we try to find a better name for this function? WasPresented* suggests
> me that the function should return a bool value.
>
> Yeah, I remember when I read through this code later I was suprised it didn't.
>
> How about OnPresentedInEmbeddedMode (); ?

Yeah sounds good to me.

>
> >
> > 374 + WindowCompositor::WeakBaseWindowPtr ptr;
> > 375 + window_compositor_->OnAllBaseWindows(std::bind(AssignWeakBaseWi
> ndowMatchingRaw, _1, bw, &ptr));
> >
> > It took me a while to understand what this code was for, maybe we can make
> it more readable.
> >
> >
>
> Yeah, it could get a bit like that for a codebase that's not really
> designed around callbacks. It might actually make sense to wrap this
> up in a helper function:
>
> nux::WindowCompositor::WeakBaseWindowPtr const&
> nux::WindowCompositor::FindWeakBaseWindowPtrForRawPtr (nux::BaseWindow
> *);
>
> Then the helper function could look something like:
>
> window_compositor_->OnAllBaseWindows(std::bind(AssignWeakBaseWindowPtrIfMatche
> sRawPtr,
> _1, bw, &ptr));
>
> Its basically a glorified:
>
> nux::WindowCompositor::WeakBaseWindowPtr weak;
>
> for (nux::WeakBaseWindowPtr const& ptr : weak_base_window)
> if (ptr.get () == raw)
> {
> weak = ptr;
> break;
> }
>
> Having it this way would have been preferable, but then it would have
> meant exposing the internals of WindowCompositor to BaseWindow, which
> was something I wanted to avoid (nux already exposes too many
> internals to clients).
>
> What do you think?
>

Ok just never mind. That comment was/is not blocking the MP ;)

>
>
> --
> Sam Spilsbury

> Just the only points warranting some discussion:
> 
> On Tue, Mar 26, 2013 at 8:50 AM, Andrea Azzarone <azzaronea@gmail.com> wrote:
> > 39      +  void BaseWindow::WasPresentedInEmbeddedMode()
> > 40      +  {
> > 41      +    _present_in_embedded_mode = false;
> > 42      +    _last_presented_geometry_in_embedded_mode =
> GetAbsoluteGeometry();
> > 43      +  }
> >
> > Can we try to find a better name for this function? WasPresented* suggests
> me that the function should return a bool value.
> 
> Yeah, I remember when I read through this code later I was suprised it didn't.
> 
> How about OnPresentedInEmbeddedMode (); ?

Yeah sounds good to me.

> 
> >
> > 374     +    WindowCompositor::WeakBaseWindowPtr ptr;
> > 375     +    window_compositor_->OnAllBaseWindows(std::bind(AssignWeakBaseWi
> ndowMatchingRaw, _1, bw, &ptr));
> >
> > It took me a while to understand what this code was for, maybe we can make
> it more readable.
> >
> >
> 
> Yeah, it could get a bit like that for a codebase that's not really
> designed around callbacks. It might actually make sense to wrap this
> up in a helper function:
> 
> nux::WindowCompositor::WeakBaseWindowPtr const&
> nux::WindowCompositor::FindWeakBaseWindowPtrForRawPtr (nux::BaseWindow
> *);
> 
> Then the helper function could look something like:
> 
> window_compositor_->OnAllBaseWindows(std::bind(AssignWeakBaseWindowPtrIfMatche
> sRawPtr,
> _1, bw, &ptr));
> 
> Its basically a glorified:
> 
> nux::WindowCompositor::WeakBaseWindowPtr weak;
> 
> for (nux::WeakBaseWindowPtr const& ptr : weak_base_window)
>     if (ptr.get () == raw)
>     {
>         weak = ptr;
>         break;
>     }
> 
> Having it this way would have been preferable, but then it would have
> meant exposing the internals of WindowCompositor to BaseWindow, which
> was something I wanted to avoid (nux already exposes too many
> internals to clients).
> 
> What do you think?
>

Ok just never mind. That comment was/is not blocking the MP ;)

> 
> 
> --
> Sam Spilsbury

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-04-13: Posted in a previous version of this proposal

PASSED: Continuous integration, rev:757
http://jenkins.qa.ubuntu.com/job/nux-ci/1/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/nux-raring-amd64-ci/1
    SUCCESS: http://jenkins.qa.ubuntu.com/job/nux-raring-armhf-ci/1
    SUCCESS: http://jenkins.qa.ubuntu.com/job/nux-raring-i386-ci/1

Click here to trigger a rebuild:
http://s-jenkins:8080/job/nux-ci/1/rebuild

review: Approve (continuous-integration)

Revision history for this message

Sam Spilsbury (smspillaz) wrote on 2013-04-13: Posted in a previous version of this proposal

Okay, so I've talked to Didier about this and we've come up with a small plan of action. We'll need to give this a week solid of QA at the beginning of the S cycle and then make sure that we don't have any AP regressions. That'll happen in about two weeks.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-04-14: Posted in a previous version of this proposal

PASSED: Continuous integration, rev:758
http://jenkins.qa.ubuntu.com/job/nux-ci/21/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/nux-raring-amd64-ci/21
    SUCCESS: http://jenkins.qa.ubuntu.com/job/nux-raring-armhf-ci/21
    SUCCESS: http://jenkins.qa.ubuntu.com/job/nux-raring-i386-ci/21

Click here to trigger a rebuild:
http://s-jenkins:8080/job/nux-ci/21/rebuild

review: Approve (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-04-24: Posted in a previous version of this proposal

PASSED: Continuous integration, rev:760
http://jenkins.qa.ubuntu.com/job/nux-ci/25/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/nux-raring-amd64-ci/25
    SUCCESS: http://jenkins.qa.ubuntu.com/job/nux-raring-armhf-ci/25
    SUCCESS: http://jenkins.qa.ubuntu.com/job/nux-raring-i386-ci/25

Click here to trigger a rebuild:
http://s-jenkins:8080/job/nux-ci/25/rebuild

review: Approve (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-05-03: Posted in a previous version of this proposal

PASSED: Continuous integration, rev:761
http://jenkins.qa.ubuntu.com/job/nux-ci/33/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/nux-raring-amd64-ci/33
    SUCCESS: http://jenkins.qa.ubuntu.com/job/nux-raring-armhf-ci/33
    SUCCESS: http://jenkins.qa.ubuntu.com/job/nux-raring-i386-ci/33

Click here to trigger a rebuild:
http://s-jenkins:8080/job/nux-ci/33/rebuild

review: Approve (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-05-08: Posted in a previous version of this proposal

PASSED: Continuous integration, rev:762
http://jenkins.qa.ubuntu.com/job/nux-ci/35/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/nux-raring-amd64-ci/35
    SUCCESS: http://jenkins.qa.ubuntu.com/job/nux-raring-armhf-ci/35
    SUCCESS: http://jenkins.qa.ubuntu.com/job/nux-raring-i386-ci/35

Click here to trigger a rebuild:
http://s-jenkins:8080/job/nux-ci/35/rebuild

review: Approve (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-05-08: Posted in a previous version of this proposal

PASSED: Continuous integration, rev:764
http://jenkins.qa.ubuntu.com/job/nux-ci/36/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/nux-raring-amd64-ci/36
    SUCCESS: http://jenkins.qa.ubuntu.com/job/nux-raring-armhf-ci/36
    SUCCESS: http://jenkins.qa.ubuntu.com/job/nux-raring-i386-ci/36

Click here to trigger a rebuild:
http://s-jenkins:8080/job/nux-ci/36/rebuild

review: Approve (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-05-26:

PASSED: Continuous integration, rev:792
http://jenkins.qa.ubuntu.com/job/nux-ci/46/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/nux-raring-amd64-ci/46
    SUCCESS: http://jenkins.qa.ubuntu.com/job/nux-raring-armhf-ci/46
    SUCCESS: http://jenkins.qa.ubuntu.com/job/nux-raring-i386-ci/46

Click here to trigger a rebuild:
http://s-jenkins:8080/job/nux-ci/46/rebuild

review: Approve (continuous-integration)

Revision history for this message

MC Return (mc-return) wrote on 2013-05-29:

News:
http://support.amd.com/us/kbarticles/Pages/amdcatalyst13-6linbetadriver.aspx

AMD's fglrx 13.6 Beta driver now also supports GLX_EXT_buffer_age extension !!! \o/

Revision history for this message

Stephen M. Webb (bregma) wrote on 2013-05-29:

This is (obviously) an ABI-breaking change. We need to bump the ABI string, SONAME, package version, and debian/changelog upstream package version at the same time.

This patchset should also include changing configure.ac to bump nux_micro_version to 3, nux_abi_version to, say, 20130530.0, and create a new UNRELEASED entry in debian/changelog for 4.0.3-0ubuntu1.

review: Needs Fixing

Revision history for this message

Sam Spilsbury (smspillaz) wrote on 2013-05-29:

Okay.

I think I've been bumping the abi in configure.ac for some time now, but
I'll bump the other bits too - I'll be about 2 days with that though as uni
is keeping me busy. Is that OK?
On 29/05/2013 9:51 PM, "Stephen M. Webb" <email address hidden> wrote:

> Review: Needs Fixing
>
> This is (obviously) an ABI-breaking change. We need to bump the ABI
> string, SONAME, package version, and debian/changelog upstream package
> version at the same time.
>
> This patchset should also include changing configure.ac to bump
> nux_micro_version to 3, nux_abi_version to, say, 20130530.0, and create a
> new UNRELEASED entry in debian/changelog for 4.0.3-0ubuntu1.
>
> --
> https://code.launchpad.net/~smspillaz/nux/nux.fix_1091589.1/+merge/165772
> You are the owner of lp:~smspillaz/nux/nux.fix_1091589.1.
>

Revision history for this message

Sam Spilsbury (smspillaz) wrote on 2013-05-31:

Thanks for the feedback, done.

lp://qastaging/~smspillaz/nux/nux.fix_1091589.1 updated on 2013-05-31

793. By Sam Spilsbury on 2013-05-31: Bump ABI version and debian/changelog version

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-05-31:

PASSED: Continuous integration, rev:793
http://jenkins.qa.ubuntu.com/job/nux-ci/48/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/nux-raring-amd64-ci/48
    SUCCESS: http://jenkins.qa.ubuntu.com/job/nux-raring-armhf-ci/48
    SUCCESS: http://jenkins.qa.ubuntu.com/job/nux-raring-i386-ci/48

Click here to trigger a rebuild:
http://s-jenkins:8080/job/nux-ci/48/rebuild

review: Approve (continuous-integration)

Revision history for this message

Brandon Schaefer (brandontschaefer) wrote on 2013-06-10:

I like this function :)
183 + void WindowCompositor::OnAllBaseWindows(const WindowMutatorFunc &func)

Very functional style! One suggestion would be a different name, possibly. MapFunctionOnAllBaseWindows, as what you are doing a specific higher order function. (Look up map function if yo want :)

547 + if (std::find (m_presentation_list_embedded.begin(),
548 + m_presentation_list_embedded.end(),
549 + ptr) != m_presentation_list_embedded.end())
550 + return true;

Might I suggest a simple utility function that will do this? That way you don't have to repeat your self twice and clean that up a bit...(though thats just being nit picky...)

866 + * This is really important. Don't remove it */
867 + result->_OpenGLID = id;

Thats like don't press this button! Now I want to remove it!! (joke no problem here)

No real *needs fixing* just some suggestions. Looking at actually running the branch now :)

Revision history for this message

Sam Spilsbury (smspillaz) wrote on 2013-06-11:

On 11/06/2013 7:00 AM, "Brandon Schaefer" <email address hidden>
wrote:
>
> I like this function :)
> 183 + void WindowCompositor::OnAllBaseWindows(const WindowMutatorFunc
&func)
>
> Very functional style! One suggestion would be a different name,
possibly. MapFunctionOnAllBaseWindows, as what you are doing a specific
higher order function. (Look up map function if yo want :)

I could, although not everyone knows about map/reduce style. I think the
former is a bit clearer.

>
> 547 + if (std::find (m_presentation_list_embedded.begin(),
> 548 + m_presentation_list_embedded.end(),
> 549 + ptr) != m_presentation_list_embedded.end())
> 550 + return true;
>
> Might I suggest a simple utility function that will do this? That way you
don't have to repeat your self twice and clean that up a bit...(though
thats just being nit picky...)

Doable.

>
> 866 + * This is really important. Don't remove it */
> 867 + result->_OpenGLID = id;
>
> Thats like don't press this button! Now I want to remove it!! (joke no
problem here)

Someone removed it a while ago in a cleanup and cost me a day or two
debugging it. I think there's a test in place for it now.

>
> No real *needs fixing* just some suggestions. Looking at actually running
the branch now :)
>
> --
> https://code.launchpad.net/~smspillaz/nux/nux.fix_1091589.1/+merge/165772
> You are the owner of lp:~smspillaz/nux/nux.fix_1091589.1.

Revision history for this message

Brandon Schaefer (brandontschaefer) wrote on 2013-06-11:

Cool, well we need to coordinate merging this at the same time as the unity branch, otherwise we are good to go on this branch. Marking as approved, but we need to wait to approved globally until the unity branch is ready :)

review: Approve

lp://qastaging/~smspillaz/nux/nux.fix_1091589.1 updated on 2013-06-12

794. By Sam Spilsbury on 2013-06-11: Merge lp:nux

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-06-12:

PASSED: Continuous integration, rev:794
http://jenkins.qa.ubuntu.com/job/nux-ci/49/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/nux-saucy-amd64-ci/1
    SUCCESS: http://jenkins.qa.ubuntu.com/job/nux-saucy-armhf-ci/1
    SUCCESS: http://jenkins.qa.ubuntu.com/job/nux-saucy-i386-ci/1

Click here to trigger a rebuild:
http://s-jenkins:8080/job/nux-ci/49/rebuild

review: Approve (continuous-integration)

lp://qastaging/~smspillaz/nux/nux.fix_1091589.1 updated on 2013-06-15

795. By Sam Spilsbury on 2013-06-12: Split presentation and render into separate functions, call the present
function on any rendered-windows as soon as we need to restore the reference
framebuffer - this will ensure that should we need to do that we can
use the contents of already drawn BaseWindows in the backbuffer as some
part of unity expect.
796. By Sam Spilsbury on 2013-06-15: Cleanup