GSoC update: Integrate Spotify into Amarok

2012-07-22 Thread Ryan Feng 
Hi,

Now the Spotify collection feature works well in "Unmerged view" of
collection browser[1], but it's still not stable under "Merged view".
What I've done this week:

   - Added TrackProxy class to retrieve track information in the playlist
   when loading Amarok
   - Added SpotifyConfigWidget class to display a dialog for users to input
   username and password.
   - Fixed 0 track length issue
   - Fixed crash when exiting

Things to do next week:

   - Finish SpotifyConfigWidget and SpotifySettings class
   - Avoid adding duplicated results to the collection, clear all tracks
   before each query
   - Remove MemoryCollection support in SpotifyCollection
   - Rewriting SpotifyQueryMaker to make it stable
   - Add basic playlist sync support if possible

[1] http://i.imgur.com/bfdOi.png

Cheers,
Ryan
___
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel

Re: GSoC update: Integrate Spotify into Amarok

2012-07-22 Thread Teo Mrnjavac 
On Sun, Jul 22, 2012 at 2:45 PM, Ryan Feng  wrote:
> Hi,
>
> Now the Spotify collection feature works well in "Unmerged view" of
> collection browser[1], but it's still not stable under "Merged view".
> What I've done this week:
>
> Added TrackProxy class to retrieve track information in the playlist when
> loading Amarok
> Added SpotifyConfigWidget class to display a dialog for users to input
> username and password.
> Fixed 0 track length issue
> Fixed crash when exiting
>
> Things to do next week:
>
> Finish SpotifyConfigWidget and SpotifySettings class
> Avoid adding duplicated results to the collection, clear all tracks before
> each query
> Remove MemoryCollection support in SpotifyCollection
> Rewriting SpotifyQueryMaker to make it stable
> Add basic playlist sync support if possible
>
> [1] http://i.imgur.com/bfdOi.png

Very nice, thanks for the screenshot and the detailed update :)

Cheers,
-- 
Teo Mrnjavac
http://teom.org  |  t...@kde.org
___
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel

Re: Review Request: SoK - Unit Test : core/meta/support/PrivateMetaRegistry

2012-07-22 Thread Matěj Laitl 

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105525/#review16229
---

Ship it!


Looks good, but there seems to be logic error, see below.


tests/core/meta/support/TestPrivateMetaRegistry.cpp


Oh, isn't this an opposite? You verify the fact they are non-null, but in 
fact you should verify they are null, right? (you didn't update the output, I 
wonder if this tests pass as written here)


- Matěj Laitl


On July 20, 2012, 2:13 p.m., Jasneet Bhatti wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105525/
> ---
> 
> (Updated July 20, 2012, 2:13 p.m.)
> 
> 
> Review request for Amarok, Matěj Laitl and Sven Krohlas.
> 
> 
> Description
> ---
> 
> Added unit test for core/meta/support/PrivateMetaRegistry.
> 
> Also, tidied up the code style in the class being tested.
> 
> 
> Diffs
> -
> 
>   src/core/meta/support/PrivateMetaRegistry.h f5959ac 
>   tests/core/meta/CMakeLists.txt 3ae78c9 
>   tests/core/meta/support/CMakeLists.txt PRE-CREATION 
>   tests/core/meta/support/TestPrivateMetaRegistry.h PRE-CREATION 
>   tests/core/meta/support/TestPrivateMetaRegistry.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/105525/diff/
> 
> 
> Testing
> ---
> 
> Builds, links and runs fine.
> 
> Output of running test with -v2 flag:
> 
> * Start testing of TestPrivateMetaRegistry *
> Config: Using QTest library 4.8.1, Qt 4.8.1
> INFO   : TestPrivateMetaRegistry::initTestCase() entering
> QSYSTEM: TestPrivateMetaRegistry::initTestCase() qttest(16692)/kdecore 
> (K*TimeZone*): No time zone information obtained from ktimezoned 
> PASS   : TestPrivateMetaRegistry::initTestCase()
> INFO   : TestPrivateMetaRegistry::testInsertAlbum() entering
> INFO   : TestPrivateMetaRegistry::testInsertAlbum(Track01.ogg) COMPARE()
>Loc: 
> [/home/jasneet/amarok/tests/core/meta/support/TestPrivateMetaRegistry.cpp(91)]
> INFO   : TestPrivateMetaRegistry::testInsertAlbum(Track02.ogg) COMPARE()
>Loc: 
> [/home/jasneet/amarok/tests/core/meta/support/TestPrivateMetaRegistry.cpp(91)]
> INFO   : TestPrivateMetaRegistry::testInsertAlbum(Platz 01.mp3) COMPARE()
>Loc: 
> [/home/jasneet/amarok/tests/core/meta/support/TestPrivateMetaRegistry.cpp(91)]
> INFO   : TestPrivateMetaRegistry::testInsertAlbum(Platz 02.mp3) COMPARE()
>Loc: 
> [/home/jasneet/amarok/tests/core/meta/support/TestPrivateMetaRegistry.cpp(91)]
> INFO   : TestPrivateMetaRegistry::testInsertAlbum(Platz 03.mp3) COMPARE()
>Loc: 
> [/home/jasneet/amarok/tests/core/meta/support/TestPrivateMetaRegistry.cpp(91)]
> PASS   : TestPrivateMetaRegistry::testInsertAlbum()
> INFO   : TestPrivateMetaRegistry::testInsertArtist() entering
> INFO   : TestPrivateMetaRegistry::testInsertArtist(Track01.ogg) COMPARE()
>Loc: 
> [/home/jasneet/amarok/tests/core/meta/support/TestPrivateMetaRegistry.cpp(108)]
> INFO   : TestPrivateMetaRegistry::testInsertArtist(Track02.ogg) COMPARE()
>Loc: 
> [/home/jasneet/amarok/tests/core/meta/support/TestPrivateMetaRegistry.cpp(108)]
> INFO   : TestPrivateMetaRegistry::testInsertArtist(Platz 01.mp3) COMPARE()
>Loc: 
> [/home/jasneet/amarok/tests/core/meta/support/TestPrivateMetaRegistry.cpp(108)]
> INFO   : TestPrivateMetaRegistry::testInsertArtist(Platz 02.mp3) COMPARE()
>Loc: 
> [/home/jasneet/amarok/tests/core/meta/support/TestPrivateMetaRegistry.cpp(108)]
> INFO   : TestPrivateMetaRegistry::testInsertArtist(Platz 03.mp3) COMPARE()
>Loc: 
> [/home/jasneet/amarok/tests/core/meta/support/TestPrivateMetaRegistry.cpp(108)]
> PASS   : TestPrivateMetaRegistry::testInsertArtist()
> INFO   : TestPrivateMetaRegistry::testInsertComposer() entering
> INFO   : TestPrivateMetaRegistry::testInsertComposer(Track01.ogg) COMPARE()
>Loc: 
> [/home/jasneet/amarok/tests/core/meta/support/TestPrivateMetaRegistry.cpp(125)]
> INFO   : TestPrivateMetaRegistry::testInsertComposer(Track02.ogg) COMPARE()
>Loc: 
> [/home/jasneet/amarok/tests/core/meta/support/TestPrivateMetaRegistry.cpp(125)]
> INFO   : TestPrivateMetaRegistry::testInsertComposer(Platz 01.mp3) COMPARE()
>Loc: 
> [/home/jasneet/amarok/tests/core/meta/support/TestPrivateMetaRegistry.cpp(125)]
> INFO   : TestPrivateMetaRegistry::testInsertComposer(Platz 02.mp3) COMPARE()
>Loc: 
> [/home/jasneet/amarok/tests/core/meta/support/TestPrivateMetaRegistry.cpp(125)]
> INFO   : TestPrivateMetaRegistry::testInsertComposer(Platz 03.mp3) COMPARE()
>Loc: 
> [/home/jasneet/amarok/tests/core/meta/support/TestPrivateMetaRegistry.cpp(125)]
> PASS   : TestPrivateMetaRegistry::testInsertComposer()
> INFO   : TestPrivateMetaRegistry::testInsertGenre() entering
> INFO

Re: Review Request: SqlScanResultProcessor: fix data-loss bug; squashed commits, recent on top

2012-07-22 Thread Matěj Laitl 


> On July 11, 2012, 9:20 a.m., Ralf Engels wrote:
> > Looks good for now.
> > What I would really like is some more test cases to check for the current 
> > problems.
> > 
> > Also I am working on a solution for the database schema as you can see from 
> > the database table .svg file in the docs directory.
> 
> Matěj Laitl wrote:
> > Looks good for now.
> > What I would really like is some more test cases to check for the 
> current problems.
> 
> Agreed, but due to their complexity, I won't be able to write them in 
> Amarok 2.6 timeframe. Do you think we can merge this for 2.6 even without 
> tests?
> 
> > Also I am working on a solution for the database schema as you can see 
> from the database table .svg file in the docs directory.
> 
> Yeah, I've seen it and generally it's a right step forward IMO. But 
> please let me look at it in more detail after 2.6 is released. Quick 
> question: don't you think that multiple artists (etc) for a track will make 
> code much more complicated?

Guys, I'd really like to push this into master for 2.6 RC to let users test 
this. Anyone brave enough to give this an ack?


- Matěj


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105488/#review15660
---


On July 10, 2012, 2:54 p.m., Matěj Laitl wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105488/
> ---
> 
> (Updated July 10, 2012, 2:54 p.m.)
> 
> 
> Review request for Amarok and Ralf Engels.
> 
> 
> Description
> ---
> 
> SqlScanResultProcessor: debugging; not intended to be merged to master
> 
> 
> SqlScanResultProcessor: cope with non-unique uniqueid in the database
> 
> Unfortunately, the uniqueid column (or rather its index) of our urls
> table is not defined as unique and unfortunately at least some code in
> SqlCollection doesn't check for duplicates before inserting to the
> table. This can be provoked for example by using the "Organize
> Collection" functionality.
> 
> While fixing SqlCollectionLocation in short-term and making the
> uniqueid index unique in long-term is probably needed, we need to cope
> with existing user databases. This change is needed because
> SqlScanResultProcessor identified tracks fully by their uniqueid which
> resulted in unpredictable and incorrect behaviour - it for example
> never removed the "old" duplicate entry in deleteDeletedTracks( int )
> and sometimes found incorrect entry when importing a track in
> commitTrack().
> 
> SqlScanResultProcessor: skip removeTrack() if there were errors
> 
> Another in a series that try to minimize chance of users losing their
> tracks, statistics etc.
> 
> SqlScanResultProcessor: don't accidentally delete tracks, defensive rewrite
> 
> This fixes data-loss that I can trigger every time by toggling "Local
> Files & USB Mass Storage Backend" in Config -> Plugins, restarting
> Amarok and triggering collection update / rescan.
> 
> In theory, more things such as cloning changing disk could trigger this
> problem, from SqlScanResultProcessor::deleteDeletedDirectories() comment:
> 
> We need to match directories by their (absolute) path, otherwise following
> scenario triggers statistics loss (bug 298275):
> 
> 1. user relocates collection to different filesystem, but clones path 
> structure
>or toggles MassStorageDeviceHandler enabled in Config -> plugins.
> 2. collectionscanner knows nothings about directory ids, so it doesn't detect
>any track changes and emits a bunch of skipped (unchanged) dirs with no
>tracks.
> 3. SqlRegistry::getDirectory() called there from returns different directory 
> id
>then in past.
> 4. deleteDeletedDirectories() is called, and if it operates on directory ids,
>it happily removes _all_ directories, taking tracks with it.
> 5. Tracks disappear from the UI until full rescan, stats, lyrics, labels are
>lost forever.
> 
> Also add a handful of asserts, ScanResultProcessor is very complicated
> and small error or corner-case in logic may result in horrible data
> losses.
> 
> Reporters of linked bugs, please try to reproduce your data-loss with
> this patch applied and report back in both cases. In negative case,
> please reopen and attach full updated amarok --debug log.
> 
> After this patch, only (statistics, lyrics and labels)-loss operations
> should be:
>  a) moving track out of mounted collection folders [by design]
>  b) changing both metadata and url from outside of a track not tagged
> by amarok_afttagger [we can do nothing about this]
> 
> BUG: 298275
> FIXED-IN: 2.6
> REVIEW: 105488
> 
> SqlMeta::Track: remove unused methods deviceId() and rpath()
> 
> There are unused and rather internal, to remove them.

Re: Review Request: Multiple EngineController and related fixes, incl. fix for release_blocker bug 299890 (squashed commits, recent on top)

2012-07-22 Thread Matěj Laitl 

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105610/#review16232
---


Hmm, I think I've found a solution for trackFinishedPlaying(), not a 
workaround, hold on with reviewing until I update this.

- Matěj Laitl


On July 19, 2012, 7:14 p.m., Matěj Laitl wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105610/
> ---
> 
> (Updated July 19, 2012, 7:14 p.m.)
> 
> 
> Review request for Amarok and Bart Cerneels.
> 
> 
> Description
> ---
> 
> MetaStream: match track by QUrl, not by QString
> 
> Fixes a bug where MetaStram::Track didn't update tags here because
> pretty url was used in signal, but it compared against encoded url
> (the one with %20 instead of spaces).
> 
> EngineController: reduce code duplication among play( TrackPtr ) and stop()
> 
> 
> EngineController: fix play count increased by 2 in some cases
> 
> First, I tried to fix this using the "correct" approach by emitting
> trackFinishedPlaying() in slotFinished() [connected to Phonon
> finished()] and in slotNewTrackPlaying() [connected to Phonon
> currentSourceChanged()], but I failed, because even
> currentSourceChanged() is emitted twice by some back-ends (at least
> phonon-vlc 0.5) plus slotNewTrackPlaying() has hard time determining
> whether m_currentTrack was really played.
> 
> So I've solved it by adding guard variable to detect
> slotAboutToFinish() being called twice per song. We would need this
> even for other things, I had following bug at the end of the playlist:
> first slotAboutToFinish() calls playlistActions()->requestNextTrack()
> which correctly says "nothing more to play", but the next call to it
> re-starts playback from the start of the playlist.
> 
> BUG: 299890
> FIXED-IN: 2.6
> 
> EngineController: fix isStream(), style fix for trackPosition()
> 
> 
> MetaStream: big clean-up, implement play statistics methods
> 
>  * remove many unused and useless methods such as setTitle() and
>friends
>  * additionally take genre, comment, track number from Engine
>Controller's signal
>  * implement rating, score, first & last play and play count. These are
>rather debugging tool for EngineController, but they also give you
>nice info on how many songs have played in the stream
>  * no need to reimplement observer pattern, use MetaBase implementation
>  * implement length, EngineController will emit length info soon
> 
> EngineController: introduce trackFinishedPlaying() signal
> 
> ...to be used in Last.fm scrobbling when it is emitted for streams,
> too.
> 
> EngineController: rework slotMetaDataChanged(), nearly no functional change
> 
>  * get data from Phonon to meta QVariantMap more intelligently
>  * m_lastTrack and trackChanged had effect only in debug message, ditch
>them
>  * note that m_currentTrack may be inaccurate there
>  * rename isMetadataSpam() to isInRecentMetaDataHistory() to describe
>the functionality better
>  * less debug() spam, specifically be rather quiet for duplicate
>signals from phonon
>  * Phonon doesn't provide track length, but it does provide track
>description that we save as "comments" now.
>  * trackData() method was unused and likely meant for
>slotMetaDataChanged(); ditch it
> 
> EngineController: remove unimplemented methods
> 
> No one could have called them, so remove them altogether to reduce
> developer confusion.
> 
> PlaylistFileProvider: fix switch { } compiler warning
> 
> 
> This addresses bug 299890.
> https://bugs.kde.org/show_bug.cgi?id=299890
> 
> 
> Diffs
> -
> 
>   ChangeLog 1d4f1e92fb5a033500c0f18d3dca257a89f1a139 
>   src/EngineController.h ad2e0c4a5e7c80c79bf448bf74cd6b52cd1f0ed3 
>   src/EngineController.cpp 83f0a6ed0a92ae992e1809800cee65d9349dc680 
>   src/core-impl/meta/stream/Stream.h 2b0a5824eae49345807eef94a465e133996624a1 
>   src/core-impl/meta/stream/Stream.cpp 
> b0fcfad5808b9ae428cb6612bec1737b3af82d3b 
>   src/core-impl/meta/stream/Stream_p.h 
> 58b715f27cb43d014d5837f1afec9d60cb71cc48 
>   src/playlistmanager/file/PlaylistFileProvider.cpp 
> bafdf69f24e606e0a1e879fda78cedfef7325cbb 
> 
> Diff: http://git.reviewboard.kde.org/r/105610/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Matěj Laitl
> 
>

___
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel