Unity

Merge lp://qastaging/~3v1n0/unity/unity-decorations into lp://qastaging/unity

unity-decorations
Merge into trunk

Proposed by Marco Trevisan (Treviño) on 2014-01-22

Status:

Merged

Approved by:

Christopher Townsend on 2014-01-28

Approved revision:

no longer in the source branch.

Merged at revision:

3632

Proposed branch:

lp://qastaging/~3v1n0/unity/unity-decorations

Merge into:

lp://qastaging/unity

Diff against target:

10234 lines (+6824/-1487)

64 files modified

CMakeLists.txt (+5/-0)
config.h.cmake (+1/-0)
decorations/CMakeLists.txt (+38/-0)
decorations/DecoratedWindow.cpp (+654/-0)
decorations/DecoratedWindow.h (+73/-0)
decorations/DecorationsDataPool.cpp (+153/-0)
decorations/DecorationsDataPool.h (+59/-0)
decorations/DecorationsEdge.cpp (+137/-0)
decorations/DecorationsEdge.h (+64/-0)
decorations/DecorationsEdgeBorders.cpp (+99/-0)
decorations/DecorationsEdgeBorders.h (+43/-0)
decorations/DecorationsGrabEdge.cpp (+102/-0)
decorations/DecorationsGrabEdge.h (+53/-0)
decorations/DecorationsInputMixer.cpp (+176/-0)
decorations/DecorationsInputMixer.h (+66/-0)
decorations/DecorationsManager.cpp (+386/-0)
decorations/DecorationsManager.h (+74/-0)
decorations/DecorationsPriv.h (+165/-0)
decorations/DecorationsTitle.cpp (+116/-0)
decorations/DecorationsTitle.h (+61/-0)
decorations/DecorationsWidgets.cpp (+493/-0)
decorations/DecorationsWidgets.h (+190/-0)
decorations/DecorationsWindowButton.cpp (+216/-0)
decorations/DecorationsWindowButton.h (+58/-0)
decorations/pch/decorations_pch.hh (+39/-0)
panel/PanelMenuView.cpp (+116/-254)
panel/PanelMenuView.h (+9/-14)
plugins/unityshell/CMakeLists.txt (+27/-2)
plugins/unityshell/src/unityshell.cpp (+291/-412)
plugins/unityshell/src/unityshell.h (+29/-17)
plugins/unityshell/src/unityshell_glow.cpp (+4/-9)
plugins/unityshell/unityshell.xml.in (+69/-1)
tests/CMakeLists.txt (+14/-9)
tests/decoration_mock_item.h (+88/-0)
tests/test_decorations_input_mixer.cpp (+493/-0)
tests/test_decorations_widgets.cpp (+512/-0)
tests/test_texture_cache.cpp (+16/-1)
tests/test_unity_window_style.cpp (+3/-3)
unity-shared/CMakeLists.txt (+14/-1)
unity-shared/CompizUtils.cpp (+181/-0)
unity-shared/CompizUtils.h (+116/-0)
unity-shared/DebugDBusInterface.cpp (+6/-1)
unity-shared/DecorationStyle.cpp (+640/-0)
unity-shared/DecorationStyle.h (+141/-0)
unity-shared/Introspectable.cpp (+4/-1)
unity-shared/IntrospectionData.cpp (+21/-0)
unity-shared/IntrospectionData.h (+8/-0)
unity-shared/PanelStyle.cpp (+58/-259)
unity-shared/PanelStyle.h (+16/-38)
unity-shared/PluginAdapter.cpp (+28/-311)
unity-shared/PluginAdapter.h (+0/-22)
unity-shared/StandaloneDecorationStyle.cpp (+146/-0)
unity-shared/StandaloneWindowManager.cpp (+15/-20)
unity-shared/StandaloneWindowManager.h (+2/-3)
unity-shared/TextureCache.cpp (+6/-3)
unity-shared/TextureCache.h (+3/-2)
unity-shared/UnityWindowStyle.cpp (+1/-1)
unity-shared/UnityWindowStyle.h (+1/-1)
unity-shared/WindowButtonPriv.h (+1/-0)
unity-shared/WindowButtons.cpp (+69/-79)
unity-shared/WindowButtons.h (+1/-0)
unity-shared/WindowManager.h (+4/-5)
unity-shared/XWindowManager.cpp (+146/-11)
unity-shared/XWindowManager.h (+4/-7)

To merge this branch:

bzr merge lp://qastaging/~3v1n0/unity/unity-decorations

Related bugs:

Bug #691817: Framing around window decorations on windows and panel should be identical.	Medium	Fix Released
Bug #827537: Decorations needs to support anti aliased rounded corners	Medium	Fix Released
Bug #1281370: Unity should use "Normal" resize mode by default	Medium	Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Christopher Townsend (community)			Approve on 2014-01-28
Brandon Schaefer (community)		2014-01-22	Approve on 2014-01-27
PS Jenkins bot (community)	continuous-integration		Approve on 2014-01-23
Review via email: mp+202582@code.qastaging.launchpad.net

Commit message

decorations: added new unity-decorations fully gtk-css-themed inside unity

Description of the change

Added new decorations to unity, inside unity itself instead of depending on the
compiz "decor" plugin and on a decorator (gtk-window-decorator).
My mission: «Everything changes, but nothing changes»; so completely new backend,
same results.

- Support for full Gtk3 Theming (https://wiki.ubuntu.com/Unity/Theming)
- Improved resizing speed when using "Normal Resize" in compiz
- Removed duplicated textures, IPC between compiz and the decorator
- Anti aliased corners!!

Screenshot:
- Ambiance: http://ubuntuone.com/0QIWISIXmf1zRJfDiIqLHz
- Radiance: http://ubuntuone.com/1CfUWwRj0xICU3otSfjTyY

Things to do:
- Add right-click window menu support
- Add "force quit" dialog

Not supported:
- Window buttons positions (we want a consistent user experience in unity, that
covers both the windows and the top panel and here the window buttons are, by
design on the top left corner of a window. Always).
- Custom actions on bar clicks (maybe to be supported later)
- Different decorators, just use themes.

This branch needs has a "soft dependency" on lp:~3v1n0/ubuntu-themes/unity-decorations
(for proper theming) and on lp:~3v1n0/compiz/disable-decor-with-unity to disable
decor plugin on settings.

=================

Technical details (use a better diff visualizer for review, like bzr qdiff):

- Added DecorationsManager: that handles all the windows, creates them and
redirect to the proper target the relative events.
- DecoratedWindow: wraps a CompWindow, set the extents, generates the background
textures, the top layout and draws them on screen.

Both DecorationsManager is allocated and controlled by UnityScreen (that redirects
the events to it), while each UnityWindow controls a DecoratedWindow, and redirects
to it the relevant compiz calls.

As for the decoration widgets, since I needed a simple way to handle textures and
to create a simple layout for displaying them, depending on the window size and
positioning, I've decided to write a small "widgets library" used to paint elements
and handle input events. What I called decoration::Item is basically a simple
widget with a geometry and that can be added into a container and managed by it.
Also, I've built an "InputMixer" (event compositor) that has the role to redirect
events to the proper widget that it manages.

So, with these tools, I've built all the widgets that are in a decoration:
- A DecorationsEdgeBorders, that is a container of DecorationsGrabEdge, that are
   input-only widgets that care about mouse events, draw the right mouse pointers
   and allow to resize and move a window (now the GrabEdge has the same logic of
   the PanelGrabArea).
- A Layout containing the window buttons and the window title.

To properly theme the decorations I've built a decoration::Style class that
defines custom gtk-css parameters for an "UnityDecoration" widget that is used
for theming the whole decorations; read the values, caches the relevant settings
and emits signals when theme or font parameters changes. panel::Style has been
rewritten to use decoration::Style as its main source.

Other aspects:
- Removed all the unneeded code for decorating, undecorating and monitoring
  decorations from PluginAdapter.
- Moved some XLib-only calls from PluginAdapter to XWindowManager and optimized
  GetWindowName using the new GetStringProperty.
- Removing the unneeded code from PanelMenuView that was handling the decorations
  and use decoration::Style on it; also remove a lot of unneeded redraws.
- Use decoration::Style for painting fake-decorations in UnityWindow.
- Added CompizUtils to unity-shared-compiz, that contains some structs, classes
  and functions that are useful for other components that are handling things
  with compiz (allows removing duplicated code from UnityWindow as well).
- Added decoration::DataPool utility class to load and cache shared resources
- Shadows are based on simple pixmaps that are generated through cairo just
  once per session (or when the shadow settings change) and shared by all
  the windows.
- Fixed linking of standalone components (and tests) against libunity-protocol-private
  (now the library dir is added to dpath, no need to use LD_LIBRARY_PATH any more).
- Added compiz settings to override theme shadow parameters.
- Improved fallback window buttons (now they're filled with foreground color).
- All the decoration objects support debug introspection for autopilot tests.
- Added unit tests for decoration::Item and InputMixer, more tests to come later.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-01-22:

PASSED: Continuous integration, rev:3890
http://jenkins.qa.ubuntu.com/job/unity-ci/563/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-trusty-amd64-ci/99
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-trusty-armhf-ci/95
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-trusty-i386-ci/97

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-ci/563/rebuild

review: Approve (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-01-22:

PASSED: Continuous integration, rev:3891
http://jenkins.qa.ubuntu.com/job/unity-ci/565/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-trusty-amd64-ci/101
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-trusty-armhf-ci/97
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-trusty-i386-ci/99

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-ci/565/rebuild

review: Approve (continuous-integration)

Revision history for this message

Brandon Schaefer (brandontschaefer) wrote on 2014-01-23:

On a side note this only causes 4 conflicts in my panel high dpi code!

Text conflict in unity-shared/PanelStyle.cpp
Text conflict in unity-shared/PanelStyle.h
Text conflict in unity-shared/WindowButtonPriv.h
Text conflict in unity-shared/WindowButtons.cpp
4 conflicts encountered.

So it shouldn't be to bad to merge those over this. Ill get on that tomorrow after I finish going through this!

Revision history for this message

Brandon Schaefer (brandontschaefer) wrote on 2014-01-23:

Download full text (6.5 KiB)

Ignore what I already sent you! Im at about 7-8k (The test looked good!) What I have so far:

You shouldn't need the ';' but i could be wrong

+ set(UNITY_STANDALONE_LADD ${UNITY_STANDALONE_LADD};${UNITY_PROTOCOL_PRIVATE_LIB})

----

To make things a but more readable, we should split this function into 2 smaller functions that accept the frame_geo as an arguments:

+void Window::Impl::UpdateFrame()

1, + if (!frame_), should then just call CreateNewFrame(frame_geo);

2, + if (frame_geo_ != frame_geo), UpdateFrameGeo(frame_geo) ... or something like that, so the function will just look like:

+void Window::Impl::UpdateFrame()
+{
+ auto const& input = win_->input();
+ auto const& server = win_->serverGeometry();
+ nux::Geometry frame_geo(0, 0, server.widthIncBorders() + input.left + input.right,
+ server.heightIncBorders() + input.top + input.bottom);
+
+ if (win_->shaded())
+ frame_geo.height = input.top + input.bottom;
+
+ if (!frame_)
+ CreateNewFrame(frame_geo);
+
+ if (frame_geo_ != frame_geo)
+ UpdateFrameGeo(frame_geo);
+}

Just makes things a bit more readable, imo.

----

An if statement is a bit more readable here.

+ return active() || parent_->scaled() ? mi->active_shadow_pixmap_->texture() : mi->inactive_shadow_pixmap_->texture();

+ return active() || parent_->scaled() ? manager_->active_shadow_radius() : manager_->inactive_shadow_radius();

----

Should be a const pointer, or rather could be a const pointer to const data:

auto const* const texture.

+ auto* texture = ShadowTexture();

+ auto* win = impl_->win_;

----

Are these just for testing? As I see a lot of friend classes...which kind of defeats the purpose of encapsulation. It'll make it hard to track down bugs when all these other classes have access to the private members.... I would much rather provide an access function to these for Read only if thats whats needed. (Vs having read/write of private members.)

+ friend class Manager;

+ friend class Window;

+ friend class Window;
+ friend struct Manager::Impl;

+ friend class Manager;
+ friend struct Window::Impl;

+ friend class InputMixer;

+ friend class Item;

+ friend class decoration::Manager;

+ friend class decoration::Window;
+ friend class decoration::Manager;

----

Why make a global read/write variable for this? This should be part of the class.

+Display* dpy = nullptr;

----

You can simply this:

Ignore what I already sent you! Im at about 7-8k (The test looked good!) What I have so far:

You shouldn't need the ';' but i could be wrong

+ set(UNITY_STANDALONE_LADD ${UNITY_STANDALONE_LADD};${UNITY_PROTOCOL_PRIVATE_LIB})

----

To make things a but more readable, we should split this function into 2 smaller functions that accept the frame_geo as an arguments:

+void Window::Impl::UpdateFrame()

1, +  if (!frame_), should then just call CreateNewFrame(frame_geo);

2, +  if (frame_geo_ != frame_geo), UpdateFrameGeo(frame_geo) ... or something like that, so the function will just look like:

+void Window::Impl::UpdateFrame()
+{
+  auto const& input = win_->input();
+  auto const& server = win_->serverGeometry();
+  nux::Geometry frame_geo(0, 0, server.widthIncBorders() + input.left + input.right,
+                          server.heightIncBorders() + input.top  + input.bottom);
+
+  if (win_->shaded())
+    frame_geo.height = input.top + input.bottom;
+
+  if (!frame_)
+    CreateNewFrame(frame_geo);
+
+  if (frame_geo_ != frame_geo)
+    UpdateFrameGeo(frame_geo);
+}

Just makes things a bit more readable, imo.

----

An if statement is a bit more readable here.

+  return active() || parent_->scaled() ? mi->active_shadow_pixmap_->texture() : mi->inactive_shadow_pixmap_->texture();

+  return active() || parent_->scaled() ? manager_->active_shadow_radius() : manager_->inactive_shadow_radius();

----

Should be a const pointer, or rather could be a const pointer to const data:

auto const* const texture.

+  auto* texture = ShadowTexture();

+  auto* win = impl_->win_;

----

+  friend class Manager;

+  friend class Window;

+  friend class Window;
+  friend struct Manager::Impl;

+  friend class Manager;
+  friend struct Window::Impl;

+  friend class InputMixer;

+  friend class Item;

+  friend class decoration::Manager;

+  friend class decoration::Window;
+  friend class decoration::Manager;

----

Why make a global read/write variable for this? This should be part of the class.

+Display* dpy = nullptr;

----

You can simply this:

+EdgeBorders::EdgeBorders(CompWindow* win)
+{
+  items_.resize(size_t(Edge::Type::Size));
+
+  auto item = std::make_shared<Edge>(win, Edge::Type::TOP);
+  items_[unsigned(Edge::Type::TOP)] = item;
+
+  item = std::make_shared<Edge>(win, Edge::Type::TOP_LEFT);
+  items_[unsigned(Edge::Type::TOP_LEFT)] = item;
+
+  item = std::make_shared<Edge>(win, Edge::Type::TOP_RIGHT);
+  items_[unsigned(Edge::Type::TOP_RIGHT)] = item;
+
+  item = std::make_shared<Edge>(win, Edge::Type::LEFT);
+  items_[unsigned(Edge::Type::LEFT)] = item;
+
+  item = std::make_shared<Edge>(win, Edge::Type::RIGHT);
+  items_[unsigned(Edge::Type::RIGHT)] = item;
+
+  item = std::make_shared<Edge>(win, Edge::Type::BOTTOM);
+  items_[unsigned(Edge::Type::BOTTOM)] = item;
+
+  item = std::make_shared<Edge>(win, Edge::Type::BOTTOM_LEFT);
+  items_[unsigned(Edge::Type::BOTTOM_LEFT)] = item;
+
+  item = std::make_shared<Edge>(win, Edge::Type::BOTTOM_RIGHT);
+  items_[unsigned(Edge::Type::BOTTOM_RIGHT)] = item;
+
+  item = std::make_shared<GrabEdge>(win);
+  items_[unsigned(Edge::Type::GRAB)] = item;
+
+  Relayout();
+}

To:

+EdgeBorders::EdgeBorders(CompWindow* win)
+{
+  auto item = std::make_shared<GrabEdge>(win);
+  items_[unsigned(Edge::Type::GRAB)] = item;
+
+  // Skip the Type::GRAB
+  for (unsigned i = 1; i < Edge::Type::Size; ++i)
+  {
+    item = std::make_shared<Edge>(win, Edge::Type(i));
+    items_[i] = item;
+  }
+
+  Relayout();
+}

If it complains about casing an unsigned to an Enum Class, then you can just remove the class part :). Though thats up to you!

----

For Items::List i would suggest a list vs a dequeue, as a list doesn't have to resize after each erase (plus it has a nice remove function). The only downside, no random iterator access which you use in 2 functions, but it wouldn't be hard to switch over to a std::list. As they are both doubly linked, so adding to the front/back is constant.

+EdgeBorders::EdgeBorders(CompWindow* win)

// Looks a little harder to rework with out random acess...
+void EdgeBorders::Relayout()

Just an idea!

----

You shouldn't need global variables, what happens if another manager is created? (the first one will start using the newly assigned parent). Best to just make member variables...

It also seems you only use the dpi twice, possibly just use screen->dpy() instead of making any kind of variable.

+Display* dpy = nullptr;
+Manager* manager_ = nullptr;

----

I could be missing something...but why does this need to be set in the dtor?

+  enable_add_supported_atoms_ = false;

----

These are already defined the same way in std::(min | max);

+template <typename T> constexpr T max(T a, T b) { return (a > b) ? a : b; }
+template <typename T> constexpr T min(T a, T b) { return (a < b) ? a : b; }

You shouldn't need these :)

----

Should we consider to just return a copy of CompRect?

+CompRect const& Item::Geometry() const
+{
+  return const_cast<Item*>(this)->InternalGeo();
+}

----

Const?

+CompRect& TexturedItem::InternalGeo()
+{
+  return texture_.quad.box;
+}

----

Move code outside of header! Unless its template code! (There could be more i missed)

+  virtual void SetCoords(int x, int y);
+  virtual void SetX(int x) { SetCoords(x, Geometry().y()); }
+  virtual void SetY(int y) { SetCoords(Geometry().x(), y); }
+  virtual void SetSize(int width, int height);
+  virtual void SetWidth(int width) { SetSize(width, Geometry().height()); }
+  virtual void SetHeight(int height) { SetSize(Geometry().width(), height); };
+
+  int GetMaxWidth() const { return max_.width; };
+  int GetMaxHeight() const { return max_.height; };
+  int GetMinWidth() const { return min_.width; };
+  int GetMinHeight() const { return min_.height; };

+  virtual bool IsContainer() const { return false; }

+  std::string GetName() const { return "Item"; }

+  CompRect& InternalGeo() { return rect_; }

----

This causes AP tests to fail on multi monitor. Revert changes to LauncherIcon.cpp please! Otherwise chris will be unhappy :)

-  .add("center", _center[unity::UScreen::GetDefault()->GetMonitorWithMouse()])
+  .add("center", _center[0])

----

Use decoration::Window::Ptr here!

+  std::shared_ptr<decoration::Window> deco_win_;

Revision history for this message

Marco Trevisan (Treviño) (3v1n0) wrote on 2014-01-23:

Download full text (8.3 KiB)

> You shouldn't need the ';' but i could be wrong
>
> + set(UNITY_STANDALONE_LADD ${UNITY_STANDALONE_LADD};${UNITY_PROTOCOL_PRIVATE_LIB})

Oh, I thought I had a linking error without it, but it seems to go...
So, I've removed it. This is with gold btw.

> To make things a but more readable, we should split this function into 2 smaller
> functions that accept the frame_geo as an arguements:
>
> +void Window::Impl::UpdateFrame()
>
> 1, + if (!frame_), should then just call CreateNewFrame(frame_geo);

> 2, + if (frame_geo_ != frame_geo), UpdateFrameGeo(frame_geo) ... or something like that, so the function will just look like:
>
> +void Window::Impl::UpdateFrame()
> +{
> + auto const& input = win_->input();
> + auto const& server = win_->serverGeometry();
> + nux::Geometry frame_geo(0, 0, server.widthIncBorders() + input.left + input.right,
> + server.heightIncBorders() + input.top + input.bottom);
> +
> + if (win_->shaded())
> + frame_geo.height = input.top + input.bottom;
> +
> + if (!frame_)
> + CreateNewFrame(frame_geo);
> +
> + if (frame_geo_ != frame_geo)
> + UpdateFrameGeo(frame_geo);
> +}
>

Ok, sounds good!
I didn't that to keep the frame_geo in the same function without passing it, and
not to reclare input, in UpdateFrameGeo, but it's not really a problem.
I think you're right.

> An if statement is a bit more readable here.

> + return active() || parent_->scaled() ? mi->active_shadow_pixmap_->texture() : mi->inactive_shadow_pixmap_->texture();

> + return active() || parent_->scaled() ? manager_->active_shadow_radius() : manager_->inactive_shadow_radius();

Agreedo, it was smaller at the beginning, then it got bigger :P.

> Should be a const pointer, or rather could be a const pointer to const data:
>
> auto const* const texture.
>
> + auto* texture = ShadowTexture();
>
> + auto* win = impl_->win_;
>
> ----

It doesn't change much, but ok.

> Are these just for testing? As I see a lot of friend classes...which kind of
> defeats the purpose of encaspulation. It'll make it hard to track down bugs when
> all these other classes have access to the private members.... I would much rather
> provide an access funtion to these for Read only if thats whats needed. (Vs having
> read/write of private members.)
>
> + friend class Manager;

Unfortunately I need these to make possible to a Window to access to Impl
functions that are private, but not for manager... I mean, decoration::Manager
is strictly connected to the windows and it has to access to some methods that I
don't want to make visible to everyone... So, unfortunately here we can't use
protected in the way is in java or other languages (access to the members of the
"package"), so I need to use this way... That I don't like, but it's the only
method we have to access to private methods in very limited cases.
For sure the friendship with UnityScreen/UnityWindow can be removed (it was there
for testing), and I could also remove the friendship with the Impl structs, by
just adding some more "public" methods to them, so that there's no private write
access to the members... That's what already happens in fact, but if we want to
protect more I...

> You shouldn't need the ';' but i could be wrong
>
> + set(UNITY_STANDALONE_LADD ${UNITY_STANDALONE_LADD};${UNITY_PROTOCOL_PRIVATE_LIB})

Oh, I thought I had a linking error without it, but it seems to go...
So, I've removed it. This is with gold btw.

> To make things a but more readable, we should split this function into 2 smaller
> functions that accept the frame_geo as an arguements:
>
> +void Window::Impl::UpdateFrame()
>
> 1, +  if (!frame_), should then just call CreateNewFrame(frame_geo);

> 2, +  if (frame_geo_ != frame_geo), UpdateFrameGeo(frame_geo) ... or something like that, so the function will just look like:
>
> +void Window::Impl::UpdateFrame()
> +{
> +  auto const& input = win_->input();
> +  auto const& server = win_->serverGeometry();
> +  nux::Geometry frame_geo(0, 0, server.widthIncBorders() + input.left + input.right,
> +                          server.heightIncBorders() + input.top  + input.bottom);
> +
> +  if (win_->shaded())
> +    frame_geo.height = input.top + input.bottom;
> +
> +  if (!frame_)
> +    CreateNewFrame(frame_geo);
> +
> +  if (frame_geo_ != frame_geo)
> +    UpdateFrameGeo(frame_geo);
> +}
>

Ok, sounds good!
I didn't that to keep the frame_geo in the same function without passing it, and
not to reclare input, in UpdateFrameGeo, but it's not really a problem.
I think you're right.

> An if statement is a bit more readable here.

> +  return active() || parent_->scaled() ? mi->active_shadow_pixmap_->texture() : mi->inactive_shadow_pixmap_->texture();

> +  return active() || parent_->scaled() ? manager_->active_shadow_radius() : manager_->inactive_shadow_radius();

Agreedo, it was smaller at the beginning, then it got bigger :P.

> Should be a const pointer, or rather could be a const pointer to const data:
>
> auto const* const texture.
>
> +  auto* texture = ShadowTexture();
>
> +  auto* win = impl_->win_;
>
> ----

It doesn't change much, but ok.

>
> ----
>
> Why make a global read/write variable for this? This should be part of the class.
>
> +Display* dpy = nullptr;

Yeah, it was there for testing purposes... Also it was an extern variable provided
by DecorationsPriv, and thus accessible by windows, but I can safely remove it now.

>
> You can simply this:
>
> +EdgeBorders::EdgeBorders(CompWindow* win)
> +{
> +  items_.resize(size_t(Edge::Type::Size));
> +
> +  auto item = std::make_shared<Edge>(win, Edge::Type::TOP);
> +  items_[unsigned(Edge::Type::TOP)] = item;
> .....
> +
> +  item = std::make_shared<GrabEdge>(win);
> +  items_[unsigned(Edge::Type::GRAB)] = item;
> +
> +  Relayout();
> +}
>
> To:
>
> +EdgeBorders::EdgeBorders(CompWindow* win)
> +{
> +  auto item = std::make_shared<GrabEdge>(win);
> +  items_[unsigned(Edge::Type::GRAB)] = item;
> +
> +  // Skip the Type::GRAB
> +  for (unsigned i = 1; i < Edge::Type::Size; ++i)
> +  {
> +    item = std::make_shared<Edge>(win, Edge::Type(i));
> +    items_[i] = item;
> +  }
> +
> +  Relayout();
> +}

Mhmhm... This is true. I've addede everything in the loop btw :P

> For Items::List i would suggest a list vs a deque, as a list doesn't have to
> resize after each erase (plus it has a nice remove function). The only downside,
> no random iterator access which you use in 2 functions, but it wouldn't be hard
> to swtich over to a std::list. As they are both doubly linked, so adding to the
> front/back is constant.

As said, the deque performs much better on the tests I've written here. I've been
using lists before, but since I'm not erasing much, but I'm iterating a lot
over elements, it's a case where deque can do things in a better way.

> You shouldn't need global variables, what happens if another manager is created?
> (the first one will start using the newly assigned parent). Best to just make
> memeber v ariables...
>
> +Manager* manager_ = nullptr;

I know... But this is just not possible... As each manager creates the windows
and it can be the only parent of them. Also we only have a Manager for the full
unity session, if someone creates a new manager I guess the problems are more than
these. However, the reason why I used this was to access to textures and parameters.
I could have added a new shared class for this task, but I just didn't want to add
another member class for each window that is just the repetition of the same pointer
when I can have just one. Yeah, it's not saving tons of memory, but every bit counts :P

> It also seems you only use the dpi twice, possibly just use screen->dpy() instead
> of making any kind of variable.
>
> +Display* dpy = nullptr;

Yeah, I've already removed dpy, even it was used also by the Windows, but using
screen is fine enough (until we don't destroy compiz... As screen is nullified
after that UnityScreen is destroyed. But Since decorated Windows are removed
by DecorationsManager, this won't happen).

> These are already defined the same way in std::(min | max);
>
> +template <typename T> constexpr T max(T a, T b) { return (a > b) ? a : b; }
> +template <typename T> constexpr T min(T a, T b) { return (a < b) ? a : b; }
>
> You shouldn't need these :)

I shouldn't, but I do... :/ In std:: they've been defined as functions, not as
constexpr, and I want to use them inside a constexpr (clamp_size), so I've added
them... It's just two lines, so I think it's not really a problem to keep them here

> Should we consider to just return a copy of CompRect?
>
> +CompRect const& Item::Geometry() const
> +{
> +  return const_cast<Item*>(this)->InternalGeo();
> +}

Mh, no... I hate to copy things :).
Especially when it's not needed, and in most of the code here I have the assumption
that this call doesn't copy anything.
And, even this const_cast looks like a bad thing, this safe to do since I'm not
actually touching the InternalGeo (I'm not actually writing), so I preferred to
use this cast internally to expose the function as a const outside. Removing
the const was enough not to cast, but I wanted to make sure that this doesn't
really touch any internal bit.

> Const?
>
> +CompRect& TexturedItem::InternalGeo()
> +{
> +  return texture_.quad.box;
> +}

Nope, InternalGeo is a pure-virtual function that all the Items should expose
to give to the Item the address of the CompRect that is going to be used as its
geometry (since an item has no geometry by itself); and while Geometry returns
it for external clients, InternalGeo is callable internally to set the geometry.
I decided to do this, because some things like TexturedItems have members that
already provide a CompRect that can be used as geometry without duplicating the
main one.

> This causes AP tests to fail on multi monitor. Revert changes to
> LauncherIcon.cpp please! Otherwise chris will be unhappy :)
>
> -  .add("center", _center[unity::UScreen::GetDefault()->GetMonitorWithMouse()])
> +  .add("center", _center[0])

Oh, bzr merge overwritten it... I hope it didn't other mistakes. Sorry for that.

> Use decoration::Window::Ptr here!
>
> +  std::shared_ptr<decoration::Window> deco_win_;

I can't... As decoration::Window is using the forward-declaration, and this is a
little price to pay... I could have used a typedef, but I really think its too
much for just one definition here.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-01-23:

FAILED: Continuous integration, rev:3898
http://jenkins.qa.ubuntu.com/job/unity-ci/566/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-trusty-amd64-ci/102
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-trusty-armhf-ci/98
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-trusty-i386-ci/100/console