ScreenLock implenentation for streams and Web inteface #167

ppodhajski · 2017-07-04T13:26:27Z

No description provided.

rdumusc · 2017-07-05T15:48:41Z

tide/core/ScreenLock.cpp

+    emit modified(shared_from_this());
+}
+
+void ScreenLock::_remove(QString uri)


const QString&

rdumusc · 2017-07-05T15:49:11Z

tide/core/ScreenLock.cpp

+{
+    if (std::find(_streams.begin(), _streams.end(), uri) != _streams.end())
+    {
+        _streams.erase(std::find(_streams.begin(), _streams.end(), uri));


do std::find only once by storing the returned iterator

rdumusc · 2017-07-05T15:56:04Z

tide/core/ScreenLock.h

+    ScreenLock();
+
+    /** Close the specified stream. */
+    Q_INVOKABLE void closeStream(const QString uri);


please reorder the methods for API readability, something like:

lock();
unlock();
isLocked();

registerStream();
releaseStream();
deregisterStream();
closeStream();
getStreamList();

Also I am not sure if so many methods are needed (deregister and close?)

in closeStream method I emit streamClosed(uri), to close a hidden stream, which is not needed in deregisterStream as the stream is closed elsewhere.

rdumusc · 2017-07-05T16:01:17Z

tide/core/resources/SideControl.qml

-            }
-            Clock {
-                displayedHeight: parent.width
+            height:  width


what happened to the contentActionButton? I think we should keep it!

rdumusc · 2017-07-05T16:02:03Z

tide/core/resources/StreamNotifier.qml

+    height: 500
+
+    ListView {
+        height: displaygroup / 2


displaygroup / 2 ??

rdumusc · 2017-07-06T08:36:45Z

tide/master/ui/MasterQuickView.h

@@ -58,7 +58,8 @@ class MasterQuickView : public QQuickView

 public:
    /** Constructor. */
-    MasterQuickView(OptionsPtr options, const MasterConfiguration& config);
+    MasterQuickView(OptionsPtr options, const MasterConfiguration& config,
+                    LockPtr lock);


For readability group options and lock parameters, leaving MasterConfiguration at the end.

rdumusc · 2017-07-06T08:37:08Z

tide/master/ui/MasterWindow.h

@@ -62,7 +62,7 @@ class MasterWindow : public QMainWindow
 public:
    /** Constructor. */
    MasterWindow(DisplayGroupPtr displayGroup, OptionsPtr options,
-                 MasterConfiguration& config);
+                 MasterConfiguration& config, LockPtr lock);


For readability group displaygroup, options and lock parameters, leaving MasterConfiguration at the end.

rdumusc · 2017-07-06T08:39:07Z

tide/wall/DisplayGroupRenderer.h

@@ -85,6 +85,9 @@ class DisplayGroupRenderer : public QObject
    /** @return true if the renderer requires a redraw. */
    bool needRedraw() const;

+    /** Set the ScreenLock replacing the previous one. */
+    void setScreenLock(LockPtr lock);


move up after setRenderingOptions for API readability

rdumusc · 2017-07-06T08:39:47Z

tide/wall/DisplayGroupRenderer.h

@@ -93,6 +96,7 @@ public slots:
    QQmlEngine& _engine;
    DataProvider& _provider;
    DisplayGroupPtr _displayGroup;
+    LockPtr _lock;


Keep same order as in the API for readability: place it after _options

rdumusc · 2017-07-06T08:45:01Z

tide/core/ScreenLock.h

+    Q_INVOKABLE void lock();
+
+    /** Release the specified stream. */
+    Q_INVOKABLE void releaseStream(const QString uri);


style: no const for param in header

ppodhajski · 2017-07-07T14:29:24Z

TODO: fix the RestServerTests and PixelStreamWindowManagerTests

rdumusc · 2017-07-06T12:21:46Z

tide/core/resources/SideControl.qml

-                    sourceComponent: buttonDelegate
-                }
+            }
+            Loader {


Loader not inside the image?

rdumusc · 2017-07-06T12:47:43Z

doc/Changelog.md

@@ -3,6 +3,9 @@ Changelog {#changelog}

 # Release 1.4 (git 1.4-dev)

+* [167](https://github.com/BlueBrain/Tide/pull/167):
+  Tide can be locked now. It prevents unwanted streams from opening and
+  freezes HTML interface.


Tide can be locked when doing a presentation. The lock prevents unwanted streams from opening and blocks interactions from the HTML interface.

rdumusc · 2017-08-04T07:15:42Z

tests/cpp/core/PixelStreamWindowManagerTests.cpp

@@ -271,7 +271,7 @@ BOOST_AUTO_TEST_CASE(hideAndShowWindow)
    windowManager.handleStreamStart(uri);
    ContentWindowPtr window = windowManager.getWindow(uri);

-    BOOST_REQUIRE(!window->isHidden());
+    BOOST_REQUIRE(window->isHidden());
    BOOST_REQUIRE(!window->isPanel());

    windowManager.hideWindow(uri);


reverse the order of the tests then, it is not very useful to hide a hidden window

rdumusc · 2017-08-04T07:19:03Z

tests/cpp/core/RestServerTests.cpp


-    BOOST_CHECK_EQUAL(response.toStdString(), "Forbidden");
+    BOOST_CHECK_EQUAL(responseGET.second, int(QNetworkReply::NoError));


check all the values: responseGET.first/second, responsePUT.first/second.
You should also validate that doing a PUT works before doing the block.

rdumusc · 2017-08-04T07:19:49Z

tests/cpp/core/RestServerTests.cpp

@@ -88,17 +92,27 @@ BOOST_AUTO_TEST_CASE(testServerReturnsSimpleContent)
    const auto url = QString("http://localhost:%1/test").arg(server.getPort());
    const auto response = sendHttpRequest(url);

-    BOOST_CHECK_EQUAL(response.toStdString(), "Hello World!");
+    BOOST_CHECK_EQUAL(response.first, "Hello World!");
+    BOOST_CHECK_EQUAL(response.second, int(QNetworkReply::NoError));
 }

 BOOST_AUTO_TEST_CASE(block_all)


this test does not block all? Rename to block_put_requests?

rdumusc · 2017-08-04T07:26:43Z

tide/master/PixelStreamWindowManager.cpp

@@ -141,6 +141,12 @@ void PixelStreamWindowManager::handleStreamStart(const QString uri)
        return;
    }

+    if (uri == "Launcher")
+    {
+        openWindow(uri, QPointF(), QSize(), StreamType::LAUNCHER);


this is a duplication of the line below! Do:

// external streams don't have a window yet, create one now const auto streamType = _isPanel(uri) ? StreamType::LAUNCHER : ...; openWindow(uri, QPointF(), QSize(), streamType);

then rename the existing _isPanel() helper function to _isLauncher()

rdumusc · 2017-08-04T07:28:42Z

tide/master/rest/HttpServer.cpp

@@ -47,7 +47,7 @@ using namespace zeroeq;
 std::future<http::Response> HttpServer::respondTo(http::Request& request) const
 {
    if (_blockedMethods.count(request.method) &&
-        !_isWhitelisted(request.source))
+        !_isWhitelisted(request.source.substr(0, request.source.find(":"))))


the substring extraction deserves a local variable or a helper function for readability

rdumusc · 2017-08-07T14:21:13Z

tests/cpp/core/RestServerTests.cpp

+    BOOST_CHECK_EQUAL(response.second, int(QNetworkReply::NoError));
+}
+
+BOOST_AUTO_TEST_CASE(block_all)


this still does not test "block_all", merely PUT

rdumusc · 2017-08-07T14:22:56Z

tests/cpp/core/RestServerTests.cpp

+        return http::make_ready_response(http::Code::OK);
+    });
+
+    server.block(http::Method::PUT);


test PUT works before block(). Test unblock().

rdumusc · 2017-08-07T14:27:40Z

tide/core/ScreenLock.h

+    ScreenLock();
+
+    /**
+     * Lock the screen. Notify about icoming streams and disallow


Notify about icoming streams?

rdumusc · 2017-08-07T14:34:33Z

tide/core/ScreenLock.h

+    Q_INVOKABLE void closeStream(QString uri);
+
+    /** Release the specified stream. */
+    Q_INVOKABLE void releaseStream(QString uri);


I thought we had discussed and agreed to rename these acceptStream and rejectStream?

rdumusc · 2017-08-07T14:35:43Z

tide/core/ScreenLock.h

+    void registerStream(QString uri);
+
+    /** Get the list of registered streams */
+    QStringList getStreamList();


I would call this getPendingStreams or getBlockedStreams

rdumusc · 2017-08-07T15:34:50Z

tide/master/rest/RestServer.cpp

+
+bool RestServer::_isWhitelisted(const std::string& source) const
+{
+    return _whitelist.count(source.substr(0, source.find(":")));


source.substr(0, source.find(":")) needs to be in a separate method _getHostname(source)

rdumusc · 2017-08-07T15:36:44Z

tide/master/rest/RestWindows.cpp

@@ -0,0 +1,161 @@
+/*********************************************************************/


Rebase error? rm this file!

rdumusc · 2017-08-07T15:38:15Z

tide/master/rest/serialization.cpp

@@ -88,6 +88,7 @@ QJsonObject to_json_object(ContentWindowPtr window, const DisplayGroup& group)
        {"fullscreen", window->isFullscreen()},
        {"focus", window->isFocused()},
        {"uri", window->getContent()->getURI()},
+        {"visible", window->getState() == ContentWindow::HIDDEN ? false : true},


Wouldn't things be simpler if we just omitted hidden windows?

rdumusc · 2017-08-07T15:40:26Z

tide/wall/RenderController.h

@@ -64,6 +64,7 @@ public slots:
    void updateRequestScreenshot();
    void updateDisplayGroup(DisplayGroupPtr displayGroup);
    void updateInactivityTimer(InactivityTimerPtr timer);
+    void updateLock(ScreenLockPtr markers);


ScreenLockPtr markers, right ;-)

rdumusc · 2017-08-07T15:45:34Z

tide/wall/RenderController.h

@@ -82,6 +83,7 @@ public slots:
    SwapSyncObject<InactivityTimerPtr> _syncInactivityTimer;
    SwapSyncObject<OptionsPtr> _syncOptions;
    SwapSyncObject<MarkersPtr> _syncMarkers;
+    SwapSyncObject<ScreenLockPtr> _syncLock;


please make sure that all the methods associated with syncLock are in the same order wrt. the others objects

rdumusc

Did I miss the unit tests for the ScreenLock class? ;-)

rdumusc · 2017-08-09T07:46:14Z

tests/cpp/core/RestServerTests.cpp

+        BOOST_CHECK_EQUAL(
+            response.second,
+            int(QNetworkReply::ContentOperationNotPermittedError));
+    }


rdumusc · 2017-08-09T08:29:41Z

tide/master/rest/RestInterface.h

@@ -57,8 +56,6 @@
 */
 class RestInterface : public QObject


You forgot the : public QObject!!

rdumusc · 2017-08-09T08:33:28Z

tide/master/rest/RestServer.cpp

@@ -53,7 +60,7 @@ RestServer::RestServer(const int port)
    : zeroeq::http::Server{zeroeq::URI{QString(":%1").arg(port).toStdString()}}
 {
    _init();
-    addToWhitelist("127.0.0.1");
+    _whitelist.insert("127.0.0.1");


this should must be in init(), otherwise it is not called by the default constructor!

Or more simply do it in the header:
std::set<std::string> _whitelist{{"127.0.0.1"}};

Or even simpler, since there is just one entry no need for a set:

bool RestServer::_isWhitelisted(const std::string& source) const { return _getHostname(source) == "127.0.0.1"; }

rdumusc · 2017-08-09T08:39:03Z

tide/master/MasterApplication.cpp

@@ -94,9 +94,9 @@ MasterApplication::MasterApplication(int& argc_, char** argv_,
    , _masterToForkerChannel(new MasterToForkerChannel(forkChannel))
    , _masterToWallChannel(new MasterToWallChannel(worldChannel))
    , _masterFromWallChannel(new MasterFromWallChannel(worldChannel))
+    , _lock(new ScreenLock)


since ScreenLock derives from enable_shared_from_this, please follow the same logic as for Options and Markers to enforce the creation of the object via a static create() method that returns a shared_ptr

rdumusc · 2017-08-09T08:41:07Z

tide/core/ScreenLock.h

-    /** Register a new stream to the lock. */
-    void registerStream(QString uri);
+    /** Request a new stream acceptance. */
+    void requestStreamAcceptance(QString uri);


consider reordering the methods in the logical order they can be used, with the "positive" action first:

requestStreamAcceptance
cancelStreamAcceptance

acceptStream
rejectStream

rdumusc · 2017-08-09T08:47:38Z

tide/master/PixelStreamWindowManager.h

@@ -167,7 +167,7 @@ class PixelStreamWindowManager : public QObject
     *
     * @param uri the URI of the streamer
     */
-    void streamAdded(QString uri);
+    void externalStreamOpening(QString uri);


The documentation should mention that at this point the stream window it hidden and the user is responsible to call showWindow() or handleStreamEnd().

Also, this class has unit tests and this new behaviour MUST be tested properly.

rdumusc · 2017-08-09T08:50:29Z

tide/master/rest/RestServer.h

@@ -72,6 +73,10 @@ class RestServer : public zeroeq::http::Server
    /** @return the port of the server. */
    int getPort() const;

+    /** Re-implemented handling of http requests from Server. */
+    std::future<zeroeq::http::Response> respondTo(


this whitelist behaviour should be unit tested too

rdumusc · 2017-08-10T16:17:10Z

tide/core/ScreenLock.h

@@ -60,6 +60,8 @@ class ScreenLock : public QObject,
        QStringList streamList READ getPendingStreams NOTIFY streamListChanged)

 public:
+    /** Create a shared ScreenLock object. */
+    static ScreenLockPtr create() { return ScreenLockPtr(new ScreenLock); }


-> now make the constructor private

rdumusc · 2017-08-10T16:23:12Z

tests/cpp/core/ScreenLockTests.cpp

+    screenLock->requestStreamAcceptance(STREAM2);
+    BOOST_CHECK(screenLock->getPendingStreams().length() == 2);
+
+    screenLock->unlock();


also check that calling unlock() emits streamAccepted() for each of the two pending streams

rdumusc · 2017-08-10T16:26:01Z

tests/cpp/core/ScreenLockTests.cpp

+
+    screenLock->rejectStream(STREAM1);
+    BOOST_CHECK_EQUAL(streamRejected, true);
+    BOOST_CHECK(screenLock->getPendingStreams().length() == 0);


please also check that calling rejectStream(STREAM1); a second time does not emit streamRejected.

rdumusc · 2017-08-10T16:26:55Z

tests/cpp/core/ScreenLockTests.cpp

+
+    screenLock->acceptStream(STREAM1);
+    BOOST_CHECK_EQUAL(streamAccepted, true);
+    BOOST_CHECK(screenLock->getPendingStreams().length() == 0);


please also check that calling acceptStream(STREAM1); a second time does not emit streamAccepted.

rdumusc · 2017-08-10T16:28:51Z

tests/cpp/core/ScreenLockTests.cpp

+const QString STREAM2("Stream2");
+}
+
+BOOST_AUTO_TEST_CASE(test_locking)


good, this is testing the locked behaviour, now do a second test case for the unlocked behaviour

rdumusc · 2017-08-22T09:36:46Z

Mostly good, except In ScreenLock some signals may still be emitted even when nothing changes. This should also be covered by the tests. I suggest we look at it together when you're back.

ppodhajski force-pushed the feature/Lock branch 5 times, most recently from b8198d2 to 35454ad Compare July 5, 2017 08:36

rdumusc suggested changes Jul 6, 2017

View reviewed changes

ppodhajski force-pushed the feature/Lock branch 4 times, most recently from 1386eda to 5920db6 Compare July 7, 2017 09:51

ppodhajski force-pushed the feature/Lock branch from e133bee to 76e3eef Compare August 3, 2017 11:41

rdumusc self-requested a review August 3, 2017 15:14

rdumusc reviewed Aug 4, 2017

View reviewed changes

ppodhajski force-pushed the feature/Lock branch from 76e3eef to acab80d Compare August 4, 2017 10:13

rdumusc self-assigned this Aug 4, 2017

rdumusc suggested changes Aug 7, 2017

View reviewed changes

ppodhajski force-pushed the feature/Lock branch 4 times, most recently from 0c885a0 to 036f606 Compare August 8, 2017 12:30

rdumusc reviewed Aug 9, 2017

View reviewed changes

rdumusc reviewed Aug 10, 2017

View reviewed changes

ppodhajski force-pushed the feature/Lock branch from 697afd4 to 480bcb6 Compare August 18, 2017 10:20

ppodhajski force-pushed the feature/Lock branch from 2db6f25 to 44dae75 Compare August 29, 2017 09:51

ScrenLock implemented for streams and HTML interface

c76cd07

ppodhajski force-pushed the feature/Lock branch 3 times, most recently from 480bcb6 to 174d2f6 Compare September 4, 2017 14:21

fix for Travis ci

e9a90ac

ppodhajski force-pushed the feature/Lock branch 3 times, most recently from e61aea2 to 8661409 Compare September 5, 2017 10:03

ppodhajski added 2 commits September 5, 2017 15:18

fixes for MacOS Sierra

847ee37

fixes

68cbf00

ppodhajski force-pushed the feature/Lock branch from 8661409 to 68cbf00 Compare September 5, 2017 13:27

Update RestServerTests.cpp

8cc9dca

rdumusc approved these changes Sep 6, 2017

View reviewed changes

rdumusc merged commit afa9f58 into BlueBrain:1.4-dev Sep 6, 2017

ppodhajski deleted the feature/Lock branch September 6, 2017 08:03


		BOOST_CHECK_EQUAL(response.toStdString(), "Forbidden");
		BOOST_CHECK_EQUAL(responseGET.second, int(QNetworkReply::NoError));

		@@ -0,0 +1,161 @@
		/*********************************************************************/

ScreenLock implenentation for streams and Web inteface #167

ScreenLock implenentation for streams and Web inteface #167

Conversation

ppodhajski commented Jul 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ppodhajski commented Jul 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdumusc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdumusc commented Aug 22, 2017