Merge lp:~widelands-dev/widelands/bug-1808169-disable-focus into lp:widelands

Proposed by Notabilis
Status: Merged
Merged at revision: 8971
Proposed branch: lp:~widelands-dev/widelands/bug-1808169-disable-focus
Merge into: lp:widelands
Diff against target: 108 lines (+4/-12)
4 files modified
src/ui_basic/button.cc (+2/-4)
src/ui_basic/checkbox.cc (+0/-3)
src/ui_basic/panel.cc (+1/-1)
src/ui_basic/slider.cc (+1/-4)
To merge this branch: bzr merge lp:~widelands-dev/widelands/bug-1808169-disable-focus
Reviewer Review Type Date Requested Status
Toni Förster played Approve
Review via email: mp+361097@code.launchpad.net

Commit message

Allowing hotkey usage while windows are open.

Description of the change

Allowing to use arrow keys while windows (e.g., ware statistics) are open.

Changed two points:
- The UI::Panel no longer grabs the keyboard focus when it is clicked but shouldn't grab it
- Some components (buttons, slider, checkbox) no longer accept the keyboard focus. I don't know why they ever did so, though. It possibly was connected to the Panel being able to switch between components with the Tab key. But since nearly no component handles any keys, I guess it is no (big) loss. But still my main problem with this merge request is: Does someone knows about or finds a functionality that breaks due to the no longer applied keyboard focus?

To post a comment you must log in.
Revision history for this message
bunnybot (widelandsofficial) wrote :

Continuous integration builds have changed state:

Travis build 4347. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/469721440.
Appveyor build 4141. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1808169_disable_focus-4141.

Revision history for this message
GunChleoc (gunchleoc) wrote :

I don't see a problem with how things are now. We will need to reconsider this though if we want to implement full keyboard navigation, e.g. tab through checkboxes and activate/deactivate them by hitting the space bar.

8946. By Toni Förster

merged trunk

Revision history for this message
bunnybot (widelandsofficial) wrote :

Continuous integration builds have changed state:

Travis build 4365. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/472186290.
Appveyor build 4158. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1808169_disable_focus-4158.

8947. By Toni Förster

merged trunk

Revision history for this message
bunnybot (widelandsofficial) wrote :

Continuous integration builds have changed state:

Travis build 4404. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/479143097.
Appveyor build 4195. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1808169_disable_focus-4195.

Revision history for this message
Toni Förster (stonerl) wrote :

Can this be merged? I tested this and found no issues.

review: Approve (played)
Revision history for this message
GunChleoc (gunchleoc) wrote :

Let's have it. We can always revisit it whenever we get around to implementing more keyboard navigation.

@bunnybot merge

Revision history for this message
bunnybot (widelandsofficial) wrote :

Refusing to merge, since Travis is not green. Use @bunnybot merge force for merging anyways.

Travis build 4404. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/479143097.

Revision history for this message
Toni Förster (stonerl) wrote :

Again a Travis hiccup.

@bunnybot merge force

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/ui_basic/button.cc'
2--- src/ui_basic/button.cc 2018-12-13 07:24:01 +0000
3+++ src/ui_basic/button.cc 2019-01-13 20:25:18 +0000
4@@ -60,7 +60,8 @@
5 title_image_(title_image),
6 background_style_(g_gr->styles().button_style(init_style)) {
7 set_thinks(false);
8- set_can_focus(true);
9+ // Don't allow focus
10+ assert(!get_can_focus());
11 }
12
13 Button::Button // for textual buttons. If h = 0, h will resize according to the font's height.
14@@ -143,8 +144,6 @@
15 if (enabled_ == on)
16 return;
17
18- set_can_focus(on);
19-
20 // disabled buttons should look different...
21 if (on)
22 enabled_ = true;
23@@ -316,7 +315,6 @@
24 return false;
25
26 if (enabled_) {
27- focus();
28 grab_mouse(true);
29 pressed_ = true;
30 if (repeating_) {
31
32=== modified file 'src/ui_basic/checkbox.cc'
33--- src/ui_basic/checkbox.cc 2018-12-13 07:24:01 +0000
34+++ src/ui_basic/checkbox.cc 2019-01-13 20:25:18 +0000
35@@ -50,7 +50,6 @@
36 uint16_t h = pic->height();
37 set_desired_size(w, h);
38 set_size(w, h);
39- set_can_focus(true);
40 set_flags(Has_Custom_Picture, true);
41 }
42
43@@ -98,7 +97,6 @@
44 * Args: enabled true if the checkbox should be enabled, false otherwise
45 */
46 void Statebox::set_enabled(bool const enabled) {
47- set_can_focus(enabled);
48 if (((flags_ & Is_Enabled) > 1) && enabled)
49 return;
50
51@@ -177,7 +175,6 @@
52 */
53 bool Statebox::handle_mousepress(const uint8_t btn, int32_t, int32_t) {
54 if (btn == SDL_BUTTON_LEFT && (flags_ & Is_Enabled)) {
55- focus();
56 clicked();
57 return true;
58 }
59
60=== modified file 'src/ui_basic/panel.cc'
61--- src/ui_basic/panel.cc 2018-12-13 07:24:01 +0000
62+++ src/ui_basic/panel.cc 2019-01-13 20:25:18 +0000
63@@ -528,7 +528,7 @@
64 * \return true if the mouseclick was processed, false otherwise
65 */
66 bool Panel::handle_mousepress(const uint8_t btn, int32_t, int32_t) {
67- if (btn == SDL_BUTTON_LEFT) {
68+ if (btn == SDL_BUTTON_LEFT && get_can_focus()) {
69 focus();
70 }
71 return false;
72
73=== modified file 'src/ui_basic/slider.cc'
74--- src/ui_basic/slider.cc 2018-12-13 07:24:01 +0000
75+++ src/ui_basic/slider.cc 2019-01-13 20:25:18 +0000
76@@ -78,7 +78,7 @@
77 bar_size_(bar_size),
78 cursor_size_(cursor_size) {
79 set_thinks(false);
80- set_can_focus(true);
81+ assert(!get_can_focus());
82 calculate_cursor_position();
83 }
84
85@@ -205,7 +205,6 @@
86 if (enabled_ == enabled)
87 return;
88
89- set_can_focus(enabled);
90 enabled_ = enabled;
91 if (!enabled) {
92 pressed_ = false;
93@@ -402,7 +401,6 @@
94 if (btn != SDL_BUTTON_LEFT)
95 return false;
96
97- focus();
98 if (x >= cursor_pos_ && x <= cursor_pos_ + cursor_size_) {
99 // click on cursor
100 cursor_pressed(x);
101@@ -469,7 +467,6 @@
102 if (btn != SDL_BUTTON_LEFT)
103 return false;
104
105- focus();
106 if (y >= cursor_pos_ && y <= cursor_pos_ + cursor_size_) {
107 // click on cursor
108 cursor_pressed(y);

Subscribers

People subscribed via source and target branches

to status/vote changes: