Adapting html interface to zeroeq changes by ppodhajski · Pull Request #128 · BlueBrain/Tide

ppodhajski · 2017-03-20T12:17:34Z

No description provided.

rdumusc · 2017-03-20T12:33:48Z

    core/JsonOptionsTests.cpp
    core/LoggingUtilityTest.cpp
    core/RestCommandTests.cpp
+    core/RestWindowsTests.cpp


alphab order

rdumusc · 2017-03-20T12:34:50Z

-    , _contentFilters( filters )
-{
-}
+#define BOOST_TEST_MODULE WebInterfaceTest


RestWindowsTests (same as file name)

rdumusc · 2017-03-20T12:36:52Z

+
+    auto future = windowsContent.getWindowInfo( std::string(), std::string( ));
+    auto response = future.get();
+    BOOST_CHECK( response.code ==  200 );


prefer BOOST_CHECK_EQUAL( a, b ) when comparing two values

rdumusc · 2017-03-20T12:37:42Z

+
+    const auto uuid = window->getID().toString().replace( _regex, "" );
+
+    sleep(2);


rdumusc · 2017-03-20T12:42:37Z

+    ContentWindowPtr window( new ContentWindow( content ));
+    displayGroup->addContentWindow( window );
+
+    auto future = windowsContent.getWindowInfo( std::string(), std::string( ));


also check the payload and headers (content type) for all requests

rdumusc · 2017-03-20T14:10:01Z

-    const auto url = _getThumbnailUri( *window );
-    const QString uri = window->getContent().get()->getURI();
-    _httpServer.handleGET(url.toStdString(), [uri] ()
+    QtConcurrent::run( QThreadPool::globalInstance(),


I think that QThreadPool::globalInstance() is the default, it can be omitted (also remove the include)

rdumusc · 2017-03-20T14:10:27Z

+    QtConcurrent::run( QThreadPool::globalInstance(),
+                       [this, window]()
    {
+        const QString& uri = window->getContent().get()->getURI();


rdumusc · 2017-03-20T14:12:15Z

 #define RESTWINDOWS_H

-#include <servus/serializable.h> // base class
+#include "zeroeq/http/response.h"


use <> for non-local includes and move after local includes

rdumusc · 2017-03-20T14:13:20Z

-    /** @return the string used as an endpoint by REST interface. */
-    std::string getTypeName() const final;
+    /**
+     * Expose the list of windows to REST Interface


update comment, add name and doc to the two parameters

rdumusc · 2017-03-20T14:13:52Z

     */
-    RestWindows( zeroeq::http::Server& server,
-                 const DisplayGroup& displayGroup );
+    RestWindows( const DisplayGroup& displayGroup );


update doc, server was removed from the params

rdumusc · 2017-03-22T10:03:14Z

+    DisplayGroupPtr displayGroup( new DisplayGroup( wallSize ));
+    FileReceiver fileReceiver;
+
+    bool open;


= false, otherwise the check is unpredictable ;-)

rdumusc · 2017-03-22T10:11:51Z

    response = future.get();
-    BOOST_CHECK( response.payload ==  _getThumbnail() );
-    BOOST_CHECK( response.code ==  200 );
+    BOOST_CHECK_EQUAL( response.code, 204 );


shouldn't it be a 404 for a window that has been removed? Otherwise the client will keep asking for it?

rdumusc · 2017-03-22T10:14:18Z

+    auto filePath = QString::fromStdString( path );
+    if( !_preparedPaths.contains( filePath ))
+        return _makeResponse( http::Code::FORBIDDEN, "info",
+                              "uploaded not prepared"  );


typo upload

rdumusc · 2017-03-22T10:14:23Z

                             const std::string& payload )
 {
+
+    auto filePath = QString::fromStdString( path );


rdumusc · 2017-03-22T10:20:17Z

    const bool success = openPromise->get_future().get();

-    _server.remove( "tide/upload/" + path );
+    _preparedPaths.removeOne( filePath );


can be moved right after file.close(), no need to wait for open() to succeed if it is removed always

rdumusc · 2017-03-22T10:50:19Z

-     * Expose the content of a directory to REST Interface
+     * List the content of a directory to be exposed to REST Interface
+     *
+     * @param pathPoint the path of a directory to list


missing @return

rdumusc · 2017-03-22T10:53:18Z

+        const auto endpointSplit = endpoint.split( "/" );
+        if( endpointSplit.size() == 2 && endpointSplit[1] == "thumbnail")
+        {
+            const auto uuid = endpointSplit[0];


const auto&

rdumusc · 2017-03-22T10:54:01Z

    /**
     * Expose the list of windows to REST Interface
+     *
+     * @param path the url part including window uuid and action


missing @return

rdumusc · 2017-03-22T10:55:25Z

+    QFile file( imageUri );
+    file.open( QIODevice::ReadOnly );
+    auto payload = file.readAll().toStdString();
+


you could also already check that open is false until here:
BOOST_REQUIRE_EQUAL( open, false );

rdumusc · 2017-03-22T10:56:25Z

+    /**
+     * Prepare an upload of a file via REST Interface.
+     *
+     * @param payload the name of a file client wants to upload.


@return missing

rdumusc · 2017-04-05T16:42:11Z

still need to fix travis build: https://travis-ci.org/BlueBrain/Tide/jobs/218908492

rdumusc suggested changes Mar 20, 2017

View reviewed changes

rdumusc reviewed Mar 22, 2017

View reviewed changes

ppodhajski force-pushed the feature/nonblocking branch 4 times, most recently from 44dd10b to f8e8fd8 Compare March 27, 2017 07:47

ppodhajski assigned rdumusc Mar 27, 2017

ppodhajski force-pushed the feature/nonblocking branch 3 times, most recently from 1d98dad to e3dbe6a Compare March 28, 2017 11:44

ppodhajski added 8 commits March 28, 2017 14:36

Adapting html interface to zeroeq changes

a615aad

CR1

2b255b7

cr2

dd84153

adding options and appps to web interface

d8dc977

callback function to accept Request

6e95122

hyphenated endpoints instead of camel case

71c34c8

dropped content to open according to drop event X and Y

50903b0

file path fix

551172b

ppodhajski force-pushed the feature/nonblocking branch from e3dbe6a to 551172b Compare March 28, 2017 13:27

Raphael Dumusc and others added 7 commits March 31, 2017 17:28

Code review, multiple fixes and cleanups, improved unit tests

bf0df11

fielupload fixes

f68aaf6

safari pointer fix

d859e19

adapt to use zeroeq::http::make_ready_response

32b74f2

preventing dragging a window with a control icon

ffbe1b4

minHeight, minWidth and zIndex hardcoded for focused windows

bbce1a4

homepage registered under empty endpoint

81dcb22

update SHA of ZeroEQ subproject

aff37ee

ppodhajski force-pushed the feature/nonblocking branch from 72887b2 to aff37ee Compare April 6, 2017 08:36

rdumusc approved these changes Apr 6, 2017

View reviewed changes

rdumusc merged commit 2c2e9d9 into BlueBrain:master Apr 6, 2017

ppodhajski deleted the feature/nonblocking branch May 4, 2017 10:15


		const auto uuid = window->getID().toString().replace( _regex, "" );

		sleep(2);

Conversation

ppodhajski commented Mar 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rdumusc commented Apr 5, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants