Merge lp:~alan-griffiths/miral/better-resize-in-titlebar-wmp into lp:miral

Proposed by Alan Griffiths
Status: Merged
Approved by: Gerry Boland
Approved revision: 345
Merged at revision: 348
Proposed branch: lp:~alan-griffiths/miral/better-resize-in-titlebar-wmp
Merge into: lp:miral
Prerequisite: lp:~alan-griffiths/miral/BasicWindowManager-modify_window-doesnt-set-policy
Diff against target: 174 lines (+81/-21)
2 files modified
miral-shell/titlebar_window_manager.cpp (+72/-21)
miral-shell/titlebar_window_manager.h (+9/-0)
To merge this branch: bzr merge lp:~alan-griffiths/miral/better-resize-in-titlebar-wmp
Reviewer Review Type Date Requested Status
Gerry Boland (community) Approve
Alan Griffiths Abstain
Review via email: mp+305965@code.launchpad.net

Commit message

Less broken resize handling for surfaces with size constraints in TitlebarWindowManagerPolicy

To post a comment you must log in.
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

*Needs Discussion*

+void TitlebarWindowManagerPolicy::keep_size_within_limits(
+ WindowInfo const& window_info, Displacement& delta, Width& new_width, Height& new_height) const
+{

Following the changes that Daniel just made to WindowInfo::constrain_resize() I wonder if the different size constraints should be split out there so they can be accessed directly. Vis:

    void constrain_resize_by_minmax(mir::geometry::Point& requested_pos, mir::geometry::Size& requested_size) const;

    void constrain_resize_by_aspect(mir::geometry::Point& requested_pos, mir::geometry::Size& requested_size) const;

    void constrain_resize_by_increment(mir::geometry::Point& requested_pos, mir::geometry::Size& requested_size) const;

Being able to apply these independently could remove this logic from TitlebarWindowManagerPolicy. Would it help in general?

review: Needs Information
Revision history for this message
Gerry Boland (gerboland) wrote :

> *Needs Discussion*
>
> +void TitlebarWindowManagerPolicy::keep_size_within_limits(
> + WindowInfo const& window_info, Displacement& delta, Width& new_width,
> Height& new_height) const
> +{
>
> Following the changes that Daniel just made to WindowInfo::constrain_resize()
> I wonder if the different size constraints should be split out there so they
> can be accessed directly. Vis:
>
> void constrain_resize_by_minmax(mir::geometry::Point& requested_pos,
> mir::geometry::Size& requested_size) const;
>
> void constrain_resize_by_aspect(mir::geometry::Point& requested_pos,
> mir::geometry::Size& requested_size) const;
>
> void constrain_resize_by_increment(mir::geometry::Point& requested_pos,
> mir::geometry::Size& requested_size) const;
>
> Being able to apply these independently could remove this logic from
> TitlebarWindowManagerPolicy. Would it help in general?

In all honesty, I cannot think of a purpose for such an API in unity8 just yet. I think it is best to hold off until we definitely see we need it.

Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

> In all honesty, I cannot think of a purpose for such an API in unity8 just
> yet. I think it is best to hold off until we definitely see we need it.

Fair enough. In that case I'd like to land this MP "as is" - even though it duplicates logic it does improve on the current, broken, resizing of size-constrained surfaces.

review: Abstain
Revision history for this message
Gerry Boland (gerboland) wrote :

Yep, works good

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'miral-shell/titlebar_window_manager.cpp'
2--- miral-shell/titlebar_window_manager.cpp 2016-08-15 08:47:18 +0000
3+++ miral-shell/titlebar_window_manager.cpp 2016-09-16 14:25:46 +0000
4@@ -101,12 +101,35 @@
5 }
6 }
7 }
8+
9+ if (resizing && !is_resize_event)
10+ end_resize();
11
12 resizing = is_resize_event;
13 old_cursor = cursor;
14 return consumes_event;
15 }
16
17+void TitlebarWindowManagerPolicy::end_resize()
18+{
19+ if (auto window = tools.active_window())
20+ {
21+ auto& window_info = tools.info_for(window);
22+
23+ auto new_size = window.size();
24+ auto new_pos = window.top_left();
25+ window_info.constrain_resize(new_pos, new_size);
26+
27+ WindowSpecification modifications;
28+ modifications.top_left() = new_pos;
29+ modifications.size() = new_size;
30+ tools.modify_window(window_info, modifications);
31+ }
32+
33+ resizing = false;
34+ pinching = false;
35+}
36+
37 bool TitlebarWindowManagerPolicy::handle_touch_event(MirTouchEvent const* event)
38 {
39 auto const count = mir_touch_event_point_count(event);
40@@ -182,23 +205,23 @@
41 auto const delta_width = DeltaX{touch_pinch_width - old_touch_pinch_width};
42 auto const delta_height = DeltaY{touch_pinch_height - old_touch_pinch_height};
43
44- auto const delta_x = DeltaX{touch_pinch_left - old_touch_pinch_left};
45- auto const delta_y = DeltaY{touch_pinch_top - old_touch_pinch_top};
46-
47- auto const new_width = std::max(old_size.width + delta_width, Width{5});
48- auto const new_height = std::max(old_size.height + delta_height, Height{5});
49-
50- auto new_pos = window.top_left() + delta_x + delta_y;
51+ auto new_width = std::max(old_size.width + delta_width, Width{5});
52+ auto new_height = std::max(old_size.height + delta_height, Height{5});
53+ Displacement delta{
54+ DeltaX{touch_pinch_left - old_touch_pinch_left},
55+ DeltaY{touch_pinch_top - old_touch_pinch_top}};
56+
57+ auto& window_info = tools.info_for(window);
58+ keep_size_within_limits(window_info, delta, new_width, new_height);
59+
60+ auto new_pos = window.top_left() + delta;
61 Size new_size{new_width, new_height};
62
63- auto& window_info = tools.info_for(window);
64-
65- window_info.constrain_resize(new_pos, new_size);
66-
67 WindowSpecification modifications;
68 modifications.top_left() = new_pos;
69 modifications.size() = new_size;
70 tools.modify_window(window_info, modifications);
71+ pinching = true;
72 }
73 consumes_event = true;
74 }
75@@ -209,6 +232,9 @@
76 tools.select_active_window(window);
77 }
78
79+ if (!consumes_event && pinching)
80+ end_resize();
81+
82 old_cursor = cursor;
83 old_touch_pinch_top = touch_pinch_top;
84 old_touch_pinch_left = touch_pinch_left;
85@@ -285,6 +311,9 @@
86 auto const scan_code = mir_keyboard_event_scan_code(event);
87 auto const modifiers = mir_keyboard_event_modifiers(event) & modifier_mask;
88
89+ if (action != mir_keyboard_action_repeat)
90+ end_resize();
91+
92 if (action == mir_keyboard_action_down && scan_code == KEY_F11)
93 {
94 switch (modifiers)
95@@ -450,6 +479,22 @@
96 auto new_width = old_pos.size.width + x_sign * delta.dx;
97 auto new_height = old_pos.size.height + y_sign * delta.dy;
98
99+ keep_size_within_limits(window_info, delta, new_width, new_height);
100+
101+ Size new_size{new_width, new_height};
102+ Point new_pos = top_left + left_resize*delta.dx + top_resize*delta.dy;
103+
104+ WindowSpecification modifications;
105+ modifications.top_left() = new_pos;
106+ modifications.size() = new_size;
107+ tools.modify_window(tools.info_for(window), modifications);
108+
109+ return true;
110+}
111+
112+void TitlebarWindowManagerPolicy::keep_size_within_limits(
113+ WindowInfo const& window_info, Displacement& delta, Width& new_width, Height& new_height) const
114+{
115 auto const min_width = std::max(window_info.min_width(), Width{5});
116 auto const min_height = std::max(window_info.min_height(), Height{5});
117
118@@ -467,16 +512,22 @@
119 delta.dy = DeltaY{0};
120 }
121
122- Size new_size{new_width, new_height};
123- Point new_pos = top_left + left_resize*delta.dx + top_resize*delta.dy;
124- window_info.constrain_resize(new_pos, new_size);
125-
126- WindowSpecification modifications;
127- modifications.top_left() = new_pos;
128- modifications.size() = new_size;
129- tools.modify_window(tools.info_for(window), modifications);
130-
131- return true;
132+ auto const max_width = window_info.max_width();
133+ auto const max_height = window_info.max_height();
134+
135+ if (new_width > max_width)
136+ {
137+ new_width = max_width;
138+ if (delta.dx < DeltaX{0})
139+ delta.dx = DeltaX{0};
140+ }
141+
142+ if (new_height > max_height)
143+ {
144+ new_height = max_height;
145+ if (delta.dy < DeltaY{0})
146+ delta.dy = DeltaY{0};
147+ }
148 }
149
150 WindowSpecification TitlebarWindowManagerPolicy::place_new_surface(
151
152=== modified file 'miral-shell/titlebar_window_manager.h'
153--- miral-shell/titlebar_window_manager.h 2016-08-10 11:48:02 +0000
154+++ miral-shell/titlebar_window_manager.h 2016-09-16 14:25:46 +0000
155@@ -85,10 +85,19 @@
156 int old_touch_pinch_left = 0;
157 int old_touch_pinch_width = 0;
158 int old_touch_pinch_height = 0;
159+ bool pinching = false;
160
161 SpinnerSplash const spinner;
162
163 std::unique_ptr<TitlebarProvider> const titlebar_provider;
164+
165+ void end_resize();
166+
167+ void keep_size_within_limits(
168+ miral::WindowInfo const& window_info,
169+ Displacement& delta,
170+ Width& new_width,
171+ Height& new_height) const;
172 };
173
174 #endif //MIRAL_SHELL_TITLEBAR_WINDOW_MANAGER_H

Subscribers

People subscribed via source and target branches