bug fixes + improvements in whiteboard application #96

ppodhajski · 2016-10-24T12:05:38Z

Improvements to the Whiteboard application:

Added freezing of the canvas/touch-area when save panel visible
Bug fixed on canvas extending
Bug fixed when multiple touch points present
Changed behaviour of color/brush size selector

rdumusc · 2016-10-31T09:42:35Z

apps/Whiteboard/qml/whiteboard.qml

+                infoBox.z = 2
+                infoText.text = "Are you sure? File already exists"
+                textInput.focus = false
+                butOk.visible = true


please avoid custom, ambiguous abbreviations for variables names:
butOK -> buttonOK
butCancel -> buttonCancel

rdumusc · 2016-10-31T09:43:40Z

apps/Whiteboard/qml/whiteboard.qml

+            }
+        }
+    }
+    function saveCanvas(t) {


also here, what does t stand for? Give this variable a self-explanatory name

rdumusc · 2016-10-31T09:46:09Z

apps/Whiteboard/qml/whiteboard.qml

+                id: areaAux
+                anchors.fill: parent
+                enabled: false
+                z: 0


I think z = 0 by default so no need to specify that, right? (multiple occurences)

rdumusc · 2016-10-31T09:49:56Z

apps/Whiteboard/qml/whiteboard.qml

+            textInput.focus = true
+            infoBox.z = 0
+            area.enabled = false
+            areaAux.enabled = true


I think all these changes should be applied by a State change, or as property bindings, whichever makes more sense

rdumusc · 2016-10-31T09:52:07Z

apps/Whiteboard/qml/whiteboard.qml

-
-
+            MultiPointTouchArea {
+                id: areaAux


areaAux is not a very clear name, what does it really do? Is it the savePanelBackround?

rdumusc · 2016-10-31T09:53:03Z

apps/Whiteboard/qml/whiteboard.qml

+                   checkFileExists()
+                   if (textInput.text == "")
+                    Qt.inputMethod.show()
+


extra line here

rdumusc · 2016-10-31T09:55:41Z

apps/Whiteboard/qml/whiteboard.qml

+                validator: RegExpValidator {
+                    regExp: /[\w.]*/
+                }
+               onAccepted: {


something is not correctly indented here

rdumusc · 2016-10-31T09:56:41Z

apps/Whiteboard/qml/whiteboard.qml

                    anchors.fill: parent
+                    anchors.margins: 10
+                    z: parent.z


I'm expecting that this is the default behaviour

rdumusc · 2016-10-31T10:08:51Z

tide/master/PixelStreamWindowManager.cpp

-                                    ContentFactory::getWebbrowserContent( uri ) :
-                                    ContentFactory::getPixelStreamContent( uri );
+
+    ContentPtr content;


Hmmm, creating the appropriate type of Content should be the responsibility of the ContentFactory. There was already an exception with the getWebbrowserContent(), but this is becoming too many exceptions. I think we should move the StreamType to ContentType, and pass this as the second parameter to ContentFactory::getPixelStreamContent and remove getWebbrowserContent.

I guess re-using ContentType::CONTENT_TYPE would be ok?

Not quite... We could add a ContentType::Whiteboard, along with a matching WhiteboardContent class. But there are two use cases where we want a StreamType as a parameter, not an arbitrary ContentType: PixelStreamWindowManager::openWindow() and the new ContentFactory::getPixelStreamContent().

rdumusc · 2016-10-31T10:09:17Z

tide/core/scene/PixelStreamContent.h

@@ -56,7 +56,7 @@ class PixelStreamContent : public Content
     * @param uri The unique stream identifier.
     * @param keyboard Show the keyboard action.
     */
-    explicit PixelStreamContent( const QString& uri, bool keyboard = true );
+    explicit PixelStreamContent( const QString& uri, bool keyboard = false );


missing corresponding changes in PixelStreamContent.cpp?

forget this comment, I read too quickly :-)

rdumusc

FileInfoHelper.h has been deleted, not moved to the master lib

rdumusc · 2016-10-31T15:16:04Z

tide/core/scene/ContentType.h

@@ -53,9 +53,17 @@ enum CONTENT_TYPE
    CONTENT_TYPE_TEXTURE,
    CONTENT_TYPE_PDF,
    CONTENT_TYPE_WEBBROWSER,
+    CONTENT_TYPE_WHITEBOARD,


Unused and incomplete, remove!

rdumusc · 2016-10-31T15:26:16Z

tide/core/scene/ContentFactory.cpp

-#endif
+    if ( stream == WEBBROWSER )
+    {
+        #if TIDE_USE_QT5WEBKITWIDGETS || TIDE_USE_QT5WEBENGINE


Leave #if macros indented to the left.

The idea of passing the StreamType here was also to remove the keyboard bool parameter and decide here to hide it for the whiteboards, which in turns makes the code cleaner in PixelStreamWindowManager.cpp

rdumusc · 2016-10-31T15:27:06Z

tide/core/scene/ContentFactory.h

-
-    /** Create a Webbrowser Content (special type of PixelStream). */
-    static ContentPtr getWebbrowserContent( const QString& uri );
+    static ContentPtr getPixelStreamContent( const QString& uri, const StreamType stream = STANDARD, const bool keyboard = true );


don't specify const for pass-by-value parameters in header files

rdumusc · 2016-10-31T15:30:10Z

tide/core/scene/ContentType.h

    CONTENT_TYPE_IMAGE_PYRAMID
 };

+enum StreamType
+{
+    STANDARD,


don't forget to rebase your branch on master, this enum was modified:
https://github.com/BlueBrain/Tide/pull/97/files#diff-c5d7e3f8e70fc822ebefde7ca74b268fR57

ppodhajski force-pushed the master branch from bc91980 to ce72486 Compare October 24, 2016 12:30

ppodhajski assigned rdumusc Oct 24, 2016

ppodhajski force-pushed the master branch 2 times, most recently from ad2f0cd to 1917508 Compare October 28, 2016 13:38

rdumusc suggested changes Oct 31, 2016

View reviewed changes

ppodhajski force-pushed the master branch 2 times, most recently from ac1be94 to 6480b7e Compare October 31, 2016 14:37

rdumusc reviewed Oct 31, 2016

View reviewed changes

ppodhajski force-pushed the master branch 3 times, most recently from 210eb18 to bfa062c Compare October 31, 2016 16:16

bug fixes + improvements in whiteboard application

a1d77c2

ppodhajski force-pushed the master branch 9 times, most recently from d1d77c1 to 490a321 Compare November 4, 2016 08:43

Unifying UI of Whiteboard with Launcher

236af65

ppodhajski force-pushed the master branch from 490a321 to 236af65 Compare November 4, 2016 08:44

rdumusc approved these changes Nov 4, 2016

View reviewed changes

rdumusc merged commit dcfae34 into BlueBrain:master Nov 4, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bug fixes + improvements in whiteboard application #96

bug fixes + improvements in whiteboard application #96

ppodhajski commented Oct 24, 2016

rdumusc Oct 31, 2016

rdumusc Oct 31, 2016

rdumusc Oct 31, 2016

rdumusc Oct 31, 2016

rdumusc Oct 31, 2016

rdumusc Oct 31, 2016

rdumusc Oct 31, 2016

rdumusc Oct 31, 2016

rdumusc Oct 31, 2016

ppodhajski Oct 31, 2016

rdumusc Oct 31, 2016

rdumusc Oct 31, 2016

rdumusc Oct 31, 2016

rdumusc left a comment

rdumusc Oct 31, 2016

rdumusc Oct 31, 2016

rdumusc Oct 31, 2016

rdumusc Oct 31, 2016

bug fixes + improvements in whiteboard application #96

bug fixes + improvements in whiteboard application #96

Conversation

ppodhajski commented Oct 24, 2016

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