Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added QML whiteboard application #93

Merged
merged 1 commit into from
Oct 20, 2016
Merged

Added QML whiteboard application #93

merged 1 commit into from
Oct 20, 2016

Conversation

ppodhajski
Copy link
Contributor

whiteboard application integrated with launcher

rdumusc
rdumusc suggested changes Oct 18, 2016
View reviewed changes
apps/Whiteboard/CMakeLists.txt

common_application(whiteboard)
if(TARGET VirtualKeyboard)
add_dependencies(tideLauncher VirtualKeyboard)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are you using the virtualkeyboard? I didn't see it the in the demo this morning

apps/Whiteboard/CMakeLists.txt

# Copyright (c) 2016, EPFL/Blue Brain Project
# Ahmet Bilgili <ahmet.bilgili@epfl.ch>
# Raphael Dumusc <raphael.dumusc@epfl.ch>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know you were called Ahmet and Raphael :-)

apps/Whiteboard/Whiteboard.cpp
/*********************************************************************/
/* Copyright (c) 2016, EPFL/Blue Brain Project */
/* Raphael Dumusc <raphael.dumusc@epfl.ch> */
/* Ahmet Bilgili <ahmet.bilgili@epfl.ch> */
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

apps/Whiteboard/Whiteboard.h
/*********************************************************************/
/* Copyright (c) 2016, EPFL/Blue Brain Project */
/* Raphael Dumusc <raphael.dumusc@epfl.ch> */
/* Ahmet Bilgili <ahmet.bilgili@epfl.ch> */
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dito

apps/Whiteboard/Whiteboard.h


/**
* Separate application which streams the white board using deflect::Qt API.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"the white board" -> "a whiteboard" (I think it is used as a single word in other places)

tide/master/PixelStreamWindowManager.h
@@ -100,7 +100,7 @@ class PixelStreamWindowManager : public QObject
* @param webbrowser create a webbrowser window.
*/
void openWindow( const QString& uri, const QPointF& pos, const QSize& size,
bool webbrowser = false );
bool webbrowser = false, bool whiteboard = false );
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that is starting to make too many booleans with impossible combinations (it can't be a webbrowser AND a whiteboard at the same time). Replace with an enum and also adjust the function documentation.

tide/master/MasterConfiguration.cpp
@@ -50,6 +50,7 @@ namespace
const int DEFAULT_WEBSERVICE_PORT = 8888;
const QRegExp TRIM_REGEX( "[\\n\\t\\r]" );
const QString DEFAULT_URL( "http://www.google.com" );
const QString DEFAULT_WHITEBOARD_SAVE_URL( "/tmp/whiteboard_" );
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why a trailing _ ?

tide/master/MasterConfiguration.h
* @return The URL defined in the configuration file, or a default value if
* none is found.
*/
const QString& getWhiteboardDefaultSaveURL() const;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getWhiteboardDefaultSaveURL() -> getWhiteboardSaveFolder ?

Please add unit tests for this new functionality in tests/cpp/core/ConfigurationTests.cpp.

tide/master/MasterConfiguration.h
@@ -108,6 +108,13 @@ class MasterConfiguration : public Configuration
const QString& getWebBrowserDefaultURL() const;

/**
* Get the URL used for saving whiteboard
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Get the folder used for saving whiteboard images

apps/Whiteboard/CMakeLists.txt

set(WHITEBOARD_LINK_LIBRARIES DeflectQt TideMaster)

common_application(whiteboard)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to name it tideWhiteboard

rdumusc
rdumusc suggested changes Oct 19, 2016
View reviewed changes
tide/master/PixelStreamWindowManager.cpp
@@ -96,7 +96,7 @@ void PixelStreamWindowManager::showWindow( const QString& uri )
void PixelStreamWindowManager::openWindow( const QString& uri,
const QPointF& pos,
const QSize& size,
const bool webbrowser )
StreamType stream)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const StreamType stream )

tide/master/PixelStreamWindowManager.cpp
ContentFactory::getWebbrowserContent( uri ) :
ContentFactory::getPixelStreamContent( uri );
ContentPtr content;
if (stream == PixelStreamWindowManager::WEBBROWSER){
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is javascript code style, does not match c++ style, but just leave it as it was:
ContentPtr content = (stream == PixelStreamWindowManager::WEBBROWSER) ? ContentFactory::getWebbrowserContent( uri ) :
ContentFactory::getPixelStreamContent( uri );

tide/master/PixelStreamWindowManager.h
@@ -97,10 +105,10 @@ class PixelStreamWindowManager : public QObject
* @param pos the desired position for the center of the window in pixels.
* If pos.isNull(), the window is centered on the DisplayGroup.
* @param size the desired size of the window in pixels.
* @param webbrowser create a webbrowser window.
* @param StreamType enum controls the focus of a window upon opening. WEBBROWSER creates a webbrowser window.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reveals too much about the implementation, users should not rely on the internal behaviour. Could be put more simply:
@param stream the type of window to create

tide/master/PixelStreamWindowManager.h
void openWindow( const QString& uri, const QPointF& pos, const QSize& size,
bool webbrowser = false );

void openWindow(const QString& uri, const QPointF& pos, const QSize& size, StreamType = STANDARD);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing spaces inside (), missing variable name for reference in the docstring above:
StreamType stream= STANDARD

tide/master/localstreamer/PixelStreamerLauncher.cpp
const QString url )
{
static int webbrowserCounter = 0;
const QString& uri = QString( "WebBrowser_%1" ).arg( webbrowserCounter++ );

const QSize viewportSize = !size.isEmpty() ? size : WEBBROWSER_DEFAULT_SIZE;
_windowManager.openWindow( uri, pos, viewportSize, true );
const qreal x = 0.5 * _config.getTotalWidth();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this code duplicated in openWhiteboard(). Make it to a function:
QPointF PixelStreamerLauncher::_getDefaultWindowPosition() const
{
return { 0.5 * _config.getTotalWidth(), 0.35 * _config.getTotalHeight() };
}

tide/master/localstreamer/PixelStreamerLauncher.cpp
const QPointF centerPos( x, y );
const QSize viewportSize = WHITEBOARD_DEFAULT_SIZE;

_windowManager.openWindow( uri, centerPos, viewportSize, PixelStreamWindowManager::WHITEBOARD );
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

80 char line length limit exceeded

tide/master/localstreamer/PixelStreamerLauncher.cpp
options.setConfiguration( _config.getFilename( ));

_processes.insert( uri );
const QString command = _getWhiteboardBin() + QString( ' ' ) + options.getCommandLine();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

80 char line length limit exceeded

rdumusc
rdumusc suggested changes Oct 20, 2016
View reviewed changes
Copy link

@rdumusc rdumusc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting there, last changes hopefully

tide/master/localstreamer/PixelStreamerLauncher.cpp
}

const QString PixelStreamerLauncher::launcherUri = QString( "Launcher" );

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is that space doing here?

tide/master/localstreamer/PixelStreamerLauncher.cpp
_windowManager.openWindow( uri, pos, viewportSize, true );

if ( pos.isNull() ){
pos=_getDefaultWindowPosition();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

arrgggh, coding style please:

if( pos.isNull( ))
    pos = _getDefaultWindowPosition();

tide/master/localstreamer/PixelStreamerLauncher.cpp
_windowManager.openWindow( uri, centerPos, viewportSize,
PixelStreamWindowManager::WHITEBOARD );
CommandLineOptions options;
options.setPixelStreamerType( PS_WEBKIT );
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong, the whiteboard is not a webkit. This line can be deleted.

tide/master/PixelStreamWindowManager.h
@@ -97,10 +105,11 @@ class PixelStreamWindowManager : public QObject
* @param pos the desired position for the center of the window in pixels.
* If pos.isNull(), the window is centered on the DisplayGroup.
* @param size the desired size of the window in pixels.
* @param webbrowser create a webbrowser window.
* @param stream the type of a window to create
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

of a window -> of window

apps/Whiteboard/Whiteboard.cpp
#include "tide/master/MasterConfiguration.h"

#include <QHostInfo>
#include <QQmlContext>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

those two includes are not used, remove them

@rdumusc rdumusc changed the title QML whiteboard implemented Added QML whiteboard application Oct 20, 2016
@ppodhajski ppodhajski merged commit 0770b26 into BlueBrain:master Oct 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rdumusc rdumusc rdumusc approved these changes

Assignees

@rdumusc rdumusc

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@ppodhajski @rdumusc