Re: Review Request 110426: KWalletHelper class for services using the KWallet
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110426/#review32518 --- src/services/magnatune/CMakeLists.txt http://git.reviewboard.kde.org/r/110426/#comment24195 The problem seems to be here. There's MagnatuneConfig.cpp here too. So you'll probably have to add KWalletHelper.cpp to this list as well to link it properly. - Jasneet Bhatti On May 14, 2013, 6:13 p.m., Vedant Agarwala wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110426/ --- (Updated May 14, 2013, 6:13 p.m.) Review request for Amarok. Description --- I have created a KWalletHelper class so that services like Maganatune, Last.fm and GPodder can use this rather than duplicating code. Currently the patch applies only to Magnatune. The KWalletHelper class complies but it doesn't link properly to the MagnatuneConfig class. Diffs - src/CMakeLists.txt 4dcb316 src/services/KWalletHelper.h e69de29 src/services/KWalletHelper.cpp e69de29 src/services/magnatune/CMakeLists.txt 91f24c0 src/services/magnatune/MagnatuneConfig.h 552bcf8 src/services/magnatune/MagnatuneConfig.cpp 5842c63 Diff: http://git.reviewboard.kde.org/r/110426/diff/ Testing --- The KWalletHelper.cpp complies but fails to link to ManatuneConfig.cpp. Output of make command: http://paste.kde.org/743792/ Thanks, Vedant Agarwala ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request 109470: A patch which allows us to distinguish between cached lyrics and lyrics newly downloaded from the internet.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109470/#review29229 --- While we wait for the active developers to decide if this feature is going to be introduced, there are a few formatting issues to be addressed before we proceed. As a rule, before you submit a patch, always make sure it adheres to the coding style laid out in HACKING/intro_and_style.txt And don't let this bother you too much, almost everyone has coding style issues in their first patch. src/context/applets/lyrics/LyricsApplet.cpp http://git.reviewboard.kde.org/r/109470/#comment21837 The defines and includes shouldn't be clubbed together. Let the newline remain here. src/context/applets/lyrics/LyricsApplet.cpp http://git.reviewboard.kde.org/r/109470/#comment21838 No need for a newline here. src/context/applets/lyrics/LyricsApplet.cpp http://git.reviewboard.kde.org/r/109470/#comment21843 Again, no need for the newlines. Keep the coding style consistent. src/context/applets/lyrics/LyricsApplet.cpp http://git.reviewboard.kde.org/r/109470/#comment21840 Leading/Trailing whitespaces to be removed. You might not be aware of their presence in the text editor/IDE you're using. So make sure the indentation is done using spaces and not tabs. If you're using Kate, you can do this by going to Settings-Configure Kate-Editor Component-Editing-Indentation and setting 'Indent using' to 'Spaces'. You could also use Mark's Vim configuration present in HACKING/intro_and_style.txt if you are using Vim. src/context/applets/lyrics/LyricsApplet.cpp http://git.reviewboard.kde.org/r/109470/#comment21844 Correct the formatting issues here. Refer to the 'Formatting' section of HACKING/intro_and_style to correct the issues : spaces between brackets and arguments, trailing whitespaces, indentation, etc. src/context/applets/lyrics/LyricsApplet.cpp http://git.reviewboard.kde.org/r/109470/#comment21845 Trailing whitespace. src/context/applets/lyrics/LyricsApplet.cpp http://git.reviewboard.kde.org/r/109470/#comment21846 Unnecessary newlines. src/context/applets/lyrics/LyricsApplet.cpp http://git.reviewboard.kde.org/r/109470/#comment21847 Here too. src/context/applets/lyrics/LyricsApplet.cpp http://git.reviewboard.kde.org/r/109470/#comment21848 Indentation issue. src/context/applets/lyrics/LyricsApplet.cpp http://git.reviewboard.kde.org/r/109470/#comment21849 Superfluous newline. src/context/engines/lyrics/LyricsEngine.h http://git.reviewboard.kde.org/r/109470/#comment21850 Indentation issue. src/context/engines/lyrics/LyricsEngine.cpp http://git.reviewboard.kde.org/r/109470/#comment21851 As mentioned previously, defines and includes are not to be clubbed together. src/context/engines/lyrics/LyricsEngine.cpp http://git.reviewboard.kde.org/r/109470/#comment21852 Trailing whitespace. src/context/engines/lyrics/LyricsEngine.cpp http://git.reviewboard.kde.org/r/109470/#comment21856 Spaces needed between brackets and argument : newCacheLyrics( lyrics ) src/context/engines/lyrics/LyricsEngine.cpp http://git.reviewboard.kde.org/r/109470/#comment21853 Superfluous newline. src/context/engines/lyrics/LyricsEngine.cpp http://git.reviewboard.kde.org/r/109470/#comment21854 Indentation issue. src/context/engines/lyrics/LyricsEngine.cpp http://git.reviewboard.kde.org/r/109470/#comment21855 Spaces needed between brackets and function arguments. src/context/engines/lyrics/LyricsEngine.cpp http://git.reviewboard.kde.org/r/109470/#comment21857 Let the newline remain here. - Jasneet Bhatti On March 14, 2013, 7:10 p.m., mayank jha wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109470/ --- (Updated March 14, 2013, 7:10 p.m.) Review request for Amarok, Samikshan Bairagya, Jasneet Bhatti, Bartosz Brachaczek, Tirtha Chatterjee, and Matěj Laitl. Description --- It required modifications, when there is no change in the lyrics downloaded and lyrics retrieved from cache the title display of the lyrics browser changes to Cached Lyrics from Lyrics so we can tell the difference between old and new. Diffs - src/context/applets/lyrics/LyricsApplet.cpp 2394964 src/context/engines/lyrics/LyricsEngine.h b187b73 src/context/engines/lyrics/LyricsEngine.cpp 2befa91 Diff: http://git.reviewboard.kde.org/r/109470/diff/ Testing --- Its working fine! Thanks, mayank jha ___ 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
: TestPrivateMetaRegistry::testInsertGenre() INFO : TestPrivateMetaRegistry::testInsertYear() entering INFO : TestPrivateMetaRegistry::testInsertYear(Track01.ogg) COMPARE() Loc: [/home/jasneet/amarok/tests/core/meta/support/TestPrivateMetaRegistry.cpp(159)] INFO : TestPrivateMetaRegistry::testInsertYear(Track02.ogg) COMPARE() Loc: [/home/jasneet/amarok/tests/core/meta/support/TestPrivateMetaRegistry.cpp(159)] INFO : TestPrivateMetaRegistry::testInsertYear(Platz 01.mp3) COMPARE() Loc: [/home/jasneet/amarok/tests/core/meta/support/TestPrivateMetaRegistry.cpp(159)] INFO : TestPrivateMetaRegistry::testInsertYear(Platz 02.mp3) COMPARE() Loc: [/home/jasneet/amarok/tests/core/meta/support/TestPrivateMetaRegistry.cpp(159)] INFO : TestPrivateMetaRegistry::testInsertYear(Platz 03.mp3) COMPARE() Loc: [/home/jasneet/amarok/tests/core/meta/support/TestPrivateMetaRegistry.cpp(159)] PASS : TestPrivateMetaRegistry::testInsertYear() INFO : TestPrivateMetaRegistry::testNull() entering INFO : TestPrivateMetaRegistry::testNull() COMPARE() Loc: [/home/jasneet/amarok/tests/core/meta/support/TestPrivateMetaRegistry.cpp(165)] INFO : TestPrivateMetaRegistry::testNull() COMPARE() Loc: [/home/jasneet/amarok/tests/core/meta/support/TestPrivateMetaRegistry.cpp(166)] INFO : TestPrivateMetaRegistry::testNull() COMPARE() Loc: [/home/jasneet/amarok/tests/core/meta/support/TestPrivateMetaRegistry.cpp(167)] INFO : TestPrivateMetaRegistry::testNull() COMPARE() Loc: [/home/jasneet/amarok/tests/core/meta/support/TestPrivateMetaRegistry.cpp(168)] INFO : TestPrivateMetaRegistry::testNull() COMPARE() Loc: [/home/jasneet/amarok/tests/core/meta/support/TestPrivateMetaRegistry.cpp(169)] PASS : TestPrivateMetaRegistry::testNull() INFO : TestPrivateMetaRegistry::cleanupTestCase() entering PASS : TestPrivateMetaRegistry::cleanupTestCase() Totals: 8 passed, 0 failed, 0 skipped * Finished testing of TestPrivateMetaRegistry * Thanks, Jasneet Bhatti ___ 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
On July 22, 2012, 8:19 p.m., Matěj Laitl wrote: tests/core/meta/support/TestPrivateMetaRegistry.cpp, line 165 http://git.reviewboard.kde.org/r/105525/diff/1-2/?file=72251#file72251line165 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) Oops ! Was in a hurry then but will not drop standards again. Rest assured I would never have pushed a broken test to master though. I always build and check twice before pushing and this issue would have been resolved then if not here. - Jasneet --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105525/#review16229 --- On July 24, 2012, 4:32 p.m., Jasneet Bhatti wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105525/ --- (Updated July 24, 2012, 4:32 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
Re: Review Request: SoK - Unit Test : core/meta/support/MetaKeys( TrackKey )
On July 18, 2012, 8:50 p.m., Matěj Laitl wrote:
src/core/meta/support/MetaKeys.h, line 25
http://git.reviewboard.kde.org/r/105454/diff/3/?file=72593#file72593line25
Same here: is this really needed? friend class should be enough IMO,
but I'm too lazy to try. :-|
TestMetaAlbumKey is declared in a global namespace.
So, to be able to add it as a friend class, we need to either do the explicit
forward declaration as above and use the :: operator to refer to it
OR
do:
namespace
{
class TestMetaAlbumKey;
}
and then use it without the :: operator like any other class
Had it been declared in some user defined namespace, only the friend class
declaration would have sufficed.
- Jasneet
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105454/#review16093
---
On July 14, 2012, 4:06 a.m., Jasneet Bhatti wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105454/
---
(Updated July 14, 2012, 4:06 a.m.)
Review request for Amarok, Matěj Laitl and Sven Krohlas.
Description
---
Added a unit test for class TrackKey of core/meta/support/MetaKeys
Diffs
-
src/core/meta/support/MetaKeys.h 1a2fc25
src/core/meta/support/MetaKeys.cpp d3fd722
tests/core/meta/CMakeLists.txt 3ae78c9
tests/core/meta/support/CMakeLists.txt PRE-CREATION
tests/core/meta/support/TestMetaTrackKey.h PRE-CREATION
tests/core/meta/support/TestMetaTrackKey.cpp PRE-CREATION
Diff: http://git.reviewboard.kde.org/r/105454/diff/
Testing
---
Builds, and runs fine.
Output of running test with -v2 flag:
* Start testing of TestMetaTrackKey *
Config: Using QTest library 4.8.1, Qt 4.8.1
INFO : TestMetaTrackKey::initTestCase() entering
PASS : TestMetaTrackKey::initTestCase()
INFO : TestMetaTrackKey::testOperatorAssignment() entering
QSYSTEM: TestMetaTrackKey::testOperatorAssignment() qttest(4918)/kdecore
(K*TimeZone*): No time zone information obtained from ktimezoned
INFO : TestMetaTrackKey::testOperatorAssignment() QVERIFY(!( trackKey1 ==
trackKey2 ))
Loc:
[/home/jasneet/amarok/tests/core/meta/support/TestMetaTrackKey.cpp(41)]
INFO : TestMetaTrackKey::testOperatorAssignment() QVERIFY(trackKey1 ==
trackKey2)
Loc:
[/home/jasneet/amarok/tests/core/meta/support/TestMetaTrackKey.cpp(45)]
PASS : TestMetaTrackKey::testOperatorAssignment()
INFO : TestMetaTrackKey::cleanupTestCase() entering
PASS : TestMetaTrackKey::cleanupTestCase()
Totals: 3 passed, 0 failed, 0 skipped
* Finished testing of TestMetaTrackKey *
Thanks,
Jasneet Bhatti
___
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/MetaKeys( AlbumKey )
On July 18, 2012, 8:36 p.m., Matěj Laitl wrote:
src/core/meta/support/MetaKeys.h, line 25
http://git.reviewboard.kde.org/r/105497/diff/2/?file=72605#file72605line25
Is this really needed here? From my experience the friend class
declaration serves as a forward declaration, too.
TestMetaAlbumKey is declared in a global namespace.
So, to be able to add it as a friend class, we need to either do the explicit
forward declaration as above and use the :: operator to refer to it
OR
do:
namespace
{
class TestMetaAlbumKey;
}
and then use it without the :: operator like any other class
Had it been declared in some user defined namespace, only the friend class
declaration would have sufficed.
- Jasneet
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105497/#review16091
---
On July 14, 2012, 6:03 a.m., Jasneet Bhatti wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105497/
---
(Updated July 14, 2012, 6:03 a.m.)
Review request for Amarok, Matěj Laitl and Sven Krohlas.
Description
---
Unit test for class AlbumKey of core/meta/support/MetaKeys
Created a couple of silent tracks( take negligible space ) for testing with
the artist and album names comprehensively. Seemed like a smarter way than to
create large mocks since no other functionality is to be tested for.
Diffs
-
src/core/meta/support/MetaKeys.h 1a2fc25
tests/core/meta/CMakeLists.txt 3ae78c9
tests/core/meta/support/CMakeLists.txt PRE-CREATION
tests/core/meta/support/TestMetaAlbumKey.h PRE-CREATION
tests/core/meta/support/TestMetaAlbumKey.cpp PRE-CREATION
tests/data/audio/album2/Track01.ogg PRE-CREATION
tests/data/audio/album2/Track02.ogg PRE-CREATION
Diff: http://git.reviewboard.kde.org/r/105497/diff/
Testing
---
Builds, links and runs fine.
Output of running test with -v2 flag:
* Start testing of TestMetaAlbumKey *
Config: Using QTest library 4.8.1, Qt 4.8.1
INFO : TestMetaAlbumKey::initTestCase() entering
QSYSTEM: TestMetaAlbumKey::initTestCase() qttest(431)/kdecore (K*TimeZone*):
No time zone information obtained from ktimezoned
PASS : TestMetaAlbumKey::initTestCase()
INFO : TestMetaAlbumKey::testOperatorAssignment() entering
INFO : TestMetaAlbumKey::testOperatorAssignment() QVERIFY(!( albumKey1 ==
albumKey2 ))
Loc:
[/home/jasneet/amarok/tests/core/meta/support/TestMetaAlbumKey.cpp(55)]
INFO : TestMetaAlbumKey::testOperatorAssignment() QVERIFY(albumKey1 ==
tempAlbumKey)
Loc:
[/home/jasneet/amarok/tests/core/meta/support/TestMetaAlbumKey.cpp(59)]
INFO : TestMetaAlbumKey::testOperatorAssignment() QVERIFY(!( albumKey1 ==
albumKey2 ))
Loc:
[/home/jasneet/amarok/tests/core/meta/support/TestMetaAlbumKey.cpp(64)]
INFO : TestMetaAlbumKey::testOperatorAssignment() QVERIFY(albumKey1 ==
tempAlbumKey)
Loc:
[/home/jasneet/amarok/tests/core/meta/support/TestMetaAlbumKey.cpp(68)]
PASS : TestMetaAlbumKey::testOperatorAssignment()
INFO : TestMetaAlbumKey::testOperatorLessThan() entering
INFO : TestMetaAlbumKey::testOperatorLessThan() QVERIFY(albumKey1
albumKey2)
Loc:
[/home/jasneet/amarok/tests/core/meta/support/TestMetaAlbumKey.cpp(79)]
INFO : TestMetaAlbumKey::testOperatorLessThan() QVERIFY(!( albumKey1
albumKey1 ))
Loc:
[/home/jasneet/amarok/tests/core/meta/support/TestMetaAlbumKey.cpp(82)]
INFO : TestMetaAlbumKey::testOperatorLessThan() QVERIFY(albumKey2
albumKey3)
Loc:
[/home/jasneet/amarok/tests/core/meta/support/TestMetaAlbumKey.cpp(85)]
INFO : TestMetaAlbumKey::testOperatorLessThan() QVERIFY(albumKey1
albumKey3)
Loc:
[/home/jasneet/amarok/tests/core/meta/support/TestMetaAlbumKey.cpp(88)]
INFO : TestMetaAlbumKey::testOperatorLessThan() QVERIFY(albumKey4
albumKey5)
Loc:
[/home/jasneet/amarok/tests/core/meta/support/TestMetaAlbumKey.cpp(95)]
INFO : TestMetaAlbumKey::testOperatorLessThan() QVERIFY(!( albumKey4
albumKey4 ))
Loc:
[/home/jasneet/amarok/tests/core/meta/support/TestMetaAlbumKey.cpp(98)]
INFO : TestMetaAlbumKey::testOperatorLessThan() QVERIFY(albumKey5
albumKey6)
Loc:
[/home/jasneet/amarok/tests/core/meta/support/TestMetaAlbumKey.cpp(101)]
INFO : TestMetaAlbumKey::testOperatorLessThan() QVERIFY(albumKey4
albumKey6)
Loc:
[/home/jasneet/amarok/tests/core/meta/support/TestMetaAlbumKey.cpp(104)]
PASS : TestMetaAlbumKey::testOperatorLessThan()
INFO : TestMetaAlbumKey::cleanupTestCase() entering
PASS : TestMetaAlbumKey::cleanupTestCase()
Totals: 4 passed, 0 failed, 0 skipped
* Finished testing of TestMetaAlbumKey
Re: Review Request: SoK - Unit Test : core/meta/support/PrivateMetaRegistry
(142)] PASS : TestPrivateMetaRegistry::testInsertGenre() INFO : TestPrivateMetaRegistry::testInsertYear() entering INFO : TestPrivateMetaRegistry::testInsertYear(Track01.ogg) COMPARE() Loc: [/home/jasneet/amarok/tests/core/meta/support/TestPrivateMetaRegistry.cpp(159)] INFO : TestPrivateMetaRegistry::testInsertYear(Track02.ogg) COMPARE() Loc: [/home/jasneet/amarok/tests/core/meta/support/TestPrivateMetaRegistry.cpp(159)] INFO : TestPrivateMetaRegistry::testInsertYear(Platz 01.mp3) COMPARE() Loc: [/home/jasneet/amarok/tests/core/meta/support/TestPrivateMetaRegistry.cpp(159)] INFO : TestPrivateMetaRegistry::testInsertYear(Platz 02.mp3) COMPARE() Loc: [/home/jasneet/amarok/tests/core/meta/support/TestPrivateMetaRegistry.cpp(159)] INFO : TestPrivateMetaRegistry::testInsertYear(Platz 03.mp3) COMPARE() Loc: [/home/jasneet/amarok/tests/core/meta/support/TestPrivateMetaRegistry.cpp(159)] PASS : TestPrivateMetaRegistry::testInsertYear() INFO : TestPrivateMetaRegistry::testNull() entering INFO : TestPrivateMetaRegistry::testNull() COMPARE() Loc: [/home/jasneet/amarok/tests/core/meta/support/TestPrivateMetaRegistry.cpp(165)] INFO : TestPrivateMetaRegistry::testNull() COMPARE() Loc: [/home/jasneet/amarok/tests/core/meta/support/TestPrivateMetaRegistry.cpp(166)] INFO : TestPrivateMetaRegistry::testNull() COMPARE() Loc: [/home/jasneet/amarok/tests/core/meta/support/TestPrivateMetaRegistry.cpp(167)] INFO : TestPrivateMetaRegistry::testNull() COMPARE() Loc: [/home/jasneet/amarok/tests/core/meta/support/TestPrivateMetaRegistry.cpp(168)] INFO : TestPrivateMetaRegistry::testNull() COMPARE() Loc: [/home/jasneet/amarok/tests/core/meta/support/TestPrivateMetaRegistry.cpp(169)] PASS : TestPrivateMetaRegistry::testNull() INFO : TestPrivateMetaRegistry::cleanupTestCase() entering PASS : TestPrivateMetaRegistry::cleanupTestCase() Totals: 8 passed, 0 failed, 0 skipped * Finished testing of TestPrivateMetaRegistry * Thanks, Jasneet Bhatti ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Review Request: SoK - Unit Test : core/meta/support/MetaUtility
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105629/ --- Review request for Amarok, Matěj Laitl and Sven Krohlas. Description --- Test for core/meta/support/MetaUtility THIS IS NOT YET COMPLETE. The testMprisMapFromTrack() and testMpris20MapFromTrack() test functions are not setting the cover image properly and I'm unable to understand why. I've added a couple of qDebug() statements to show the same. I'd be grateful if someone could tell me what the problem actually is. Everything else is working fine. Diffs - src/core-impl/collections/db/sql/SqlRegistry.h 8d80117 src/core/meta/support/MetaUtility.h c1ba442 src/core/meta/support/MetaUtility.cpp 133076b tests/core/meta/CMakeLists.txt 3ae78c9 tests/core/meta/support/CMakeLists.txt PRE-CREATION tests/core/meta/support/TestMetaUtility.h PRE-CREATION tests/core/meta/support/TestMetaUtility.cpp PRE-CREATION tests/data/cover/amarok.png PRE-CREATION Diff: http://git.reviewboard.kde.org/r/105629/diff/ Testing --- Builds and runs fine. Just the image setting problem exists. Thanks, Jasneet Bhatti ___ 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/MetaKeys( AlbumKey )
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105497/ --- (Updated July 14, 2012, 6:03 a.m.) Review request for Amarok, Matěj Laitl and Sven Krohlas. Changes --- Added a test case and fixed a few issues ( see review above ). Description --- Unit test for class AlbumKey of core/meta/support/MetaKeys Created a couple of silent tracks( take negligible space ) for testing with the artist and album names comprehensively. Seemed like a smarter way than to create large mocks since no other functionality is to be tested for. Diffs (updated) - src/core/meta/support/MetaKeys.h 1a2fc25 tests/core/meta/CMakeLists.txt 3ae78c9 tests/core/meta/support/CMakeLists.txt PRE-CREATION tests/core/meta/support/TestMetaAlbumKey.h PRE-CREATION tests/core/meta/support/TestMetaAlbumKey.cpp PRE-CREATION tests/data/audio/album2/Track01.ogg PRE-CREATION tests/data/audio/album2/Track02.ogg PRE-CREATION Diff: http://git.reviewboard.kde.org/r/105497/diff/ Testing --- Builds, links and runs fine. Output of running test with -v2 flag: * Start testing of TestMetaAlbumKey * Config: Using QTest library 4.8.1, Qt 4.8.1 INFO : TestMetaAlbumKey::initTestCase() entering QSYSTEM: TestMetaAlbumKey::initTestCase() qttest(431)/kdecore (K*TimeZone*): No time zone information obtained from ktimezoned PASS : TestMetaAlbumKey::initTestCase() INFO : TestMetaAlbumKey::testOperatorAssignment() entering INFO : TestMetaAlbumKey::testOperatorAssignment() QVERIFY(!( albumKey1 == albumKey2 )) Loc: [/home/jasneet/amarok/tests/core/meta/support/TestMetaAlbumKey.cpp(55)] INFO : TestMetaAlbumKey::testOperatorAssignment() QVERIFY(albumKey1 == tempAlbumKey) Loc: [/home/jasneet/amarok/tests/core/meta/support/TestMetaAlbumKey.cpp(59)] INFO : TestMetaAlbumKey::testOperatorAssignment() QVERIFY(!( albumKey1 == albumKey2 )) Loc: [/home/jasneet/amarok/tests/core/meta/support/TestMetaAlbumKey.cpp(64)] INFO : TestMetaAlbumKey::testOperatorAssignment() QVERIFY(albumKey1 == tempAlbumKey) Loc: [/home/jasneet/amarok/tests/core/meta/support/TestMetaAlbumKey.cpp(68)] PASS : TestMetaAlbumKey::testOperatorAssignment() INFO : TestMetaAlbumKey::testOperatorLessThan() entering INFO : TestMetaAlbumKey::testOperatorLessThan() QVERIFY(albumKey1 albumKey2) Loc: [/home/jasneet/amarok/tests/core/meta/support/TestMetaAlbumKey.cpp(79)] INFO : TestMetaAlbumKey::testOperatorLessThan() QVERIFY(!( albumKey1 albumKey1 )) Loc: [/home/jasneet/amarok/tests/core/meta/support/TestMetaAlbumKey.cpp(82)] INFO : TestMetaAlbumKey::testOperatorLessThan() QVERIFY(albumKey2 albumKey3) Loc: [/home/jasneet/amarok/tests/core/meta/support/TestMetaAlbumKey.cpp(85)] INFO : TestMetaAlbumKey::testOperatorLessThan() QVERIFY(albumKey1 albumKey3) Loc: [/home/jasneet/amarok/tests/core/meta/support/TestMetaAlbumKey.cpp(88)] INFO : TestMetaAlbumKey::testOperatorLessThan() QVERIFY(albumKey4 albumKey5) Loc: [/home/jasneet/amarok/tests/core/meta/support/TestMetaAlbumKey.cpp(95)] INFO : TestMetaAlbumKey::testOperatorLessThan() QVERIFY(!( albumKey4 albumKey4 )) Loc: [/home/jasneet/amarok/tests/core/meta/support/TestMetaAlbumKey.cpp(98)] INFO : TestMetaAlbumKey::testOperatorLessThan() QVERIFY(albumKey5 albumKey6) Loc: [/home/jasneet/amarok/tests/core/meta/support/TestMetaAlbumKey.cpp(101)] INFO : TestMetaAlbumKey::testOperatorLessThan() QVERIFY(albumKey4 albumKey6) Loc: [/home/jasneet/amarok/tests/core/meta/support/TestMetaAlbumKey.cpp(104)] PASS : TestMetaAlbumKey::testOperatorLessThan() INFO : TestMetaAlbumKey::cleanupTestCase() entering PASS : TestMetaAlbumKey::cleanupTestCase() Totals: 4 passed, 0 failed, 0 skipped * Finished testing of TestMetaAlbumKey * Thanks, Jasneet Bhatti ___ 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/MetaKeys( AlbumKey )
On July 13, 2012, 1:03 p.m., Matěj Laitl wrote: tests/core/meta/support/TestMetaAlbumKey.cpp, line 33 http://git.reviewboard.kde.org/r/105497/diff/1/?file=71972#file71972line33 How big are the added files? They should be no more than a few kilobytes in order not to make Amarok source unnecessary big. You can even create tracks in test programatically, see TestSqlScanManager::createTrack() The two added files are 4.1 KB each, which I think is acceptable. Moreover, I think future tests will require a few tracks with different tags for trivial testing. It would save time and effort to use these instead of creating tracks as in TestSqlScanManager. OR The track/album/etc. generation code can be moved out of TestSqlScanManager and made into a helper class of its own. - Jasneet --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105497/#review15794 --- On July 14, 2012, 6:03 a.m., Jasneet Bhatti wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105497/ --- (Updated July 14, 2012, 6:03 a.m.) Review request for Amarok, Matěj Laitl and Sven Krohlas. Description --- Unit test for class AlbumKey of core/meta/support/MetaKeys Created a couple of silent tracks( take negligible space ) for testing with the artist and album names comprehensively. Seemed like a smarter way than to create large mocks since no other functionality is to be tested for. Diffs - src/core/meta/support/MetaKeys.h 1a2fc25 tests/core/meta/CMakeLists.txt 3ae78c9 tests/core/meta/support/CMakeLists.txt PRE-CREATION tests/core/meta/support/TestMetaAlbumKey.h PRE-CREATION tests/core/meta/support/TestMetaAlbumKey.cpp PRE-CREATION tests/data/audio/album2/Track01.ogg PRE-CREATION tests/data/audio/album2/Track02.ogg PRE-CREATION Diff: http://git.reviewboard.kde.org/r/105497/diff/ Testing --- Builds, links and runs fine. Output of running test with -v2 flag: * Start testing of TestMetaAlbumKey * Config: Using QTest library 4.8.1, Qt 4.8.1 INFO : TestMetaAlbumKey::initTestCase() entering QSYSTEM: TestMetaAlbumKey::initTestCase() qttest(431)/kdecore (K*TimeZone*): No time zone information obtained from ktimezoned PASS : TestMetaAlbumKey::initTestCase() INFO : TestMetaAlbumKey::testOperatorAssignment() entering INFO : TestMetaAlbumKey::testOperatorAssignment() QVERIFY(!( albumKey1 == albumKey2 )) Loc: [/home/jasneet/amarok/tests/core/meta/support/TestMetaAlbumKey.cpp(55)] INFO : TestMetaAlbumKey::testOperatorAssignment() QVERIFY(albumKey1 == tempAlbumKey) Loc: [/home/jasneet/amarok/tests/core/meta/support/TestMetaAlbumKey.cpp(59)] INFO : TestMetaAlbumKey::testOperatorAssignment() QVERIFY(!( albumKey1 == albumKey2 )) Loc: [/home/jasneet/amarok/tests/core/meta/support/TestMetaAlbumKey.cpp(64)] INFO : TestMetaAlbumKey::testOperatorAssignment() QVERIFY(albumKey1 == tempAlbumKey) Loc: [/home/jasneet/amarok/tests/core/meta/support/TestMetaAlbumKey.cpp(68)] PASS : TestMetaAlbumKey::testOperatorAssignment() INFO : TestMetaAlbumKey::testOperatorLessThan() entering INFO : TestMetaAlbumKey::testOperatorLessThan() QVERIFY(albumKey1 albumKey2) Loc: [/home/jasneet/amarok/tests/core/meta/support/TestMetaAlbumKey.cpp(79)] INFO : TestMetaAlbumKey::testOperatorLessThan() QVERIFY(!( albumKey1 albumKey1 )) Loc: [/home/jasneet/amarok/tests/core/meta/support/TestMetaAlbumKey.cpp(82)] INFO : TestMetaAlbumKey::testOperatorLessThan() QVERIFY(albumKey2 albumKey3) Loc: [/home/jasneet/amarok/tests/core/meta/support/TestMetaAlbumKey.cpp(85)] INFO : TestMetaAlbumKey::testOperatorLessThan() QVERIFY(albumKey1 albumKey3) Loc: [/home/jasneet/amarok/tests/core/meta/support/TestMetaAlbumKey.cpp(88)] INFO : TestMetaAlbumKey::testOperatorLessThan() QVERIFY(albumKey4 albumKey5) Loc: [/home/jasneet/amarok/tests/core/meta/support/TestMetaAlbumKey.cpp(95)] INFO : TestMetaAlbumKey::testOperatorLessThan() QVERIFY(!( albumKey4 albumKey4 )) Loc: [/home/jasneet/amarok/tests/core/meta/support/TestMetaAlbumKey.cpp(98)] INFO : TestMetaAlbumKey::testOperatorLessThan() QVERIFY(albumKey5 albumKey6) Loc: [/home/jasneet/amarok/tests/core/meta/support/TestMetaAlbumKey.cpp(101)] INFO : TestMetaAlbumKey::testOperatorLessThan() QVERIFY(albumKey4 albumKey6) Loc: [/home/jasneet/amarok/tests/core/meta/support/TestMetaAlbumKey.cpp(104)] PASS : TestMetaAlbumKey::testOperatorLessThan() INFO : TestMetaAlbumKey::cleanupTestCase() entering PASS : TestMetaAlbumKey::cleanupTestCase() Totals: 4 passed, 0
Re: Review Request: SoK - Unit Test : core/meta/support/MetaKeys( TrackKey )
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105454/ --- (Updated July 14, 2012, 4:06 a.m.) Review request for Amarok, Matěj Laitl and Sven Krohlas. Changes --- Added a test method( see review above ). Updated the TrackKey() constructor to initialize integer data members Description --- Added a unit test for class TrackKey of core/meta/support/MetaKeys Diffs (updated) - src/core/meta/support/MetaKeys.h 1a2fc25 src/core/meta/support/MetaKeys.cpp d3fd722 tests/core/meta/CMakeLists.txt 3ae78c9 tests/core/meta/support/CMakeLists.txt PRE-CREATION tests/core/meta/support/TestMetaTrackKey.h PRE-CREATION tests/core/meta/support/TestMetaTrackKey.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/105454/diff/ Testing --- Builds, and runs fine. Output of running test with -v2 flag: * Start testing of TestMetaTrackKey * Config: Using QTest library 4.8.1, Qt 4.8.1 INFO : TestMetaTrackKey::initTestCase() entering PASS : TestMetaTrackKey::initTestCase() INFO : TestMetaTrackKey::testOperatorAssignment() entering QSYSTEM: TestMetaTrackKey::testOperatorAssignment() qttest(4918)/kdecore (K*TimeZone*): No time zone information obtained from ktimezoned INFO : TestMetaTrackKey::testOperatorAssignment() QVERIFY(!( trackKey1 == trackKey2 )) Loc: [/home/jasneet/amarok/tests/core/meta/support/TestMetaTrackKey.cpp(41)] INFO : TestMetaTrackKey::testOperatorAssignment() QVERIFY(trackKey1 == trackKey2) Loc: [/home/jasneet/amarok/tests/core/meta/support/TestMetaTrackKey.cpp(45)] PASS : TestMetaTrackKey::testOperatorAssignment() INFO : TestMetaTrackKey::cleanupTestCase() entering PASS : TestMetaTrackKey::cleanupTestCase() Totals: 3 passed, 0 failed, 0 skipped * Finished testing of TestMetaTrackKey * Thanks, Jasneet Bhatti ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: SoK - Unit Test : core/collections/support/TrackForUrlWorker
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105389/ --- (Updated July 11, 2012, 7:31 p.m.) Review request for Amarok, Matěj Laitl and Sven Krohlas. Changes --- Fixed typo Description --- Added unit test for core/collections/support/TrackForUrlWorker Just the one slot completeJob() to test. Tested for both KUrl and QString types of urls. Diffs (updated) - tests/core/collections/CMakeLists.txt b01b655 tests/core/collections/support/CMakeLists.txt PRE-CREATION tests/core/collections/support/TestTrackForUrlWorker.h PRE-CREATION tests/core/collections/support/TestTrackForUrlWorker.cpp PRE-CREATION tests/mocks/MockTrackForUrlWorker.h PRE-CREATION tests/mocks/MockTrackForUrlWorker.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/105389/diff/ Testing --- Builds and runs fine. Thanks, Jasneet Bhatti ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: SoK - Unit Test : core/collections/support/TrackForUrlWorker
::testCompleteJobQString(track 1) QVERIFY(receivedDestroyed) Loc: [/home/jasneet/amarok/tests/core/collections/support/TestTrackForUrlWorker.cpp(112)] INFO : TestTrackForUrlWorker::testCompleteJobQString(track 2) QVERIFY(trackForUrlWorker) Loc: [/home/jasneet/amarok/tests/core/collections/support/TestTrackForUrlWorker.cpp(74)] INFO : TestTrackForUrlWorker::testCompleteJobQString(track 2) QVERIFY(receivedDone) Loc: [/home/jasneet/amarok/tests/core/collections/support/TestTrackForUrlWorker.cpp(101)] INFO : TestTrackForUrlWorker::testCompleteJobQString(track 2) COMPARE() Loc: [/home/jasneet/amarok/tests/core/collections/support/TestTrackForUrlWorker.cpp(104)] INFO : TestTrackForUrlWorker::testCompleteJobQString(track 2) COMPARE() Loc: [/home/jasneet/amarok/tests/core/collections/support/TestTrackForUrlWorker.cpp(108)] INFO : TestTrackForUrlWorker::testCompleteJobQString(track 2) QVERIFY(receivedDestroyed) Loc: [/home/jasneet/amarok/tests/core/collections/support/TestTrackForUrlWorker.cpp(112)] INFO : TestTrackForUrlWorker::testCompleteJobQString(track 3) QVERIFY(trackForUrlWorker) Loc: [/home/jasneet/amarok/tests/core/collections/support/TestTrackForUrlWorker.cpp(74)] INFO : TestTrackForUrlWorker::testCompleteJobQString(track 3) QVERIFY(receivedDone) Loc: [/home/jasneet/amarok/tests/core/collections/support/TestTrackForUrlWorker.cpp(101)] INFO : TestTrackForUrlWorker::testCompleteJobQString(track 3) COMPARE() Loc: [/home/jasneet/amarok/tests/core/collections/support/TestTrackForUrlWorker.cpp(104)] INFO : TestTrackForUrlWorker::testCompleteJobQString(track 3) COMPARE() Loc: [/home/jasneet/amarok/tests/core/collections/support/TestTrackForUrlWorker.cpp(108)] INFO : TestTrackForUrlWorker::testCompleteJobQString(track 3) QVERIFY(receivedDestroyed) Loc: [/home/jasneet/amarok/tests/core/collections/support/TestTrackForUrlWorker.cpp(112)] PASS : TestTrackForUrlWorker::testCompleteJobQString() INFO : TestTrackForUrlWorker::cleanupTestCase() entering PASS : TestTrackForUrlWorker::cleanupTestCase() Totals: 4 passed, 0 failed, 0 skipped * Finished testing of TestTrackForUrlWorker * Thanks, Jasneet Bhatti ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Review Request: SoK - Unit Test : core/meta/support/MetaKeys( AlbumKey )
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105497/ --- Review request for Amarok, Matěj Laitl and Sven Krohlas. Description --- Unit test for class AlbumKey of core/meta/support/MetaKeys Created a couple of silent tracks( take negligible space ) for testing with the artist and album names comprehensively. Seemed like a smarter way than to create large mocks since no other functionality is to be tested for. Diffs - tests/core/meta/CMakeLists.txt 3ae78c9 tests/core/meta/support/CMakeLists.txt PRE-CREATION tests/core/meta/support/TestMetaAlbumKey.h PRE-CREATION tests/core/meta/support/TestMetaAlbumKey.cpp PRE-CREATION tests/data/audio/album PRE-CREATION tests/data/audio/album PRE-CREATION Diff: http://git.reviewboard.kde.org/r/105497/diff/ Testing --- Builds, links and runs fine. Thanks, Jasneet Bhatti ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: SoK - Unit Test : core/collections/support/TrackForUrlWorker
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105389/ --- (Updated July 6, 2012, 8:54 a.m.) Review request for Amarok, Matěj Laitl and Sven Krohlas. Changes --- Now only one mock class exists. Contrived helper methods done away with and _real usage_ implementation for testing done. Also, test data now comes from the existing tracks available for tests in the tests/data folder instead of mock tracks. Description --- Added unit test for core/collections/support/TrackForUrlWorker Just the one slot completeJob() to test. Tested for both KUrl and QString types of urls. Diffs (updated) - tests/core/collections/CMakeLists.txt b01b655 tests/core/collections/support/CMakeLists.txt PRE-CREATION tests/core/collections/support/TestTrackForUrlWorker.h PRE-CREATION tests/core/collections/support/TestTrackForUrlWorker.cpp PRE-CREATION tests/mocks/MockTrackForUrlWorker.h PRE-CREATION tests/mocks/MockTrackForUrlWorker.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/105389/diff/ Testing --- Builds and runs fine. Thanks, Jasneet Bhatti ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: SoK - Unit Test : core/collections/support/TrackForUrlWorker
On July 5, 2012, 9:29 p.m., Matěj Laitl wrote: tests/core/collections/support/TestTrackForUrlWorker.h, line 29 http://git.reviewboard.kde.org/r/105389/diff/2/?file=71361#file71361line29 Why these are public? Data members should be normally private. Done away with these On July 5, 2012, 9:29 p.m., Matěj Laitl wrote: tests/core/collections/support/TestTrackForUrlWorker.cpp, line 94 http://git.reviewboard.kde.org/r/105389/diff/2/?file=71362#file71362line94 testCompleteJobKUrl_data() and testCompleteJobQString_data() can share code, too, I suggest non-slot private testCompleteJobInternal_data() Apparently, having a standalone textCompleteJobInternal_data() doesn't suffice as textCompleteJobInternal() isn't a test function. So, testCompleteJobKUrl_data() and testCompleteJobQString_data() both call textCompleteJobInternal_data() to add test data. - Jasneet --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105389/#review15417 --- On July 6, 2012, 8:54 a.m., Jasneet Bhatti wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105389/ --- (Updated July 6, 2012, 8:54 a.m.) Review request for Amarok, Matěj Laitl and Sven Krohlas. Description --- Added unit test for core/collections/support/TrackForUrlWorker Just the one slot completeJob() to test. Tested for both KUrl and QString types of urls. Diffs - tests/core/collections/CMakeLists.txt b01b655 tests/core/collections/support/CMakeLists.txt PRE-CREATION tests/core/collections/support/TestTrackForUrlWorker.h PRE-CREATION tests/core/collections/support/TestTrackForUrlWorker.cpp PRE-CREATION tests/mocks/MockTrackForUrlWorker.h PRE-CREATION tests/mocks/MockTrackForUrlWorker.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/105389/diff/ Testing --- Builds and runs fine. Thanks, Jasneet Bhatti ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: SoK - Unit Test : core/collections/support/TrackForUrlWorker
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105389/ --- (Updated July 6, 2012, 5:38 p.m.) Review request for Amarok, Matěj Laitl and Sven Krohlas. Changes --- I guess this should solve all issues. Some of the code style issues arise from the autocomplete feature of KDevelop, where I edit one part to comply with the style and miss some other part :P Description --- Added unit test for core/collections/support/TrackForUrlWorker Just the one slot completeJob() to test. Tested for both KUrl and QString types of urls. Diffs (updated) - tests/core/collections/CMakeLists.txt b01b655 tests/core/collections/support/CMakeLists.txt PRE-CREATION tests/core/collections/support/TestTrackForUrlWorker.h PRE-CREATION tests/core/collections/support/TestTrackForUrlWorker.cpp PRE-CREATION tests/mocks/MockTrackForUrlWorker.h PRE-CREATION tests/mocks/MockTrackForUrlWorker.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/105389/diff/ Testing --- Builds and runs fine. Thanks, Jasneet Bhatti ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: SoK - Unit Test : core/collections/support/TrackForUrlWorker
On July 6, 2012, 9:57 a.m., Matěj Laitl wrote: tests/core/collections/support/TestTrackForUrlWorker.cpp, line 113 http://git.reviewboard.kde.org/r/105389/diff/3/?file=71458#file71458line113 You can use m_emittedTrack on the left side here. Also, why don't you use QCOMPARE? It gives better messages in case of error. Silly me ! Of course private members are accessible within the class. *After effects of sleep deprivation* And the argument you hold here for QCOMPARE is exactly the reason I used it in the previous issue. But yes, it doesn't really matter there but very much here. - Jasneet --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105389/#review15450 --- On July 6, 2012, 5:38 p.m., Jasneet Bhatti wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105389/ --- (Updated July 6, 2012, 5:38 p.m.) Review request for Amarok, Matěj Laitl and Sven Krohlas. Description --- Added unit test for core/collections/support/TrackForUrlWorker Just the one slot completeJob() to test. Tested for both KUrl and QString types of urls. Diffs - tests/core/collections/CMakeLists.txt b01b655 tests/core/collections/support/CMakeLists.txt PRE-CREATION tests/core/collections/support/TestTrackForUrlWorker.h PRE-CREATION tests/core/collections/support/TestTrackForUrlWorker.cpp PRE-CREATION tests/mocks/MockTrackForUrlWorker.h PRE-CREATION tests/mocks/MockTrackForUrlWorker.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/105389/diff/ Testing --- Builds and runs fine. Thanks, Jasneet Bhatti ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Review Request: SoK - Unit Test : core/meta/support/MetaKeys( TrackKey )
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105454/ --- Review request for Amarok, Matěj Laitl and Sven Krohlas. Description --- Added a unit test for class TrackKey of core/meta/support/MetaKeys Diffs - tests/core/meta/CMakeLists.txt 3ae78c9 tests/core/meta/support/CMakeLists.txt PRE-CREATION tests/core/meta/support/TestMetaTrackKey.h PRE-CREATION tests/core/meta/support/TestMetaTrackKey.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/105454/diff/ Testing --- Builds, and runs fine Thanks, Jasneet Bhatti ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: SoK - Unit Test : core/collections/support/TrackForUrlWorker
On July 3, 2012, 10:17 a.m., Matěj Laitl wrote: tests/core/collections/support/TestTrackForUrlWorker.cpp, line 44 http://git.reviewboard.kde.org/r/105389/diff/1/?file=70647#file70647line44 Hmm, I don't think this is the correct test. Instead, you should implement run() in MockTrackForUrlWorker and set m_track inside to to different values (perhaps using data-driven testing, MockTrackForUrlWorker can be documented what variable name and type it fetches) and then ensue that finishedLookup signal emits the correct TrackPtr. QTest::kWaitForSignal may be handy for it. Implemented data driven testing, but with mock tracks as test data. Didn't find any reason to include or test using track related info( name, artist, etc. ), since the track pointer comparison is enough to verify that the correct track is emitted. On July 3, 2012, 10:17 a.m., Matěj Laitl wrote: tests/mocks/MockTrackForUrlWorker.h, line 53 http://git.reviewboard.kde.org/r/105389/diff/1/?file=70648#file70648line53 I don't think this helper method should exist. completeJob() is private. So we do require a public method to call it directly or indirectly. I went for this as it also tests if the connection was properly made in the constructor ( although Qt should guarantee that, still ! ). - Jasneet --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105389/#review15334 --- On July 5, 2012, 12:07 a.m., Jasneet Bhatti wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105389/ --- (Updated July 5, 2012, 12:07 a.m.) Review request for Amarok, Matěj Laitl and Sven Krohlas. Description --- Added unit test for core/collections/support/TrackForUrlWorker Just the one slot completeJob() to test. Tested for both KUrl and QString types of urls. Diffs - tests/core/collections/CMakeLists.txt b01b655 tests/core/collections/support/CMakeLists.txt PRE-CREATION tests/core/collections/support/TestTrackForUrlWorker.h PRE-CREATION tests/core/collections/support/TestTrackForUrlWorker.cpp PRE-CREATION tests/mocks/MockTrackForUrlWorker.h PRE-CREATION tests/mocks/MockTrackForUrlWorker.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/105389/diff/ Testing --- Builds and runs fine. Thanks, Jasneet Bhatti ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: SoK - Unit Test : core/collections/QueryMaker
On June 26, 2012, 9:29 a.m., Matěj Laitl wrote: tests/core/collections/MockQueryMaker.cpp, line 18 http://git.reviewboard.kde.org/r/105172/diff/3/?file=70259#file70259line18 No need to include moc. if the Q_OBJECT is in .h file. When I remove the cpp implementation, I get an 'undefined vtable for MockQueryMaker' error on running make. Do I need to add/include the header file somewhere in the CMakeLists ? - Jasneet --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105172/#review15160 --- On June 23, 2012, 7:05 p.m., Jasneet Bhatti wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105172/ --- (Updated June 23, 2012, 7:05 p.m.) Review request for Amarok, Matěj Laitl and Sven Krohlas. Description --- Implements unit test for the QueryMaker class in core/collections/QueryMaker Diffs - tests/core/collections/CMakeLists.txt b01b655 tests/core/collections/MockQueryMaker.h PRE-CREATION tests/core/collections/MockQueryMaker.cpp PRE-CREATION tests/core/collections/TestQueryMaker.h PRE-CREATION tests/core/collections/TestQueryMaker.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/105172/diff/ Testing --- Test builds and runs fine Thanks, Jasneet Bhatti ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: SoK - Unit Test : core/collections/QueryMaker
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105172/ --- (Updated June 26, 2012, 8:05 p.m.) Review request for Amarok, Matěj Laitl and Sven Krohlas. Changes --- Made new methods of mock class virtual so that subclasses can reimplement them if they need to. Description --- Implements unit test for the QueryMaker class in core/collections/QueryMaker Diffs (updated) - tests/core/collections/CMakeLists.txt b01b655 tests/core/collections/TestQueryMaker.h PRE-CREATION tests/core/collections/TestQueryMaker.cpp PRE-CREATION tests/mocks/MockQueryMaker.h PRE-CREATION tests/mocks/MockQueryMaker.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/105172/diff/ Testing --- Test builds and runs fine Thanks, Jasneet Bhatti ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: SoK - Unit Test : core/collections/QueryMaker
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105172/ --- (Updated June 23, 2012, 7:02 p.m.) Review request for Amarok and Sven Krohlas. Changes --- Only setAutoDelete requires any real testing, so tested it using the data driven approach Not sure if the mock class should be here or in tests/mock . I don't really think this mock is reusable as it implements it's own method and slot, so kept it here. Description --- This is basically a patch creating an empty test for abstract class core/collections/QueryMaker The test has not been implemented though as the class can only be tested in tests for derived classes implementing the pure virtual functions It may need to be rewritten and implemented if the class is rewritten in the future Diffs (updated) - tests/core/collections/CMakeLists.txt b01b655 tests/core/collections/MockQueryMaker.h PRE-CREATION tests/core/collections/MockQueryMaker.cpp PRE-CREATION tests/core/collections/TestQueryMaker.h PRE-CREATION tests/core/collections/TestQueryMaker.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/105172/diff/ Testing --- Able to build Amarok after writing the test Thanks, Jasneet Bhatti ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: SoK - Unit Test : core/collections/QueryMaker
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105172/ --- (Updated June 23, 2012, 7:03 p.m.) Review request for Amarok and Sven Krohlas. Changes --- Updated the description and testing status Description (updated) --- Implements unit test for the QueryMaker class in core/collections/QueryMaker Diffs - tests/core/collections/CMakeLists.txt b01b655 tests/core/collections/MockQueryMaker.h PRE-CREATION tests/core/collections/MockQueryMaker.cpp PRE-CREATION tests/core/collections/TestQueryMaker.h PRE-CREATION tests/core/collections/TestQueryMaker.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/105172/diff/ Testing (updated) --- Test builds and runs fine Thanks, Jasneet Bhatti ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: SoK - Unit Test : core/collections/QueryMaker
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105172/ --- (Updated June 23, 2012, 7:05 p.m.) Review request for Amarok, Matěj Laitl and Sven Krohlas. Changes --- Added a developer in the reviewers list Description --- Implements unit test for the QueryMaker class in core/collections/QueryMaker Diffs - tests/core/collections/CMakeLists.txt b01b655 tests/core/collections/MockQueryMaker.h PRE-CREATION tests/core/collections/MockQueryMaker.cpp PRE-CREATION tests/core/collections/TestQueryMaker.h PRE-CREATION tests/core/collections/TestQueryMaker.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/105172/diff/ Testing --- Test builds and runs fine Thanks, Jasneet Bhatti ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: SoK Unit test : core/collections/Collection
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105166/ --- (Updated June 21, 2012, 1:38 p.m.) Review request for Amarok and Sven Krohlas. Changes --- Rewrote the entire test. TrackProvider has trivial functions that have been tested directly CollectionBase has been tested using a mock subclass, similar to the way I did with class MetaCapability of core/meta/Meta Collection is an abstract class, so tested the non pure virtual functions using a mock subclass Description --- This patch implements a unit test for core/collections/Collection There are abstract classes to be tested as well, which can only be done when subclasses define the pure virtual functions. So tests for those will be done along with the subclasses. Diffs (updated) - tests/core/collections/CMakeLists.txt 2efd1fe tests/core/collections/TestCollection.h PRE-CREATION tests/core/collections/TestCollection.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/105166/diff/ Testing --- Test passes on my repository Thanks, Jasneet Bhatti ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: SoK Unit test : core/collections/Collection
On June 21, 2012, 2:08 p.m., Matěj Laitl wrote: Matěj Laitl wrote: Collection has really just 3 implemented methods. CollectionBase shouldn't exist, because it really is Meta::MetaCapability. This should be changed by some Amarok dev as soon as 2.6 is released. Then the change is done, CollectionBase methods will be already tested in MetaCapability, so you don't have to test them here. So I suggest you remove the tests for is() and create(), too. Ok. Will remove the unnecessary ones. On June 21, 2012, 2:08 p.m., Matěj Laitl wrote: tests/core/collections/TestCollection.h, line 27 http://git.reviewboard.kde.org/r/105166/diff/2/?file=70024#file70024line27 Strange indentation. Don't you mix tabs and spaces, jasneet? Actually, I had some code from the previous version of this test and some new, so consistency problems with the indentation. Will correct these. On June 21, 2012, 2:08 p.m., Matěj Laitl wrote: tests/core/collections/TestCollection.cpp, line 34 http://git.reviewboard.kde.org/r/105166/diff/2/?file=70025#file70025line34 Strange indentation again. Hmm? Don't understand this one. There are two levels of indentation and the variable definition is according to intro_and_style. Please explain what is the fault. On June 21, 2012, 2:08 p.m., Matěj Laitl wrote: tests/core/collections/TestCollection.cpp, line 203 http://git.reviewboard.kde.org/r/105166/diff/2/?file=70025#file70025line203 While entire code shouldn't be here, I wonder why you just dont QCOMPARE( collection-usedCapacity(), 0.0 )? You really want to pretend static_cast doesn't exist and use it only in at a least resort. 0.0 is a double and usedCapacity returns a float, so there was a warning on compilation. Used the cast to eliminated the warning. http://qt-project.org/doc/qt-4.8/qtest.html#QCOMPARE In the case of comparing floats and doubles, qFuzzyCompare() is used for comparing. This means that comparing to 0 will likely fail. One solution to this is to compare to 1, and add 1 to the produced output. I'm not sure if the above applies for 0.0 too, so added 1 to not take a chance. On June 21, 2012, 2:08 p.m., Matěj Laitl wrote: tests/core/collections/TestCollection.cpp, line 228 http://git.reviewboard.kde.org/r/105166/diff/2/?file=70025#file70025line228 No no no, you should actually test that isWritable() returns what location()-isWritable() returns. To do this you can subclass your CollectionMock to return CollectionLocationMock where you'll cause different isWritable() values to be returned and verified. You can use http://qt-project.org/doc/qt-4.8/qtestlib-tutorial2.html + implement TestCollection::testIsWritable_data(). Then you can use QFETCH(bool, isWritable); in CollectionLocationMock. Working on it now. On June 21, 2012, 2:08 p.m., Matěj Laitl wrote: tests/core/collections/TestCollection.cpp, line 237 http://git.reviewboard.kde.org/r/105166/diff/2/?file=70025#file70025line237 Same as comment for testIsWritable() Working on it now. - Jasneet --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105166/#review14948 --- On June 21, 2012, 1:38 p.m., Jasneet Bhatti wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105166/ --- (Updated June 21, 2012, 1:38 p.m.) Review request for Amarok and Sven Krohlas. Description --- This patch implements a unit test for core/collections/Collection There are abstract classes to be tested as well, which can only be done when subclasses define the pure virtual functions. So tests for those will be done along with the subclasses. Diffs - tests/core/collections/CMakeLists.txt 2efd1fe tests/core/collections/TestCollection.h PRE-CREATION tests/core/collections/TestCollection.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/105166/diff/ Testing --- Test passes on my repository Thanks, Jasneet Bhatti ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: SoK Unit test : core/collections/Collection
On June 21, 2012, 2:08 p.m., Matěj Laitl wrote: tests/core/collections/TestCollection.cpp, line 203 http://git.reviewboard.kde.org/r/105166/diff/2/?file=70025#file70025line203 While entire code shouldn't be here, I wonder why you just dont QCOMPARE( collection-usedCapacity(), 0.0 )? You really want to pretend static_cast doesn't exist and use it only in at a least resort. Jasneet Bhatti wrote: 0.0 is a double and usedCapacity returns a float, so there was a warning on compilation. Used the cast to eliminated the warning. http://qt-project.org/doc/qt-4.8/qtest.html#QCOMPARE In the case of comparing floats and doubles, qFuzzyCompare() is used for comparing. This means that comparing to 0 will likely fail. One solution to this is to compare to 1, and add 1 to the produced output. I'm not sure if the above applies for 0.0 too, so added 1 to not take a chance. Matěj Laitl wrote: 0.0 is a double and usedCapacity returns a float, so there was a warning on compilation. Used the cast to eliminated the warning. Oh I see. Then preferred way to cast basic types is to use their constructor: float( 0.0 ) In the case of comparing floats and doubles, qFuzzyCompare() is used for comparing. This means that comparing to 0 will likely fail. One solution to this is to compare to 1, and add 1 to the produced output. I see, then you can use QVERIFY( actual == expected ) to avoid qFuzzyCompare(). I'm not sure if the above applies for 0.0 too, so added 1 to not take a chance. For doubles and floats, 0.0 == 0. Ok. Will do this. On June 21, 2012, 2:08 p.m., Matěj Laitl wrote: tests/core/collections/TestCollection.cpp, line 34 http://git.reviewboard.kde.org/r/105166/diff/2/?file=70025#file70025line34 Strange indentation again. Hmm? Jasneet Bhatti wrote: Don't understand this one. There are two levels of indentation and the variable definition is according to intro_and_style. Please explain what is the fault. Matěj Laitl wrote: There is no indentation from what I see here. Well, the patch I uploaded and my repo both show the indentation, not sure why it isn't visible to you ? - Jasneet --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105166/#review14948 --- On June 21, 2012, 1:38 p.m., Jasneet Bhatti wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105166/ --- (Updated June 21, 2012, 1:38 p.m.) Review request for Amarok and Sven Krohlas. Description --- This patch implements a unit test for core/collections/Collection There are abstract classes to be tested as well, which can only be done when subclasses define the pure virtual functions. So tests for those will be done along with the subclasses. Diffs - tests/core/collections/CMakeLists.txt 2efd1fe tests/core/collections/TestCollection.h PRE-CREATION tests/core/collections/TestCollection.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/105166/diff/ Testing --- Test passes on my repository Thanks, Jasneet Bhatti ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: SoK Unit test : core/collections/Collection
On June 21, 2012, 2:08 p.m., Matěj Laitl wrote: tests/core/collections/TestCollection.cpp, line 34 http://git.reviewboard.kde.org/r/105166/diff/2/?file=70025#file70025line34 Strange indentation again. Hmm? Jasneet Bhatti wrote: Don't understand this one. There are two levels of indentation and the variable definition is according to intro_and_style. Please explain what is the fault. Matěj Laitl wrote: There is no indentation from what I see here. Jasneet Bhatti wrote: Well, the patch I uploaded and my repo both show the indentation, not sure why it isn't visible to you ? Sam Lade wrote: There is indentation present, but again it's a tab rather than spaces (and tabs and spaces are mixed throughout the patch). Please use spaces only for indentation (see HACKING/intro_and_style.txt). You should configure your IDE or text editor to do this automatically to make life easier. Oh I get it now. This will sound silly but I didn't know that four spaces and one tab of width four appear differently and so was confused about what mixing tabs and spaces meant. Will make sure to never press Tab again while writing patches. Thanks for clearing that. - Jasneet --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105166/#review14948 --- On June 21, 2012, 1:38 p.m., Jasneet Bhatti wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105166/ --- (Updated June 21, 2012, 1:38 p.m.) Review request for Amarok and Sven Krohlas. Description --- This patch implements a unit test for core/collections/Collection There are abstract classes to be tested as well, which can only be done when subclasses define the pure virtual functions. So tests for those will be done along with the subclasses. Diffs - tests/core/collections/CMakeLists.txt 2efd1fe tests/core/collections/TestCollection.h PRE-CREATION tests/core/collections/TestCollection.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/105166/diff/ Testing --- Test passes on my repository Thanks, Jasneet Bhatti ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: SoK Unit test : core/collections/Collection
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105166/ --- (Updated June 21, 2012, 10:20 p.m.) Review request for Amarok and Sven Krohlas. Changes --- Changed the null pointer comparison method to comply with the coding style Description --- This patch implements a unit test for core/collections/Collection There are abstract classes to be tested as well, which can only be done when subclasses define the pure virtual functions. So tests for those will be done along with the subclasses. Diffs (updated) - tests/core/collections/CMakeLists.txt 2efd1fe tests/core/collections/TestCollection.h PRE-CREATION tests/core/collections/TestCollection.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/105166/diff/ Testing --- Test passes on my repository Thanks, Jasneet Bhatti ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: SoK - Unit test : core/playlists/PlaylistFormat
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105306/ --- (Updated June 20, 2012, 2:05 p.m.) Review request for Amarok, Bart Cerneels, Matěj Laitl, and Sven Krohlas. Changes --- Added a few more test cases and removed an extra newline at the end of CMakeLists.txt Description --- Adds a unit test for core/playlists/PlaylistFormat Diffs (updated) - tests/core/CMakeLists.txt 8f7db22 tests/core/playlists/CMakeLists.txt PRE-CREATION tests/core/playlists/TestPlaylistFormat.h PRE-CREATION tests/core/playlists/TestPlaylistFormat.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/105306/diff/ Testing --- Compiles, links and runs fine. Thanks, Jasneet Bhatti ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: SoK - Unit test : core/playlists/PlaylistFormat
On June 20, 2012, 1:09 p.m., Sven Krohlas wrote:
tests/core/playlists/TestPlaylistFormat.cpp, line 84
http://git.reviewboard.kde.org/r/105306/diff/1/?file=69931#file69931line84
You might also want to feed it a string without any . at all. Or
several of them.
Done.
And forgot to mention in the diff change description that ${QT_QTGUI_LIBRARY}
was also removed as link target as it isn't needed.
- Jasneet
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105306/#review14910
---
On June 20, 2012, 2:05 p.m., Jasneet Bhatti wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105306/
---
(Updated June 20, 2012, 2:05 p.m.)
Review request for Amarok, Bart Cerneels, Matěj Laitl, and Sven Krohlas.
Description
---
Adds a unit test for core/playlists/PlaylistFormat
Diffs
-
tests/core/CMakeLists.txt 8f7db22
tests/core/playlists/CMakeLists.txt PRE-CREATION
tests/core/playlists/TestPlaylistFormat.h PRE-CREATION
tests/core/playlists/TestPlaylistFormat.cpp PRE-CREATION
Diff: http://git.reviewboard.kde.org/r/105306/diff/
Testing
---
Compiles, links and runs fine.
Thanks,
Jasneet Bhatti
___
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: SoK - Unit test : core/playlists/PlaylistFormat
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105306/ --- (Updated June 20, 2012, 2:17 p.m.) Review request for Amarok, Bart Cerneels, Matěj Laitl, and Sven Krohlas. Changes --- Added a test case with empty filename Description --- Adds a unit test for core/playlists/PlaylistFormat Diffs (updated) - tests/core/CMakeLists.txt 8f7db22 tests/core/playlists/CMakeLists.txt PRE-CREATION tests/core/playlists/TestPlaylistFormat.h PRE-CREATION tests/core/playlists/TestPlaylistFormat.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/105306/diff/ Testing --- Compiles, links and runs fine. Thanks, Jasneet Bhatti ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: SoK - Unit Test : core/meta/Meta (MetaCapability)
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105291/
---
(Updated June 19, 2012, 2:02 p.m.)
Review request for Amarok, Bart Cerneels, Matěj Laitl, and Sven Krohlas.
Changes
---
Added ${QT_QTGUI_LIBRARY} to target_link_libraries
Description
---
Unit Test for class MetaCapability of core/meta/Meta
Diffs (updated)
-
tests/core/meta/CMakeLists.txt 0e85d83
tests/core/meta/TestMetaCapability.h PRE-CREATION
tests/core/meta/TestMetaCapability.cpp PRE-CREATION
Diff: http://git.reviewboard.kde.org/r/105291/diff/
Testing
---
Unit Test builds and runs as desired
Thanks,
Jasneet Bhatti
___
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: SoK - Unit test : core/capabilities/ActionsCapability
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105144/
---
(Updated June 19, 2012, 11:20 p.m.)
Review request for Amarok.
Changes
---
Changed link target from ${KDE4_KDEUI_LIBS} to ${QT_QTGUI_LIBRARY}
Description
---
This is a patch implementing unit testing of core/capabilities/ActionsCapability
Diffs (updated)
-
tests/core/CMakeLists.txt c66d3be
tests/core/capabilities/CMakeLists.txt PRE-CREATION
tests/core/capabilities/TestActionsCapability.h PRE-CREATION
tests/core/capabilities/TestActionsCapability.cpp PRE-CREATION
Diff: http://git.reviewboard.kde.org/r/105144/diff/
Testing
---
Thanks,
Jasneet Bhatti
___
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel
Review Request: SoK - Unit Test : core/meta/Meta (MetaCapability)
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105291/ --- Review request for Amarok, Bart Cerneels, Matěj Laitl, and Sven Krohlas. Description --- Unit Test for class MetaCapability of core/meta/Meta Diffs - tests/core/meta/CMakeLists.txt 0e85d83 tests/core/meta/TestMetaCapability.h PRE-CREATION tests/core/meta/TestMetaCapability.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/105291/diff/ Testing --- Unit Test builds and runs as desired Thanks, Jasneet Bhatti ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: SoK - Unit test : core/capabilities/ActionsCapability
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105144/ --- (Updated June 18, 2012, 8:15 p.m.) Review request for Amarok. Changes --- Formatting changes to match coding style for Amarok Description --- This is a patch implementing unit testing of core/capabilities/ActionsCapability Diffs (updated) - src/core/capabilities/ActionsCapability.h 08de31d tests/core/CMakeLists.txt c66d3be tests/core/capabilities/CMakeLists.txt e69de29 tests/core/capabilities/TestActionsCapability.h e69de29 tests/core/capabilities/TestActionsCapability.cpp e69de29 Diff: http://git.reviewboard.kde.org/r/105144/diff/ Testing --- Thanks, Jasneet Bhatti ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Review Request: SoK - Unit Test : core/collections/QueryMaker
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105172/ --- Review request for Amarok and Sven Krohlas. Description --- This is basically a patch creating an empty test for abstract class core/collections/QueryMaker The test has not been implemented though as the class can only be tested in tests for derived classes implementing the pure virtual functions It may need to be rewritten and implemented if the class is rewritten in the future Diffs - tests/core/collections/TestQueryMaker.h PRE-CREATION tests/core/collections/TestQueryMaker.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/105172/diff/ Testing --- Able to build Amarok after writing the test Thanks, Jasneet Bhatti ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: SoK Unit test : core/collections/Collection
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105166/ --- (Updated June 7, 2012, 12:58 p.m.) Review request for Amarok and Sven Krohlas. Changes --- Changed the Summary to better describe the test Summary (updated) - SoK Unit test : core/collections/Collection Description --- This patch implements a unit test for core/collections/Collection There are abstract classes to be tested as well, which can only be done when subclasses define the pure virtual functions. So tests for those will be done along with the subclasses. Diffs - tests/core/collections/CMakeLists.txt 2efd1fe tests/core/collections/TestCollection.h PRE-CREATION tests/core/collections/TestCollection.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/105166/diff/ Testing --- Test passes on my repository Thanks, Jasneet Bhatti ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: SoK - Unit Test : core/collections/QueryMaker
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105172/ --- (Updated June 7, 2012, 1:40 p.m.) Review request for Amarok and Sven Krohlas. Changes --- Modified the implementation file to include the correct header file Description --- This is basically a patch creating an empty test for abstract class core/collections/QueryMaker The test has not been implemented though as the class can only be tested in tests for derived classes implementing the pure virtual functions It may need to be rewritten and implemented if the class is rewritten in the future Diffs (updated) - tests/core/collections/TestQueryMaker.h PRE-CREATION tests/core/collections/TestQueryMaker.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/105172/diff/ Testing --- Able to build Amarok after writing the test Thanks, Jasneet Bhatti ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Unit test : ActionsCapability
On June 4, 2012, 7:01 a.m., Bart Cerneels wrote: I have a few nitpicks that are more about our API, ActionsCapability in particular. As a part of the unit test patches you should improve the documentation as well. Will help to understand (and get us to properly define) the use cases of the interface. Yes, I agree. Will do it for sure. On June 4, 2012, 7:01 a.m., Bart Cerneels wrote: tests/core/capabilities/TestActionsCapability.cpp, line 46 http://git.reviewboard.kde.org/r/105144/diff/2/?file=66220#file66220line46 Does ActionsCapability actually claim that it has to return the same objects in the same order as creation? Perhaps you should test the order and QProprties of the returned actions instead. The actions list( m_actions ) is a protected member of ActionsCapability. So subclasses cannot modify it and there is no function in ActionsCapability either that would seem to have a need to modify it. So I guess we can safely assume that the actions list should remain the same both in order and values, in which case I believe a simple '==' comparison would suffice. On June 4, 2012, 7:01 a.m., Bart Cerneels wrote: tests/core/capabilities/TestActionsCapability.cpp, line 64 http://git.reviewboard.kde.org/r/105144/diff/2/?file=66220#file66220line64 To verify this you could have created the capability with an empty list. Does not influence the test in any way. Yes, that would make the intention of that test much clearer. Done. - Jasneet --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105144/#review14402 --- On June 3, 2012, 3:45 p.m., Jasneet Bhatti wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105144/ --- (Updated June 3, 2012, 3:45 p.m.) Review request for Amarok. Description --- This is a patch implementing unit testing of core/capabilities/ActionsCapability Diffs - tests/core/capabilities/TestActionsCapability.cpp PRE-CREATION tests/core/CMakeLists.txt c66d3be tests/core/capabilities/CMakeLists.txt PRE-CREATION tests/core/capabilities/TestActionsCapability.h PRE-CREATION Diff: http://git.reviewboard.kde.org/r/105144/diff/ Testing --- Thanks, Jasneet Bhatti ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Unit test : ActionsCapability
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105144/ --- (Updated June 4, 2012, 10:53 a.m.) Review request for Amarok. Changes --- Modified testCapabilityInterfaceType() to create capability with an empty list. Added a comment in ActionsCapability.cpp to make sure subclasses can assume the actions list will remain unmodified during their lifetime Description --- This is a patch implementing unit testing of core/capabilities/ActionsCapability Diffs (updated) - src/core/capabilities/ActionsCapability.h 08de31d tests/core/CMakeLists.txt c66d3be tests/core/capabilities/CMakeLists.txt e69de29 tests/core/capabilities/TestActionsCapability.h e69de29 tests/core/capabilities/TestActionsCapability.cpp e69de29 Diff: http://git.reviewboard.kde.org/r/105144/diff/ Testing --- Thanks, Jasneet Bhatti ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
GSoC 2012 Proof Of Concept : Amarok Unit Testing - Sample test
I have been extremely busy with university exam preparations for the past few days, so haven't been as active here as I've been before. But I've written a sample test to demonstrate my test writing skills. P.S. : This is just a sample test, so I've not put it up on the kde reviewboard Regards Jasneet sampletest.patch Description: Binary data ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Implemented Bug 214721 - Enable bookmark marker to be moved
On March 25, 2012, 1:48 p.m., Matěj Laitl wrote: Okay, I've commited this with a few formatting changes (breaking long lines), ChangeLog and appendArg() - setArg() rename in tests. (Jasneet, you may want to build Amarok with KDE4_BUILD_TESTS=ON cmake swtich); I've tried to upload updated patch here twice so that you can see the changes, but it somehow doesn't show. Anyways, thanks and I'd be glad to commit more your review requests, Jasneet, pick any little bug that annoys you. Yoohoo!!! Thanks for the suggestions and improvements. And I didn't know much about the testing aspect, but I'm glad I'm finding out more. I've also been studying about it recently. - Jasneet --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104307/#review11839 --- On March 24, 2012, 12:03 p.m., Jasneet Bhatti wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104307/ --- (Updated March 24, 2012, 12:03 p.m.) Review request for Amarok. Description --- This patch implements https://bugs.kde.org/show_bug.cgi?id=214721 . The bookmark is movable within the slider. If it is dragged outside the range, it will revert back to its previous valid location. The bookmark is activated( seek is called ) only when the bookmark is clicked and its position hasn't changed. Diffs - src/amarokurls/AmarokUrl.h 6a1d67f src/amarokurls/AmarokUrl.cpp 19ba210 src/amarokurls/BookmarkModel.h 73ae345 src/amarokurls/BookmarkModel.cpp 9218088 src/amarokurls/ContextUrlGenerator.cpp 16986f6 src/amarokurls/NavigationUrlGenerator.cpp d1e21e2 src/amarokurls/PlayUrlGenerator.h 131b737 src/amarokurls/PlayUrlGenerator.cpp 90e71ff src/context/applets/similarartists/ArtistWidget.cpp 18d8cc2 src/context/applets/upcomingevents/UpcomingEventsApplet.cpp 9664201 src/core-impl/collections/db/sql/CapabilityDelegateImpl.cpp dc5c500 src/playlist/PlaylistViewUrlGenerator.cpp 0ffcb9f src/services/ServiceCapabilities.cpp 6129f8e src/widgets/BookmarkTriangle.h 46e9118 src/widgets/BookmarkTriangle.cpp 4c59d42 src/widgets/SliderWidget.cpp 5e72e13 Diff: http://git.reviewboard.kde.org/r/104307/diff/ Testing --- Tested it on ubuntu 11.04 with kubuntu-desktop. Works fine for me. Thanks, Jasneet Bhatti ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Implemented Bug 214721 - Enable bookmark marker to be moved
On March 25, 2012, 1:48 p.m., Matěj Laitl wrote: Okay, I've commited this with a few formatting changes (breaking long lines), ChangeLog and appendArg() - setArg() rename in tests. (Jasneet, you may want to build Amarok with KDE4_BUILD_TESTS=ON cmake swtich); I've tried to upload updated patch here twice so that you can see the changes, but it somehow doesn't show. Anyways, thanks and I'd be glad to commit more your review requests, Jasneet, pick any little bug that annoys you. Jasneet Bhatti wrote: Yoohoo!!! Thanks for the suggestions and improvements. And I didn't know much about the testing aspect, but I'm glad I'm finding out more. I've also been studying about it recently. Oh, I forgot ! If it's convenient, please mail me the updated patch at jazneetbha...@gmail.com - Jasneet --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104307/#review11839 --- On March 24, 2012, 12:03 p.m., Jasneet Bhatti wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104307/ --- (Updated March 24, 2012, 12:03 p.m.) Review request for Amarok. Description --- This patch implements https://bugs.kde.org/show_bug.cgi?id=214721 . The bookmark is movable within the slider. If it is dragged outside the range, it will revert back to its previous valid location. The bookmark is activated( seek is called ) only when the bookmark is clicked and its position hasn't changed. Diffs - src/amarokurls/AmarokUrl.h 6a1d67f src/amarokurls/AmarokUrl.cpp 19ba210 src/amarokurls/BookmarkModel.h 73ae345 src/amarokurls/BookmarkModel.cpp 9218088 src/amarokurls/ContextUrlGenerator.cpp 16986f6 src/amarokurls/NavigationUrlGenerator.cpp d1e21e2 src/amarokurls/PlayUrlGenerator.h 131b737 src/amarokurls/PlayUrlGenerator.cpp 90e71ff src/context/applets/similarartists/ArtistWidget.cpp 18d8cc2 src/context/applets/upcomingevents/UpcomingEventsApplet.cpp 9664201 src/core-impl/collections/db/sql/CapabilityDelegateImpl.cpp dc5c500 src/playlist/PlaylistViewUrlGenerator.cpp 0ffcb9f src/services/ServiceCapabilities.cpp 6129f8e src/widgets/BookmarkTriangle.h 46e9118 src/widgets/BookmarkTriangle.cpp 4c59d42 src/widgets/SliderWidget.cpp 5e72e13 Diff: http://git.reviewboard.kde.org/r/104307/diff/ Testing --- Tested it on ubuntu 11.04 with kubuntu-desktop. Works fine for me. Thanks, Jasneet Bhatti ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Implemented Bug 214721 - Enable bookmark marker to be moved
On March 22, 2012, 9:46 p.m., Matěj Laitl wrote: src/widgets/BookmarkTriangle.h, line 37 http://git.reviewboard.kde.org/r/104307/diff/1/?file=53589#file53589line37 What if slider width changes during the bookmark lifetime? Also, please also document new (or better, even the old) parameters. I don't know of a case when the slider width changes. Does it happen while streaming live ? On March 22, 2012, 9:46 p.m., Matěj Laitl wrote: src/amarokurls/AmarokUrl.h, line 60 http://git.reviewboard.kde.org/r/104307/diff/1/?file=53585#file53585line60 This method shoudn't exist. AmarokUrl is used for all kinds of internal Amarok bookmarks, not just track position bookmarks. AmarokUrl should know nothing about track position bookmarks. I suggest you rename AmarokUrl::appendArg() to setArg() because it is what it does. (and please document it along the way) Then you can call this method to replace pos argument followed by saveToDb(); Done. On March 22, 2012, 9:46 p.m., Matěj Laitl wrote: src/amarokurls/AmarokUrl.cpp, line 225 http://git.reviewboard.kde.org/r/104307/diff/1/?file=53586#file53586line225 Err, what is this? This does something that I woudn't expect and it doesn't even document why. Please explain what you try to achieve with this, with examples. This is actually not really related to this bug. It's another bug I found: Whenever you rename two bookmarks to the same name, and then delete the bookmark that was created/renamed first, the one created later gets deleted. So I was trying to implement the rename function in such a way that whenever the user renames the bookmark, the position of the bookmark stays appended to the bookmark name(the regular expression and subsequent conditions check if the user has kept the position or has deleted it after renaming the bookmark). But as you said, AmarokUrl is used for other kinds of bookmarks too, so I guess this method might not be a good idea. Please do suggest an alternative. Meanwhile, I'm reverting back to the original rename function. On March 22, 2012, 9:46 p.m., Matěj Laitl wrote: src/amarokurls/BookmarkModel.h, line 103 http://git.reviewboard.kde.org/r/104307/diff/1/?file=53587#file53587line103 Similar issue here: moveBookmark() is play-bookmark-specific and BookMarkModel shoudn't know about it. I suggest you rather implement setBookmarkArg( name, key, value ); Also, this method then shouldn't be called directly by BookmarkTriangle, but rather trough PlayUrlGenerator, where moveTrackBookmark( Meta::Track, qint64 newMiliseconds, QString name = QString()); can be. Remaining issue is that would still have to rename the bookmark by name, which I consider a really bad design. Done(except the renaming part). On March 22, 2012, 9:46 p.m., Matěj Laitl wrote: src/amarokurls/BookmarkModel.cpp, line 571 http://git.reviewboard.kde.org/r/104307/diff/1/?file=53588#file53588line571 Please don't add DEBUG_BLOCKs and debug() for code that you are not actually debugging. (I know, it is in other methods here, you can take this as an opportunity to remove these, too) I don't completely understand the concept of DEBUG_BLOCKs. So I tried to structure the function like other similar functions. How do you determine that we are not debugging a piece of code, because there is debug() to output the messages ? And what all can be deleted here ? - Jasneet --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104307/#review11770 --- On March 22, 2012, 8:33 p.m., Jasneet Bhatti wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104307/ --- (Updated March 22, 2012, 8:33 p.m.) Review request for Amarok. Description --- This patch implements https://bugs.kde.org/show_bug.cgi?id=214721 . The bookmark is movable within the slider. If it is dragged outside the range, it will revert back to its previous valid location. The bookmark is activated( seek is called ) only when the bookmark is clicked and its position hasn't changed. In addition, also fixed a bug that caused deletion of the wrong bookmark when two bookmarks had the same name(possible by manual renaming), by making sure the location of the bookmark is appended to its name at all times. Diffs - src/amarokurls/BookmarkModel.cpp 9218088 src/amarokurls/AmarokUrl.h 6a1d67f src/amarokurls/AmarokUrl.cpp 19ba210 src/amarokurls/BookmarkModel.h 73ae345 src/widgets/BookmarkTriangle.h 46e9118 src/widgets/BookmarkTriangle.cpp 4c59d42 src/widgets/SliderWidget.cpp 5e72e13 Diff: http
Re: Review Request: Implemented Bug 214721 - Enable bookmark marker to be moved
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104307/ --- (Updated March 23, 2012, 10:50 p.m.) Review request for Amarok. Changes --- Updated diff and description. Description (updated) --- This patch implements https://bugs.kde.org/show_bug.cgi?id=214721 . The bookmark is movable within the slider. If it is dragged outside the range, it will revert back to its previous valid location. The bookmark is activated( seek is called ) only when the bookmark is clicked and its position hasn't changed. Diffs (updated) - src/context/applets/similarartists/ArtistWidget.cpp 18d8cc2 src/context/applets/upcomingevents/UpcomingEventsApplet.cpp 9664201 src/core-impl/collections/db/sql/CapabilityDelegateImpl.cpp dc5c500 src/amarokurls/PlayUrlGenerator.h 131b737 src/amarokurls/PlayUrlGenerator.cpp 90e71ff src/amarokurls/ContextUrlGenerator.cpp 16986f6 src/amarokurls/NavigationUrlGenerator.cpp d1e21e2 src/amarokurls/BookmarkModel.h 73ae345 src/amarokurls/BookmarkModel.cpp 9218088 src/amarokurls/AmarokUrl.cpp 19ba210 src/amarokurls/AmarokUrl.h 6a1d67f src/playlist/PlaylistViewUrlGenerator.cpp 0ffcb9f src/services/ServiceCapabilities.cpp 6129f8e src/widgets/BookmarkTriangle.h 46e9118 src/widgets/BookmarkTriangle.cpp 4c59d42 src/widgets/SliderWidget.cpp 5e72e13 Diff: http://git.reviewboard.kde.org/r/104307/diff/ Testing --- Tested it on ubuntu 11.04 with kubuntu-desktop. Works fine for me. Thanks, Jasneet Bhatti ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Implemented Bug 214721 - Enable bookmark marker to be moved
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104307/ --- (Updated March 24, 2012, 12:03 p.m.) Review request for Amarok. Changes --- Refined the patch : Made more sensible function calls, provided documentation, removed unnecessary statements. Description --- This patch implements https://bugs.kde.org/show_bug.cgi?id=214721 . The bookmark is movable within the slider. If it is dragged outside the range, it will revert back to its previous valid location. The bookmark is activated( seek is called ) only when the bookmark is clicked and its position hasn't changed. Diffs (updated) - src/amarokurls/AmarokUrl.h 6a1d67f src/amarokurls/AmarokUrl.cpp 19ba210 src/amarokurls/BookmarkModel.h 73ae345 src/amarokurls/BookmarkModel.cpp 9218088 src/amarokurls/ContextUrlGenerator.cpp 16986f6 src/amarokurls/NavigationUrlGenerator.cpp d1e21e2 src/amarokurls/PlayUrlGenerator.h 131b737 src/amarokurls/PlayUrlGenerator.cpp 90e71ff src/context/applets/similarartists/ArtistWidget.cpp 18d8cc2 src/context/applets/upcomingevents/UpcomingEventsApplet.cpp 9664201 src/core-impl/collections/db/sql/CapabilityDelegateImpl.cpp dc5c500 src/playlist/PlaylistViewUrlGenerator.cpp 0ffcb9f src/services/ServiceCapabilities.cpp 6129f8e src/widgets/BookmarkTriangle.h 46e9118 src/widgets/BookmarkTriangle.cpp 4c59d42 src/widgets/SliderWidget.cpp 5e72e13 Diff: http://git.reviewboard.kde.org/r/104307/diff/ Testing --- Tested it on ubuntu 11.04 with kubuntu-desktop. Works fine for me. Thanks, Jasneet Bhatti ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Implemented Bug 214721 - Enable bookmark marker to be moved
On March 22, 2012, 9:46 p.m., Matěj Laitl wrote: src/amarokurls/BookmarkModel.cpp, line 571 http://git.reviewboard.kde.org/r/104307/diff/1/?file=53588#file53588line571 Please don't add DEBUG_BLOCKs and debug() for code that you are not actually debugging. (I know, it is in other methods here, you can take this as an opportunity to remove these, too) Jasneet Bhatti wrote: I don't completely understand the concept of DEBUG_BLOCKs. So I tried to structure the function like other similar functions. How do you determine that we are not debugging a piece of code, because there is debug() to output the messages ? And what all can be deleted here ? Matěj Laitl wrote: I don't completely understand the concept of DEBUG_BLOCKs They are simple, they print BEGIN methodName() to Amarok's debugging output (amarok --debug) when method is entered and __END: methodName() when the block goes out of scope. How do you determine that we are not debugging a piece of code, because there is debug() to output the messages ? And what all can be deleted here ? When adding new code, you know whether you debug it or not. For existing code, I usually look at the time the line was last modified (git blame shows that). If it is a year or so, the debugging was probably just left out by mistake or laziness. (Ok, there are cases where debugging printouts should be keft forefer for tricky code, these should ba scarce though) Answering your question, probably all DEBUG_BLOCKs and debug() calls should be removed from BookmarkModel, but please don't do it in this patch, just don't add it to the new code. (because each commit should focus on just one thing) Done. On March 22, 2012, 9:46 p.m., Matěj Laitl wrote: src/widgets/BookmarkTriangle.h, line 37 http://git.reviewboard.kde.org/r/104307/diff/1/?file=53589#file53589line37 What if slider width changes during the bookmark lifetime? Also, please also document new (or better, even the old) parameters. Jasneet Bhatti wrote: I don't know of a case when the slider width changes. Does it happen while streaming live ? Matěj Laitl wrote: I thought about the case when Amarok window is resized during playback. (plase correct me if I'm wrong) The patch works fine even in that case. I believe on resizing the window, updateTimecodes() is called that takes care of the redrawing the bookmarks at the correct location. - Jasneet --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104307/#review11770 --- On March 23, 2012, 10:50 p.m., Jasneet Bhatti wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104307/ --- (Updated March 23, 2012, 10:50 p.m.) Review request for Amarok. Description --- This patch implements https://bugs.kde.org/show_bug.cgi?id=214721 . The bookmark is movable within the slider. If it is dragged outside the range, it will revert back to its previous valid location. The bookmark is activated( seek is called ) only when the bookmark is clicked and its position hasn't changed. Diffs - src/context/applets/similarartists/ArtistWidget.cpp 18d8cc2 src/context/applets/upcomingevents/UpcomingEventsApplet.cpp 9664201 src/core-impl/collections/db/sql/CapabilityDelegateImpl.cpp dc5c500 src/amarokurls/PlayUrlGenerator.h 131b737 src/amarokurls/PlayUrlGenerator.cpp 90e71ff src/amarokurls/ContextUrlGenerator.cpp 16986f6 src/amarokurls/NavigationUrlGenerator.cpp d1e21e2 src/amarokurls/BookmarkModel.h 73ae345 src/amarokurls/BookmarkModel.cpp 9218088 src/amarokurls/AmarokUrl.cpp 19ba210 src/amarokurls/AmarokUrl.h 6a1d67f src/playlist/PlaylistViewUrlGenerator.cpp 0ffcb9f src/services/ServiceCapabilities.cpp 6129f8e src/widgets/BookmarkTriangle.h 46e9118 src/widgets/BookmarkTriangle.cpp 4c59d42 src/widgets/SliderWidget.cpp 5e72e13 Diff: http://git.reviewboard.kde.org/r/104307/diff/ Testing --- Tested it on ubuntu 11.04 with kubuntu-desktop. Works fine for me. Thanks, Jasneet Bhatti ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Implemented Bug 214721 - Enable bookmark marker to be moved
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104307/ --- (Updated March 22, 2012, 8:33 p.m.) Review request for Amarok. Changes --- Updated the description Description (updated) --- This patch implements https://bugs.kde.org/show_bug.cgi?id=214721 . The bookmark is movable within the slider. If it is dragged outside the range, it will revert back to its previous valid location. The bookmark is activated( seek is called ) only when the bookmark is clicked and its position hasn't changed. In addition, also fixed a bug that caused deletion of the wrong bookmark when two bookmarks had the same name(possible by manual renaming), by making sure the location of the bookmark is appended to its name at all times. Diffs - src/amarokurls/BookmarkModel.cpp 9218088 src/amarokurls/AmarokUrl.h 6a1d67f src/amarokurls/AmarokUrl.cpp 19ba210 src/amarokurls/BookmarkModel.h 73ae345 src/widgets/BookmarkTriangle.h 46e9118 src/widgets/BookmarkTriangle.cpp 4c59d42 src/widgets/SliderWidget.cpp 5e72e13 Diff: http://git.reviewboard.kde.org/r/104307/diff/ Testing --- Tested it on ubuntu 11.04 with kubuntu-desktop. Works fine for me. Thanks, Jasneet Bhatti ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Implemented Bug 214721 - Enable bookmark marker to be moved
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104307/ --- (Updated March 17, 2012, 9:02 a.m.) Review request for Amarok. Description (updated) --- This patch implements https://bugs.kde.org/show_bug.cgi?id=214721 . The bookmark is movable within the slider. If it is dragged outside the range, it will revert to its previous valid location. The bookmark is activated( seek is called ) only when the bookmark's position hasn't changed. In addition, also fixed a bug that deleted a different bookmark that shared the same name(possible by manual renaming), by appending the location of the bookmark even in case of manual renaming. Diffs - src/amarokurls/BookmarkModel.cpp 9218088 src/amarokurls/AmarokUrl.h 6a1d67f src/amarokurls/AmarokUrl.cpp 19ba210 src/amarokurls/BookmarkModel.h 73ae345 src/widgets/BookmarkTriangle.h 46e9118 src/widgets/BookmarkTriangle.cpp 4c59d42 src/widgets/SliderWidget.cpp 5e72e13 Diff: http://git.reviewboard.kde.org/r/104307/diff/ Testing --- Tested it on ubuntu 11.04 with kubuntu-desktop. Works fine for me. Thanks, Jasneet Bhatti ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
GSoC : Implement MediaArtStorageSpec
I'm interested in working on Project: Implement MediaArtStorageSpec as a GSoC project. I've read the information on the links provided on the page https://bugs.kde.org/show_bug.cgi?id=296049 . What more do I need to study and how should I go about working on this ? ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Review Request: Implemented Bug 214721 - Enable bookmark marker to be moved
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104307/ --- Review request for Amarok. Description --- This patch implements https://bugs.kde.org/show_bug.cgi?id=214721. The bookmark is movable within the slider. If it is dragged outside the range, it will revert to its previous valid location. The bookmark is activated( seek is called ) only when the bookmark's position hasn't changed. In addition, also fixed a bug that deleted a different bookmark that shared the same name(possible by manual renaming), by appending the location of the bookmark even in case of manual renaming. Diffs - src/amarokurls/BookmarkModel.cpp 9218088 src/amarokurls/AmarokUrl.h 6a1d67f src/amarokurls/AmarokUrl.cpp 19ba210 src/amarokurls/BookmarkModel.h 73ae345 src/widgets/BookmarkTriangle.h 46e9118 src/widgets/BookmarkTriangle.cpp 4c59d42 src/widgets/SliderWidget.cpp 5e72e13 Diff: http://git.reviewboard.kde.org/r/104307/diff/ Testing --- Tested it on ubuntu 11.04 with kubuntu-desktop. Works fine for me. Thanks, Jasneet Bhatti ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Bug 173814 - JJ: add keyboard shortcut for Edit Track Information...
On Feb. 15, 2012, 8:08 a.m., Bart Cerneels wrote: Except for that one capital 'O' it's perfect and can be merged. Very good work Jasneet. I'd be more then happy to review more junior jobs from you. I was trying to post this round about the same time I uploaded the final patch but my internet connection went down and is back on only now. Thanks for the compliments and more so for the help and reviews. Everyone has been extremely supportive and encouraging, and I'm motivated to keep working and contributing to the community. On to the next one. - Jasneet --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103960/#review10650 --- On Feb. 15, 2012, 8:25 a.m., Jasneet Bhatti wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103960/ --- (Updated Feb. 15, 2012, 8:25 a.m.) Review request for Amarok. Description --- This patch fixes the bug : https://bugs.kde.org/show_bug.cgi?id=173814 I've created a new slot that is called when the key combination is pressed. This slot in turn calls the concerned function to display Edit Track Details dialog. Diffs - src/MainWindow.h 984aa28 src/MainWindow.cpp ea99659 src/playlist/PlaylistDock.h 897be1d src/playlist/PlaylistDock.cpp b217e3c Diff: http://git.reviewboard.kde.org/r/103960/diff/ Testing --- I've tested this on ubuntu 11.10 with kubuntu-desktop and it seems to work fine. Thanks, Jasneet Bhatti ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Bug 173814 - JJ: add keyboard shortcut for Edit Track Information...
On Feb. 13, 2012, 8:40 a.m., Matěj Laitl wrote: src/playlist/PlaylistDock.cpp, line 327 http://git.reviewboard.kde.org/r/103960/diff/1/?file=49412#file49412line327 Is there any specific reason this call is here? Jasneet Bhatti wrote: I was unable to completely understand it's role. It was apparently working fine without the call but I added it because it was there in all the other member function definitions, so I figured that it must be important. Bart Cerneels wrote: It's a hack. As far as I can tell it it not required in thus function, but you might want to test your shortcut at a moment when the view has not fully initialized yet (like during startup). Ok, I'm uploading the patch without that function. And I've tested my shortcut in all ways that I could imagine. It works just as good as any other shortcut. - Jasneet --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103960/#review10581 --- On Feb. 14, 2012, 5:05 a.m., Jasneet Bhatti wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103960/ --- (Updated Feb. 14, 2012, 5:05 a.m.) Review request for Amarok. Description --- This patch fixes the bug : https://bugs.kde.org/show_bug.cgi?id=173814 I've created a new slot that is called when the key combination is pressed. This slot in turn calls the concerned function to display Edit Track Details dialog. Diffs - src/MainWindow.h 984aa28 src/MainWindow.cpp ea99659 src/playlist/PlaylistDock.h 897be1d src/playlist/PlaylistDock.cpp b217e3c Diff: http://git.reviewboard.kde.org/r/103960/diff/ Testing --- I've tested this on ubuntu 11.10 with kubuntu-desktop and it seems to work fine. Thanks, Jasneet Bhatti ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Bug 173814 - JJ: add keyboard shortcut for Edit Track Information...
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103960/ --- (Updated Feb. 14, 2012, 5:05 a.m.) Review request for Amarok. Changes --- Changed the action description Description --- This patch fixes the bug : https://bugs.kde.org/show_bug.cgi?id=173814 I've created a new slot that is called when the key combination is pressed. This slot in turn calls the concerned function to display Edit Track Details dialog. Diffs (updated) - src/MainWindow.h 984aa28 src/MainWindow.cpp ea99659 src/playlist/PlaylistDock.h 897be1d src/playlist/PlaylistDock.cpp b217e3c Diff: http://git.reviewboard.kde.org/r/103960/diff/ Testing --- I've tested this on ubuntu 11.10 with kubuntu-desktop and it seems to work fine. Thanks, Jasneet Bhatti ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Review Request: Bug 173814 - JJ: add keyboard shortcut for Edit Track Information...
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103960/ --- Review request for Amarok. Description --- This patch fixes the bug : https://bugs.kde.org/show_bug.cgi?id=173814 I've created a new slot that is called when the key combination is pressed. This slot in turn calls the concerned function to display Edit Track Details dialog. Diffs - src/MainWindow.h 984aa28 src/MainWindow.cpp ea99659 src/playlist/PlaylistDock.h 897be1d src/playlist/PlaylistDock.cpp b217e3c Diff: http://git.reviewboard.kde.org/r/103960/diff/ Testing --- I've tested this on ubuntu 11.10 with kubuntu-desktop and it seems to work fine. Thanks, Jasneet Bhatti ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
