Merge lp:~vanvugt/mir/mir-display-config-header into lp:mir

[Daniel van Vugt]

Correct a header file name to match the API it provides:
  MirDisplayConfig and mir_display_config_*

[Alexandros Frantzis (afrantzis)]

In the 1.0 API cleanup discussion we decided to go with MirDisplayConfiguration and mir_display_configuration_* naming. This MP proposes the opposite of what we decided.

review: Disapprove

[Mir CI Bot (mir-ci-bot)]

PASSED: Continuous integration, rev:3833
https://mir-jenkins.ubuntu.com/job/mir-ci/2212/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/2866
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/2931
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/2923
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/2923
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/2923
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2895
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2895/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2895
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2895/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2895
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2895/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2895
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2895/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2895
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2895/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2895
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2895/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/2212/rebuild

review: Approve (continuous-integration)

[Andreas Pokorny (andreas-pokorny)]

yes .. the other way round..

review: Disapprove

[Daniel van Vugt (vanvugt)]

You propose renaming all the API functions making them harder to type, rather than just fixing the inconsistently named header file?

That makes no sense.

[Alan Griffiths (alan-griffiths)]

> In the 1.0 API cleanup discussion we decided to go with
> MirDisplayConfiguration and mir_display_configuration_* naming. This MP
> proposes the opposite of what we decided.

Color me confused: mir_toolkit/mir_connection.h has this...

    /**
     * Query the display
     *
     * \deprecated Use mir_connection_create_display_configuration() instead.
     *
     * \warning return value must be destroyed via mir_display_config_destroy()
     * \warning may return null if connection is invalid
     * \param [in] connection The connection
     * \return structure that describes the display configuration
     */
    MirDisplayConfiguration* mir_connection_create_display_config(MirConnection *connection);

And mir_connection_create_display_configuration() returns MirDisplayConfig, so we're already in the process of deprecating use of MirDisplayConfiguration in favour of MirDisplayConfig and have decided to reverse that but use the new function names?

I don't see the intermediate steps that get us there.

review: Needs Information

[Alexandros Frantzis (afrantzis)]

> And mir_connection_create_display_configuration() returns MirDisplayConfig,
> so we're already in the process of deprecating use of MirDisplayConfiguration
> in favour of MirDisplayConfig and have decided to reverse that but use the
> new function names?
>
> I don't see the intermediate steps that get us there.

Since we have messed up the naming of DisplayConfig(uration) and related functions in this way, it's hard to provide a sensible deprecation path (but perhaps possible with some header trickery). We will probably need to go with a clean break at 1.0 without deprecation. It's not ideal, but we need to clean the mess, and this is our chance.

[Daniel van Vugt (vanvugt)]

There is a clean path forward. I thought it was obvious but let me explain:

1. Land this branch to make the file name consistent with the "new" API.

2. Add a new function (so it's not an API break) that wraps the remaining inconsistent function Alan mentioned, but using the correct naming convention:

MirDisplayConfig* mir_connection_get_display_config(MirConnection* connection)
{
    return mir_connection_create_display_configuration(connection);
}

3. Now we have consistently _display_config functions operating on MirDisplayConfig. We can deprecate the rest when we want.

[Daniel van Vugt (vanvugt)]

2.5. We also need new functions with correct suffixes to wrap these:

  mir_connection_preview_base_display_configuration
  mir_connection_confirm_base_display_configuration

[Alan Griffiths (alan-griffiths)]

> There is a clean path forward. I thought it was obvious but let me explain:
>
> 1. Land this branch to make the file name consistent with the "new" API.

You've already collected two "Disapproves" because this MP is not consistent the intended API - it moves the "wrong way".

It is obviously possible to get to places other than the intended API with only one ABI break, but that's not what I've been questioning.

> 2. Add a new function (so it's not an API break) that wraps the remaining
> inconsistent function Alan mentioned, but using the correct naming convention:
>
> MirDisplayConfig* mir_connection_get_display_config(MirConnection* connection)
> {
> return mir_connection_create_display_configuration(connection);
> }

Doesn't that break the convention that things that are "created" need to be released while "get" is for things that don't?

[Daniel van Vugt (vanvugt)]

The intended API you imply (which is only hearsay right now and less related to the real API that exists) is both too wordy and also involves breaking the ABI. So I still maintain this proposal is the best way forward since it does not have those problems.

As for "created" vs "get", "created" is a historical mistake. We are not creating the display config; we are only copying it from the server. Hence "get" or "copy" is a more appropriate verb than "create".

I think a reasonable person reading these comments will agree with me so am waiting on more votes.

[Alan Griffiths (alan-griffiths)]

> The intended API you imply (which is only hearsay right now and less related
> to the real API that exists) is both too wordy and also involves breaking the
> ABI. So I still maintain this proposal is the best way forward since it does
> not have those problems.

"Hearsay" is a poor characterisation of something that was agreed by the team IRL (in Den Haag). Your not paying attention or being out the room at the time does not entitle you to be so dismissive.

You don't seem to have realized it but I agree that you're suggesting a more easily reached destination. But it remains the wrong one (unless we persuade the team to change its mind).

> As for "created" vs "get", "created" is a historical mistake. We are not
> creating the display config; we are only copying it from the server. Hence
> "get" or "copy" is a more appropriate verb than "create".

Nonsense.

> I think a reasonable person reading these comments will agree with me so am
> waiting on more votes.

Good luck!

[Daniel van Vugt (vanvugt)]

I would like to not get drawn into a personal argument, but in the interest of not letting others victimize me publicly and get away with it, it must be noted that I was only not in the room at the time because I was busy doing my job; attending sessions in Den Haag.

I believe this is a reasonable change and something we should consider more generally (using "config" over "configuration") since some of our function names are getting quite long. Also most of the existing new display API uses "config" over "configuration" already. I'm not choosing it because it's the path of least resistance but also because it's the cleanest one.

And I believe even more strongly that it's poor API design to say something is "created" when it's a key requirement that the user be aware they're actually copying the server's current configuration and not "creating" a blank or default object.

[Brandon Schaefer (brandontschaefer)]

I would like to think we were all doing our jobs. Im still in favour of MirDisplayConfiguration. 6 characters isnt *to* many extra characters to type. Yes it break ABI, yes it'll be a bit more work. We are already planning on breaking the ABI. I dont see a reason we should avoid this when a majority of us agreed on a name already.

review: Disapprove

[Brandon Schaefer (brandontschaefer)]

7*

Unmerged revisions

3833. By Daniel van Vugt on 2016-11-18

Public headers should look different

3832. By Daniel van Vugt on 2016-11-18

Done.

3831. By Daniel van Vugt on 2016-11-18

Prototype

=== modified file 'include/client/mir_toolkit/mir_client_library.h'
--- include/client/mir_toolkit/mir_client_library.h	2016-07-18 07:38:38 +0000
+++ include/client/mir_toolkit/mir_client_library.h	2016-11-18 10:15:15 +0000
@@ -26,7 +26,7 @@
 #include <mir_toolkit/mir_platform_message.h>
 #include <mir_toolkit/cursors.h>
 #include <mir_toolkit/mir_cookie.h>
-#include <mir_toolkit/mir_display_configuration.h>
+#include <mir_toolkit/mir_display_config.h>
 #include <mir_toolkit/mir_input_device.h>
 #include <mir_toolkit/mir_error.h>
 

=== renamed file 'include/client/mir_toolkit/mir_display_configuration.h' => 'include/client/mir_toolkit/mir_display_config.h'
--- include/client/mir_toolkit/mir_display_configuration.h	2016-10-31 02:37:31 +0000
+++ include/client/mir_toolkit/mir_display_config.h	2016-11-18 10:15:15 +0000
@@ -16,8 +16,8 @@
  * Authored by: Christopher James Halse Rogers <[e-mail]>
  */
 
-#ifndef MIR_TOOLKIT_MIR_DISPLAY_CONFIGURATION_H_
-#define MIR_TOOLKIT_MIR_DISPLAY_CONFIGURATION_H_
+#ifndef MIR_TOOLKIT_MIR_DISPLAY_CONFIG_H_
+#define MIR_TOOLKIT_MIR_DISPLAY_CONFIG_H_
 
 #include "client_types.h"
 
@@ -551,4 +551,4 @@
 }
 #endif
 
-#endif //MIR_TOOLKIT_MIR_DISPLAY_CONFIGURATION_H_
+#endif /* MIR_TOOLKIT_MIR_DISPLAY_CONFIG_H_ */

=== added file 'include/client/mir_toolkit/mir_display_configuration.h'
--- include/client/mir_toolkit/mir_display_configuration.h	1970-01-01 00:00:00 +0000
+++ include/client/mir_toolkit/mir_display_configuration.h	2016-11-18 10:15:15 +0000
@@ -0,0 +1,26 @@
+/*
+ * Copyright © 2016 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/>.
+ */
+
+#ifndef MIR_TOOLKIT_MIR_DISPLAY_CONFIGURATION_H_
+#define MIR_TOOLKIT_MIR_DISPLAY_CONFIGURATION_H_
+
+#ifdef __GNUC__
+#warning "mir_display_configuration.h is deprecated. Please use mir_display_config.h instead."
+#endif
+
+#include <mir_toolkit/mir_display_config.h>
+
+#endif /* MIR_TOOLKIT_MIR_DISPLAY_CONFIGURATION_H_ */

=== modified file 'src/client/display_configuration_api.cpp'
--- src/client/display_configuration_api.cpp	2016-10-31 02:37:31 +0000
+++ src/client/display_configuration_api.cpp	2016-11-18 10:15:15 +0000
@@ -16,7 +16,7 @@
  * Authored by: Christopher James Halse Rogers <[e-mail]>
  */
 
-#include <mir_toolkit/mir_display_configuration.h>
+#include "mir_toolkit/mir_display_config.h"
 #include <mir/require.h>
 #include "mir/output_type_names.h"
 #include "display_configuration.h"

=== modified file 'tests/mir_test/display_config_matchers.cpp'
--- tests/mir_test/display_config_matchers.cpp	2016-10-31 02:37:31 +0000
+++ tests/mir_test/display_config_matchers.cpp	2016-11-18 10:15:15 +0000
@@ -20,7 +20,7 @@
 #include "mir/graphics/display_configuration.h"
 #include "mir_protobuf.pb.h"
 #include "mir_toolkit/client_types.h"
-#include "mir_toolkit/mir_display_configuration.h"
+#include "mir_toolkit/mir_display_config.h"
 #include <gtest/gtest.h>
 
 namespace mg = mir::graphics;