Mir

Merge lp:~mir-team/mir/cursor-spike-phase-7-name-some-cursors into lp:mir

Proposed by Robert Carr
Status: Merged
Approved by: Robert Carr
Approved revision: no longer in the source branch.
Merged at revision: 1782
Proposed branch: lp:~mir-team/mir/cursor-spike-phase-7-name-some-cursors
Merge into: lp:mir
Prerequisite: lp:~mir-team/mir/cursor-spike-phase-6-load-x-cursors
Diff against target: 391 lines (+283/-17)
7 files modified
examples/CMakeLists.txt (+6/-0)
examples/cursors_demo_client.c (+75/-0)
include/client/mir_toolkit/mir_cursor_configuration.h (+3/-2)
include/shared/mir_toolkit/common.h (+2/-11)
include/shared/mir_toolkit/cursors.h (+115/-0)
src/client/mir_cursor_api.cpp (+13/-0)
src/server/input/xcursor_loader.cpp (+69/-4)
To merge this branch: bzr merge lp:~mir-team/mir/cursor-spike-phase-7-name-some-cursors
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Approve
Kevin DuBois (community) Approve
Cemil Azizoglu (community) Approve
Alan Griffiths Approve
Review via email: mp+225242@code.launchpad.net

This proposal supersedes a proposal from 2014-07-01.

Commit message

Provide some symbolic names for cursors.

Description of the change

Add some cursor names and translate to the standard xcursor names. The xcursor names are quite a mess (caret->"xterm" pointing hand->"hand2"),etc...so we want to provide clients something better than this.

Inspired by: http://qt-project.org/doc/qt-4.8/qt.html#CursorShape-enum

Adds a demo client which makes a small window and cycles through cursors.

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal

FAILED: Continuous integration, rev:1558
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~mir-team/mir/cursor-spike-phase-7-name-some-cursors/+merge/225204/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2049/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/787
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/793
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/786
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/570
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/570/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/568
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/568/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/2393
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/2393/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1940
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/9119

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/2049/rebuild

review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

181 +/* This is C code. Not C++. */
...
197 +extern char const* const mir_arrow_cursor_name;

I thought in "in C code" the * goes to the right?

review: Needs Information
Revision history for this message
Kevin DuBois (kdub) wrote :

needs info:
include/shared/mir_toolkit/cursors.h
Is providing a bunch of names as part of the API flexible enough?

nit:
89 + configure_cursor(mir_eglapp_native_surface(), cursor_index);
90 +
91 + // Of course we are subject to hypothetical overflow here, but it is of no consequence
92 + cursor_index++;

I'd be just as happy with:
configure_cursor(mir_eglapp_native_surface(), cursor_index++);

review: Needs Information
Revision history for this message
Robert Carr (robertcarr) wrote :

Alan:

Are you just referring to the whitespace? i.e. extern char const *const ? I couldn't find this in the style guide...I feel like I must be doing something stupid here :p

Kevin:
Removed strangely OCD comment lol.

In addition to providing a bunch of names we will need to support custom upload of cursor data. We want to provide some way for apps to request cursors from the system theme though. In X, the situation is a little hairy. One can request cursors from the X cursor theme, but only using an obscure series of names, for which there is literally no official documentation (in addition the names are surprising "Caret" is "xterm"! Toolkits improve on this by creating a set of enums etc, for standard cursors. This list was based off of a survey of Qt, GTK, and cursors available in the Ubuntu default theme. I think removing the responsibility from toolkits to maintain this strange list of names...is an improvement. I think names are more appropriate than enums as there is no serious use case to enumerate available cursors.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

I'm not convinced an if..elseif chain is the most efficient lookup, but OK

review: Approve
Revision history for this message
Robert Carr (robertcarr) wrote :

Of course :) I think it's an infrequent operation though so I wasn't too worried and it didnt seem worth to use a better string lookup.

I thought about using an enum but strings felt marginally more flexible, and ultimately more clear (cursor shapes aren't really meaningfully enumerable?)

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Cemil Azizoglu (cemil-azizoglu) wrote :

LGTM...

o Not sure what the following comment is... for sarcasm?
323 + return "xterm"; // Yep

review: Approve
Revision history for this message
Kevin DuBois (kdub) wrote :

I still hope that we move away from the x cursor names, but at least I understand what's going on a bit better. +1

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) :
review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'examples/CMakeLists.txt'
--- examples/CMakeLists.txt 2014-06-11 16:11:32 +0000
+++ examples/CMakeLists.txt 2014-07-21 15:42:48 +0000
@@ -36,6 +36,12 @@
36target_link_libraries(mir_demo_client_eglplasma36target_link_libraries(mir_demo_client_eglplasma
37 eglapp37 eglapp
38)38)
39add_executable(mir_demo_client_cursors
40 cursors_demo_client.c
41)
42target_link_libraries(mir_demo_client_cursors
43 eglapp
44)
3945
40add_executable(mir_demo_client_basic46add_executable(mir_demo_client_basic
41 basic.c47 basic.c
4248
=== added file 'examples/cursors_demo_client.c'
--- examples/cursors_demo_client.c 1970-01-01 00:00:00 +0000
+++ examples/cursors_demo_client.c 2014-07-21 15:42:48 +0000
@@ -0,0 +1,75 @@
1/*
2 * Copyright © 2014 Canonical LTD
3 *
4 * This program is free software: you can redistribute it and/or modify
5 * it under the terms of the GNU General Public License version 3 as
6 * published by the Free Software Foundation.
7 *
8 * This program is distributed in the hope that it will be useful,
9 * but WITHOUT ANY WARRANTY; without even the implied warranty of
10 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
11 * GNU General Public License for more details.
12 *
13 * You should have received a copy of the GNU General Public License
14 * along with this program. If not, see <http://www.gnu.org/licenses/>.
15 *
16 * Author: Robert Carr <robert.carr@canonical.com>
17 */
18
19#include "mir_toolkit/mir_client_library.h"
20
21#include "eglapp.h"
22#include <stdio.h>
23#include <unistd.h>
24#include <GLES2/gl2.h>
25
26void configure_cursor(MirSurface *surface, unsigned int cursor_index)
27{
28 char const *const cursors[] = {
29 mir_arrow_cursor_name,
30 mir_busy_cursor_name,
31 mir_caret_cursor_name,
32 mir_pointing_hand_cursor_name,
33 mir_open_hand_cursor_name,
34 mir_closed_hand_cursor_name,
35 mir_horizontal_resize_cursor_name,
36 mir_vertical_resize_cursor_name,
37 mir_diagonal_resize_bottom_to_top_cursor_name,
38 mir_diagonal_resize_top_to_bottom_cursor_name,
39 mir_omnidirectional_resize_cursor_name,
40 mir_vsplit_resize_cursor_name,
41 mir_hsplit_resize_cursor_name
42 };
43
44 size_t num_cursors = sizeof(cursors)/sizeof(*cursors);
45 size_t real_index = cursor_index % num_cursors;
46
47 MirCursorConfiguration *conf = mir_cursor_configuration_from_name(cursors[real_index]);
48
49 mir_wait_for(mir_surface_configure_cursor(surface, conf));
50
51 mir_cursor_configuration_destroy(conf);
52}
53
54int main(int argc, char *argv[])
55{
56 unsigned int width = 128, height = 128;
57
58 if (!mir_eglapp_init(argc, argv, &width, &height))
59 return 1;
60
61 glClearColor(0.5, 0.5, 0.5, mir_eglapp_background_opacity);
62 glClear(GL_COLOR_BUFFER_BIT);
63 mir_eglapp_swap_buffers();
64
65 unsigned int cursor_index = 0;
66 while (mir_eglapp_running())
67 {
68 configure_cursor(mir_eglapp_native_surface(), cursor_index++);
69 sleep(1);
70 }
71
72 mir_eglapp_shutdown();
73
74 return 0;
75}
076
=== modified file 'include/client/mir_toolkit/mir_cursor_configuration.h'
--- include/client/mir_toolkit/mir_cursor_configuration.h 2014-06-17 22:07:15 +0000
+++ include/client/mir_toolkit/mir_cursor_configuration.h 2014-07-21 15:42:48 +0000
@@ -42,8 +42,9 @@
4242
43/**43/**
44 * Returns a new MirCursorConfiguration representing a named cursor44 * Returns a new MirCursorConfiguration representing a named cursor
45 * from the system cursor theme. Currently only the symbolic values,45 * from the system cursor theme. Symbolic cursor names, such as
46 * mir_default_cursor_name and mir_disabled_cursor_name are supported46 * mir_default_cursor_name and mir_caret_cursor_name are available
47 * see (mir_toolkit/cursors.h).
47 * as input.48 * as input.
48 * \param [in] name The cursor name49 * \param [in] name The cursor name
49 * \return A cursor parameters object which must be passed50 * \return A cursor parameters object which must be passed
5051
=== modified file 'include/shared/mir_toolkit/common.h'
--- include/shared/mir_toolkit/common.h 2014-07-08 17:16:01 +0000
+++ include/shared/mir_toolkit/common.h 2014-07-21 15:42:48 +0000
@@ -21,6 +21,8 @@
21#ifndef MIR_COMMON_H_21#ifndef MIR_COMMON_H_
22#define MIR_COMMON_H_22#define MIR_COMMON_H_
2323
24#include <mir_toolkit/cursors.h>
25
24/**26/**
25 * \addtogroup mir_toolkit27 * \addtogroup mir_toolkit
26 * @{28 * @{
@@ -137,17 +139,6 @@
137 mir_orientation_right = 270139 mir_orientation_right = 270
138} MirOrientation;140} MirOrientation;
139141
140/**
141 * A special cursor name for use with mir_cursor_configuration_from_name
142 * representing the system default cursor.
143 */
144extern char const *const mir_default_cursor_name;
145/**
146 * A special cursor name for use with mir_cursor_configuration_from_name
147 * representing a disabled cursor image.
148 */
149extern char const *const mir_disabled_cursor_name;
150
151/**@}*/142/**@}*/
152143
153#endif144#endif
154145
=== added file 'include/shared/mir_toolkit/cursors.h'
--- include/shared/mir_toolkit/cursors.h 1970-01-01 00:00:00 +0000
+++ include/shared/mir_toolkit/cursors.h 2014-07-21 15:42:48 +0000
@@ -0,0 +1,115 @@
1/*
2 * Cursor name definitions.
3 *
4 * Copyright © 2014 Canonical Ltd.
5 *
6 * This program is free software: you can redistribute it and/or modify
7 * it under the terms of the GNU Lesser General Public License version 3 as
8 * published by the Free Software Foundation.
9 *
10 * This program is distributed in the hope that it will be useful,
11 * but WITHOUT ANY WARRANTY; without even the implied warranty of
12 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
13 * GNU Lesser General Public License for more details.
14 *
15 * You should have received a copy of the GNU Lesser General Public License
16 * along with this program. If not, see <http://www.gnu.org/licenses/>.
17 *
18 * Author: Robert Carr <robert.carr@canonical.com>
19 */
20
21#ifndef MIR_CURSORS_H_
22#define MIR_CURSORS_H_
23
24/**
25 * \addtogroup mir_toolkit
26 * @{
27 */
28
29/* This is C code. Not C++. */
30
31/**
32 * A special cursor name for use with mir_cursor_configuration_from_name
33 * representing the system default cursor.
34 */
35extern char const *const mir_default_cursor_name;
36/**
37 * A special cursor name for use with mir_cursor_configuration_from_name
38 * representing a disabled cursor image.
39 */
40extern char const *const mir_disabled_cursor_name;
41
42/**
43 * The standard arrow cursor (typically the system default)
44 */
45extern char const* const mir_arrow_cursor_name;
46
47/**
48 * The "wait" cursor, typically an hourglass or watch used during operations
49 * which prevent the user from interacting.
50 */
51extern char const* const mir_busy_cursor_name;
52
53/**
54 * The caret or ibeam cursor, indicating acceptance of text input
55 */
56extern char const* const mir_caret_cursor_name;
57
58/**
59 * The pointing hand cursor, typically used for clickable elements such
60 * as hyperlinks.
61 */
62extern char const* const mir_pointing_hand_cursor_name;
63
64/**
65 * The open handed cursor, typically used to indicate that the area beneath
66 * the cursor may be clicked and dragged around.
67 */
68extern char const* const mir_open_hand_cursor_name;
69
70/**
71 * The close handed cursor, typically used to indicate that a drag operation is in process
72 * which involves scrolling.
73 */
74extern char const* const mir_closed_hand_cursor_name;
75
76/**
77 * The cursor used to indicate a horizontal resize operation.
78 */
79extern char const* const mir_horizontal_resize_cursor_name;
80
81/**
82 * The cursor used to indicate a vertical resize operation.
83 */
84extern char const* const mir_vertical_resize_cursor_name;
85
86/**
87 * The cursor used to indicate diagonal resize from top-right and bottom-left corners.
88 */
89extern char const* const mir_diagonal_resize_bottom_to_top_cursor_name;
90
91/**
92 * The cursor used to indicate diagonal resize from bottom-left and top-right corners.
93 */
94extern char const* const mir_diagonal_resize_top_to_bottom_cursor_name;
95
96/**
97 * The cursor used to indicate resize with no directional constraint.
98 */
99extern char const* const mir_omnidirectional_resize_cursor_name;
100
101/**
102 * The cursor used for vertical splitters, indicating that a handle may be
103 * dragged to adjust vertical space.
104 */
105extern char const* const mir_vsplit_resize_cursor_name;
106
107/**
108 * The cursor used for horizontal splitters, indicating that a handle may be
109 * dragged to adjust horizontal space.
110 */
111extern char const* const mir_hsplit_resize_cursor_name;
112
113/**@}*/
114
115#endif
0116
=== modified file 'src/client/mir_cursor_api.cpp'
--- src/client/mir_cursor_api.cpp 2014-06-17 22:07:15 +0000
+++ src/client/mir_cursor_api.cpp 2014-07-21 15:42:48 +0000
@@ -23,6 +23,19 @@
2323
24char const *const mir_default_cursor_name = "default";24char const *const mir_default_cursor_name = "default";
25char const *const mir_disabled_cursor_name = "disabled";25char const *const mir_disabled_cursor_name = "disabled";
26char const* const mir_arrow_cursor_name = "arrow";
27char const* const mir_busy_cursor_name = "busy";
28char const* const mir_caret_cursor_name = "caret";
29char const* const mir_pointing_hand_cursor_name = "pointing-hand";
30char const* const mir_open_hand_cursor_name = "open-hand";
31char const* const mir_closed_hand_cursor_name = "closed-hand";
32char const* const mir_horizontal_resize_cursor_name = "horizontal-resize";
33char const* const mir_vertical_resize_cursor_name = "vertical-resize";
34char const* const mir_diagonal_resize_bottom_to_top_cursor_name = "diagonal-resize-bottom-to-top";
35char const* const mir_diagonal_resize_top_to_bottom_cursor_name = "diagonal-resize-top_to_bottom";
36char const* const mir_omnidirectional_resize_cursor_name = "omnidirectional-resize";
37char const* const mir_vsplit_resize_cursor_name = "vsplit-resize";
38char const* const mir_hsplit_resize_cursor_name = "hsplit-resize";
2639
27MirCursorConfiguration::MirCursorConfiguration(char const* name) :40MirCursorConfiguration::MirCursorConfiguration(char const* name) :
28 name{name ? name : std::string()}41 name{name ? name : std::string()}
2942
=== modified file 'src/server/input/xcursor_loader.cpp'
--- src/server/input/xcursor_loader.cpp 2014-06-25 05:16:43 +0000
+++ src/server/input/xcursor_loader.cpp 2014-07-21 15:42:48 +0000
@@ -25,6 +25,8 @@
2525
26#include <string.h>26#include <string.h>
2727
28#include <mir_toolkit/cursors.h>
29
28// Unforunately this can not be compiled as C++...so we can not namespace30// Unforunately this can not be compiled as C++...so we can not namespace
29// these symbols. In order to differentiate from internal symbols31// these symbols. In order to differentiate from internal symbols
30// we refer to them via their _ prefixed version, i.e. _XcursorImage32// we refer to them via their _ prefixed version, i.e. _XcursorImage
@@ -75,6 +77,71 @@
75 _XcursorImage *image;77 _XcursorImage *image;
76 std::shared_ptr<_XcursorImages> const save_resource;78 std::shared_ptr<_XcursorImages> const save_resource;
77};79};
80
81std::string const
82xcursor_name_for_mir_cursor(std::string const& mir_cursor_name)
83{
84 if (mir_cursor_name == mir_default_cursor_name)
85 {
86 return "arrow";
87 }
88 else if (mir_cursor_name == mir_arrow_cursor_name)
89 {
90 return "arrow";
91 }
92 else if (mir_cursor_name == mir_busy_cursor_name)
93 {
94 return "watch";
95 }
96 else if (mir_cursor_name == mir_caret_cursor_name)
97 {
98 return "xterm"; // Yep
99 }
100 else if (mir_cursor_name == mir_pointing_hand_cursor_name)
101 {
102 return "hand2";
103 }
104 else if (mir_cursor_name == mir_open_hand_cursor_name)
105 {
106 return "hand";
107 }
108 else if (mir_cursor_name == mir_closed_hand_cursor_name)
109 {
110 return "grabbing";
111 }
112 else if (mir_cursor_name == mir_horizontal_resize_cursor_name)
113 {
114 return "h_double_arrow";
115 }
116 else if (mir_cursor_name == mir_vertical_resize_cursor_name)
117 {
118 return "v_double_arrow";
119 }
120 else if (mir_cursor_name == mir_diagonal_resize_bottom_to_top_cursor_name)
121 {
122 return "top_right_corner";
123 }
124 else if (mir_cursor_name == mir_diagonal_resize_top_to_bottom_cursor_name)
125 {
126 return "bottom_right_corner";
127 }
128 else if (mir_cursor_name == mir_omnidirectional_resize_cursor_name)
129 {
130 return "fleur";
131 }
132 else if (mir_cursor_name == mir_vsplit_resize_cursor_name)
133 {
134 return "v_double_arrow";
135 }
136 else if (mir_cursor_name == mir_hsplit_resize_cursor_name)
137 {
138 return "h_double_arrow";
139 }
140 else
141 {
142 return mir_cursor_name;
143 }
144}
78}145}
79146
80mi::XCursorLoader::XCursorLoader()147mi::XCursorLoader::XCursorLoader()
@@ -129,9 +196,7 @@
129std::shared_ptr<mg::CursorImage> mi::XCursorLoader::image(std::string const& cursor_name,196std::shared_ptr<mg::CursorImage> mi::XCursorLoader::image(std::string const& cursor_name,
130 geom::Size const& size)197 geom::Size const& size)
131{198{
132 // 'arrow' is the default cursor in XCursor themes.199 auto xcursor_name = xcursor_name_for_mir_cursor(cursor_name);
133 if (cursor_name == "default")
134 return image("arrow", size);
135200
136 if (size != mi::default_cursor_size)201 if (size != mi::default_cursor_size)
137 BOOST_THROW_EXCEPTION(202 BOOST_THROW_EXCEPTION(
@@ -139,7 +204,7 @@
139 204
140 std::lock_guard<std::mutex> lg(guard);205 std::lock_guard<std::mutex> lg(guard);
141206
142 auto it = loaded_images.find(cursor_name);207 auto it = loaded_images.find(xcursor_name);
143 if (it != loaded_images.end())208 if (it != loaded_images.end())
144 return it->second;209 return it->second;
145210

Subscribers

People subscribed via source and target branches