Make frame attributes writable (herbstluftwm#1201)

The frame attributes of FrameLeaf and FrameSplit are now writable and
allow to adjust the frame locally. As usual, the test cases consume more
lines than the actual source code accomplishing this change.

In the source code, the getTag() function seemed to be unused.
Unfortunately, I had to remove the 'const' qualifier from getFraction()
because otherwise the DynAttribute_ constructor failed to match.

This is another step towards a full implementation of frame objects (#2)
and this fixes #7.

Author: t-wissmann

NEWS

@@ -13,6 +13,8 @@ Current git version
   * Do not draw frame background behind clients (so for semi-transparent
     client decorations, one does not see the frame decoration behind but the
     wallpaper instead)
+  * The frame attributes ('selection', 'algorithm', 'fraction', 'split_type')
+    are now writable.
   * The setting 'monitors_locked' is now explicitly an unsigned integer.
   * The 'shift' command now moves the window to a neighboured monitor if
     the window cannot be moved within a tag in the desired direction.

src/layout.cpp

@@ -42,8 +42,8 @@ Frame::~Frame() = default;
 FrameLeaf::FrameLeaf(HSTag* tag, Settings* settings, weak_ptr<FrameSplit> parent)
     : Frame(tag, settings, parent)
     , client_count_(this, "client_count", [this]() {return clientCount(); })
-    , selectionAttr_(this, "selection", &FrameLeaf::getSelection)
-    , algorithmAttr_(this, "algorithm", &FrameLeaf::getLayout)
+    , selectionAttr_(this, "selection", &FrameLeaf::getSelection, &FrameLeaf::userSetsSelection)
+    , algorithmAttr_(this, "algorithm", &FrameLeaf::getLayout, &FrameLeaf::userSetsLayout)
 {
     auto l = settings->default_frame_layout();
     if (l >= layoutAlgorithmCount()) {
@@ -58,9 +58,9 @@ FrameSplit::FrameSplit(HSTag* tag, Settings* settings, weak_ptr<FrameSplit> pare
                        FixPrecDec fraction, SplitAlign align, shared_ptr<Frame> a,
                        shared_ptr<Frame> b)
              : Frame(tag, settings, parent)
-             , splitTypeAttr_(this, "split_type", &FrameSplit::getAlign)
-             , fractionAttr_(this, "fraction", &FrameSplit::getFraction)
-             , selectionAttr_(this, "selection", [this]() { return selection_;})
+             , splitTypeAttr_(this, "split_type", &FrameSplit::getAlign, &FrameSplit::userSetsSplitType)
+             , fractionAttr_(this, "fraction", &FrameSplit::getFraction, &FrameSplit::userSetsFraction)
+             , selectionAttr_(this, "selection", &FrameSplit::getSelection, &FrameSplit::userSetsSelection)
              , aLink_(*this, "0")
              , bLink_(*this, "1")
 {
@@ -158,6 +158,30 @@ shared_ptr<FrameSplit> FrameSplit::thisSplit() {
     return dynamic_pointer_cast<FrameSplit>(shared_from_this());
 }
 
+string FrameSplit::userSetsSplitType(SplitAlign align)
+{
+    align_ = align;
+    relayout();
+    return {};
+}
+
+string FrameSplit::userSetsSelection(int idx)
+{
+    if (idx < 0 || idx > 1) {
+        return "index out of range";
+    }
+    selection_ = idx;
+    relayout();
+    return {};
+}
+
+string FrameSplit::userSetsFraction(FixPrecDec fraction)
+{
+    setFraction(fraction);
+    relayout();
+    return {};
+}
+
 string Frame::frameIndex() const
 {
     auto p = parent_.lock();
@@ -171,6 +195,16 @@ string Frame::frameIndex() const
     }
 }
 
+/**
+ * @brief trigger relayouting of the frame tree.
+ * This should only be called if the user changes something
+ * via the attribute system.
+ */
+void Frame::relayout()
+{
+    tag_->needsRelayout_.emit();
+}
+
 TilingResult FrameLeaf::layoutLinear(Rectangle rect, bool vertical) {
     TilingResult res;
     auto cur = rect;
@@ -667,6 +701,29 @@ int FrameLeaf::getInnerNeighbourIndex(Direction direction) {
     return index;
 }
 
+string FrameLeaf::userSetsLayout(LayoutAlgorithm algo)
+{
+    setLayout(algo);
+    relayout();
+    return {};
+}
+
+string FrameLeaf::userSetsSelection(int index)
+{
+    if (clients.empty() && index == 0) {
+        // if there is no client in this frame
+        // then index 0 is the fallback value and
+        // there is nothing to be done here
+        return {};
+    }
+    if (index < 0 || static_cast<size_t>(index) >= clients.size()) {
+        return "index out of range";
+    }
+    setSelection(index);
+    relayout();
+    return {};
+}
+
 void FrameLeaf::moveClient(int new_index) {
     swap(clients[new_index], clients[selection]);
     selection = new_index;

src/layout.h

@@ -53,8 +53,6 @@ class Frame : public std::enable_shared_from_this<Frame>, public Object {
     // count the number of splits to the root with alignment "align"
     virtual int splitsToRoot(SplitAlign align);
 
-    HSTag* getTag() { return tag_; };
-
     void setVisibleRecursive(bool visible);
 
     /*! a case distinction on the type of tree node. If `this` is a
@@ -85,6 +83,7 @@ class Frame : public std::enable_shared_from_this<Frame>, public Object {
     virtual std::shared_ptr<FrameSplit> isSplit() { return std::shared_ptr<FrameSplit>(); };
     virtual std::shared_ptr<FrameLeaf> isLeaf() { return std::shared_ptr<FrameLeaf>(); };
 protected:
+    void relayout();
     void foreachClient(ClientAction action);
     HSTag* tag_;
     Settings* settings_;
@@ -136,6 +135,8 @@ class FrameLeaf : public Frame, public FrameDataLeaf {
     DynAttribute_<int> selectionAttr_;
     DynAttribute_<LayoutAlgorithm> algorithmAttr_;
 private:
+    std::string userSetsLayout(LayoutAlgorithm algo);
+    std::string userSetsSelection(int index);
     friend class FrameDecoration;
     friend class FrameTree;
     // layout algorithms
@@ -175,19 +176,23 @@ class FrameSplit : public Frame, public FrameDataSplit<Frame> {
     void swapChildren();
     void adjustFraction(FixPrecDec delta);
     void setFraction(FixPrecDec fraction);
-    FixPrecDec getFraction() const { return fraction_; }
+    FixPrecDec getFraction() { return fraction_; }
     static FixPrecDec clampFraction(FixPrecDec fraction);
     std::shared_ptr<FrameSplit> thisSplit();
     std::shared_ptr<FrameSplit> isSplit() override { return thisSplit(); }
     SplitAlign getAlign() { return align_; }
     void swapSelection() { selection_ = selection_ == 0 ? 1 : 0; }
     void setSelection(int s) { selection_ = s; }
+    int getSelection() { return selection_; }
     DynAttribute_<SplitAlign> splitTypeAttr_;
     DynAttribute_<FixPrecDec> fractionAttr_;
     DynAttribute_<int> selectionAttr_;
     Link_<Frame> aLink_;
     Link_<Frame> bLink_;
 private:
+    std::string userSetsSplitType(SplitAlign align);
+    std::string userSetsFraction(FixPrecDec fraction);
+    std::string userSetsSelection(int idx);
     friend class FrameTree;
 };
 

tests/test_layout.py

@@ -1527,3 +1527,151 @@ def test_focus_shift_completion(hlwm):
         # actually, passing both -i and -e makes no sense,
         # but ArgParse does not know that the flags exclude each other
         hlwm.command_has_all_args([cmd, 'down', '-i', '-e'])
+
+
+def test_frame_leaf_selection_change(hlwm):
+    """test the attribute FrameLeaf::selection"""
+    clients = hlwm.create_clients(3)
+
+    def layout(idx):
+        return f"(clients vertical:{idx} {clients[0]} {clients[1]} {clients[2]})"
+
+    hlwm.call(['load', layout(0)])
+
+    for i in range(0, 3):
+        hlwm.attr.tags.focus.tiling.focused_frame.selection = i
+        assert hlwm.attr.clients.focus.winid() == clients[i]
+        assert hlwm.call('dump').stdout == layout(i)
+
+
+def test_frame_leaf_selection_if_empty(hlwm):
+    assert hlwm.attr.tags.focus.tiling.root.selection() == '0'
+    hlwm.attr.tags.focus.tiling.root.selection = 0
+    assert hlwm.attr.tags.focus.tiling.root.selection() == '0'
+
+    hlwm.call_xfail('attr tags.focus.tiling.root.selection 1') \
+        .expect_stderr('out of range')
+
+    hlwm.call_xfail('attr tags.focus.tiling.root.selection 2') \
+        .expect_stderr('out of range')
+
+    hlwm.call_xfail('attr tags.focus.tiling.root.selection -1') \
+        .expect_stderr('out of range')
+
+
+def test_frame_split_selection_change(hlwm):
+    """test the attribute FrameSplit::selection"""
+    clients = hlwm.create_clients(2)
+
+    def layout(idx):
+        return normalize_layout_string(f"""
+            (split horizontal:0.5:{idx}
+                (clients vertical:0 {clients[0]})
+                (clients vertical:0 {clients[1]}))
+        """)
+
+    hlwm.call(['load', layout(0)])
+
+    for i in [0, 1]:
+        hlwm.attr.tags.focus.tiling.root.selection = i
+        assert hlwm.attr.clients.focus.winid() == clients[i]
+        assert hlwm.call('dump').stdout == layout(i)
+
+
+def test_frame_selection_invalid_arg(hlwm):
+    hlwm.create_clients(6)  # 6 clients
+    hlwm.call('split explode')  # 2 frames, so 3 clients per frame
+    hlwm.call('dump')
+
+    # for frame splits, the selection can be 0 or 1
+    hlwm.call('attr tags.focus.tiling.root.selection 0')  # no failure
+    hlwm.call('attr tags.focus.tiling.root.selection 1')  # no failure
+    hlwm.call_xfail('attr tags.focus.tiling.root.selection -1') \
+        .expect_stderr('out of range')
+    hlwm.call_xfail('attr tags.focus.tiling.root.selection 2') \
+        .expect_stderr('out of range')
+
+    # for frame leafs, the selection only must be lower than the client count
+    hlwm.call('attr tags.focus.tiling.focused_frame.selection 0')  # no failure
+    hlwm.call('attr tags.focus.tiling.focused_frame.selection 1')  # no failure
+    hlwm.call('attr tags.focus.tiling.focused_frame.selection 2')  # no failure
+    hlwm.call_xfail('attr tags.focus.tiling.focused_frame.selection -1') \
+        .expect_stderr('out of range')
+    hlwm.call_xfail('attr tags.focus.tiling.focused_frame.selection 3') \
+        .expect_stderr('out of range')
+
+
+def test_frame_split_attribute_size_changes(hlwm, x11):
+    """
+    Test that changing the attributes 'fraction' or 'split_type' of FrameSplit
+    affects window sizes
+    """
+    # two clients side by side:
+    win1, winid1 = x11.create_client()
+    win2, winid2 = x11.create_client()
+    layout = f'(split horizontal:0.5:0 (clients max:0 {winid1}) (clients max:0 {winid2}))'
+    layout = normalize_layout_string(layout)
+    for attr, value in [('fraction', '0.6'), ('split_type', 'vertical')]:
+        hlwm.call(['load', layout])
+        x11.sync_with_hlwm()
+        width1 = win1.get_geometry().width
+        width2 = win2.get_geometry().width
+
+        hlwm.attr.tags.focus.tiling.root[attr] = value
+        assert hlwm.attr.tags.focus.tiling.root[attr]() == value
+        assert hlwm.call('dump').stdout.strip() != layout
+
+        x11.sync_with_hlwm()
+        # both updated attributes must lead to an increase of width of
+        # the first client:
+        assert width1 < win1.get_geometry().width
+        assert width2 != win2.get_geometry().width
+
+
+def test_frame_split_fraction_invalid_arg(hlwm):
+    hlwm.call(['load', '(split horizontal:0.5:0 (clients max:0) (clients max:0))'])
+
+    # test values that clamp to 0.1
+    for val in ['0.0', '0', '-0.1', '0.05', '0.1', '0.09', '-5', '-0.11']:
+        # reset the fraction
+        hlwm.attr.tags.focus.tiling.root.fraction = '0.5'
+        assert hlwm.attr.tags.focus.tiling.root.fraction() == '0.5'
+
+        hlwm.attr.tags.focus.tiling.root.fraction = val
+        assert hlwm.attr.tags.focus.tiling.root.fraction() == '0.1'
+
+    # test values that clamp to 0.9
+    for val in ['12', '1', '0.9', '0.95', '0.9', '0.91', '12312', '+23']:
+        # reset the fraction
+        hlwm.attr.tags.focus.tiling.root.fraction = '0.87'
+        assert hlwm.attr.tags.focus.tiling.root.fraction() == '0.87'
+
+        hlwm.attr.tags.focus.tiling.root.fraction = val
+        assert hlwm.attr.tags.focus.tiling.root.fraction() == '0.9'
+
+
+def test_frame_leaf_algorithm_change(hlwm, x11):
+    """
+    Test that changing the layout algorithm affects window sizes
+    """
+    # two clients below each other
+    win1, winid1 = x11.create_client()
+    win2, winid2 = x11.create_client()
+    hlwm.call('set_layout vertical')
+
+    geom1_before = win1.get_geometry()
+    geom2_before = win2.get_geometry()
+    assert geom1_before.width == geom2_before.width
+    assert geom1_before.height == geom2_before.height
+
+    # put them side by side
+    hlwm.attr.tags.focus.tiling.root.algorithm = 'horizontal'
+
+    x11.sync_with_hlwm()
+    geom1_now = win1.get_geometry()
+    geom2_now = win2.get_geometry()
+    # side by side implies: width decreases but height increases
+    assert geom1_before.width > geom1_now.width
+    assert geom1_before.height < geom1_now.height
+    assert geom1_now.width == geom2_now.width
+    assert geom1_now.height == geom2_now.height