Merge lp://qastaging/~unity-team/nux/nux.fix_1091589.1 into lp://qastaging/nux

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.

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

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

> 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

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

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.

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

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

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

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

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

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

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/

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.

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

Thanks for the feedback, done.

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

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 :)

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.

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 :)

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

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

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

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

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