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: +EdgeBorders::EdgeBorders(CompWindow* win) +{ + items_.resize(size_t(Edge::Type::Size)); + + auto item = std::make_shared(win, Edge::Type::TOP); + items_[unsigned(Edge::Type::TOP)] = item; + + item = std::make_shared(win, Edge::Type::TOP_LEFT); + items_[unsigned(Edge::Type::TOP_LEFT)] = item; + + item = std::make_shared(win, Edge::Type::TOP_RIGHT); + items_[unsigned(Edge::Type::TOP_RIGHT)] = item; + + item = std::make_shared(win, Edge::Type::LEFT); + items_[unsigned(Edge::Type::LEFT)] = item; + + item = std::make_shared(win, Edge::Type::RIGHT); + items_[unsigned(Edge::Type::RIGHT)] = item; + + item = std::make_shared(win, Edge::Type::BOTTOM); + items_[unsigned(Edge::Type::BOTTOM)] = item; + + item = std::make_shared(win, Edge::Type::BOTTOM_LEFT); + items_[unsigned(Edge::Type::BOTTOM_LEFT)] = item; + + item = std::make_shared(win, Edge::Type::BOTTOM_RIGHT); + items_[unsigned(Edge::Type::BOTTOM_RIGHT)] = item; + + item = std::make_shared(win); + items_[unsigned(Edge::Type::GRAB)] = item; + + Relayout(); +} To: +EdgeBorders::EdgeBorders(CompWindow* win) +{ + auto item = std::make_shared(win); + items_[unsigned(Edge::Type::GRAB)] = item; + + // Skip the Type::GRAB + for (unsigned i = 1; i < Edge::Type::Size; ++i) + { + item = std::make_shared(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 constexpr T max(T a, T b) { return (a > b) ? a : b; } +template 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(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 deco_win_;