Mir

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

cursor-spike-phase-7-name-some-cursors
Merge into development-branch

Proposed by Robert Carr on 2014-07-02

Status:	Merged
Approved by:	Robert Carr on 2014-07-21
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
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot (community)	continuous-integration	2014-07-02	Approve on 2014-07-21
Kevin DuBois (community)			Approve on 2014-07-21
Cemil Azizoglu (community)			Approve on 2014-07-21
Alan Griffiths		2014-07-02	Approve on 2014-07-09
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.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-07-01: Posted in a previous version of this proposal

FAILED: Continuous integration, rev:1557
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/2048/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/784
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/790/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/783/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/569/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/567/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/2381/console

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

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-07-01: 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

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 on 2014-07-02:

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

review: Approve (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-07-02:

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 on 2014-07-07:

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 on 2014-07-07:

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 on 2014-07-08:

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

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-07-08:

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

review: Approve (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-07-08:

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

review: Approve (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-07-09:

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 on 2014-07-09:

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 on 2014-07-16:

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

review: Approve (continuous-integration)

Revision history for this message

Cemil Azizoglu (cemil-azizoglu) wrote on 2014-07-21:

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 on 2014-07-21:

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) on 2014-07-21:

review: Approve (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-07-21:

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

review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Emanuele Antonio Faraone

Gerry Boland

Mir development team

 === 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 @@
  target_link_libraries(mir_demo_client_eglplasma
    eglapp
+ )
++add_executable(mir_demo_client_cursors
++  cursors_demo_client.c
++)
++target_link_libraries(mir_demo_client_cursors
++  eglapp
++)
  add_executable(mir_demo_client_basic
    basic.c
 === 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 @@
++/*
++ * Copyright © 2014 Canonical LTD
++ *
++ * This program is free software: you can redistribute it and/or modify
++ * it under the terms of the GNU General Public License version 3 as
++ * published by the Free Software Foundation.
++ *
++ * This program is distributed in the hope that it will be useful,
++ * but WITHOUT ANY WARRANTY; without even the implied warranty of
++ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
++ * GNU General Public License for more details.
++ *
++ * You should have received a copy of the GNU General Public License
++ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
++ *
++ * Author: Robert Carr <robert.carr@canonical.com>
++ */
++
++#include "mir_toolkit/mir_client_library.h"
++
++#include "eglapp.h"
++#include <stdio.h>
++#include <unistd.h>
++#include <GLES2/gl2.h>
++
++void configure_cursor(MirSurface *surface, unsigned int cursor_index)
++{
++    char const *const cursors[] = {
++        mir_arrow_cursor_name,
++        mir_busy_cursor_name,
++        mir_caret_cursor_name,
++        mir_pointing_hand_cursor_name,
++        mir_open_hand_cursor_name,
++        mir_closed_hand_cursor_name,
++        mir_horizontal_resize_cursor_name,
++        mir_vertical_resize_cursor_name,
++        mir_diagonal_resize_bottom_to_top_cursor_name,
++        mir_diagonal_resize_top_to_bottom_cursor_name,
++        mir_omnidirectional_resize_cursor_name,
++        mir_vsplit_resize_cursor_name,
++        mir_hsplit_resize_cursor_name
++    };
++
++    size_t num_cursors = sizeof(cursors)/sizeof(*cursors);
++    size_t real_index = cursor_index % num_cursors;
++
++    MirCursorConfiguration *conf = mir_cursor_configuration_from_name(cursors[real_index]);
++
++    mir_wait_for(mir_surface_configure_cursor(surface, conf));
++
++    mir_cursor_configuration_destroy(conf);
++}
++
++int main(int argc, char *argv[])
++{
++    unsigned int width = 128, height = 128;
++
++    if (!mir_eglapp_init(argc, argv, &width, &height))
++        return 1;
++
++    glClearColor(0.5, 0.5, 0.5, mir_eglapp_background_opacity);
++    glClear(GL_COLOR_BUFFER_BIT);
++    mir_eglapp_swap_buffers();
++
++    unsigned int cursor_index = 0;
++    while (mir_eglapp_running())
++    {
++        configure_cursor(mir_eglapp_native_surface(), cursor_index++);
++        sleep(1);
++    }
++
++    mir_eglapp_shutdown();
++
++    return 0;
++}
 === 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 @@
  /**
   * Returns a new MirCursorConfiguration representing a named cursor
-- * from the system cursor theme. Currently only the symbolic values,
-- * mir_default_cursor_name and mir_disabled_cursor_name are supported
++ * from the system cursor theme. Symbolic cursor names, such as
++ * mir_default_cursor_name and mir_caret_cursor_name are available
++ * see (mir_toolkit/cursors.h).
   * as input.
   *    \param [in] name The cursor name
   *    \return A cursor parameters object which must be passed
 === 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 @@
  #ifndef MIR_COMMON_H_
  #define MIR_COMMON_H_
++#include <mir_toolkit/cursors.h>
++
  /**
   * \addtogroup mir_toolkit
   * @{
@@ -137,17 +139,6 @@
      mir_orientation_right = 270
  } MirOrientation;
--/**
-- * A special cursor name for use with mir_cursor_configuration_from_name
-- * representing the system default cursor.
-- */
--extern char const *const mir_default_cursor_name;
--/**
-- * A special cursor name for use with mir_cursor_configuration_from_name
-- * representing a disabled cursor image.
-- */
--extern char const *const mir_disabled_cursor_name;
--
  /**@}*/
  #endif
 === 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 @@
++/*
++ * Cursor name definitions.
++ *
++ * Copyright © 2014 Canonical Ltd.
++ *
++ * This program is free software: you can redistribute it and/or modify
++ * it under the terms of the GNU Lesser General Public License version 3 as
++ * published by the Free Software Foundation.
++ *
++ * This program is distributed in the hope that it will be useful,
++ * but WITHOUT ANY WARRANTY; without even the implied warranty of
++ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
++ * GNU Lesser General Public License for more details.
++ *
++ * You should have received a copy of the GNU Lesser General Public License
++ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
++ *
++ * Author: Robert Carr <robert.carr@canonical.com>
++ */
++
++#ifndef MIR_CURSORS_H_
++#define MIR_CURSORS_H_
++
++/**
++ * \addtogroup mir_toolkit
++ * @{
++ */
++
++/* This is C code. Not C++. */
++
++/**
++ * A special cursor name for use with mir_cursor_configuration_from_name
++ * representing the system default cursor.
++ */
++extern char const *const mir_default_cursor_name;
++/**
++ * A special cursor name for use with mir_cursor_configuration_from_name
++ * representing a disabled cursor image.
++ */
++extern char const *const mir_disabled_cursor_name;
++
++/**
++ * The standard arrow cursor (typically the system default)
++ */
++extern char const* const mir_arrow_cursor_name;
++
++/**
++ * The "wait" cursor, typically an hourglass or watch used during operations
++ * which prevent the user from interacting.
++ */
++extern char const* const mir_busy_cursor_name;
++
++/**
++ * The caret or ibeam cursor, indicating acceptance of text input
++ */
++extern char const* const mir_caret_cursor_name;
++
++/**
++ * The pointing hand cursor, typically used for clickable elements such
++ * as hyperlinks.
++ */
++extern char const* const mir_pointing_hand_cursor_name;
++
++/**
++ * The open handed cursor, typically used to indicate that the area beneath
++ * the cursor may be clicked and dragged around.
++ */
++extern char const* const mir_open_hand_cursor_name;
++
++/**
++ * The close handed cursor, typically used to indicate that a drag operation is in process
++ * which involves scrolling.
++ */
++extern char const* const mir_closed_hand_cursor_name;
++
++/**
++ * The cursor used to indicate a horizontal resize operation.
++ */
++extern char const* const mir_horizontal_resize_cursor_name;
++
++/**
++ * The cursor used to indicate a vertical resize operation.
++ */
++extern char const* const mir_vertical_resize_cursor_name;
++
++/**
++ * The cursor used to indicate diagonal resize from top-right and bottom-left corners.
++ */
++extern char const* const mir_diagonal_resize_bottom_to_top_cursor_name;
++
++/**
++ * The cursor used to indicate diagonal resize from bottom-left and top-right corners.
++ */
++extern char const* const mir_diagonal_resize_top_to_bottom_cursor_name;
++
++/**
++ * The cursor used to indicate resize with no directional constraint.
++ */
++extern char const* const mir_omnidirectional_resize_cursor_name;
++
++/**
++ * The cursor used for vertical splitters, indicating that a handle may be
++ * dragged to adjust vertical space.
++ */
++extern char const* const mir_vsplit_resize_cursor_name;
++
++/**
++ * The cursor used for horizontal splitters, indicating that a handle may be
++ * dragged to adjust horizontal space.
++ */
++extern char const* const mir_hsplit_resize_cursor_name;
++
++/**@}*/
++
++#endif
 === 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 @@
  char const *const mir_default_cursor_name = "default";
  char const *const mir_disabled_cursor_name = "disabled";
++char const* const mir_arrow_cursor_name = "arrow";
++char const* const mir_busy_cursor_name = "busy";
++char const* const mir_caret_cursor_name = "caret";
++char const* const mir_pointing_hand_cursor_name = "pointing-hand";
++char const* const mir_open_hand_cursor_name = "open-hand";
++char const* const mir_closed_hand_cursor_name = "closed-hand";
++char const* const mir_horizontal_resize_cursor_name = "horizontal-resize";
++char const* const mir_vertical_resize_cursor_name = "vertical-resize";
++char const* const mir_diagonal_resize_bottom_to_top_cursor_name = "diagonal-resize-bottom-to-top";
++char const* const mir_diagonal_resize_top_to_bottom_cursor_name = "diagonal-resize-top_to_bottom";
++char const* const mir_omnidirectional_resize_cursor_name = "omnidirectional-resize";
++char const* const mir_vsplit_resize_cursor_name = "vsplit-resize";
++char const* const mir_hsplit_resize_cursor_name = "hsplit-resize";
  MirCursorConfiguration::MirCursorConfiguration(char const* name) :
      name{name ? name : std::string()}
 === 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 @@
  #include <string.h>
++#include <mir_toolkit/cursors.h>
++
  // Unforunately this can not be compiled as C++...so we can not namespace
  // these symbols. In order to differentiate from internal symbols
  //  we refer to them via their _ prefixed version, i.e. _XcursorImage
@@ -75,6 +77,71 @@
      _XcursorImage *image;
      std::shared_ptr<_XcursorImages> const save_resource;
  };
++
++std::string const
++xcursor_name_for_mir_cursor(std::string const& mir_cursor_name)
++{
++    if (mir_cursor_name == mir_default_cursor_name)
++    {
++        return "arrow";
++    }
++    else if (mir_cursor_name == mir_arrow_cursor_name)
++    {
++        return "arrow";
++    }
++    else if (mir_cursor_name == mir_busy_cursor_name)
++    {
++        return "watch";
++    }
++    else if (mir_cursor_name == mir_caret_cursor_name)
++    {
++        return "xterm"; // Yep
++    }
++    else if (mir_cursor_name == mir_pointing_hand_cursor_name)
++    {
++        return "hand2";
++    }
++    else if (mir_cursor_name == mir_open_hand_cursor_name)
++    {
++        return "hand";
++    }
++    else if (mir_cursor_name == mir_closed_hand_cursor_name)
++    {
++        return "grabbing";
++    }
++    else if (mir_cursor_name == mir_horizontal_resize_cursor_name)
++    {
++        return "h_double_arrow";
++    }
++    else if (mir_cursor_name == mir_vertical_resize_cursor_name)
++    {
++        return "v_double_arrow";
++    }
++    else if (mir_cursor_name == mir_diagonal_resize_bottom_to_top_cursor_name)
++    {
++        return "top_right_corner";
++    }
++    else if (mir_cursor_name == mir_diagonal_resize_top_to_bottom_cursor_name)
++    {
++        return "bottom_right_corner";
++    }
++    else if (mir_cursor_name == mir_omnidirectional_resize_cursor_name)
++    {
++        return "fleur";
++    }
++    else if (mir_cursor_name == mir_vsplit_resize_cursor_name)
++    {
++        return "v_double_arrow";
++    }
++    else if (mir_cursor_name == mir_hsplit_resize_cursor_name)
++    {
++        return "h_double_arrow";
++    }
++    else
++    {
++        return mir_cursor_name;
++    }
++}
+ }
  mi::XCursorLoader::XCursorLoader()
@@ -129,9 +196,7 @@
  std::shared_ptr<mg::CursorImage> mi::XCursorLoader::image(std::string const& cursor_name,
      geom::Size const& size)
+ {
--    // 'arrow' is the default cursor in XCursor themes.
--    if (cursor_name == "default")
--        return image("arrow", size);
++    auto xcursor_name = xcursor_name_for_mir_cursor(cursor_name);
      if (size != mi::default_cursor_size)
          BOOST_THROW_EXCEPTION(
@@ -139,7 +204,7 @@
      std::lock_guard<std::mutex> lg(guard);
--    auto it = loaded_images.find(cursor_name);
++    auto it = loaded_images.find(xcursor_name);
      if (it != loaded_images.end())
          return it->second;