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

Already on GitHub? Sign in to your account

Dummy pull request for comments / review #1

Closed
wants to merge 34 commits into
from

Conversation

Projects
None yet
3 participants
Owner

lfranchi commented Mar 20, 2013

No description provided.

AltarBeastiful and others added some commits Feb 10, 2013

@AltarBeastiful AltarBeastiful Add KQOauth lib in thirdparty. 6225ee8
@AltarBeastiful AltarBeastiful Add empty Dropbox account manager and config ui. abaaf03
@AltarBeastiful AltarBeastiful Add all parameters returned along with the OAuth token received in th…
…e signals accesToken and temporaryToken.
ab825ba
@AltarBeastiful AltarBeastiful Merge branch 'master' of https://github.com/tomahawk-player/tomahawk
Conflicts:
	src/libtomahawk/CMakeLists.txt
5a55a63
@AltarBeastiful AltarBeastiful Change the dropbox SVG icon to a lighter one. d9c9a70
@AltarBeastiful AltarBeastiful Add the link with the custom dropbox account and the JS resolver.
Add a layout to DropboxConfig.ui
9122e1e
@AltarBeastiful AltarBeastiful Add the possibility for JS resolvers to register for signals on their…
… config.ui.

Remove custom Dropbox account factory from QtResolver.
1831838
@AltarBeastiful AltarBeastiful Remove Dropbox custom account manager from tomahawk. ac82eac
@AltarBeastiful AltarBeastiful Add method to load an external JS file from the resolver folder. 92b70fb
@AltarBeastiful AltarBeastiful Add a method to request a QWebView from a JS resolver. 2f3a2e5
@AltarBeastiful AltarBeastiful Add Asynch POST request to tomahawk.js dc08224
@AltarBeastiful AltarBeastiful Merge remote-tracking branch 'tomaOrigin/master'
Conflicts:
	resources.qrc
dd0f6d0
@AltarBeastiful AltarBeastiful Merge remote-tracking branch 'tomaOrigin/master' 80f8a4d
@AltarBeastiful AltarBeastiful Merge remote-tracking branch 'tomaOrigin/master' 4fb4612
@AltarBeastiful AltarBeastiful Add bind function in tomahawk.js 16b636e
loclamor add a cloudstream class to manage cloud stream ; add a Q_INVOKABLE me…
…thode in QtScriptResolver to parse ID3Tags from a stream ; modify cmake configuration to use taglib 1.8 at least
170a926
loclamor merge resolution on QtScriptResolver cf0bbcb
@AltarBeastiful AltarBeastiful Change QtScriptResolverHelper::customIODeviceFactory to accept url wi…
…th HTTP headers.
1ad944d
@AltarBeastiful AltarBeastiful Merge branch 'master' of github.com:ISI-Peasy/tomahawk a1eef2a
@loclamor loclamor update of cloudStream reader 6bf196c
@AltarBeastiful AltarBeastiful Fix errors in parsing of ID3 tags (ReadCloudFile) b43e45a
@loclamor loclamor messages de debug sur les headers d196528
@loclamor loclamor ajout du parametre ID a readCloudFile d965af2
@loclamor loclamor ajout du parametre fileID a redCloudFile 5479175
@loclamor loclamor header authorization replacement 6950daa
@loclamor loclamor tentative de modification du oaut_nonce 013edb6
@AltarBeastiful AltarBeastiful Add synchronous post request to tomahawk.js c8522b7
@AltarBeastiful AltarBeastiful QwebInspector for Javascript resolvers. 78351e0
@AltarBeastiful AltarBeastiful Merge branch 'master' of https://github.com/tomahawk-player/tomahawk 8081cf4
@AltarBeastiful AltarBeastiful Extra headers and error message for Tomahawk.synchrequest 72fb384
@AltarBeastiful AltarBeastiful Change error test for duration of a track 0ff9909
@AltarBeastiful AltarBeastiful Merge branch 'feature/id3tag' ab46933
@AltarBeastiful AltarBeastiful Merge branch 'master', remote-tracking branch 'tomaOrigin/master' 142237c
@lfranchi lfranchi Merge branch 'master' into isi d5dd4d7

@lfranchi lfranchi commented on the diff Mar 20, 2013

data/js/tomahawk.js
xmlHttpRequest.send(null);
if (xmlHttpRequest.status == 200){
return xmlHttpRequest.responseText;
+ }else if (xmlHttpRequest.readyState === 4) {
@lfranchi

lfranchi Mar 20, 2013

Owner

Javascript coding style---please put spaces after } :)

@lfranchi lfranchi commented on the diff Mar 20, 2013

data/js/tomahawk.js
}
};
+Tomahawk.syncPostRequest = function(url, params, extraHeaders)
@lfranchi

lfranchi Mar 20, 2013

Owner

Might be good to call this syncFormPostRequest, as it x-www-form-urlencodes things. Is there no body to the POST?

@lfranchi lfranchi commented on the diff Mar 20, 2013

data/js/tomahawk.js
@@ -387,6 +449,29 @@ Tomahawk.sha256=function(s){
};
+if (!Function.prototype.bind) {
@lfranchi

lfranchi Mar 20, 2013

Owner

nitpick---is this taken from somewhere? Would be good to reference it

@lfranchi lfranchi commented on the diff Mar 20, 2013

src/libtomahawk/resolvers/QtScriptResolver.cpp
#include <boost/bind.hpp>
+//--- includes readcloudFile
+//#include "taghandlers/tag.h"
+# include "utils/cloudstream.h"
@lfranchi

lfranchi Mar 20, 2013

Owner

Capitalization :)

@loclamor

loclamor Mar 27, 2013

what do you mean here ?

@lfranchi

lfranchi Mar 27, 2013

Owner

we CapitalizeFiles.cpp like this :)

@lfranchi lfranchi commented on the diff Mar 20, 2013

src/libtomahawk/resolvers/QtScriptResolver.cpp
@@ -60,6 +100,7 @@
{
m_scriptPath = scriptPath;
m_resolver = parent;
+ network = new QNetworkAccessManager(this);
@lfranchi

lfranchi Mar 20, 2013

Owner

We have TomahawkUtils::nam(), do you need a new one here?

@loclamor

loclamor Mar 27, 2013

maybe not, i'll look about it

@lfranchi lfranchi commented on the diff Mar 20, 2013

src/libtomahawk/resolvers/QtScriptResolver.cpp
@@ -314,19 +355,208 @@
}
+void
+QtScriptResolverHelper::ReadCloudFile(const QString& fileName, const QString& fileId, const QString& sizeS, const QString& mime_type, const QVariant& requestJS, const QString& javascriptCallbackFunction)
@lfranchi

lfranchi Mar 20, 2013

Owner

Style :)

This goes for this file and the rest of the pull request---please follow the Tomahawk coding style. That means (look at other files for guidance):

  • Spaces between ()
  • { and } on new lines
  • return type on new line

@lfranchi lfranchi commented on the diff Mar 20, 2013

src/libtomahawk/resolvers/QtScriptResolver.cpp
+ }
+ else
+ {
+ download_url = QUrl(requestJS.toString());
+ }
+
+ tDebug( LOGINFO ) << "#### ReadCloudFile : Loading tags of " << fileName << " from " << download_url.toString() << " which have id " << fileId;
+
+
+ m["fileId"] = fileId;
+ m["mimetype"] = mime_type.toUtf8();
+
+
+ CloudStream* stream = new CloudStream(
+ download_url, fileName, size, headers, network);
+ stream->Precache();
@lfranchi

lfranchi Mar 20, 2013

Owner

Is this blocking?

@lfranchi lfranchi commented on the diff Mar 20, 2013

src/libtomahawk/resolvers/QtScriptResolver.cpp
+ }
+/* if (tag->tag()->albumArtist()) {
+ m["albumartist"] = tag->tag()->albumArtist();
+ }
+ if (tag->tag()->composer()) {
+ m["composer"] = tag->tag()->composer();
+ }
+ if (tag->tag()->discNumber() != 0) {
+ m["discnumber"] = tag->tag()->discNumber();
+ }
+*/
+ }
+
+ QString tabTagsJSON = "{";
+ //we convert the QVariantMap to JSON to give it as an argument of the callback function
+ int nbTags = m.count();
@lfranchi

lfranchi Mar 20, 2013

Owner

Please use QJson::Serializer instead of hand-coding this :)

@loclamor

loclamor Mar 27, 2013

this is corrected

@lfranchi lfranchi commented on the diff Mar 20, 2013

src/libtomahawk/resolvers/QtScriptResolver.cpp
+
+ m_resolver->m_engine->mainFrame()->evaluateJavaScript( getUrl );
+}
+
+
+void
+QtScriptResolverHelper::addLocalJSFile( const QString &jsFilePath )
+{
+ m_resolver->m_engine->mainFrame()->evaluateJavaScript( readRaw(jsFilePath) );
+}
+
+
+void
+QtScriptResolverHelper::requestWebView(const QString &varName, const QString &url)
+{
+ QWebView *view = new QWebView();
@lfranchi

lfranchi Mar 20, 2013

Owner

What do you need this for?

@AltarBeastiful

AltarBeastiful Mar 21, 2013

This is the OAuth webpage we redirect the user to accept our app in googleDrive/Dropbox.

Another solution is to open the default webbrowser, which one do you prefer?

@lfranchi lfranchi commented on the diff Mar 20, 2013

src/libtomahawk/resolvers/QtScriptResolver.cpp
+{
+ QWebView *view = new QWebView();
+ view->load(QUrl(url));
+
+ //TODO: move this to JS.
+ view->setWindowModality(Qt::ApplicationModal);
+
+ m_resolver->m_engine->mainFrame()->addToJavaScriptWindowObject(varName, view);
+}
+
+void
+QtScriptResolverHelper::showWebInspector()
+{
+ QWebInspector *inspector = new QWebInspector;
+ inspector->setPage(m_resolver->m_engine);
+ inspector->show();
@lfranchi

lfranchi Mar 20, 2013

Owner

Likewise, is this just for debugging internally?

@AltarBeastiful

AltarBeastiful Mar 22, 2013

Yeah, it was supposed to! Never got it working and as it was at the end of the implementation of the resolvers we sticked with logging and bashing heads on the wall.
I'm going to see if it cannot work, as it would be really useful to create new resolvers.
For now we only have "TypeError: 'undefined' is not a function" 0 "undefined" which is kinda thin!

@lfranchi lfranchi commented on the diff Mar 20, 2013

src/libtomahawk/resolvers/QtScriptResolver.cpp
QString getUrl = QString( "Tomahawk.resolver.instance.%1( '%2' );" ).arg( m_urlCallback )
.arg( QString( QUrl( result->url() ).toEncoded() ) );
- QString urlStr = m_resolver->m_engine->mainFrame()->evaluateJavaScript( getUrl ).toString();
+ QVariant jsResult = m_resolver->m_engine->mainFrame()->evaluateJavaScript( getUrl ).toString();
+
+ if(jsResult.type() == QVariant::Map)
@lfranchi

lfranchi Mar 20, 2013

Owner

I don't like how we have 2 different return value types (string or map)... can we standardize on a QVariantMap (and require a url parameter)?

@AltarBeastiful

AltarBeastiful Mar 22, 2013

I'd like that too. However changing this would impact all resolvers with a custom url handler ! Maybe push this version first, update all resolvers, then change to QVariantMap only?

@lfranchi lfranchi commented on the diff Mar 20, 2013

src/libtomahawk/resolvers/QtScriptResolver.h
@@ -59,6 +64,17 @@ class DLLEXPORT QtScriptResolverHelper : public QObject
Q_INVOKABLE QByteArray base64Encode( const QByteArray& input );
Q_INVOKABLE QByteArray base64Decode( const QByteArray& input );
+
+ // send ID3Tags of the stream as argument of the callback function
+ Q_INVOKABLE void
+ ReadCloudFile(const QString& fileName, const QString& fileId, const QString& sizeS, const QString& mime_type, const QVariant& requestJS, const QString& javascriptCallbackFunction);
@lfranchi

lfranchi Mar 20, 2013

Owner

camelCase please :)

@lfranchi lfranchi commented on the diff Mar 20, 2013

src/libtomahawk/resolvers/QtScriptResolver.h
@@ -87,6 +103,7 @@ class DLLEXPORT QtScriptResolverHelper : public QObject
#ifdef QCA2_FOUND
QCA::Initializer m_qcaInit;
#endif
+ QNetworkAccessManager* network;
@lfranchi

lfranchi Mar 20, 2013

Owner

You shouldn't need this (see comment above), but anyway instance variables should be prefixed with m_

@lfranchi lfranchi commented on the diff Mar 20, 2013

src/libtomahawk/utils/cloudstream.cpp
+ // than the raw data so we must disable compression.
+ if (url_.host() == "files.one.ubuntu.com") {
+ request.setRawHeader("Accept-Encoding", "identity");
+ }
+
+ //tDebug() << request.rawHeader("Authorization");
+ tDebug() << "######## CloudStream : HTTP request : ";
+ foreach(const QByteArray& header, request.rawHeaderList()){
+ tDebug() << "#### CloudStream : header request : " << header << " = " << request.rawHeader(header);
+ }
+
+ QNetworkReply* reply = network_->get(request);
+ connect(reply, SIGNAL(sslErrors(QList<QSslError>)), SLOT(SSLErrors(QList<QSslError>)));
+ ++num_requests_;
+
+ QEventLoop loop;
@lfranchi

lfranchi Mar 20, 2013

Owner

Do you need this to be blocking/synchronous? I know this is hatstand's code and not yours, but maybe clementine had a different requirement. Remember that any blocking call in JS in our resolvers will block the main UI thread because QtWebkit is run on the main thread.

Any net requests (especially slow ones like this!) have the potential to lock up the UI so should be async :)

Owner

lfranchi commented Mar 20, 2013

What do you need kqoauth for? I would rather avoid including it if possible... I want to rope @jefferai into this conversation, but basically OAuth2 is not really standard, and each site implements it slightly differently. What do you need this for, and will it over all the user cases we have for it (all services we might want)?

Ya actually kqoauth is gonna disappear, part of an old proof on concept. All oauth is implemented in the resolvers.

@lfranchi lfranchi referenced this pull request in tomahawk-player/tomahawk-resolvers Mar 22, 2013

Open

Google Drive and Dropbox resolvers #41

@lfranchi lfranchi closed this Mar 22, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment