Clementine+vk

Clementine+vk

[arnaud.bienner, 2013-10-29 16:30:29]

Quick review: I just had a look to few files and notice few things.

https://codereview.appspot.com/19160044/diff/1/src/internet/vkmusiccache.cpp
File src/internet/vkmusiccache.cpp (right):

https://codereview.appspot.com/19160044/diff/1/src/internet/vkmusiccache.cpp#...
src/internet/vkmusiccache.cpp:112: delete file_;
You should probably reset it to NULL, otherwise the "if (file_)" will succeed
next time, but file_ will contain a dangling pointer.
btw, delete NULL is harmless, so the if sounds superfluous here (except for the
logging message, which might be the reason why you're doing so :)

https://codereview.appspot.com/19160044/diff/1/src/internet/vkmusiccache.cpp#...
src/internet/vkmusiccache.cpp:128: connect(reply_, SIGNAL(finished()),
SLOT(Downloaded()));
Would have been nice to use Closure instead (but maybe this didn't exist when
you started to work on this?)

https://codereview.appspot.com/19160044/diff/1/src/internet/vkmusiccache.cpp#...
src/internet/vkmusiccache.cpp:196: QString VkMusicCache::CachedFilename(QUrl
url) {
should be "const QUrl&" to avoid unnecessary copy probably

[shedwardx, 2013-10-29 17:31:43]

>
https://codereview.appspot.com/19160044/diff/1/src/internet/vkmusiccache.cpp#...
> src/internet/vkmusiccache.cpp:128: connect(reply_, SIGNAL(finished()),
> SLOT(Downloaded()));
> Would have been nice to use Closure instead (but maybe this didn't exist when
> you started to work on this?)
There is any advantage of Closure usage?

Also, how can I upload fixes to issue? What should I write to --rev option or I
should create new issue.
I don't fully understand how to work with Rietveld.

[arnaud.bienner, 2013-10-29 17:48:00]

Hi

I haven't used it a lot, so I cannot help about the updating only diff,
sorry :( Maybe you can add a patch file, which will be compared against
what you already submitted?
I think it would be better to reply to comments directly in the website, so
others can see them (in case someone else will review this).

About Closure, it's better you can pass objects directly to the function
called by some signal, so you don't have to connect the signal yourself
but, more importantly, you don't have to keep the value you would like to
pass to the slot as a class attribute. For instance:
http://code.google.com/p/clementine-player/source/browse/src/internet/grooves...
don't have to store the album_id somewhere: I can pass it directly to
the
GetAlbumSongsFinished() slot.

Arnaud


2013/10/29 <shedwardx@gmail.com>

> Reviewers: arnaud.bienner,
>
> Message:
>
>
> https://codereview.appspot.**com/19160044/diff/1/src/**
>
internet/vkmusiccache.cpp#**newcode128<https://codereview.appspot.com/19160044/diff/1/src/internet/vkmusiccache.cpp#newcode128>
>
>> src/internet/vkmusiccache.cpp:**128: connect(reply_, SIGNAL(finished()),
>> SLOT(Downloaded()));
>> Would have been nice to use Closure instead (but maybe this didn't
>>
> exist when
>
>> you started to work on this?)
>>
> There is any advantage of Closure usage?
>
> Also, how can I upload fixes to issue? What should I write to --rev
> option or I should create new issue.
> I don't fully understand how to work with Rietveld.
>
>
> Description:
> Clementine+vk
>
> Please review this at
https://codereview.appspot.**com/19160044/<https://codereview.appspot.com/191...
>
> Affected files (+14546, -56 lines):
>   M .gitignore
>   A 3rdparty/vreen/CMakeLists.txt
>   A 3rdparty/vreen/vreen/**CMakeLists.txt
>   A 3rdparty/vreen/vreen/cmake/**CommonUtils.cmake
>   A 3rdparty/vreen/vreen/cmake/**CompilerUtils.cmake
>   A 3rdparty/vreen/vreen/cmake/**FindLibraryWithDebug.cmake
>   A 3rdparty/vreen/vreen/cmake/**FindQCA2.cmake
>   A 3rdparty/vreen/vreen/cmake/**FindQOAuth.cmake
>   A 3rdparty/vreen/vreen/cmake/**FindQt3D.cmake
>   A 3rdparty/vreen/vreen/cmake/**FindQt3DQuick.cmake
>   A 3rdparty/vreen/vreen/cmake/**MocUtils.cmake
>   A 3rdparty/vreen/vreen/cmake/**QtBundleUtils.cmake
>   A + 3rdparty/vreen/vreen/cmake/**README
>   A
3rdparty/vreen/vreen/cmake_**uninstall.cmake.in<http://cmake_uninstall.cmake.in>
>   A 3rdparty/vreen/vreen/src/**3rdparty/CMakeLists.txt
>   A 3rdparty/vreen/vreen/src/**3rdparty/k8json/CMakeLists.txt
>   A 3rdparty/vreen/vreen/src/**3rdparty/k8json/k8json.cpp
>   A 3rdparty/vreen/vreen/src/**3rdparty/k8json/k8json.h
>   A 3rdparty/vreen/vreen/src/**3rdparty/k8json/k8json.pc.**cmake
>   A 3rdparty/vreen/vreen/src/**CMakeLists.txt
>   A 3rdparty/vreen/vreen/src/api/**CMakeLists.txt
>   A 3rdparty/vreen/vreen/src/api/**abstractlistmodel.cpp
>   A 3rdparty/vreen/vreen/src/api/**abstractlistmodel.h
>   A 3rdparty/vreen/vreen/src/api/**attachment.cpp
>   A 3rdparty/vreen/vreen/src/api/**attachment.h
>   A 3rdparty/vreen/vreen/src/api/**audio.cpp
>   A 3rdparty/vreen/vreen/src/api/**audio.h
>   A 3rdparty/vreen/vreen/src/api/**audioitem.cpp
>   A 3rdparty/vreen/vreen/src/api/**audioitem.h
>   A 3rdparty/vreen/vreen/src/api/**chatsession.cpp
>   A 3rdparty/vreen/vreen/src/api/**chatsession.h
>   A 3rdparty/vreen/vreen/src/api/**client.cpp
>   A 3rdparty/vreen/vreen/src/api/**client.h
>   A 3rdparty/vreen/vreen/src/api/**client_p.h
>   A 3rdparty/vreen/vreen/src/api/**commentssession.cpp
>   A 3rdparty/vreen/vreen/src/api/**commentssession.h
>   A 3rdparty/vreen/vreen/src/api/**connection.cpp
>   A 3rdparty/vreen/vreen/src/api/**connection.h
>   A 3rdparty/vreen/vreen/src/api/**connection_p.h
>   A 3rdparty/vreen/vreen/src/api/**contact.cpp
>   A 3rdparty/vreen/vreen/src/api/**contact.h
>   A 3rdparty/vreen/vreen/src/api/**contact_p.h
>   A 3rdparty/vreen/vreen/src/api/**contentdownloader.cpp
>   A 3rdparty/vreen/vreen/src/api/**contentdownloader.h
>   A 3rdparty/vreen/vreen/src/api/**contentdownloader_p.h
>   A 3rdparty/vreen/vreen/src/api/**dynamicpropertydata.cpp
>   A 3rdparty/vreen/vreen/src/api/**dynamicpropertydata_p.h
>   A 3rdparty/vreen/vreen/src/api/**friendrequest.cpp
>   A 3rdparty/vreen/vreen/src/api/**friendrequest.h
>   A 3rdparty/vreen/vreen/src/api/**groupchatsession.cpp
>   A 3rdparty/vreen/vreen/src/api/**groupchatsession.h
>   A 3rdparty/vreen/vreen/src/api/**groupmanager.cpp
>   A 3rdparty/vreen/vreen/src/api/**groupmanager.h
>   A 3rdparty/vreen/vreen/src/api/**groupmanager_p.h
>   A 3rdparty/vreen/vreen/src/api/**json.cpp
>   A 3rdparty/vreen/vreen/src/api/**json.h
>   A 3rdparty/vreen/vreen/src/api/**localstorage.cpp
>   A 3rdparty/vreen/vreen/src/api/**localstorage.h
>   A 3rdparty/vreen/vreen/src/api/**longpoll.cpp
>   A 3rdparty/vreen/vreen/src/api/**longpoll.h
>   A 3rdparty/vreen/vreen/src/api/**longpoll_p.h
>   A 3rdparty/vreen/vreen/src/api/**message.cpp
>   A 3rdparty/vreen/vreen/src/api/**message.h
>   A 3rdparty/vreen/vreen/src/api/**messagemodel.cpp
>   A 3rdparty/vreen/vreen/src/api/**messagemodel.h
>   A 3rdparty/vreen/vreen/src/api/**messagesession.cpp
>   A 3rdparty/vreen/vreen/src/api/**messagesession.h
>   A 3rdparty/vreen/vreen/src/api/**messagesession_p.h
>   A 3rdparty/vreen/vreen/src/api/**newsfeed.cpp
>   A 3rdparty/vreen/vreen/src/api/**newsfeed.h
>   A 3rdparty/vreen/vreen/src/api/**newsitem.cpp
>   A 3rdparty/vreen/vreen/src/api/**newsitem.h
>   A 3rdparty/vreen/vreen/src/api/**pollitem.cpp
>   A 3rdparty/vreen/vreen/src/api/**pollitem.h
>   A 3rdparty/vreen/vreen/src/api/**pollprovider.cpp
>   A 3rdparty/vreen/vreen/src/api/**pollprovider.h
>   A 3rdparty/vreen/vreen/src/api/**reply.cpp
>   A 3rdparty/vreen/vreen/src/api/**reply.h
>   A 3rdparty/vreen/vreen/src/api/**reply_p.h
>   A 3rdparty/vreen/vreen/src/api/**roster.cpp
>   A 3rdparty/vreen/vreen/src/api/**roster.h
>   A 3rdparty/vreen/vreen/src/api/**roster_p.h
>   A 3rdparty/vreen/vreen/src/api/**utils.cpp
>   A 3rdparty/vreen/vreen/src/api/**utils.h
>   A 3rdparty/vreen/vreen/src/api/**utils_p.h
>   A 3rdparty/vreen/vreen/src/api/**vk_global.h
>   A 3rdparty/vreen/vreen/src/api/**vreen.pc.cmake
>   A 3rdparty/vreen/vreen/src/api/**wallpost.cpp
>   A 3rdparty/vreen/vreen/src/api/**wallpost.h
>   A 3rdparty/vreen/vreen/src/api/**wallsession.cpp
>   A 3rdparty/vreen/vreen/src/api/**wallsession.h
>   A 3rdparty/vreen/vreen/src/**oauth/CMakeLists.txt
>   A 3rdparty/vreen/vreen/src/**oauth/oauthconnection.cpp
>   A 3rdparty/vreen/vreen/src/**oauth/oauthconnection.h
>   A 3rdparty/vreen/vreen/src/**oauth/vreenoauth.pc.cmake
>   M CMakeLists.txt
>   M data/data.qrc
>   A data/providers/vk.png
>   A data/vk/add.png
>   A data/vk/bookmarks.png
>   A data/vk/deactivated.gif
>   A data/vk/delete.png
>   A data/vk/discography.png
>   A data/vk/download.png
>   A data/vk/edit.png
>   A data/vk/find.png
>   A data/vk/group.png
>   A data/vk/link.png
>   A data/vk/my_music.png
>   A data/vk/play_alt.png
>   A data/vk/playlist.png
>   A data/vk/recommends.png
>   A data/vk/remove.png
>   A data/vk/upload.png
>   A data/vk/user.png
>   M src/CMakeLists.txt
>   M src/config.h.in
>   M src/core/metatypes.cpp
>   A src/globalsearch/**vksearchprovider.cpp
>   A + src/globalsearch/**vksearchprovider.h
>   M src/internet/internetmodel.cpp
>   M src/internet/searchboxwidget.**cpp
>   M src/internet/searchboxwidget.h
>   A src/internet/vkmusiccache.cpp
>   A src/internet/vkmusiccache.h
>   A src/internet/vksearchdialog.**cpp
>   A src/internet/vksearchdialog.h
>   A + src/internet/vksearchdialog.ui
>   A src/internet/vkservice.cpp
>   A src/internet/vkservice.h
>   A src/internet/vksettingspage.**cpp
>   A + src/internet/vksettingspage.h
>   A src/internet/vksettingspage.ui
>   A src/internet/vkurlhandler.cpp
>   A + src/internet/vkurlhandler.h
>   M src/translations/ru.po
>   M src/ui/mainwindow.cpp
>   M src/ui/settingsdialog.cpp
>   M src/ui/settingsdialog.h
>
>
>

[shedwardx, 2013-10-29 19:37:32]

On 2013/10/29 17:48:00, arnaud.bienner wrote:
> Hi
> 
> I haven't used it a lot, so I cannot help about the updating only diff,
> sorry :( Maybe you can add a patch file, which will be compared against
> what you already submitted?
> I think it would be better to reply to comments directly in the website, so
> others can see them (in case someone else will review this).
> 
> About Closure, it's better you can pass objects directly to the function
> called by some signal, so you don't have to connect the signal yourself
> but, more importantly, you don't have to keep the value you would like to
> pass to the slot as a class attribute. For instance:
>
http://code.google.com/p/clementine-player/source/browse/src/internet/grooves...
> don't have to store the album_id somewhere: I can pass it directly to
> the
> GetAlbumSongsFinished() slot.
> 
> Arnaud

I found it, you just need provide issue id (19160044) using option -i with the
same --rev, just as note.

About Closure, yes it's was very handy for hiding replies, but there both
hidable variables used in other places on the fly. file_ used in
DownloadReadyToRead for flushing downloaded data, reply_ used in
BreakCurrentCaching.

Other defects are fixed, thank you for your admonition.

BTW all our messages appear at website below list of files.

[arnaud.bienner, 2013-10-29 19:49:53]

I found it, you just need provide issue id (19160044) using option -i

> with the same --rev, just as note.
>
>
Great :)


> About Closure, yes it's was very handy for hiding replies, but there
> both hidable variables used in other places on the fly. file_ used in
> DownloadReadyToRead for flushing downloaded data, reply_ used in
> BreakCurrentCaching.
>
>
I haven't checked deeply so if you say so, you're probably right :)
I haven't seen you used it elsewhere, so I thought you might not be aware
this exists.


> BTW all our messages appear at website below list of files.
>
>
I meant inside the code review directly (for comments regarding code).
Sorry for the confusion.


>
>
https://codereview.appspot.**com/19160044/<https://codereview.appspot.com/191...
>

[shedwardx, 2013-10-29 19:53:55]

It seems the proper way to answer to comments. Sorry for spam attack to your
email :)

https://codereview.appspot.com/19160044/diff/1/src/internet/vkmusiccache.cpp
File src/internet/vkmusiccache.cpp (right):

https://codereview.appspot.com/19160044/diff/1/src/internet/vkmusiccache.cpp#...
src/internet/vkmusiccache.cpp:112: delete file_;
On 2013/10/29 16:30:30, arnaud.bienner wrote:
> You should probably reset it to NULL, otherwise the "if (file_)" will succeed
> next time, but file_ will contain a dangling pointer.
> btw, delete NULL is harmless, so the if sounds superfluous here (except for
the
> logging message, which might be the reason why you're doing so :)

Done.

https://codereview.appspot.com/19160044/diff/1/src/internet/vkmusiccache.cpp#...
src/internet/vkmusiccache.cpp:128: connect(reply_, SIGNAL(finished()),
SLOT(Downloaded()));
On 2013/10/29 16:30:30, arnaud.bienner wrote:
> Would have been nice to use Closure instead (but maybe this didn't exist when
> you started to work on this?)

Both hidable variables used in other places on the fly. file_ used in
DownloadReadyToRead for flushing downloaded data, reply_ used in
BreakCurrentCaching.

https://codereview.appspot.com/19160044/diff/1/src/internet/vkmusiccache.cpp#...
src/internet/vkmusiccache.cpp:196: QString VkMusicCache::CachedFilename(QUrl
url) {
On 2013/10/29 16:30:30, arnaud.bienner wrote:
> should be "const QUrl&" to avoid unnecessary copy probably

Done.