Re: Review Request: GSoC report: Integrate Spotify into Amarok #3 (squashed commits, recent on top)
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105201/#review17591 --- I can only see the changed copyright header. Is this request still valid? Can we reject it? - Ralf Engels On June 11, 2012, 1:26 p.m., Zhengliang Feng wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105201/ --- (Updated June 11, 2012, 1:26 p.m.) Review request for Amarok. Description --- Add Spotify collection code Currently implemented SpotifyCollection, SpotifyQueryMaker and SpotifyMeta. The ScriptResolver is the class handles communcation with standalone Spotify resolver, the code is mainly from original ScriptResolver, but added more functions to handle messages separately. The controller class is used to start a ScriptResolver in a separate thread and handles queries. Diffs - src/core-impl/collections/spotifycollection/support/ScriptResolver.h PRE-CREATION src/core-impl/collections/spotifycollection/support/ScriptResolver.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/105201/diff/ Testing --- Communication between ScriptResolver and Spotify resolver( from Tomahawk resolver repo https://github.com/ofan/tomahawk-resolvers ). Logging into Spotify using a username and password. Thanks, Zhengliang Feng ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: GSoC report: Integrate Spotify into Amarok #3 (squashed commits, recent on top)
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105201/#review17592 --- I can only see the changed copyright header. Is this request still valid? Can we reject it? - Ralf Engels On June 11, 2012, 1:26 p.m., Zhengliang Feng wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105201/ --- (Updated June 11, 2012, 1:26 p.m.) Review request for Amarok. Description --- Add Spotify collection code Currently implemented SpotifyCollection, SpotifyQueryMaker and SpotifyMeta. The ScriptResolver is the class handles communcation with standalone Spotify resolver, the code is mainly from original ScriptResolver, but added more functions to handle messages separately. The controller class is used to start a ScriptResolver in a separate thread and handles queries. Diffs - src/core-impl/collections/spotifycollection/support/ScriptResolver.h PRE-CREATION src/core-impl/collections/spotifycollection/support/ScriptResolver.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/105201/diff/ Testing --- Communication between ScriptResolver and Spotify resolver( from Tomahawk resolver repo https://github.com/ofan/tomahawk-resolvers ). Logging into Spotify using a username and password. Thanks, Zhengliang Feng ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: GSoC report: Integrate Spotify into Amarok #3 (squashed commits, recent on top)
On Aug. 17, 2012, 12:22 p.m., Ralf Engels wrote: I can only see the changed copyright header. Is this request still valid? Can we reject it? Ryan should probably close this one, rebase his gsoc branch and upload a new, full diff to a new review board. Want feedback from more people since it's GSoC evaluation time anyway. - Bart --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105201/#review17592 --- On June 11, 2012, 1:26 p.m., Zhengliang Feng wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105201/ --- (Updated June 11, 2012, 1:26 p.m.) Review request for Amarok. Description --- Add Spotify collection code Currently implemented SpotifyCollection, SpotifyQueryMaker and SpotifyMeta. The ScriptResolver is the class handles communcation with standalone Spotify resolver, the code is mainly from original ScriptResolver, but added more functions to handle messages separately. The controller class is used to start a ScriptResolver in a separate thread and handles queries. Diffs - src/core-impl/collections/spotifycollection/support/ScriptResolver.h PRE-CREATION src/core-impl/collections/spotifycollection/support/ScriptResolver.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/105201/diff/ Testing --- Communication between ScriptResolver and Spotify resolver( from Tomahawk resolver repo https://github.com/ofan/tomahawk-resolvers ). Logging into Spotify using a username and password. Thanks, Zhengliang Feng ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: GSoC report: Integrate Spotify into Amarok #3 (squashed commits, recent on top)
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105201/#review14599 --- One minor comment: Please don't remove copyright headers when copying files from another project--this is important for both legal and authorship (e.g. who do I ask when this code doesn't work) reasons. The ScriptResolver.* files come from Tomahawk, and I'm totally happy with you re-using the code that we wrote. However, you need to retain the same copyright header that exists in the original source file, and additionally adding your own copyright if you modify the file. This is the original: https://github.com/tomahawk-player/tomahawk/blob/master/src/libtomahawk/resolvers/ScriptResolver.cpp This is also why the ScriptResolver class supports external .js and non-spotify resolvers. It'll work with any external process that implements the playdar + tomahawk extensions resolver API. thanks! - Leo Franchi On June 10, 2012, 7:10 a.m., Zhengliang Feng wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105201/ --- (Updated June 10, 2012, 7:10 a.m.) Review request for Amarok. Description --- Add Spotify collection code Currently implemented SpotifyCollection, SpotifyQueryMaker and SpotifyMeta. The ScriptResolver is the class handles communcation with standalone Spotify resolver, the code is mainly from original ScriptResolver, but added more functions to handle messages separately. The controller class is used to start a ScriptResolver in a separate thread and handles queries. Diffs - src/core-impl/collections/CMakeLists.txt c78b9202ece71b51189c4e47d85acfa4a74ef8d6 src/core-impl/collections/spotifycollection/CMakeLists.txt PRE-CREATION src/core-impl/collections/spotifycollection/SpotifyCollection.h PRE-CREATION src/core-impl/collections/spotifycollection/SpotifyMeta.h PRE-CREATION src/core-impl/collections/spotifycollection/SpotifyMeta.cpp PRE-CREATION src/core-impl/collections/spotifycollection/SpotifyQueryMaker.h PRE-CREATION src/core-impl/collections/spotifycollection/SpotifyQueryMaker.cpp PRE-CREATION src/core-impl/collections/spotifycollection/support/Controller.h PRE-CREATION src/core-impl/collections/spotifycollection/support/Controller.cpp PRE-CREATION src/core-impl/collections/spotifycollection/support/QMFunctionTypes.h PRE-CREATION src/core-impl/collections/spotifycollection/support/Query.h PRE-CREATION src/core-impl/collections/spotifycollection/support/ScriptResolver.h PRE-CREATION src/core-impl/collections/spotifycollection/support/ScriptResolver.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/105201/diff/ Testing --- Communication between ScriptResolver and Spotify resolver( from Tomahawk resolver repo https://github.com/ofan/tomahawk-resolvers ). Logging into Spotify using a username and password. Thanks, Zhengliang Feng ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: GSoC report: Integrate Spotify into Amarok #3 (squashed commits, recent on top)
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105201/ --- (Updated June 11, 2012, 1:26 p.m.) Review request for Amarok. Description --- Add Spotify collection code Currently implemented SpotifyCollection, SpotifyQueryMaker and SpotifyMeta. The ScriptResolver is the class handles communcation with standalone Spotify resolver, the code is mainly from original ScriptResolver, but added more functions to handle messages separately. The controller class is used to start a ScriptResolver in a separate thread and handles queries. Diffs (updated) - src/core-impl/collections/spotifycollection/support/ScriptResolver.h PRE-CREATION src/core-impl/collections/spotifycollection/support/ScriptResolver.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/105201/diff/ Testing --- Communication between ScriptResolver and Spotify resolver( from Tomahawk resolver repo https://github.com/ofan/tomahawk-resolvers ). Logging into Spotify using a username and password. Thanks, Zhengliang Feng ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: GSoC report: Integrate Spotify into Amarok #3 (squashed commits, recent on top)
On June 11, 2012, 8:52 a.m., Leo Franchi wrote: One minor comment: Please don't remove copyright headers when copying files from another project--this is important for both legal and authorship (e.g. who do I ask when this code doesn't work) reasons. The ScriptResolver.* files come from Tomahawk, and I'm totally happy with you re-using the code that we wrote. However, you need to retain the same copyright header that exists in the original source file, and additionally adding your own copyright if you modify the file. This is the original: https://github.com/tomahawk-player/tomahawk/blob/master/src/libtomahawk/resolvers/ScriptResolver.cpp This is also why the ScriptResolver class supports external .js and non-spotify resolvers. It'll work with any external process that implements the playdar + tomahawk extensions resolver API. thanks! Sorry about that, I forgot to add them when copying from the scratch repo. Now they are added in diff r2. - Zhengliang --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105201/#review14599 --- On June 11, 2012, 1:26 p.m., Zhengliang Feng wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105201/ --- (Updated June 11, 2012, 1:26 p.m.) Review request for Amarok. Description --- Add Spotify collection code Currently implemented SpotifyCollection, SpotifyQueryMaker and SpotifyMeta. The ScriptResolver is the class handles communcation with standalone Spotify resolver, the code is mainly from original ScriptResolver, but added more functions to handle messages separately. The controller class is used to start a ScriptResolver in a separate thread and handles queries. Diffs - src/core-impl/collections/spotifycollection/support/ScriptResolver.h PRE-CREATION src/core-impl/collections/spotifycollection/support/ScriptResolver.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/105201/diff/ Testing --- Communication between ScriptResolver and Spotify resolver( from Tomahawk resolver repo https://github.com/ofan/tomahawk-resolvers ). Logging into Spotify using a username and password. Thanks, Zhengliang Feng ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: GSoC report: Integrate Spotify into Amarok #3 (squashed commits, recent on top)
On June 11, 2012, 8:41 a.m., Bart Cerneels wrote: I'm assuming this is mostly code copied and edited (but not completely) from the playdar work done by Andy for GSoC 2010. As far as I understand it the protocol used by the Tomahawk resolver is still the same (playdar API), so that should work. There are a few things you should look at before going on with this design though. First do some namespace cleanup. ScriptResolver for instance is a confusing name. Here you won't be dealing with a resolver script but the spotify-resolver application. ResolverProcessInterface looks a little long, but does cover it's function pretty well. Up to you to figure out a good name. I've not gone through the architecture of the playdar QueryMaker. It probably works, but I wonder if it has to be so complicated. QM is not a simple API to begin with though. Spotify does not support search strings for specific types (artist, album, genre, etc) AFAIU, so a mapping might be more straight forward. If this code is working keep it for now, as long as no performance issues pop up. Yes, some code like meta classe are just copy and paste. Some of them like Controller class might be deleted or replaced later, I was thinking about replacing it by ScriptResolver. QM is more complicated than I thought, it may take some time to fully understand how it works. Spotify can do specific search by using artist: or album: and using of logical operands like 'AND' or 'OR' is also possible, details are on https://developer.spotify.com/technologies/libspotify/guidelines-hardware/ in the 'Search' section. So it is easy to construct a search string for specific types. - Zhengliang --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105201/#review14597 --- On June 11, 2012, 1:26 p.m., Zhengliang Feng wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105201/ --- (Updated June 11, 2012, 1:26 p.m.) Review request for Amarok. Description --- Add Spotify collection code Currently implemented SpotifyCollection, SpotifyQueryMaker and SpotifyMeta. The ScriptResolver is the class handles communcation with standalone Spotify resolver, the code is mainly from original ScriptResolver, but added more functions to handle messages separately. The controller class is used to start a ScriptResolver in a separate thread and handles queries. Diffs - src/core-impl/collections/spotifycollection/support/ScriptResolver.h PRE-CREATION src/core-impl/collections/spotifycollection/support/ScriptResolver.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/105201/diff/ Testing --- Communication between ScriptResolver and Spotify resolver( from Tomahawk resolver repo https://github.com/ofan/tomahawk-resolvers ). Logging into Spotify using a username and password. Thanks, Zhengliang Feng ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: GSoC report: Integrate Spotify into Amarok #3 (squashed commits, recent on top)
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105201/ --- (Updated June 10, 2012, 7:10 a.m.) Review request for Amarok. Changes --- Rename the title Summary (updated) - GSoC report: Integrate Spotify into Amarok #3 (squashed commits, recent on top) Description --- Add Spotify collection code Currently implemented SpotifyCollection, SpotifyQueryMaker and SpotifyMeta. The ScriptResolver is the class handles communcation with standalone Spotify resolver, the code is mainly from original ScriptResolver, but added more functions to handle messages separately. The controller class is used to start a ScriptResolver in a separate thread and handles queries. Diffs - src/core-impl/collections/CMakeLists.txt c78b9202ece71b51189c4e47d85acfa4a74ef8d6 src/core-impl/collections/spotifycollection/CMakeLists.txt PRE-CREATION src/core-impl/collections/spotifycollection/SpotifyCollection.h PRE-CREATION src/core-impl/collections/spotifycollection/SpotifyMeta.h PRE-CREATION src/core-impl/collections/spotifycollection/SpotifyMeta.cpp PRE-CREATION src/core-impl/collections/spotifycollection/SpotifyQueryMaker.h PRE-CREATION src/core-impl/collections/spotifycollection/SpotifyQueryMaker.cpp PRE-CREATION src/core-impl/collections/spotifycollection/support/Controller.h PRE-CREATION src/core-impl/collections/spotifycollection/support/Controller.cpp PRE-CREATION src/core-impl/collections/spotifycollection/support/QMFunctionTypes.h PRE-CREATION src/core-impl/collections/spotifycollection/support/Query.h PRE-CREATION src/core-impl/collections/spotifycollection/support/ScriptResolver.h PRE-CREATION src/core-impl/collections/spotifycollection/support/ScriptResolver.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/105201/diff/ Testing --- Communication between ScriptResolver and Spotify resolver( from Tomahawk resolver repo https://github.com/ofan/tomahawk-resolvers ). Logging into Spotify using a username and password. Thanks, Zhengliang Feng ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
