Skip to content

HTML interface: file upload feature via HTML form and drag&drop#120

Merged
rdumusc merged 3 commits intoBlueBrain:masterfrom
ppodhajski:feature/droppableContentMetadata
Feb 21, 2017
Merged

HTML interface: file upload feature via HTML form and drag&drop#120
rdumusc merged 3 commits intoBlueBrain:masterfrom
ppodhajski:feature/droppableContentMetadata

Conversation

@ppodhajski
Copy link
Copy Markdown
Contributor

No description provided.

@ppodhajski ppodhajski requested a review from rdumusc February 14, 2017 15:14
@ppodhajski ppodhajski force-pushed the feature/droppableContentMetadata branch 3 times, most recently from 62512b7 to e6e0908 Compare February 15, 2017 10:14
BOOST_CHECK( files.contains( newValidFile ));
BOOST_CHECK( files.contains( "test.dcx" ));
BOOST_CHECK( files.contains( "test.dcxpreview" ));

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good, add one more check:
contentWindow->getContent()->getUri() == TEST_DIR + "/" + "uploaded.png";

*/
const QString& getContentDir() const;

/**
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

line too long. Try to keep the brief < 80 char:
Get the directory for uploading content via web interface

/**
* Get the directory for content uploaded via web interface and saved in a session.
* @return directory path
*/
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

this function should have a unit test

Comment thread tide/master/resources/tide.js Outdated
function handleUploadFromMenu(evt) {
evt.stopPropagation();
evt.preventDefault();
// var files = evt.target.files ;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

remove

Comment thread tide/master/resources/tide.js Outdated

var totalSize = 0;
for (var i = 0, f; f = files[i]; i++) {
totalSize += (f.size / 1048576);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

use a named constant for this magic number

Comment thread tide/master/rest/FileReceiver.cpp Outdated
_server.handlePUT( "tide/upload/" + path,
[ this, fileName, path ] ( const std::string& data )
{
const QString uploadDir = QDir::tempPath() + "/";
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

not really needed, directly:
QFile file( QDir::tempPath() + "/" + fileName );

Comment thread tide/master/rest/FileReceiver.cpp Outdated
QByteArray ba = QByteArray::fromRawData( data.c_str(), data.size( ));
QFile file( uploadDir + fileName );
file.open( QIODevice::WriteOnly );
file.write( ba );
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Both file.open() and file.write() could fail. This must be handled appropriately (at least return false)

Comment thread tide/master/rest/FileReceiver.h Outdated
#ifndef FILERECEIVER_H
#define FILERECEIVER_H

//#include "types.h"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

???

Comment thread tide/master/rest/RestInterface.cpp Outdated
_impl->fileReceiver.reset( new FileReceiver( _impl->httpServer.get( )));

connect( _impl->fileReceiver.get(), &FileReceiver::open,
[this]( QString uri )
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This function is duplicated from lines 124 - 127 !! It should have been a class function instead of two lambdas IF there was a need to check for relative paths, but that's not the case since FileReceiver always emits open with an absolute path, so here it is enough to just forward the signal...

if( uri.startsWith( QDir::tempPath( )))
{
const auto newUri = uploadDir + "/" + QFileInfo( uri ).fileName();
QDir().rename( uri, newUri );
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The files should be placed in a subfolder with the session name, otherwise a second file with the same name will overwrite the one from a previous session:
uploads/sessionABC/image.png
uploads/sessionXYZ/image.png
Also, log a message if the rename operation fails for some reason and don't replace the window's content...

@ppodhajski ppodhajski force-pushed the feature/droppableContentMetadata branch 10 times, most recently from 1d2f844 to 9edae5a Compare February 20, 2017 09:00
@ppodhajski ppodhajski force-pushed the feature/droppableContentMetadata branch from cddcaf6 to e5fde41 Compare February 21, 2017 09:23
@ppodhajski ppodhajski force-pushed the feature/droppableContentMetadata branch from e5fde41 to c8aaa3c Compare February 21, 2017 09:27
@rdumusc rdumusc merged commit 05284c1 into BlueBrain:master Feb 21, 2017
@ppodhajski ppodhajski deleted the feature/droppableContentMetadata branch May 4, 2017 10:15
@ppodhajski ppodhajski restored the feature/droppableContentMetadata branch May 4, 2017 10:15
@ppodhajski ppodhajski deleted the feature/droppableContentMetadata branch September 6, 2017 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants