From 7b520ab361c97758e93c752056d72236d9843ada Mon Sep 17 00:00:00 2001 From: John Maguire Date: Wed, 3 Mar 2010 20:35:19 +0000 Subject: [PATCH] Fix code review comments for r313. --- src/albumcoverfetcher.cpp | 5 +---- src/albumcoverfetcher.h | 4 ++-- src/albumcovermanager.cpp | 4 ++-- src/albumcovermanager.h | 4 +++- src/main.cpp | 5 ++++- src/mainwindow.cpp | 4 ++-- src/mainwindow.h | 4 +++- tests/albumcoverfetcher_test.cpp | 3 ++- tests/mock_networkaccessmanager.cpp | 26 ++++++++++---------------- tests/mock_networkaccessmanager.h | 27 ++++++++++++++++----------- 10 files changed, 45 insertions(+), 41 deletions(-) diff --git a/src/albumcoverfetcher.cpp b/src/albumcoverfetcher.cpp index b486ef8b4..ab3cd30e0 100644 --- a/src/albumcoverfetcher.cpp +++ b/src/albumcoverfetcher.cpp @@ -9,15 +9,12 @@ const int AlbumCoverFetcher::kMaxConcurrentRequests = 5; -AlbumCoverFetcher::AlbumCoverFetcher(QObject* parent, QNetworkAccessManager* network) +AlbumCoverFetcher::AlbumCoverFetcher(QNetworkAccessManager* network, QObject* parent) : QObject(parent), network_(network), next_id_(0), request_starter_(new QTimer(this)) { - if (!network_.get()) { - network_.reset(new QNetworkAccessManager); - } request_starter_->setInterval(1000); connect(request_starter_, SIGNAL(timeout()), SLOT(StartRequests())); } diff --git a/src/albumcoverfetcher.h b/src/albumcoverfetcher.h index 424969b11..40c53e5be 100644 --- a/src/albumcoverfetcher.h +++ b/src/albumcoverfetcher.h @@ -18,7 +18,7 @@ class AlbumCoverFetcher : public QObject { Q_OBJECT public: - AlbumCoverFetcher(QObject* parent = 0, QNetworkAccessManager* network_ = 0); + AlbumCoverFetcher(QNetworkAccessManager* network, QObject* parent = 0); virtual ~AlbumCoverFetcher() {} static const int kMaxConcurrentRequests; @@ -42,7 +42,7 @@ class AlbumCoverFetcher : public QObject { QString album; }; - boost::scoped_ptr network_; + QNetworkAccessManager* network_; quint64 next_id_; QQueue queued_requests_; diff --git a/src/albumcovermanager.cpp b/src/albumcovermanager.cpp index c9c0ed270..d6a5d41b2 100644 --- a/src/albumcovermanager.cpp +++ b/src/albumcovermanager.cpp @@ -19,11 +19,11 @@ const char* AlbumCoverManager::kSettingsGroup = "CoverManager"; -AlbumCoverManager::AlbumCoverManager(QWidget *parent) +AlbumCoverManager::AlbumCoverManager(QNetworkAccessManager* network, QWidget *parent) : QDialog(parent), constructed_(false), cover_loader_(new BackgroundThread(this)), - cover_fetcher_(new AlbumCoverFetcher(this)), + cover_fetcher_(new AlbumCoverFetcher(network, this)), artist_icon_(":/artist.png"), all_artists_icon_(":/album.png"), context_menu_(new QMenu(this)) diff --git a/src/albumcovermanager.h b/src/albumcovermanager.h index abacb8e6c..aa900b381 100644 --- a/src/albumcovermanager.h +++ b/src/albumcovermanager.h @@ -13,10 +13,12 @@ class LibraryBackend; class AlbumCoverFetcher; +class QNetworkAccessManager; + class AlbumCoverManager : public QDialog { Q_OBJECT public: - AlbumCoverManager(QWidget *parent = 0); + AlbumCoverManager(QNetworkAccessManager* network, QWidget *parent = 0); ~AlbumCoverManager(); static const char* kSettingsGroup; diff --git a/src/main.cpp b/src/main.cpp index 78d5aeb08..b6910ca62 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -14,6 +14,7 @@ #include #include #include +#include void LoadTranslation(const QString& prefix, const QString& path) { QTranslator* t = new QTranslator; @@ -52,8 +53,10 @@ int main(int argc, char *argv[]) { LoadTranslation("clementine", a.applicationDirPath()); LoadTranslation("clementine", QDir::currentPath()); + QNetworkAccessManager network; + // Window - MainWindow w; + MainWindow w(&network);; a.setActivationWindow(&w); QObject::connect(&a, SIGNAL(messageReceived(QString)), &w, SLOT(show())); diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 4766291ce..ea0bdb471 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -39,7 +39,7 @@ const int MainWindow::kStateVersion = 1; const char* MainWindow::kSettingsGroup = "MainWindow"; -MainWindow::MainWindow(QWidget *parent) +MainWindow::MainWindow(QNetworkAccessManager* network, QWidget *parent) : QMainWindow(parent), tray_icon_(new SystemTrayIcon(this)), osd_(new OSD(tray_icon_, this)), @@ -55,7 +55,7 @@ MainWindow::MainWindow(QWidget *parent) settings_dialog_(new SettingsDialog(this)), add_stream_dialog_(new AddStreamDialog(this)), shortcuts_dialog_(new ShortcutsDialog(this)), - cover_manager_(new AlbumCoverManager(this)), + cover_manager_(new AlbumCoverManager(network, this)), playlist_menu_(new QMenu(this)), library_sort_model_(new QSortFilterProxyModel(this)), track_position_timer_(new QTimer(this)) diff --git a/src/mainwindow.h b/src/mainwindow.h index a2fa75092..3a26b5d78 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -27,11 +27,13 @@ class AlbumCoverManager; class QSortFilterProxyModel; class SystemTrayIcon; +class QNetworkAccessManager; + class MainWindow : public QMainWindow { Q_OBJECT public: - MainWindow(QWidget *parent = 0); + MainWindow(QNetworkAccessManager* network, QWidget *parent = 0); ~MainWindow(); void SetHiddenInTray(bool hidden); diff --git a/tests/albumcoverfetcher_test.cpp b/tests/albumcoverfetcher_test.cpp index b3dd9b05d..d95a37549 100644 --- a/tests/albumcoverfetcher_test.cpp +++ b/tests/albumcoverfetcher_test.cpp @@ -23,6 +23,7 @@ class AlbumCoverFetcherTest : public ::testing::Test { } void SetUp() { + // Lastfm takes ownership of this. network_ = new MockNetworkAccessManager; lastfm::setNetworkAccessManager(network_); } @@ -44,7 +45,7 @@ TEST_F(AlbumCoverFetcherTest, FetchesAlbumCover) { params.clear(); MockNetworkReply* album_reply = network_->ExpectGet("http://example.com/image.jpg", params, 200, ""); - AlbumCoverFetcher fetcher(NULL, network_); + AlbumCoverFetcher fetcher(network_, NULL); QSignalSpy spy(&fetcher, SIGNAL(AlbumCoverFetched(quint64, const QImage&))); ASSERT_TRUE(spy.isValid()); fetcher.FetchAlbumCover("Foo", "Bar"); diff --git a/tests/mock_networkaccessmanager.cpp b/tests/mock_networkaccessmanager.cpp index d7e91685e..911b9fb62 100644 --- a/tests/mock_networkaccessmanager.cpp +++ b/tests/mock_networkaccessmanager.cpp @@ -26,8 +26,8 @@ class RequestForUrlMatcher : public MatcherInterface { return false; } - for (QMap::const_iterator it = expected_params_.begin(); - it != expected_params_.end(); ++it) { + for (QMap::const_iterator it = expected_params_.constBegin(); + it != expected_params_.constEnd(); ++it) { if (!url.hasQueryItem(it.key()) || url.queryItemValue(it.key()) != it.value()) { return false; @@ -55,9 +55,9 @@ MockNetworkReply* MockNetworkAccessManager::ExpectGet( const QString& contains, const QMap& expected_params, int status, - const char* data) { + const QByteArray& data) { MockNetworkReply* reply = new MockNetworkReply(data); - reply->setAttribute(QNetworkRequest::HttpStatusCodeAttribute, 200); + reply->setAttribute(QNetworkRequest::HttpStatusCodeAttribute, status); EXPECT_CALL(*this, createRequest( GetOperation, RequestForUrl(contains, expected_params), NULL)). @@ -66,32 +66,26 @@ MockNetworkReply* MockNetworkAccessManager::ExpectGet( return reply; } -MockNetworkAccessManager::~MockNetworkAccessManager() { -} - MockNetworkReply::MockNetworkReply() - : data_(NULL), - size_(0) { + : data_(NULL) { } -MockNetworkReply::MockNetworkReply(const char* data) +MockNetworkReply::MockNetworkReply(const QByteArray& data) : data_(data), - size_(strlen(data)), pos_(0) { } -void MockNetworkReply::SetData(const char* data) { +void MockNetworkReply::SetData(const QByteArray& data) { data_ = data; - size_ = strlen(data); pos_ = 0; } qint64 MockNetworkReply::readData(char* data, qint64 size) { - if (size_ == pos_) { + if (data_.size() == pos_) { return -1; } - qint64 bytes_to_read = min(size_ - pos_, size); - memcpy(data, data_, bytes_to_read); + qint64 bytes_to_read = min(data_.size() - pos_, size); + memcpy(data, data_.constData() + pos_, bytes_to_read); pos_ += bytes_to_read; return bytes_to_read; } diff --git a/tests/mock_networkaccessmanager.h b/tests/mock_networkaccessmanager.h index d34b2f173..96ffae0e0 100644 --- a/tests/mock_networkaccessmanager.h +++ b/tests/mock_networkaccessmanager.h @@ -1,7 +1,7 @@ #ifndef MOCK_NETWORKACCESSAMANGER_H #define MOCK_NETWORKACCESSMANAGER_H -#include +#include #include #include #include @@ -10,16 +10,23 @@ #include "test_utils.h" #include "gmock/gmock.h" +// Usage: +// Create a MockNetworkAccessManager. +// Call ExpectGet() with appropriate expectations and the data you want back. +// This will return a MockNetworkReply*. When you are ready for the reply to +// arrive, call MockNetworkReply::Done(). + class MockNetworkReply : public QNetworkReply { Q_OBJECT public: MockNetworkReply(); - MockNetworkReply(const char* data); - virtual ~MockNetworkReply() {} + MockNetworkReply(const QByteArray& data); - void SetData(const char* data); + // Use these to set expectations. + void SetData(const QByteArray& data); virtual void setAttribute(QNetworkRequest::Attribute code, const QVariant& value); + // Call this when you are ready for the finished() signal. void Done(); protected: @@ -27,8 +34,7 @@ class MockNetworkReply : public QNetworkReply { virtual qint64 readData(char* data, qint64); virtual qint64 writeData(const char* data, qint64); - const char* data_; - qint64 size_; + QByteArray data_; qint64 pos_; }; @@ -36,12 +42,11 @@ class MockNetworkReply : public QNetworkReply { class MockNetworkAccessManager : public QNetworkAccessManager { Q_OBJECT public: - virtual ~MockNetworkAccessManager(); MockNetworkReply* ExpectGet( - const QString& contains, - const QMap& params, - int status, - const char* ret_data); + const QString& contains, // A string that should be present in the URL. + const QMap& params, // Required URL parameters. + int status, // Returned HTTP status code. + const QByteArray& ret_data); // Returned data. protected: MOCK_METHOD3(createRequest, QNetworkReply*(Operation, const QNetworkRequest&, QIODevice*)); };