Merge lp:~alan-griffiths/miral/workspaces-example into lp:miral

[Alan Griffiths]

[miral-shell] Example workspaces implementation

[miral-shell] Example workspaces implementation

Provides four workspaces:

  o Switch workspace . . . . . . . . . . : Meta-Alt-[F1|F2|F3|F4]
  o Switch workspace taking active window: Meta-Ctrl-[F1|F2|F3|F4]

~~~

Note: the "prerequisite" branch lp:~alan-griffiths/miral/workspace-examples-prerequisites only exists to workaround the launchpad limitation of a single prerequisite.

[Gerry Boland (gerboland)]

Looks quite ok. The old state save/restore and window_state_hidden updates are a bit repetitive, but more belong in the policy implementation, so is ok

review: Approve

[Alan Griffiths (alan-griffiths)]

The active window tracking gets confused here.

Start miral-app

Start an app from the command line (e.g. mir_demo_client_eglplasma)

Move that app to another workspace (e.g. Ctrl-Meta-F4)

Switch back to first workspace (Alt-Meta-F4)

Move the terminal too (e.g. Ctrl-Meta-F4)

Now
1. Both apps show a "focussed" toolbar
2. Alt-Tab doesn't switch between apps

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

If a window in a hidden sets the state from the client then it overrides the hidden state imposed by the workspace.

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

> If a window in a hidden sets the state from the client then it overrides the
> hidden state imposed by the workspace.

Fixed

[Alan Griffiths (alan-griffiths)]

> 1. Both apps show a "focussed" toolbar
> 2. Alt-Tab doesn't switch between apps

Fixed

review: Abstain

[Brandon Schaefer (brandontschaefer)]

An issues I can reproduce:
1) Open egl plasma demo
2) Open egl triangle demo
3) egl triangle should be on top
4) switch work spaces to 2, meta + alt + f2
5) switch back to work space 1, meta + alt + f1
6) (note that egl triangle is ontop still but no window appears to be focused, ie. toolbars are greyed out)
7) switch back to work space 2, meta + alt + f2
8) switch back to work space 1, meta + alt + f1

Expected:
  egl triangle to still be on top and focused

Result:
  egl triangle is below egl plasma and plasma has focus

You can test this by alt+tabbing to get triangle back on top.

[Alan Griffiths (alan-griffiths)]

> An issues I can reproduce:
> 1) Open egl plasma demo
> 2) Open egl triangle demo
> 3) egl triangle should be on top
> 4) switch work spaces to 2, meta + alt + f2
> 5) switch back to work space 1, meta + alt + f1
> 6) (note that egl triangle is ontop still but no window appears to be focused,
> ie. toolbars are greyed out)
> 7) switch back to work space 2, meta + alt + f2
> 8) switch back to work space 1, meta + alt + f1
>
> Expected:
> egl triangle to still be on top and focused
>
> Result:
> egl triangle is below egl plasma and plasma has focus
>
> You can test this by alt+tabbing to get triangle back on top.

Fixed

[Brandon Schaefer (brandontschaefer)]

Randomly seem to be getting garbage for window addresses:
[2017-02-17 07:26:25.497852] miral::Window Management: for_each_workspace_containing window=<80><81>^GÄX^?

[lp:~alan-griffiths/miral/workspaces-example]

522. By Alan Griffiths on 2017-02-17

Add workspace keys

523. By Alan Griffiths on 2017-02-17

merge lp:~alan-griffiths/miral/workspace-examples-prerequisites

524. By Alan Griffiths on 2017-02-17

Fix logging

[Alan Griffiths (alan-griffiths)]

> Randomly seem to be getting garbage for window addresses:
> [2017-02-17 07:26:25.497852] miral::Window Management: for_each_workspace_containing window=<80><81>^GÄX^?

Fixed

[Brandon Schaefer (brandontschaefer)]

1) Open egl plasma
2) Open egl triangle
3) Switch back and forth from f1 <---> f2 a few times with meta + alt
4) Switch to f1 with meta + alt, then using meta + ctrl moving the egl triangle to f2 then back to f1 with out letting go of the hot key
5) Switch to f2 using meta + alt

Expected:
  An emtpy workspace

Result:
  Egl triangle on this workspace, note if you switch to any other workspaces the egl triangle will still be there

[Brandon Schaefer (brandontschaefer)]

Ok easier way:

1) Open egl plasma and egl triangle
2) meta + ctrl + f2
3) meta + ctrl + f1
4) meta + alt + f2

Expected:
  An emtpy workspace

Result:
  App on all workspaces

[lp:~alan-griffiths/miral/workspaces-example]

525. By Alan Griffiths on 2017-02-17

Fix

[Alan Griffiths (alan-griffiths)]

> Ok easier way:
>
> 1) Open egl plasma and egl triangle
> 2) meta + ctrl + f2
> 3) meta + ctrl + f1
> 4) meta + alt + f2
>
> Expected:
> An emtpy workspace
>
> Result:
> App on all workspaces

Fixed

[Alan Griffiths (alan-griffiths)]

> > Ok easier way:
> >
> > 1) Open egl plasma and egl triangle
> > 2) meta + ctrl + f2
> > 3) meta + ctrl + f1
> > 4) meta + alt + f2
> >
> > Expected:
> > An emtpy workspace
> >
> > Result:
> > App on all workspaces
>
> Fixed

Well, mostly fixed. There are some related scenarios I'm still qualifying that are not quite there yet.

But possibly good enough?

[lp:~alan-griffiths/miral/workspaces-example]

526. By Alan Griffiths on 2017-02-17

Fix logging

527. By Alan Griffiths on 2017-02-17

Better solution

[Alan Griffiths (alan-griffiths)]

OK now?

[Brandon Schaefer (brandontschaefer)]

Yup, looking good to me!

review: Approve

[lp:~alan-griffiths/miral/workspaces-example]

528. By Alan Griffiths on 2017-02-20

Comments to clarify code I can't simplify

529. By Alan Griffiths on 2017-02-20

DRY - reduce repetitive workspace visible/hidden code

530. By Alan Griffiths on 2017-02-20

merge :parent

531. By Alan Griffiths on 2017-02-20

merge lp:~alan-griffiths/miral/workspace-examples-prerequisites

=== modified file 'miral-shell/decoration_provider.cpp'
--- miral-shell/decoration_provider.cpp	2017-02-17 11:00:17 +0000
+++ miral-shell/decoration_provider.cpp	2017-02-20 17:31:27 +0000
@@ -169,9 +169,9 @@
             "  o Maximize/restore current window (to display size). : Alt-F11",
             "  o Maximize/restore current window (to display height): Shift-F11",
             "  o Maximize/restore current window (to display width) : Ctrl-F11",
-//            "",
-//            "  o Switch workspace: Meta-Alt-[F1|F2|F3|F4]",
-//            "  o Switch workspace taking active window: Meta-Ctrl-[F1|F2|F3|F4]",
+            "",
+            "  o Switch workspace: Meta-Alt-[F1|F2|F3|F4]",
+            "  o Switch workspace taking active window: Meta-Ctrl-[F1|F2|F3|F4]",
             "",
             "  o To exit: Ctrl-Alt-BkSp",
         };

=== modified file 'miral-shell/titlebar_window_manager.cpp'
--- miral-shell/titlebar_window_manager.cpp	2017-02-20 15:47:49 +0000
+++ miral-shell/titlebar_window_manager.cpp	2017-02-20 17:31:27 +0000
@@ -32,6 +32,18 @@
 namespace
 {
 int const title_bar_height = 12;
+
+struct PolicyData
+{
+    bool in_hidden_workspace{false};
+
+    MirWindowState old_state;
+};
+
+inline PolicyData& policy_data_for(WindowInfo const& info)
+{
+    return *std::static_pointer_cast<PolicyData>(info.userdata());
+}
 }
 
 TitlebarWindowManagerPolicy::TitlebarWindowManagerPolicy(
@@ -43,6 +55,11 @@
     decoration_provider{std::make_unique<DecorationProvider>(tools)}
 {
     launcher.launch("decorations", *decoration_provider);
+
+    for (auto key : {KEY_F1, KEY_F2, KEY_F3, KEY_F4})
+        key_to_workspace[key] = this->tools.create_workspace();
+
+    active_workspace = key_to_workspace[KEY_F1];
 }
 
 TitlebarWindowManagerPolicy::~TitlebarWindowManagerPolicy() = default;
@@ -266,17 +283,25 @@
 {
     CanonicalWindowManagerPolicy::advise_new_window(window_info);
 
+    auto const parent = window_info.parent();
+
     if (decoration_provider->is_titlebar(window_info))
     {
         decoration_provider->advise_new_titlebar(window_info);
 
-        auto const parent = window_info.parent();
-
         if (tools.active_window() == parent)
             decoration_provider->paint_titlebar_for(tools.info_for(parent), 0xFF);
         else
             decoration_provider->paint_titlebar_for(tools.info_for(parent), 0x3F);
     }
+
+    if (!parent)
+        tools.add_tree_to_workspace(window_info.window(), active_workspace);
+    else
+    {
+        if (policy_data_for(tools.info_for(parent)).in_hidden_workspace)
+            apply_workspace_hidden_to(window_info.window());
+    }
 }
 
 void TitlebarWindowManagerPolicy::handle_window_ready(WindowInfo& window_info)
@@ -337,6 +362,36 @@
     auto const scan_code = mir_keyboard_event_scan_code(event);
     auto const modifiers = mir_keyboard_event_modifiers(event) & modifier_mask;
 
+    // Switch workspaces
+    if (action == mir_keyboard_action_down &&
+        modifiers == (mir_input_event_modifier_alt | mir_input_event_modifier_meta))
+    {
+        switch (scan_code)
+        {
+        case KEY_F1:
+        case KEY_F2:
+        case KEY_F3:
+        case KEY_F4:
+            switch_workspace_to(key_to_workspace[scan_code]);
+            return true;
+        }
+    }
+
+    // Switch workspace taking the active window
+    if (action == mir_keyboard_action_down &&
+        modifiers == (mir_input_event_modifier_ctrl | mir_input_event_modifier_meta))
+    {
+        switch (scan_code)
+        {
+        case KEY_F1:
+        case KEY_F2:
+        case KEY_F3:
+        case KEY_F4:
+            switch_workspace_to(key_to_workspace[scan_code], tools.active_window());
+            return true;
+        }
+    }
+
     if (action != mir_keyboard_action_repeat)
         end_resize();
 
@@ -362,7 +417,7 @@
     }
     else if (action == mir_keyboard_action_down && scan_code == KEY_F4)
     {
-        switch (modifiers & modifier_mask)
+        switch (modifiers)
         {
         case mir_input_event_modifier_alt|mir_input_event_modifier_shift:
             if (auto const& window = tools.active_window())
@@ -569,5 +624,131 @@
     if (app_info.application() == decoration_provider->session())
         decoration_provider->place_new_decoration(parameters);
 
+    parameters.userdata() = std::make_shared<PolicyData>();
     return parameters;
 }
+
+void TitlebarWindowManagerPolicy::advise_adding_to_workspace(
+    std::shared_ptr<Workspace> const& workspace, std::vector<Window> const& windows)
+{
+    if (windows.empty())
+        return;
+
+    for (auto const& window : windows)
+    {
+        if (workspace == active_workspace)
+        {
+            apply_workspace_visible_to(window);
+        }
+        else
+        {
+            apply_workspace_hidden_to(window);
+        }
+    }
+}
+
+void TitlebarWindowManagerPolicy::switch_workspace_to(
+    std::shared_ptr<Workspace> const& workspace,
+    Window const& window)
+{
+    if (workspace == active_workspace)
+        return;
+
+    auto const old_active = active_workspace;
+    active_workspace = workspace;
+
+    auto const old_active_window = tools.active_window();
+
+    if (!old_active_window)
+    {
+        // If there's no active window, the first shown grabs focus: get the right one
+        if (auto const ww = workspace_to_active[workspace])
+        {
+            tools.for_each_workspace_containing(ww, [&](std::shared_ptr<miral::Workspace> const& ws)
+                {
+                    if (ws == workspace)
+                    {
+                        apply_workspace_visible_to(ww);
+                    }
+                });
+        }
+    }
+
+    tools.remove_tree_from_workspace(window, old_active);
+    tools.add_tree_to_workspace(window, active_workspace);
+
+    tools.for_each_window_in_workspace(active_workspace, [&](Window const& window)
+        {
+            if (decoration_provider->is_decoration(window))
+                return; // decorations are taken care of automatically
+
+        apply_workspace_visible_to(window);
+        });
+
+    bool hide_old_active = false;
+    tools.for_each_window_in_workspace(old_active, [&](Window const& window)
+        {
+            if (decoration_provider->is_decoration(window))
+                return; // decorations are taken care of automatically
+
+            if (window == old_active_window)
+            {
+                // If we hide the active window focus will shift: do that last
+                hide_old_active = true;
+                return;
+            }
+
+        apply_workspace_hidden_to(window);
+        });
+
+    if (hide_old_active)
+    {
+        apply_workspace_hidden_to(old_active_window);
+
+        // Remember the old active_window when we switch away
+        workspace_to_active[old_active] = old_active_window;
+    }
+}
+
+void TitlebarWindowManagerPolicy::apply_workspace_hidden_to(Window const& window)
+{
+    auto const& window_info = tools.info_for(window);
+    auto& pdata = policy_data_for(window_info);
+    if (!pdata.in_hidden_workspace)
+    {
+        pdata.in_hidden_workspace = true;
+        pdata.old_state = window_info.state();
+
+        WindowSpecification modifications;
+        modifications.state() = mir_window_state_hidden;
+        tools.place_and_size_for_state(modifications, window_info);
+        tools.modify_window(window_info.window(), modifications);
+    }
+}
+
+void TitlebarWindowManagerPolicy::apply_workspace_visible_to(Window const& window)
+{
+    auto const& window_info = tools.info_for(window);
+    auto& pdata = policy_data_for(window_info);
+    if (pdata.in_hidden_workspace)
+    {
+        pdata.in_hidden_workspace = false;
+        WindowSpecification modifications;
+        modifications.state() = pdata.old_state;
+        tools.place_and_size_for_state(modifications, window_info);
+        tools.modify_window(window_info.window(), modifications);
+    }
+}
+
+void TitlebarWindowManagerPolicy::handle_modify_window(WindowInfo& window_info, WindowSpecification const& modifications)
+{
+    auto mods = modifications;
+
+    auto& pdata = policy_data_for(window_info);
+
+    if (pdata.in_hidden_workspace && mods.state().is_set())
+        pdata.old_state = mods.state().consume();
+
+    CanonicalWindowManagerPolicy::handle_modify_window(window_info, mods);
+}
+

=== modified file 'miral-shell/titlebar_window_manager.h'
--- miral-shell/titlebar_window_manager.h	2017-02-20 15:47:49 +0000
+++ miral-shell/titlebar_window_manager.h	2017-02-20 17:31:27 +0000
@@ -20,10 +20,12 @@
 #define MIRAL_SHELL_TITLEBAR_WINDOW_MANAGER_H
 
 #include <miral/canonical_window_manager.h>
+#include <miral/workspace_policy.h>
 
 #include "spinner/splash.h"
 
 #include <chrono>
+#include <map>
 
 namespace miral { class InternalClientLauncher; }
 
@@ -31,7 +33,7 @@
 
 class DecorationProvider;
 
-class TitlebarWindowManagerPolicy : public miral::CanonicalWindowManagerPolicy
+class TitlebarWindowManagerPolicy : public miral::CanonicalWindowManagerPolicy, miral::WorkspacePolicy
 {
 public:
     TitlebarWindowManagerPolicy(miral::WindowManagerTools const& tools, SpinnerSplash const& spinner, miral::InternalClientLauncher const& launcher);
@@ -48,6 +50,8 @@
      *  o Maximize/restore current window (to display size): Alt-F11
      *  o Maximize/restore current window (to display height): Shift-F11
      *  o Maximize/restore current window (to display width): Ctrl-F11
+     *  o Switch workspace . . . . . . . . . . : Meta-Alt-[F1|F2|F3|F4]
+     *  o Switch workspace taking active window: Meta-Ctrl-[F1|F2|F3|F4]
      *  @{ */
     bool handle_pointer_event(MirPointerEvent const* event) override;
     bool handle_touch_event(MirTouchEvent const* event) override;
@@ -63,6 +67,8 @@
     void advise_state_change(miral::WindowInfo const& window_info, MirWindowState state) override;
     void advise_resize(miral::WindowInfo const& window_info, Size const& new_size) override;
     void advise_delete_window(miral::WindowInfo const& window_info) override;
+
+    void handle_modify_window(miral::WindowInfo& window_info, miral::WindowSpecification const& modifications) override;
     /** @} */
 
 protected:
@@ -104,6 +110,23 @@
 
     // Workaround for lp:1627697
     std::chrono::steady_clock::time_point last_resize;
+
+    void advise_adding_to_workspace(
+        std::shared_ptr<miral::Workspace> const& workspace,
+        std::vector<miral::Window> const& windows) override;
+
+    // Switch workspace, taking window (if not null)
+    void switch_workspace_to(
+        std::shared_ptr<miral::Workspace> const& workspace,
+        miral::Window const& window = miral::Window{});
+
+    std::shared_ptr<miral::Workspace> active_workspace;
+    std::map<int, std::shared_ptr<miral::Workspace>> key_to_workspace;
+    std::map<std::shared_ptr<miral::Workspace>, miral::Window> workspace_to_active;
+
+    void apply_workspace_visible_to(miral::Window const& window);
+
+    void apply_workspace_hidden_to(miral::Window const& window);
 };
 
 #endif //MIRAL_SHELL_TITLEBAR_WINDOW_MANAGER_H

=== modified file 'miral/window_management_trace.cpp'
--- miral/window_management_trace.cpp	2017-02-10 15:22:37 +0000
+++ miral/window_management_trace.cpp	2017-02-20 17:31:27 +0000
@@ -558,7 +558,7 @@
 void miral::WindowManagementTrace::for_each_workspace_containing(
     miral::Window const& window, std::function<void(std::shared_ptr<miral::Workspace> const&)> const& callback)
 try {
-    mir::log_info("%s window=%s", __func__, dump_of(window));
+    mir::log_info("%s window=%s", __func__, dump_of(window).c_str());
     wrapped.for_each_workspace_containing(window, callback);
 }
 MIRAL_TRACE_EXCEPTION