Code review comment for lp:~3v1n0/unity/unity-decorations

> 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 can ensure it.

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