Mir

Merge lp:~alan-griffiths/mir/make-set_logger-for-client-reports into lp:mir

make-set_logger-for-client-reports
Merge into development-branch

Proposed by Alan Griffiths on 2016-03-04

Status:

Merged

Approved by:

Alan Griffiths on 2016-03-09

Approved revision:

no longer in the source branch.

Merged at revision:

3379

Proposed branch:

lp:~alan-griffiths/mir/make-set_logger-for-client-reports

Merge into:

lp:mir

Prerequisite:

lp:~vanvugt/mir/fix-1546933-2

Diff against target:

130 lines (+28/-40)

2 files modified

src/client/default_connection_configuration.cpp (+9/-5)
tests/acceptance-tests/test_client_logging.cpp (+19/-35)

To merge this branch:

bzr merge lp:~alan-griffiths/mir/make-set_logger-for-client-reports

Medium

Confirmed

Link a bug report

Reviewer	Review Type	Date Requested	Status
Chris Halse Rogers		2016-03-04	Approve on 2016-03-08
Cemil Azizoglu (community)			Approve on 2016-03-08
Mir CI Bot	continuous-integration		Approve on 2016-03-07
Review via email: mp+288118@code.launchpad.net

Commit message

client: Make mir::logging::set_logger() work for client reports

Description of the change

client: Make mir::logging::set_logger() work for client reports

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2016-03-04:

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

review: Approve (continuous-integration)

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2016-03-07:

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

review: Approve (continuous-integration)

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2016-03-07:

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

review: Approve (continuous-integration)

Revision history for this message

Chris Halse Rogers (raof) wrote on 2016-03-08:

Huh. I thought we were trying to make mirclient independent of mircommon? This ties them together more.

As it stands, this is really only going to be useful in tests, right? I rather thought you might take this opportunity to add a client-facing mirclient log API (which we need at some point).

This doesn't seem wrong, though. Unless we haven't given up on making mirclient not depend on mircommon?

review: Needs Information

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2016-03-08:

> Huh. I thought we were trying to make mirclient independent of mircommon? This
> ties them together more.

I don't recognise that aspiration. Nor do I think it feasible.

> As it stands, this is really only going to be useful in tests, right?

Yes.

> I rather
> thought you might take this opportunity to add a client-facing mirclient log
> API (which we need at some point).

Providing a C style API for customizing the logging is a significantly bigger change than this simplification.

> This doesn't seem wrong, though. Unless we haven't given up on making
> mirclient not depend on mircommon?

It could be wrong, if we decide that this mechanism for customizing logging should be deprecated. (See discussion in linked the bug.)

Revision history for this message

Cemil Azizoglu (cemil-azizoglu) wrote on 2016-03-08:

Looks good

review: Approve

Revision history for this message

Chris Halse Rogers (raof) wrote on 2016-03-08:

On Tue, Mar 8, 2016 at 8:43 PM, Alan Griffiths <email address hidden>
wrote:
>> Huh. I thought we were trying to make mirclient independent of
>> mircommon? This
>> ties them together more.
>
> I don't recognise that aspiration. Nor do I think it feasible.

I'm pretty sure I recall that *you've* marked one of my merge-proposals
as Needs Fixing because “mircommon is supposed to be the things
shared between the server and the platform” ☺.

But anyway, if that's not a thing we're going for, then sure.

Revision history for this message

Chris Halse Rogers (raof) on 2016-03-08:

review: Approve

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2016-03-09:

On 08/03/16 23:39, Chris Halse Rogers wrote:
> I'm pretty sure I recall that *you've* marked one of my merge-proposals
> as Needs Fixing because “mircommon is supposed to be the things
> shared between the server and the platform” ☺.

Maybe I was confused? That describes mirplatform.

--
Alan Griffiths +44 (0)798 9938 758
Octopull Ltd http://www.octopull.co.uk/

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:

Alan Griffiths

Emanuele Antonio Faraone

Gerry Boland

Mir development team

 === modified file 'src/client/default_connection_configuration.cpp'
 --- src/client/default_connection_configuration.cpp	2016-01-29 08:18:22 +0000
 +++ src/client/default_connection_configuration.cpp	2016-03-07 14:29:24 +0000
@@ -21,7 +21,7 @@
  #include "display_configuration.h"
  #include "rpc/make_rpc_channel.h"
  #include "rpc/null_rpc_report.h"
--#include "mir/logging/dumb_console_logger.h"
++#include "mir/logging/logger.h"
  #include "mir/input/input_platform.h"
  #include "mir/input/input_devices.h"
  #include "mir/input/null_input_receiver_report.h"
@@ -76,11 +76,15 @@
  std::shared_ptr<mir::logging::Logger>
  mcl::DefaultConnectionConfiguration::the_logger()
+ {
--    return logger(
--        []
++    class ProxyLogger : public mir::logging::Logger
++    {
++        void log(mir::logging::Severity severity, const std::string& message, const std::string& component) override
+         {
--            return std::make_shared<mir::logging::DumbConsoleLogger>();
--        });
++            mir::logging::log(severity, message, component);
++        }
++    };
++
++    return logger([]{ return std::make_shared<ProxyLogger>(); });
+ }
  std::shared_ptr<mcl::ClientPlatformFactory>
 === modified file 'tests/acceptance-tests/test_client_logging.cpp'
 --- tests/acceptance-tests/test_client_logging.cpp	2016-03-07 14:29:24 +0000
 +++ tests/acceptance-tests/test_client_logging.cpp	2016-03-07 14:29:24 +0000
@@ -17,10 +17,9 @@
   */
  #include "mir_toolkit/mir_client_library.h"
--#include "mir_test_framework/stub_client_connection_configuration.h"
--#include "mir_test_framework/using_client_platform.h"
  #include "mir_test_framework/connected_client_headless_server.h"
  #include "mir/logging/logger.h"
++#include "mir/test/fake_shared.h"
  #include <gmock/gmock.h>
  #include <gtest/gtest.h>
@@ -30,49 +29,34 @@
  using namespace testing;
  using namespace mir_test_framework;
--namespace {
--
++namespace
++{
  class StringStreamLogger : public mir::logging::Logger
+ {
  public:
--    void log(mir::logging::Severity,
--             std::string const& message,
--             std::string const& component) override
--    {
--        ss << "[StringStreamLogger] "
--           << component << ": " << message << std::endl;
--    }
--    // Awkwardly static. Because the instance is hidden in UsingClientPlatform
--    static std::stringstream ss;
--};
--
--std::stringstream StringStreamLogger::ss;
--
--struct Conf : StubConnectionConfiguration
--{
--    Conf(std::string const& socket)
--        : StubConnectionConfiguration(socket)
--    {
--    }
--    std::shared_ptr<mir::logging::Logger> the_logger() override
--    {
--        return std::make_shared<StringStreamLogger>();
--    }
++    void log(mir::logging::Severity, std::string const& message, std::string const& component) override
++    {
++        out << "[StringStreamLogger] " << component << ": " << message << std::endl;
++    }
++
++    std::stringstream out;
  };
  struct ClientLogging : ConnectedClientHeadlessServer
+ {
--    UsingClientPlatform<Conf> with_custom_logger;
--    std::stringstream& client_log{StringStreamLogger::ss};
++    StringStreamLogger logger;
++
++    void SetUp() override
++    {
++        add_to_environment("MIR_CLIENT_PERF_REPORT", "log");
++        ConnectedClientHeadlessServer::SetUp();
++        mir::logging::set_logger(mir::test::fake_shared(logger));
++    }
  };
--
  } // namespace
  TEST_F(ClientLogging, reports_performance)
+ {
--    TemporaryEnvironmentValue env("MIR_CLIENT_PERF_REPORT", "log");
--    (void)env; // Avoid clang warning/error
--
      auto spec = mir_connection_create_spec_for_normal_surface(
                     connection, 123, 456, mir_pixel_format_abgr_8888);
      ASSERT_THAT(spec, NotNull());
@@ -94,10 +78,10 @@
+     }
      int reports = 0;
--    while (!client_log.eof())
++    while (!logger.out.eof())
+     {
          std::string line;
--        std::getline(client_log, line);
++        std::getline(logger.out, line);
          auto perf = line.find(" perf: ");
          if (perf != line.npos)
+         {
 === renamed file 'include/test/mir_test_framework/stub_client_connection_configuration.h' => 'tests/include/mir_test_framework/stub_client_connection_configuration.h'
 === renamed file 'include/test/mir_test_framework/using_client_platform.h' => 'tests/include/mir_test_framework/using_client_platform.h'