Adding Tide web interface by ppodhajski · Pull Request #114 · BlueBrain/Tide

ppodhajski · 2017-01-18T12:07:08Z

Web interface using rest interface to control the application and display its state

rdumusc · 2017-01-18T12:58:17Z

+    BOOST_CHECK_EQUAL( controller.getMinSizeAspectRatioCorrect(),
+                       QSize( 400, 300 ));
+
+    controller.resize( minSize, CENTER );


all those resize operations are not really expected to have an effect on the minSize. I would rather make some more checks in FULLSCREEN with sizeHints.

rdumusc · 2017-01-18T12:58:47Z

 typedef boost::shared_ptr< Markers > MarkersPtr;
 typedef boost::shared_ptr< MPIChannel > MPIChannelPtr;
 typedef boost::shared_ptr< Options > OptionsPtr;
+typedef boost::shared_ptr< MasterConfiguration > MasterConfigurationPtr;


Unused, remove

rdumusc · 2017-01-18T13:01:29Z

 class DataProvider;
 class DataSource;
 class DisplayGroup;
+class DisplayGroupController;


either forward-declare here OR #include "control/DisplayGroupController.h" in RestController.h, not both.

rdumusc · 2017-01-18T13:07:29Z

    _restInterface.get()->exposeStatistics( *(_logger.get()) );
+
+    _restInterface.get()->setController( *(_displayGroup.get()),
+                                         *(_config.get()) );


*(mySmartPtr.get()) can be just *mySmartPtr (3 occurences)

rdumusc · 2017-01-18T13:09:34Z

+QSizeF ContentWindowController::getMinSizeAspectRatioCorrect() const
+{
+    const qreal contentAspectRatio = _contentWindow.getContent()->
+                                                    getAspectRatio();


style: don't break at ->, break after = if it can't fit on one line

rdumusc · 2017-01-18T14:08:57Z

+void RestWindows::registerThumbnailUrl( ContentWindowPtr contentWindow )
+{
+    QString url = "tide/windows/" + contentWindow.get()->getID().toString()
+            .replace(_regex,"")+"/thumbnail";


rewrite this line (no break in the middle, const, etc..)

rdumusc · 2017-01-18T14:09:42Z

+    QString uri = contentWindow.get()->getContent().get()->getURI();
+    httpServer.get().handleGET(url.toStdString(), [uri] ()
+    {
+        QSize size(768,768);


magic number: it must be declared as a constant in the namespace{} at the beginnig of the file

rdumusc · 2017-01-18T14:10:32Z

+        buffer.open(QIODevice::WriteOnly);
+        image.save(&buffer, "PNG");
+        QString str = imageArray.toBase64();
+        return str.toStdString();


more compact:
return imageArray.toBase64().toStdString();

rdumusc · 2017-01-18T14:16:07Z

+    if( obj.empty( ))
+        return false;
+
+    const auto windowSize = QSizeF( obj["w"].toString().toDouble(),


QJsonValue can be directly converted to a double or an int, why make it a string first? Replace all occurences in this file

rdumusc · 2017-01-18T14:17:32Z

+    if( obj.empty( ))
+        return false;
+
+    const auto windowPos = QPointF( obj["x"].toString().toDouble(),


move the windowPos declaration below the check for if( !window ), there is no need to evaluate it if there is no window (also in the previous function).

rdumusc · 2017-01-25T08:26:51Z

+}
+
+button:active {
+    position:relative;


this is a visual effect when you click on a button

rdumusc · 2017-01-25T08:33:31Z

+  return closeButton;
+}
+
+function createFocusButton(tile) {


try to reorder in the same way as createCloseButton; if possible extract a common createButton() part, because they look very similar

rdumusc · 2017-01-25T08:39:22Z

+  curtain.css("width", wallWidth).css("height", wallHeight);
+  curtain.addClass("curtain");
+  if (type === "fs") {
+    curtain.css("z-index", 51);


use constants for the z_index

rdumusc · 2017-01-25T08:40:16Z

+  var scaleV = (viewportWidth - minimalHorizontalMargin ) / wallWidth;
+  var scaleH = (viewportHeight - minimalVerticalMargin ) / wallHeight;
+
+  zoomScale = (scaleV < scaleH) ? scaleV : scaleH;


this first value is never used

unless we have a vertical wall

rdumusc · 2017-01-25T08:41:03Z

+  var wallMargin = (window.innerWidth - (wallWidth * zoomScale)) / 2;
+
+  $("#wall").css({transform: 'scale(' + zoomScale + ')'}).css("margin-left", wallMargin).css
+  ("margin-right", wallMargin).css("margin-top", 25).css("margin-bottom", minimalVerticalMargin);


split in multiple lines for readability

rdumusc · 2017-01-25T08:59:11Z

+    bool _handleExitFullScreen();
+    bool _handleFocusWindows();
+    bool _handleMoveWindow( const std::string& string );
+    bool _handleMoveWindowToFront( const QString id );


rm const in header

rdumusc · 2017-01-25T09:00:40Z

+    bool _handleDeselectWindows();
+    bool _handleExitFullScreen();
+    bool _handleFocusWindows();
+    bool _handleMoveWindow( const std::string& string );


rename all "string" variables to "json" or something similar

rdumusc · 2017-01-25T09:01:16Z

+    bool _handleMoveWindowToFront( const QString id );
+    bool _handleMoveWindowToFullscreen( QString id );
+    bool _handleResizeWindow( const std::string& string );
+    bool _handleUnfocusWindow( const QString& string );


here rename string -> id

rdumusc · 2017-01-25T09:08:50Z

+    QJsonArray arr;
+    for( const auto& contentWindow : windows )
+    {
+        if (contentWindow.get()->getContent().get()->getURI() == "Launcher")


contentWindow->getContent()->getURI()

rdumusc · 2017-01-25T09:16:14Z

+{
+public:
+    /**
+     * Construct dynamic content used by REST interface.


dynamic content -> a JSON list of windows ?

rdumusc · 2017-01-25T13:41:31Z

-		window.get()->setSelected( false );
+            window.get()->setSelected( true );
+        else
+            window.get()->setSelected( false );


just: window->setSelected( !window->isSelected( ));

rdumusc · 2017-01-25T13:42:38Z

-  fullscreenButton.className = "windowControl";
-  fullscreenButton.style.visibility = tile.mode === 2 ? "hidden" : "visible";
-  var icon = fullscreenIcon.cloneNode(true);
+function setIcon(type) {


nice, but should be named createIcon(type)

ppodhajski assigned rdumusc Jan 18, 2017

ppodhajski requested a review from rdumusc January 18, 2017 12:52

ppodhajski force-pushed the master branch from c908adb to 9463643 Compare January 18, 2017 14:09

rdumusc suggested changes Jan 18, 2017

View reviewed changes

ppodhajski force-pushed the master branch 11 times, most recently from 744ca2d to 7efe185 Compare January 24, 2017 12:58

rdumusc reviewed Jan 25, 2017

View reviewed changes

ppodhajski force-pushed the master branch 2 times, most recently from e6ba727 to 314f6ad Compare January 25, 2017 11:50

rdumusc reviewed Jan 25, 2017

View reviewed changes

ppodhajski force-pushed the master branch from 314f6ad to 0c0791c Compare January 25, 2017 14:09

Adding web interface enabling remote control of the wall

e268935

ppodhajski force-pushed the master branch from 0c0791c to e268935 Compare January 26, 2017 13:44

Raphael Dumusc and others added 3 commits January 26, 2017 14:45

Update Changelog.md

08f8096

Update RestController.h

61ec527

Update ContentWindowControllerTests.cpp

329c489

rdumusc approved these changes Jan 26, 2017

View reviewed changes

rdumusc merged commit 4875225 into BlueBrain:master Jan 26, 2017

Conversation

ppodhajski commented Jan 18, 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants