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

added support for torrent tracker LostFilm.tv #471

Merged
merged 1 commit into from Mar 10, 2013

Conversation

Projects
None yet
3 participants
Contributor

daimor commented Mar 1, 2013

added support gzip
fixed - lost cookies after redirect on download rss feed url's
fixed - cookies for auto download torrent from rss feed

Contributor

sledgehammer999 commented Mar 4, 2013

Why would you include Netbeans specific conf files? I am almost certain that the author won't accept this request as it is. Please remove:

  • nbproject/configurations.xml
  • nbproject/private/Default.properties
  • nbproject/private/configurations.xml
  • nbproject/private/private.xml
  • nbproject/project.xml

Please make a new clean pull request. Thank you.

Contributor

daimor commented Mar 5, 2013

removed nbproject

Contributor

cdumez commented Mar 10, 2013

Please keep the changes minimal to help review the patch. For e.g. do not change the indentation, do not edit the gitignore file, ... In principal, doing the gzip decompress in qBittorrent in a good idea though. Until now, it was done in our search plugins infrastructure (in Python) but it does not affect torrents added via other means than the search engine (e.g. via an external link).

Contributor

daimor commented Mar 10, 2013

ok, I chaged commit.

@cdumez cdumez commented on an outdated diff Mar 10, 2013

src/downloadthread.cpp
@@ -40,16 +40,67 @@
#include "rsssettings.h"
#endif
#include "qinisettings.h"
+#include <zlib.h>
+#include <qt4/QtCore/qbitarray.h>
@cdumez

cdumez Mar 10, 2013

Contributor

Why is this include needed? You don't seem to use QBitArray.

@cdumez

cdumez Mar 10, 2013

Contributor

Also we use includes such as #include <QBitArray.h> in qBittorrent, no qt4/QtCore/

@cdumez cdumez commented on an outdated diff Mar 10, 2013

src/downloadthread.cpp
+
+ QByteArray result;
+
+ int ret;
+ z_stream strm;
+ static const int CHUNK_SIZE = 1024;
+ char out[CHUNK_SIZE];
+
+ /* allocate inflate state */
+ strm.zalloc = Z_NULL;
+ strm.zfree = Z_NULL;
+ strm.opaque = Z_NULL;
+ strm.avail_in = data.size();
+ strm.next_in = (Bytef*) (data.data());
+
+ ret = inflateInit2(&strm, 15 + 32); // gzip decoding
@cdumez

cdumez Mar 10, 2013

Contributor

int ret. Please declare at the same time as you initialize when possible.

@cdumez cdumez commented on an outdated diff Mar 10, 2013

src/downloadthread.cpp
+
+ ret = inflateInit2(&strm, 15 + 32); // gzip decoding
+ if (ret != Z_OK)
+ return QByteArray();
+
+ // run inflate()
+ do {
+ strm.avail_out = CHUNK_SIZE;
+ strm.next_out = (Bytef*) (out);
+
+ ret = inflate(&strm, Z_NO_FLUSH);
+ Q_ASSERT(ret != Z_STREAM_ERROR); // state not clobbered
+
+ switch (ret) {
+ case Z_NEED_DICT:
+ ret = Z_DATA_ERROR; // and fall through
@cdumez

cdumez Mar 10, 2013

Contributor

What's the point of setting ret here?

@cdumez cdumez commented on an outdated diff Mar 10, 2013

src/downloadthread.cpp
+ strm.next_out = (Bytef*) (out);
+
+ ret = inflate(&strm, Z_NO_FLUSH);
+ Q_ASSERT(ret != Z_STREAM_ERROR); // state not clobbered
+
+ switch (ret) {
+ case Z_NEED_DICT:
+ ret = Z_DATA_ERROR; // and fall through
+ case Z_DATA_ERROR:
+ case Z_MEM_ERROR:
+ (void) inflateEnd(&strm);
+ return QByteArray();
+ }
+
+ result.append(out, CHUNK_SIZE - strm.avail_out);
+ } while (strm.avail_out == 0);
@cdumez

cdumez Mar 10, 2013

Contributor

I prefer while (!strm.avail_out)

@cdumez cdumez commented on an outdated diff Mar 10, 2013

src/downloadthread.cpp
+ return QByteArray();
+ }
+
+ QByteArray result;
+
+ int ret;
+ z_stream strm;
+ static const int CHUNK_SIZE = 1024;
+ char out[CHUNK_SIZE];
+
+ /* allocate inflate state */
+ strm.zalloc = Z_NULL;
+ strm.zfree = Z_NULL;
+ strm.opaque = Z_NULL;
+ strm.avail_in = data.size();
+ strm.next_in = (Bytef*) (data.data());
@cdumez

cdumez Mar 10, 2013

Contributor

Please use C++ cast.

@cdumez cdumez commented on an outdated diff Mar 10, 2013

src/downloadthread.cpp
/** Download Thread **/
DownloadThread::DownloadThread(QObject* parent) : QObject(parent) {
connect(&m_networkManager, SIGNAL(finished (QNetworkReply*)), this, SLOT(processDlFinished(QNetworkReply*)));
#ifndef QT_NO_OPENSSL
- connect(&m_networkManager, SIGNAL(sslErrors(QNetworkReply*,QList<QSslError>)), this, SLOT(ignoreSslErrors(QNetworkReply*,QList<QSslError>)));
+ connect(&m_networkManager, SIGNAL(sslErrors(QNetworkReply*,QList<QSslError>)), this, SLOT(ignoreSslErrors(QNetworkReply*, QList<QSslError>)));
#endif
@cdumez

cdumez Mar 10, 2013

Contributor

Useless change, especially if you fix spacing in the SLOT but not the SIGNAL :)

Contributor

cdumez commented Mar 10, 2013

The change looks great overall. I would like the few nits to be fixed before merging though.

Contributor

daimor commented Mar 10, 2013

I changed cast in gUncompress

@cdumez cdumez commented on an outdated diff Mar 10, 2013

src/downloadthread.h
@@ -62,6 +63,7 @@ class DownloadThread : public QObject {
#endif
private:
+ QByteArray gUncompress(Bytef *inData, size_t len);
@cdumez

cdumez Mar 10, 2013

Contributor

Should probably be static

@cdumez cdumez commented on an outdated diff Mar 10, 2013

src/downloadthread.h
@@ -36,6 +36,7 @@
#include <QObject>
#include <QHash>
#include <QSslError>
+#include <zconf.h>
@cdumez

cdumez Mar 10, 2013

Contributor

Why not zlib.h?

@cdumez cdumez commented on an outdated diff Mar 10, 2013

src/downloadthread.cpp
+
+ ret = inflateInit2(&strm, 15 + 32); // gzip decoding
+ if (ret != Z_OK)
+ return QByteArray();
+
+ // run inflate()
+ do {
+ strm.avail_out = CHUNK_SIZE;
+ strm.next_out = reinterpret_cast<unsigned char*>(out);
+
+ ret = inflate(&strm, Z_NO_FLUSH);
+ Q_ASSERT(ret != Z_STREAM_ERROR); // state not clobbered
+
+ switch (ret) {
+ case Z_NEED_DICT:
+ ret = Z_DATA_ERROR; // and fall through
@cdumez

cdumez Mar 10, 2013

Contributor

What is the point of setting ret here? I don't see it being useful.

@cdumez cdumez commented on an outdated diff Mar 10, 2013

src/downloadthread.cpp
+
+ QByteArray result;
+
+ int ret;
+ z_stream strm;
+ static const int CHUNK_SIZE = 1024;
+ char out[CHUNK_SIZE];
+
+ /* allocate inflate state */
+ strm.zalloc = Z_NULL;
+ strm.zfree = Z_NULL;
+ strm.opaque = Z_NULL;
+ strm.avail_in = len;
+ strm.next_in = inData;
+
+ ret = inflateInit2(&strm, 15 + 32); // gzip decoding
@cdumez

cdumez Mar 10, 2013

Contributor

int ret = ...; // Please declare ret at the same time as you initialize it.

@cdumez

cdumez Mar 10, 2013

Contributor

It would be nice to have global constants to clarify what 15 and 32 stand for.

Contributor

daimor commented Mar 10, 2013

ok

@cdumez cdumez added a commit that referenced this pull request Mar 10, 2013

@cdumez cdumez Merge pull request #471 from daimor/master
Correctly handle HTTP responses with gzipped encoded content.
13e57fb

@cdumez cdumez merged commit 13e57fb into qbittorrent:master Mar 10, 2013

Contributor

cdumez commented Mar 10, 2013

Looks great, thanks.

Contributor

cdumez commented Mar 10, 2013

Backported to v3_0_x as well.

@cdumez cdumez commented on the diff Mar 10, 2013

src/downloadthread.cpp
+ }
+
+ QByteArray result;
+
+ z_stream strm;
+ static const int CHUNK_SIZE = 1024;
+ char out[CHUNK_SIZE];
+
+ /* allocate inflate state */
+ strm.zalloc = Z_NULL;
+ strm.zfree = Z_NULL;
+ strm.opaque = Z_NULL;
+ strm.avail_in = len;
+ strm.next_in = inData;
+
+ #define windowBits 15
@cdumez

cdumez Mar 10, 2013

Contributor

In the future, please avoid using #define and prefer const variables instead. I fixed it in a separate patch.

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