Review Request: Correctly read save last track played time to/from iPods
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101723/ --- Review request for Amarok. Summary --- This patch adds capability to store last play date/time onto iPods when copying files from collection. The read capability is also tweaked to detect invalid dates. Diffs - src/core-impl/collections/ipodcollection/handler/IpodHandler.cpp 216b200 Diff: http://git.reviewboard.kde.org/r/101723/diff Testing --- Works on iPod nano 2 gen and iPod Nano 5 gen. Thanks, Matěj ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Please join us on IRC and assingements of bugs
On 21. 10. 2011 Myriam Schweingruber wrote: Hi Matej, Hi Myriam and list! I have seen you are working on iPod problems in Amarok. That is really great, as we need help in this :) Welcome on board! Glad to work with such a wonderful team. :) Hello all Amarok devs. I would like to invite you to the #amarok.dev channel on irc.freenode.net. It is a more quiet channel for the Amarok team where we can discuss more easily. Also please do subscribe to amarok-devel@kde.org where all important information is found. Already subscribed to the list, but I didn't know about the channel, thanks. (I even searched for something similar with no luck) Just a side note: you should not reassign bugs to you, because that makes then disappear from the mailinglist we use for bugtracking. You can subscript to the bugs, that should be enough for your personal tracking. Ah, sorry about that, I should have asked first. A.d. personal tracking - do you mean adding myself to the CC list? I wanted to differentiate somehow between bugs I work on and these where I am part of the reporting side. One last question, what is the preferred way of letting my patches be reviewed once I have developer access? I have 5 small and unrelated iPod patches in my git repo - should I: a) create my amarok git clone on git.kde.org and push there (perhaps to a branch) b) create a branch (ipod-fixes) on git.kde.org:amarok.git and push there c) send them to amarok-dev using `git format-patch` (the Linux kernel way) d) upload them one-by-one to reviewboard (this one seems little inconvenient) I miss something like pull requests from github where a series of patches can be reviewed without squelching them into one. Regards, Matěj Laitl (aka strohel on IRC) ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Correctly read save last track played time to/from iPods
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101723/#review7528 --- Ship it! Ship It! - Matěj Laitl On June 22, 2011, noon, Matěj Laitl wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101723/ --- (Updated June 22, 2011, noon) Review request for Amarok. Description --- This patch adds capability to store last play date/time onto iPods when copying files from collection. The read capability is also tweaked to detect invalid dates. Diffs - src/core-impl/collections/ipodcollection/handler/IpodHandler.cpp 216b200 Diff: http://git.reviewboard.kde.org/r/101723/diff/diff Testing --- Works on iPod nano 2 gen and iPod Nano 5 gen. Thanks, Matěj Laitl ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Fix iphone 3G handling in amarok
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102864/#review7530 --- Ship it! The change looks innocent enough to me, sadly I cannot test (no iPhone). I can push it if you other Amarok devs agree. Manu, is there a bugreport for that 0 tracks issue? I haven't found it, could be please open a bugreport on bugs.kde.org and precisely describe the problem? Thanks. - Matěj Laitl On Oct. 14, 2011, 4:59 p.m., Manu Wagner wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102864/ --- (Updated Oct. 14, 2011, 4:59 p.m.) Review request for Amarok. Description --- Enable iphone 3G handling in amarok. Extend the previous fix delivered to ipod touch 3G to iphone 3G... Diffs - src/MediaDeviceCache.cpp 70868a6 Diff: http://git.reviewboard.kde.org/r/102864/diff/diff Testing --- iphone now visible in amarok and music properly transfered Please note that this doesn't fix the 0 track ipod/iphone bug, which is only solved through workaround by now (installing previous libmtp version) Screenshots --- iphone 3G now visible in amarok http://git.reviewboard.kde.org/r/102864/s/302/ Thanks, Manu Wagner ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Review Request: iPod handler: Correctly read save last track played time from/to iPods
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102932/ --- Review request for Amarok. Description --- This patch adds capability to store last play date/time onto iPods when copying files from collection. The read capability is also tweaked to detect invalid dates. This is version two where writing dates is fixed (and unneeded workaround removed) Diffs - src/core-impl/collections/ipodcollection/handler/IpodHandler.cpp 99c6209 Diff: http://git.reviewboard.kde.org/r/102932/diff/diff Testing --- Works on iPod nano 2 gen and iPod Nano 5 gen. Thanks, Matěj Laitl ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Review Request: Read and update BPM from/to media devices, including iPods
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102937/ --- Review request for Amarok. Description --- Read and update BPM from/to media devices, including iPods This used to be commented out when support was added long time ago back in 2009 because BPM support was lacking in Amarok these days. (this was commit bbed06ab0522cd) Diffs - src/core-impl/collections/mediadevicecollection/handler/MediaDeviceHandler.cpp c981dcccb650f08b42f201b8bca12345dc67036d Diff: http://git.reviewboard.kde.org/r/102937/diff/diff Testing --- Works with iPod nano 2G and iPod nano 4G. Thanks, Matěj Laitl ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Fix iphone 3G handling in amarok
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102864/#review7537 --- This change was pushed to git master: http://quickgit.kde.org/?p=amarok.gita=commith=c408df382f35a2ed4480a095735982dd6031b2e0 I just don't have (yet) permissions to close this request. Manu, I also forgot to mention you as patch author, sorry, should I fix that? - Matěj Laitl On Oct. 14, 2011, 4:59 p.m., Manu Wagner wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102864/ --- (Updated Oct. 14, 2011, 4:59 p.m.) Review request for Amarok. Description --- Enable iphone 3G handling in amarok. Extend the previous fix delivered to ipod touch 3G to iphone 3G... Diffs - src/MediaDeviceCache.cpp 70868a6 Diff: http://git.reviewboard.kde.org/r/102864/diff/diff Testing --- iphone now visible in amarok and music properly transfered Please note that this doesn't fix the 0 track ipod/iphone bug, which is only solved through workaround by now (installing previous libmtp version) Screenshots --- iphone 3G now visible in amarok http://git.reviewboard.kde.org/r/102864/s/302/ Thanks, Manu Wagner ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Add keyboard shortcut for collection search
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102956/#review7573 --- Looks okay to me, just a few minor things (see below). Silver, would you also mind correcting the Jump to action and methods a bit? I think both user-visible KAction name Jump to and method name slotJumpTo are uninformative, it should be something more specific such as Jump to playlist. (or Search playlist to be on par with the greyed out text in the input button and Search collection) If you want, add this to your patch. (you may also remove the DEBUG_BLOCK) src/MainWindow.h http://git.reviewboard.kde.org/r/102956/#comment6565 This perhaps deserves a doc comment that states it just focuses collection search bar, otherwise the name is a bit misleading. (it doesn't search anything it just focuses the bar) src/MainWindow.cpp http://git.reviewboard.kde.org/r/102956/#comment6566 Is the DEBUG_BLOCK necessary here? I think it would just pollute debug log. - Matěj Laitl On Oct. 23, 2011, 8:17 p.m., Silver Juurik wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102956/ --- (Updated Oct. 23, 2011, 8:17 p.m.) Review request for Amarok. Description --- The patch adds a keyboard shortcut for moving focus to collection search input box. The default shortcut is ctrl+F. The idea is from a JJ: https://bugs.kde.org/show_bug.cgi?id=257381 The keyboard shortcuts ctrl+J (an existing shortcut that moves focus to playlist search box) and ctrl+F partially solve the problem described in the bug. Diffs - src/MainWindow.h 076628f src/MainWindow.cpp 2d2ebac src/browsers/collectionbrowser/CollectionWidget.h a206914 src/browsers/collectionbrowser/CollectionWidget.cpp ace058a Diff: http://git.reviewboard.kde.org/r/102956/diff/diff Testing --- Thanks, Silver Juurik ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Review Request: iPod connection assistant: rework iPod identification
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102966/ --- Review request for Amarok, Christophe Giboudeaux and Bart Cerneels. Description --- iPod connection assistant: rework iPod identification This should fix remaining bugs where iPod is misidentified as a USB stick. Long comments are added that clarify interaction of varous code paths that deal with iPhone-like devices. BUG: 263288 REVIEW: TODO FIXED-IN: 2.5 Diffs - ChangeLog a99a4de75b1941ecdc7cb3b41cce9ad3c996df27 src/MediaDeviceCache.cpp 345837e34e5505c0900ee1a4e66ae9cf3deafa31 src/core-impl/collections/ipodcollection/support/IpodConnectionAssistant.cpp fecd07a8db2cb3eead184ec3a12198fe02124074 Diff: http://git.reviewboard.kde.org/r/102966/diff/diff Testing --- iPod nano 2G and iPod nano 4G are correctly identified. Thanks, Matěj Laitl ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Identifying iPod and iPhone-like devices using Solid (was: Amarok review request)
On 25. 10. 2011 Bart Cerneels wrote:
Looks like quite a few pretty ugly hacks are needed for proper iP* detection
with the current mediadevice codebase. In the near future (post 2.5
release) we should together try to find a simpler and easier to maintain
solution.
Thanks for your review, Bart.
I've looked around a bit and the situation seems rather sad:
* solid knows nothing about media-player-info or usbmuxd and its udev
PortableMediaPlayer implementation is just a stub [1], furthermore it doesn't
even currently attach PortableMediaPlayer interface to relevant devices as it
checks udev env ID_MEDIA_PLAYER against numeric 1 (gphoto remnant) [2,3], but
media-player-info attaches normalised player name there.
[1]
http://quickgit.kde.org/?p=kdelibs.gita=blobf=solid/solid/backends/udev/udevportablemediaplayer.cpp
[2]
http://quickgit.kde.org/?p=kdelibs.gita=blobf=solid/solid/backends/udev/udevdevice.cpp
[3]
http://quickgit.kde.org/?p=kdelibs.gita=blobf=solid/solid/backends/udev/udevmanager.cpp
In an ideal world (in my opinion):
* PortableMediaPlayer::supportedProtocols() would return a list containing
ipod for all iPod-like devices (it should just parse /usr/share/media-
player-info/${ID_MEDIA_PLAYER}.mpi)
* PortableMediaPlayer::supportedDrivers() would return a list containing
usb for traditional iPods and something like usbmuxd for iPhone-like
devices. (this would be more tricky, but could use udev env USBMUX_SUPPORTED)
Another question would be how to access iPhone-like devices the best way once
identified, but let's first solve identification.
Regards,
Matěj Laitl aka strohel
___
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel
Review Request: iPod handler: various cleanups (3 commits squashed)
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102964/ --- Review request for Amarok. Description --- iPod handler: raise libgpod dep to 0.7.93 and simplify iTunes db writing Even Debian stable has libgpod-0.7.93 packaged in main repo, so this shouldn't hurt and this version is much more convenient to work with. The change is logged in ChangeLog and README is also adjusted. -- iPod handler: cleanup slots that are called when worker threads are done There was a logic error - slot...Succeeded slots were called even when the thread did not succeed. (because they were connected to the done signals) This is fixed now. It seems that some actions (writing to debug() etc.) performed upon job failures were removed, but they were in fact already duplicated in slot...Succeeded methods. Stale orphaned needs much more work, this is just a start. -- iPod handler: remove unused attributes these attributes, enums and fwd-declarations aren't used, remove them. -- Diffs - CMakeLists.txt 1ffb60e072346b7af368adcfd2ac5e9daa6b4bf9 ChangeLog a99a4de75b1941ecdc7cb3b41cce9ad3c996df27 README 619bc56f238cbd132f0c5c0647058c17f16d0caa src/core-impl/collections/ipodcollection/CMakeLists.txt 1bf1aa49601d6eedfde741eaf7e232a81a9bd4bb src/core-impl/collections/ipodcollection/handler/IpodHandler.h 24ba45ad507b127d6bbb92ca367afb31f5e86460 src/core-impl/collections/ipodcollection/handler/IpodHandler.cpp 041eecf60d79fe0203dfb824186cbe96a9a696a5 Diff: http://git.reviewboard.kde.org/r/102964/diff/diff Testing --- Everything still works with iPod nano 2G and iPod nano 4G. It would be great if someone with iPod shuffle could test, as a part of code only relevant for shuffles is touched. Thanks, Matěj Laitl ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Find field names (filter keywords) even if they are translated
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103014/#review7858 --- src/core/meta/support/MetaConstants.cpp http://git.reviewboard.kde.org/r/103014/#comment6795 Looks like you've also changed firstplay - first? Seems both currently work in collection filter (because of a slightly different implementation in TextualQueryFilter.cpp), but I would stick with the old identifier. src/core/meta/support/MetaConstants.cpp http://git.reviewboard.kde.org/r/103014/#comment6796 lastplay ? last? src/core/meta/support/MetaConstants.cpp http://git.reviewboard.kde.org/r/103014/#comment6799 It seems that Meta::playlistNameForField() is only used in Dynamic::TagMatchBias::toXml(). Anyone knows why it cannot use Meta::nameForField()? If it can migrated, code duplication would --. - Matěj Laitl On Nov. 1, 2011, 3:09 p.m., Daniel Faust wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103014/ --- (Updated Nov. 1, 2011, 3:09 p.m.) Review request for Amarok. Description --- The filters (eg. filesize, artist) can be translated but without this patch components like the EditFilterDialog won't recognize the keywords. I haven't committed it yet because it also changes the keyword 'modifydate' to 'modified' since i think that that's a bug, too. Diffs - src/core/meta/support/MetaConstants.cpp fabb146 Diff: http://git.reviewboard.kde.org/r/103014/diff/diff Testing --- Thanks, Daniel Faust ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Add new collection filter to find exact matches
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102252/#review7864 --- src/core-impl/collections/support/TextualQueryFilter.cpp http://git.reviewboard.kde.org/r/102252/#comment6800 Daniel, when you're at it (and when http://git.reviewboard.kde.org/r/103014/ is here), could you please apply the TODO and call Meta::fieldForName() everythere instead of ad-hoc comparisons? Looks good to me, I also like the method signature changes. This patch also needs a soud entry in ChangeLog. - Matěj Laitl On Nov. 1, 2011, 3:02 p.m., Daniel Faust wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102252/ --- (Updated Nov. 1, 2011, 3:02 p.m.) Review request for Amarok. Description --- Since I use labels quite intensive it sometimes comes handy to search for songs with an exact label, not just a label containing the string I'm searching for. So I added a new filter type for this and used the '=' sign. Searching for label=pop finds songs labeled with 'pop' but not with 'electro pop'. While searching for label:pop finds songs labeled with 'pop' and 'electro pop'. This is not integrated into the GUI like the EditFilterDialog since I wanted to hear your opinions first. Diffs - src/core-impl/collections/support/Expression.h 1e49f45 src/core-impl/collections/support/Expression.cpp 531850a src/core-impl/collections/support/TextualQueryFilter.cpp 735772b src/dialogs/EditFilterDialog.cpp 91628ff src/widgets/MetaQueryWidget.h 3db49d3 src/widgets/MetaQueryWidget.cpp 5db1ded Diff: http://git.reviewboard.kde.org/r/102252/diff/diff Testing --- I'm using this patch for a few weeks now without any problems. Screenshots --- http://git.reviewboard.kde.org/r/102252/s/221/ Thanks, Daniel Faust ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Identifying iPod and iPhone-like devices using Solid (was: Amarok review request)
On 27. 10. 2011 Bart Cerneels wrote: On Wed, Oct 26, 2011 at 16:05, Matěj Laitl ma...@laitl.cz wrote: On 25. 10. 2011 Bart Cerneels wrote: Looks like quite a few pretty ugly hacks are needed for proper iP* detection with the current mediadevice codebase. In the near future (post 2.5 release) we should together try to find a simpler and easier to maintain solution. I've looked around a bit and the situation seems rather sad: * solid knows nothing about media-player-info or usbmuxd and its udev PortableMediaPlayer implementation is just a stub [1], furthermore it doesn't even currently attach PortableMediaPlayer interface to relevant devices as it checks udev env ID_MEDIA_PLAYER against numeric 1 (gphoto remnant) [2,3], but media-player-info attaches normalised player name there. Sounds to me like we need to get media-player-info implemented and deployed in Solid as fast as possible. But I'm not against implementing it completely in Amarok either, especially if that means it will also work seamless on windows. Thank you for your input, guys, in order not to just talk, there's a patch, a reborn of Alex Merry's dropped one: https://git.reviewboard.kde.org/r/103028/ This is the boring first part, the more interesting is the one that deals with hooking up udisks devices to underlying udev devices. I have something nearly-working, but it's an ugly hack. I will post that part later. Special thanks goes to Alex Merry who managed to report fix most of the upstream problems with ID_MEDIA_PLAYER. Looking forward for your reviews, Matěj Laitl ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Find field names (filter keywords) even if they are translated
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103014/#review7978 --- src/core-impl/collections/support/TextualQueryFilter.cpp http://git.reviewboard.kde.org/r/103014/#comment6860 Good! Now this looks much more nicer. But.. what about turing this into a switch..case now? This has additional benefit that compiler can warn about enum values not used in switch. I forgot what convention is used in Amarok for indenting switches, but it is in HACKING/conding_styel.. src/core/meta/support/MetaConstants.cpp http://git.reviewboard.kde.org/r/103014/#comment6863 You may perhaps want to wrap all these entries not to be that long, suggestion: else if( name.compare( filename, Qt::CaseInsensitive ) == 0 || name.compare( shortI18nForField( Meta::valUrl ), Qt::CaseInsensitive ) == 0 ) return Meta::valUrl; src/core/meta/support/MetaConstants.cpp http://git.reviewboard.kde.org/r/103014/#comment6861 Please accept also first as a special case - some users may have memorised it and now it would'n work for them. src/core/meta/support/MetaConstants.cpp http://git.reviewboard.kde.org/r/103014/#comment6862 Please also accept played as a special case to be backward-compatible. Good work, Daniel! Please fix remaining minor fix and I'll be happy to merge this. (or, do you have commit access yourself?) - Matěj Laitl On Nov. 6, 2011, 5:45 p.m., Daniel Faust wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103014/ --- (Updated Nov. 6, 2011, 5:45 p.m.) Review request for Amarok. Description --- The filters (eg. filesize, artist) can be translated but without this patch components like the EditFilterDialog won't recognize the keywords. I haven't committed it yet because it also changes the keyword 'modifydate' to 'modified' since i think that that's a bug, too. Diffs - src/core-impl/collections/support/TextualQueryFilter.cpp 2a44a30 src/core/meta/support/MetaConstants.cpp fabb146 Diff: http://git.reviewboard.kde.org/r/103014/diff/diff Testing --- Thanks, Daniel Faust ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Warn against build without taglib
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103090/#review8044 --- Ship it! Fine with me. (Anyone knows why taglib does'nt use macro_optional_log_feature?) - Matěj Laitl On Nov. 9, 2011, 4:51 a.m., Ryan McCoskrie wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103090/ --- (Updated Nov. 9, 2011, 4:51 a.m.) Review request for Amarok. Description --- Makes CMake print a stern warning against building Amarok without support of taglib if it is attempted. Diffs - CMakeLists.txt 6fb0491 Diff: http://git.reviewboard.kde.org/r/103090/diff/diff Testing --- Built Amarok with this patch applied. Thanks, Ryan McCoskrie ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
[amarok] /: Revert tag reading/writing support for mod, s3m, it and xm files
Git commit 62c6a63dbe4fe3db72f52b15a46e0c1ffcee79cf by Matěj Laitl.
Committed on 15/12/2011 at 00:43.
Pushed by laitl into branch 'master'.
Revert tag reading/writing support for mod, s3m, it and xm files
This reverts commit 3cae9ba4758c43092cd942a504ea43f5873960f4.
This was an erroneous push, Mathias Panzenböck wanted to push to his
own github repo, see his message in amarok-devel. :-)
CCMAIL: amarok-devel@kde.org
M +0-13 CMakeLists.txt
M +0-3config-amarok.h.cmake
M +1-5shared/FileType.cpp
M +1-5shared/FileType.h
M +0-35 shared/tag_helpers/TagHelper.cpp
http://commits.kde.org/amarok/62c6a63dbe4fe3db72f52b15a46e0c1ffcee79cf
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 70fb13c..cba1387 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -40,19 +40,6 @@ if(TAGLIB_FOUND)
message(FATAL_ERROR TagLib does not have MP4 support compiled in.)
endif( NOT TAGLIB_MP4_FOUND )
-check_cxx_source_compiles(#include modtag.h
-#include modfile.h
-#include s3mfile.h
-#include itfile.h
-#include xmfile.h
-int main() {
-TagLib::Mod::Tag tag;
-TagLib::Mod::File modfile(\/dev/null\);
-TagLib::S3M::File s3mfile(\/dev/null\);
-TagLib::IT::File itfile(\/dev/null\);
-TagLib::XM::File xmfile(\/dev/null\);
-return 0; } TAGLIB_MOD_FOUND)
-
set(CMAKE_REQUIRED_INCLUDES)
set(CMAKE_REQUIRED_LIBRARIES)
diff --git a/config-amarok.h.cmake b/config-amarok.h.cmake
index 6caaf5c..2d25cc7 100644
--- a/config-amarok.h.cmake
+++ b/config-amarok.h.cmake
@@ -30,9 +30,6 @@
/* have TagLib-Extras */
#cmakedefine TAGLIB_EXTRAS_FOUND 1
-/* have module file format support in TagLib */
-#cmakedefine TAGLIB_MOD_FOUND 1
-
/* have QtCrypto the Qt crypto architecture */
#cmakedefine QCA2_FOUND 1
diff --git a/shared/FileType.cpp b/shared/FileType.cpp
index 11dd727..a6e25d5 100644
--- a/shared/FileType.cpp
+++ b/shared/FileType.cpp
@@ -38,11 +38,7 @@ QStringList FileTypeSupport::s_fileTypeStrings =
QStringList()
QLatin1String( mpc )
QLatin1String( tta )
QLatin1String( wav )
- QLatin1String( wv )
- QLatin1String( mod )
- QLatin1String( s3m )
- QLatin1String( it )
- QLatin1String( xm );
+ QLatin1String( wv );
QString
diff --git a/shared/FileType.h b/shared/FileType.h
index 74e5417..5c8081f 100644
--- a/shared/FileType.h
+++ b/shared/FileType.h
@@ -38,11 +38,7 @@ enum FileType
Mpc = 7,
TrueAudio = 8,
Wav = 9,
-WavPack = 10,
-Mod = 11,
-S3M = 12,
-IT = 13,
-XM = 14
+WavPack = 10
};
diff --git a/shared/tag_helpers/TagHelper.cpp b/shared/tag_helpers/TagHelper.cpp
index 5839c30..96f763e 100644
--- a/shared/tag_helpers/TagHelper.cpp
+++ b/shared/tag_helpers/TagHelper.cpp
@@ -16,8 +16,6 @@
#include TagHelper.h
-#include config-amarok.h
-
#include QRegExp
#include QStringList
@@ -36,12 +34,6 @@
#include vorbisfile.h
#include wavfile.h
#include wavpackfile.h
-#ifdef TAGLIB_MOD_FOUND
-#include modfile.h
-#include s3mfile.h
-#include itfile.h
-#include xmfile.h
-#endif
#include APETagHelper.h
#include ASFTagHelper.h
@@ -333,33 +325,6 @@ Meta::Tag::selectHelper( const TagLib::FileRef fileref,
bool forceCreation )
else if( file-ID3v1Tag() )
tagHelper = new TagHelper( fileref.tag(), Amarok::WavPack );
}
-#ifdef TAGLIB_MOD_FOUND
-else if( TagLib::Mod::File *file = dynamic_cast TagLib::Mod::File * (
fileref.file() ) )
-{
-if( file-tag() )
-tagHelper = new TagHelper( fileref.tag(), Amarok::Mod );
-}
-else if( TagLib::S3M::File *file = dynamic_cast TagLib::S3M::File * (
fileref.file() ) )
-{
-if( file-tag() )
-tagHelper = new TagHelper( fileref.tag(), Amarok::S3M );
-}
-else if( TagLib::IT::File *file = dynamic_cast TagLib::IT::File * (
fileref.file() ) )
-{
-if( file-tag() )
-tagHelper = new TagHelper( fileref.tag(), Amarok::IT );
-}
-else if( TagLib::XM::File *file = dynamic_cast TagLib::XM::File * (
fileref.file() ) )
-{
-if( file-tag() )
-tagHelper = new TagHelper( fileref.tag(), Amarok::XM );
-}
-#endif
-else if( !fileref.isNull() )
-{
-if( fileref.tag() )
-tagHelper = new TagHelper( fileref.tag(), Amarok::Unknown );
-}
return tagHelper;
}
___
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel
Re: 2.5.0 tarball problems
On 17. 12. 2011 Modestas Vainius wrote: current 2.5.0 tarball does not build. The problem is in the doc/nl (could be related to kdelibs 4.6.5): cd ../doc/nl /usr/bin/meinproc4 --check --cache /«PKGBUILDDIR»/obj-x86_64- linux-gnu/doc/nl/index.cache.bz2 /«PKGBUILDDIR»/doc/nl/index.docbook Generating moc_BiasSolver.cpp index.docbook:21: parser error : Entity 'Thom.Castermans' not defined index.docbook:13966: parser error : Entity 'vertaling.thom' not defined We have found the root of this problem: 1. amarok-2.5.0/doc/nl/index.docbook references entity Thom.Castermans 2. meinproc4 that generates HTML from docbook during Amarok build uses /usr/share/apps/ksgmltools2/customization/nl/user.entities to load such entities (among other files) through: * ksgmltools2/customization/catalog.xml - - ksgmltools2/customization/nl/catalog.xml - - ksgmltools2/customization/nl/ 3. ksgmltools2/customization/nl/user.entities is from kdelibs, and Thom had no entry there in kdelibs 4.6 as he was added to kdelibs Which is rather unfortunate (and silly as we didn't spot it before spreading tarballs) as it effectively breaks Amarok build on KDE 4.6 which we support. The simplest workaround is probably to remove all undefined references from doc/nl/index.docbook at the cost of not attributing Thom as a docbook translator in your build. What is more, I see mysql-config error at the cmake stage even if it does not break amarok build actually (probably because include directory is in the cflags). Attached patch fixes the problem. Indeed, thanks. We'll apply the patch into master, but it isn't probably the reason to respin the tarballs. (let's see what other Amarok devs think) Regards, Matěj Laitl ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: [amarok] cmake/modules: Revert FindMySQLAmarok.cmake fix argumets when calling mysql_config
On 18. 12. 2011 Christophe Giboudeaux wrote: Revert FindMySQLAmarok.cmake fix argumets when calling mysql_config This reverts a fix for -Wmissing-include-dirs warnings. MYSQL_INCLUDE_DIR must be a path and not contain -Ipath Sorry about the incorrect fix, I will build with -Wmissing-include-dirs from now on. However I wonder how to fix the original problem. Would it be legal to remove MYSQL_INCLUDE_DIR entirely and just rely on MYSQL_CFLAGS to specify -I/path/to/mysql ? E.g. is cmake's include_directories(/dir) just a syntactic sugar for add_definitions(-I/dir) or it would break build on non-gcc platforms? I'd rather avoid filtering -I out of MYSQL_INCLUDE_DIR in CMakeLists. Regards, Matěj ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: [amarok] cmake/modules: Revert FindMySQLAmarok.cmake fix argumets when calling mysql_config
On 19. 12. 2011 Christophe Giboudeaux wrote: On Sunday 18 December 2011 23:27:08 Matej Laitl wrote: Sorry about the incorrect fix, I will build with -Wmissing-include-dirs from now on. However I wonder how to fix the original problem. Your commit doesn't mention the issue you're trying to fix. However, having the same directory included several times is not an issue (and I'm not aware of any GCC flag that would throw a warning). Ah, I tried to solve http://mail.kde.org/pipermail/amarok-devel/2011- December/009663.html Specifically, the problem is that mysql_config (since at least 5.1.56) doesn't accept --variable=pkgincludedir - it accepts only --include argument which returns something like -I/usr/include/mysql -- Found MySQL: Usage: /usr/bin/mysql_config [OPTIONS] Options: --cflags [-I/usr/include/mysql -DHAVE_ERRNO_AS_DEFINE=1 - DUNIV_LINUX -DUNIV_LINUX] --include[-I/usr/include/mysql] --libs [-Wl,-O1 -Wl,--as-needed -rdynamic -L/usr/lib64/mysql -lmysqlclient -L/usr//lib64 -lz -lcrypt -lnsl -lm -L/usr/lib64/ -lssl - lcrypto] --libs_r [-Wl,-O1 -Wl,--as-needed -rdynamic -L/usr/lib64/mysql -lmysqlclient_r -L/usr//lib64 -lz -lpthread -lcrypt -lnsl -lm -lpthread - L/usr/lib64/ -lssl -lcrypto] --plugindir [/usr/lib64/mysql/plugin] --socket [/var/run/mysqld/mysqld.sock] --port [0] --version[5.1.56] --libmysqld-libs [-Wl,-O1 -Wl,--as-needed -rdynamic -L/usr/lib64/mysql -lmysqld -ldl -L/usr//lib64 -lz -lpthread -lcrypt -lnsl -lm -lpthread -lrt - L/usr/lib64/ -lssl -lcrypto], -Wl,-O1 -Wl,--as-needed -rdynamic - L/usr/lib64/mysql -lmysqlclient -L/usr//lib64 -lz -lcrypt -lnsl -lm - L/usr/lib64/ -lssl -lcrypto (they you get even longer and *nicer* -Wmissing-include-dirs warnings) Would it be legal to remove MYSQL_INCLUDE_DIR entirely and just rely on MYSQL_CFLAGS to specify -I/path/to/mysql ? Well, they don't have the same purpose. MYSQL_CFLAGS is used by the compiler (used in 'add_definitions..' lines) while MYSQL_INCLUDE_DIR is used by cmake. ...used by cmake (just?) to add -I/path to compiler arguments? Also, if mysql_config is not available, MYSQL_CFLAGS is empty and you can only count on MYSQL_INCLUDE_DIR. Ah, I see. E.g. is cmake's include_directories(/dir) just a syntactic sugar for add_definitions(-I/dir) or it would break build on non-gcc platforms? I'd rather avoid filtering -I out of MYSQL_INCLUDE_DIR in CMakeLists. Without hint about what you're trying to fix, it's hard to answer this question. So I guess we'll have to strip that -I out of `mysql_config --include`. Can we safely assume that it is only a single directory? Regards, Matěj ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Fix for USB storage mode media players
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/100194/#review9081 --- src/core-impl/collections/ipodcollection/support/IpodConnectionAssistant.cpp http://git.reviewboard.kde.org/r/100194/#comment7510 This would break identification of iPods. KDE's solid with udisks udev backend currently doesn't attach PMP interface to them. - Matěj Laitl On Dec. 8, 2010, 9:55 p.m., Lukáš Tinkl wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/100194/ --- (Updated Dec. 8, 2010, 9:55 p.m.) Review request for Amarok. Description --- This patch fixes identifying general USB storage mode media players, plus adds some minor fixes and cleanups. The main change is in UmsConnectionAssistant::identify method. Diffs - src/MediaDeviceCache.cpp babb8ff src/core-impl/collections/ipodcollection/support/IpodConnectionAssistant.cpp 92339ff src/core-impl/collections/umscollection/support/UmsConnectionAssistant.cpp 5956a2b Diff: http://git.reviewboard.kde.org/r/100194/diff/diff Testing --- Screenshots --- Amarok in USB mode http://git.reviewboard.kde.org/r/100194/s/21/ Thanks, Lukáš Tinkl ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: [amarok] cmake/modules: Revert FindMySQLAmarok.cmake fix argumets when calling mysql_config
On 19. 12. 2011 Christophe Giboudeaux wrote: I see two solutions: - only rely on find_path(MYSQL_INCLUDE_DIR...), so move it outside the else(MYSQLCONFIG_EXECUTABLE) loop [1] - Drop FindMysqlAmarok.cmake and use FindAmarok.cmake from kdelibs instead (which worked fine here). I prefer the second solution as the FindMysql from kdelibs is really cleaner than the Amarok copy: - MYSQL_CFLAGS doesn't look that important - the linked targets gathered with 'mysql_config --libs' are already explicitly added in the CMakeLists.txt files (./src/core- impl/collections/db/sql/mysqlecollection/CMakeLists.txt, ./src/core- impl/collections/db/sql/mysqlservercollection/CMakeLists.txt and some unit tests) and the output variable is different if mysql_config is not present: * with mysql_config, MYSQL_LIBRARIES=-L/usr/lib64 -lmysqlclient -lpthread - lz -lm -lrt -lssl -lcrypto -ldl * without it, MYSQL_LIBRARIES=/usr/lib64/libmysqlclient.so. The second one is enough - the kdelibs FindAmarok has better chances to detect mysql.h under Windows, - and it also uses a cleaner way to decide if mysqle is there. I'd also like to drop FindMysqlAmarok.cmake entirely and use kdelibs one, code duplication is never good. Let's do it early in the 2.6 phase to get the most testing. Anyone opposed to me dropping FindMysqlAmarok.cmake in master for now? (can always be reverted if problems arise) Matěj Laitl ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
[amarok/strohel-for-2.6] src: Get rid of UpdateCapability entirely - it was no longer used
Git commit 2e73dd379d51a5df37e996cd392b98fd66191daf by Matěj Laitl.
Committed on 10/12/2011 at 17:51.
Pushed by laitl into branch 'strohel-for-2.6'.
Get rid of UpdateCapability entirely - it was no longer used
UpdateCapability was deprecated and thanks to previous commits, all
UpdateCapability related code was dead. Remove it completely.
CCMAIL: amarok-devel@kde.org
M +0-1src/context/applets/labels/LabelsApplet.cpp
M +0-21 src/core-impl/collections/db/sql/CapabilityDelegateImpl.cpp
M +0-24
src/core-impl/collections/mediadevicecollection/MediaDeviceMeta.cpp
M +0-1src/core/CMakeLists.txt
M +1-1src/core/capabilities/Capability.h
D +0-23 src/core/capabilities/UpdateCapability.cpp
D +0-46 src/core/capabilities/UpdateCapability.h
http://commits.kde.org/amarok/2e73dd379d51a5df37e996cd392b98fd66191daf
diff --git a/src/context/applets/labels/LabelsApplet.cpp
b/src/context/applets/labels/LabelsApplet.cpp
index 5b58def..8708f0c 100644
--- a/src/context/applets/labels/LabelsApplet.cpp
+++ b/src/context/applets/labels/LabelsApplet.cpp
@@ -24,7 +24,6 @@
#include App.h
#include amarokurls/AmarokUrl.h
#include context/widgets/AppletHeader.h
-#include core/capabilities/UpdateCapability.h
#include core/support/Debug.h
#include EngineController.h
#include PaletteHandler.h
diff --git a/src/core-impl/collections/db/sql/CapabilityDelegateImpl.cpp
b/src/core-impl/collections/db/sql/CapabilityDelegateImpl.cpp
index 4f8df54..2698f20 100644
--- a/src/core-impl/collections/db/sql/CapabilityDelegateImpl.cpp
+++ b/src/core-impl/collections/db/sql/CapabilityDelegateImpl.cpp
@@ -31,7 +31,6 @@
#include core/capabilities/FindInSourceCapability.h
#include core/capabilities/StatisticsCapability.h
#include core/capabilities/OrganiseCapability.h
-#include core/capabilities/UpdateCapability.h
#include core/collections/support/SqlStorage.h
#include core/meta/support/MetaConstants.h
#include core-impl/capabilities/timecode/TimecodeLoadCapability.h
@@ -164,21 +163,6 @@ class OrganiseCapabilityImpl : public
Capabilities::OrganiseCapability
KSharedPtrMeta::SqlTrack m_track;
};
-class UpdateCapabilityImpl : public Capabilities::UpdateCapability
-{
-Q_OBJECT
-public:
-UpdateCapabilityImpl( Meta::SqlTrack *track )
-: Capabilities::UpdateCapability()
-, m_track( track ) {}
-
-virtual void collectionUpdated() const {
m_track-collection()-collectionUpdated(); }
-
-
-private:
-KSharedPtrMeta::SqlTrack m_track;
-};
-
class TimecodeWriteCapabilityImpl : public
Capabilities::TimecodeWriteCapability
{
Q_OBJECT
@@ -296,7 +280,6 @@ TrackCapabilityDelegateImpl::hasCapabilityInterface(
Capabilities::Capability::T
case Capabilities::Capability::Actions:
case Capabilities::Capability::Importable:
case Capabilities::Capability::Organisable:
-case Capabilities::Capability::Updatable:
case Capabilities::Capability::BookmarkThis:
case Capabilities::Capability::WriteTimecode:
case Capabilities::Capability::LoadTimecode:
@@ -343,10 +326,6 @@ TrackCapabilityDelegateImpl::createCapabilityInterface(
Capabilities::Capability
case Capabilities::Capability::Organisable:
return new OrganiseCapabilityImpl( track );
-
-case Capabilities::Capability::Updatable:
-return new UpdateCapabilityImpl( track );
-
case Capabilities::Capability::BookmarkThis:
return new Capabilities::BookmarkThisCapability( new
BookmarkCurrentTrackPositionAction( 0 ) );
case Capabilities::Capability::WriteTimecode:
diff --git
a/src/core-impl/collections/mediadevicecollection/MediaDeviceMeta.cpp
b/src/core-impl/collections/mediadevicecollection/MediaDeviceMeta.cpp
index 2811d2b..242872f 100644
--- a/src/core-impl/collections/mediadevicecollection/MediaDeviceMeta.cpp
+++ b/src/core-impl/collections/mediadevicecollection/MediaDeviceMeta.cpp
@@ -25,7 +25,6 @@
#include SvgHandler.h
#include core/capabilities/ActionsCapability.h
#include core/capabilities/EditCapability.h
-#include core/capabilities/UpdateCapability.h
#include KIcon
#include KUrl
@@ -63,25 +62,6 @@ class EditCapabilityMediaDevice : public
Capabilities::EditCapability
KSharedPtrMediaDeviceTrack m_track;
};
-class UpdateCapabilityMediaDevice : public Capabilities::UpdateCapability
-{
-Q_OBJECT
-public:
-UpdateCapabilityMediaDevice( Collections::MediaDeviceCollection *coll )
-: Capabilities::UpdateCapability()
-, m_coll( coll )
-{}
-
-virtual void collectionUpdated() const
-{
-m_coll-collectionUpdated();
-m_coll-writeDatabase();
-}
-
-private:
-Collections::MediaDeviceCollection *m_coll;
-};
-
MediaDeviceTrack::MediaDeviceTrack( Collections::MediaDeviceCollection
*collection )
: Meta::Track()
@@ -402,8 +382,6
Request for review: my patch queue for 2.6
Hi Amarockers, already annouced on IRC, repeating here: please review strohel-for-2.6 branch [1] that I've pushed to Amarok git repo. [1] http://quickgit.kde.org/?p=amarok.gita=shortlogh=refs/heads/strohel- for-2.6 Most of the commits touch just media devices, but some of them affect Amarok as a whole, notably: * playlist: show also albumArtist, BPM, labels, last… * UpdateCapability removal that Bart already commented here on the ML * media device collection: more consistent handling of compilations (moves some code around) * Collection browser: show absolute free space on hover, thicker bar I would be happy to incorporate your comments and suggestions (please comment by mail, my IRC attendance is irregular) and, if no-one opposes I'd like to merge it to master eventually. Regards, Matěj Laitl ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Amarok out of string freeze, let's plan features
So what features do you have planned or would strongly suggest to make it into 2.6? Ipod collection rewrite: do not use MediaDevice friends, use MemoryMeta. The plan is: * no regressions over current implementation * working playlists * track transferring that does not suck If time permits: * podcast support for iPods * fancy track transerring usability improvements (+ support for transcoding) * last.fm scrobbling from iPod * stats synchronization (I have working prototype, needs heavy refactoring) Matěj ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Request for review: my patch queue for 2.6
On 20. 12. 2011 Bart Cerneels wrote: Just make sure you squash the commits that belong together :) Do you mean some specific commits? E.g. there are 3 UpdateCapability removal- related commits, but each one stands on its own (with commit message describing the specific change) and everything is perfectly bisectable. (I must say I'm not a big fan of squashing related changes - I instead prefer the Linux-kernel way of thinking with git: make very granular commits so that it is `git bisect` who finds erroneous changes, not you) And don't forget to Changelog in the same commit! I guess you're alluding to my past commits. I've improved, I swear! :) So it seems nobody opposes merging the patch-set, so I'll do it once I find another bit of time in this holiday time.. Matěj ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: 2.5.0 tarball problems
On 27. 12. 2011 Myriam Schweingruber wrote: We did not release a broken tarball at all. Just because your definitions are not up-to-date doesn't mean it is Amarok's fault. This is not entirely true, Myriam. The definitions are part of kdelibs and we claim to support kdelibs 4.6, so we in fact released a partially broken tarball. The thing is that handbook translations were AFAIK added only to the final tarball (not the beta) and as Bart said, no Amarok developer uses obsolete kdelibs, so it slipped through. Modestas, you warned us about this problem, thank you for that, we would normally re-spin the tarball, but Amarok 2.5 was already very delayed and we wanted to make it before Christmas, so we decided to release a tarball that is broken is some special cases, with patches (thanks to you) available in the amarok mailing list. I hope this little explanation helps not to discourage you from packaging Amarok for Debian. Regards, Matěj Laitl ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Some changes to make Amarok appearance more pretty
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103603/#review9423 --- Lucas, thanks for your review request. This is a rather bigger change, could you describe what exactly this does and perhaps provide screen-shots that show all the changes? - Matěj Laitl On Jan. 2, 2012, 1:29 a.m., Lucas Gomes wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103603/ --- (Updated Jan. 2, 2012, 1:29 a.m.) Review request for Amarok and Bart Cerneels. Description --- This is my attempt to make QTreeView subclasses items, used in Amarok, more pretty by displaying some extra information. Notice that these extra information are usually the quantity of tracks in a album, the quantity of episodes in a podcast and the quantity of episodes marked as new in a podcast. So, please help me to improve this feature even more by answering some questions: 1) Should I show the quantity of tracks on playlists listed in PlaylistBrowser too? 2) Is there any extra information that you think it's relevant to be showed somewhere (In QTreeViews)? Link for my personal repository (Look for ui-improve branch): http://quickgit.kde.org/index.php?p=clones%2Famarok%2Fgomes%2Fmaskmaster-amarok.gita=summary Diffs - src/CMakeLists.txt 7c2335d src/browsers/AbstractTreeViewDelegate.h PRE-CREATION src/browsers/AbstractTreeViewDelegate.cpp PRE-CREATION src/browsers/CollectionTreeItem.h 3438adc src/browsers/CollectionTreeItemModelBase.cpp e7f8e62 src/browsers/collectionbrowser/CollectionBrowserTreeView.cpp 35a8222 src/browsers/collectionbrowser/CollectionMergedTreeItemDelegate.h PRE-CREATION src/browsers/collectionbrowser/CollectionMergedTreeItemDelegate.cpp PRE-CREATION src/browsers/collectionbrowser/CollectionNormalTreeItemDelegate.h PRE-CREATION src/browsers/collectionbrowser/CollectionNormalTreeItemDelegate.cpp PRE-CREATION src/browsers/collectionbrowser/CollectionTreeItemDelegate.h 8a189e6 src/browsers/collectionbrowser/CollectionTreeItemDelegate.cpp 755be00 src/browsers/collectionbrowser/CollectionWidget.cpp ac1c26d src/browsers/playlistbrowser/PlaylistBrowserCategory.h 9198d43 src/browsers/playlistbrowser/PlaylistBrowserCategory.cpp 0c2f9c1 src/browsers/playlistbrowser/PlaylistBrowserView.cpp 9c4236d src/browsers/playlistbrowser/PlaylistMergedTreeItemDelegate.h PRE-CREATION src/browsers/playlistbrowser/PlaylistMergedTreeItemDelegate.cpp PRE-CREATION src/browsers/playlistbrowser/PlaylistNormalTreeItemDelegate.h PRE-CREATION src/browsers/playlistbrowser/PlaylistNormalTreeItemDelegate.cpp PRE-CREATION src/browsers/playlistbrowser/PlaylistTreeItemDelegate.h 3a094b0 src/browsers/playlistbrowser/PlaylistTreeItemDelegate.cpp bc76551 src/browsers/playlistbrowser/PlaylistsByProviderProxy.h 941268c src/browsers/playlistbrowser/PlaylistsByProviderProxy.cpp 12f2676 src/browsers/playlistbrowser/PlaylistsInFoldersProxy.h 9a01dbe src/browsers/playlistbrowser/PlaylistsInFoldersProxy.cpp 4268a82 src/browsers/playlistbrowser/PodcastCategory.cpp 1c353dc src/browsers/playlistbrowser/PodcastMergedTreeItemDelegate.h PRE-CREATION src/browsers/playlistbrowser/PodcastMergedTreeItemDelegate.cpp PRE-CREATION src/browsers/playlistbrowser/PodcastModel.h e88f4a1 src/browsers/playlistbrowser/PodcastModel.cpp 18334f6 src/browsers/playlistbrowser/PodcastNormalTreeItemDelegate.h PRE-CREATION src/browsers/playlistbrowser/PodcastNormalTreeItemDelegate.cpp PRE-CREATION src/browsers/playlistbrowser/UserPlaylistCategory.cpp b48a55f src/core-impl/podcasts/sql/SqlPodcastMeta.h 42ad039 src/core-impl/podcasts/sql/SqlPodcastMeta.cpp 1c3bdf4 src/core/podcasts/PodcastMeta.h 679f7ac src/core/podcasts/PodcastMeta.cpp b9851f7 src/widgets/TrackSelectWidget.cpp 5bd5059 Diff: http://git.reviewboard.kde.org/r/103603/diff/diff Testing --- This patch should build. Everything is working as expected and there aren't any known issues. Thanks, Lucas Gomes ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: 2.5.0 tarball problems
On 3. 1. 2012 Sven Krohlas wrote: Not enough information to check signature validity. Show Details Heya, has that patch already been committed to master? It should be part of 2.5.1. Sven, doc/nl/index.docbook is not a part of amarok master, it is added to amarok releases by release scripts. Additionally, I think this patch breaks build with KDE 4.7 and later because of entity redefinition. Matěj ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Review request: MemoryMeta changes and new iPod collection
Hi list and mainly Bart,
please review memorymeta-tweaks branch [1] in my personal git clone which
contains changes that I needed for iPod collection rewrite.
[1]
http://quickgit.kde.org/?p=clones%2Famarok%2Flaitl%2Famarok.gita=shortlogh=refs/heads/memorymeta-
tweaks
UmsCollection would get support for following for free is memorymeta-tweaks is
merged:
* album artists (was hard-coded to track artists previously)
* less memory leaks due to circular referencing removal
* capability forwarding: editing tags thanks to EditCapability.
ChangeLog is added for above entries.
With a small bit of work, UmsCollection could get support for:
* album covers if MetaFile::FileAlbum::{image(),hasImage()} is implemented
* compilations if MetaFile::FileAlbum::isCompilation() is implemented (see
ArtistHelper::bestGuessAlbumArtist())
* track removing reflected in collection browser, call
MapChanger::removeTrack() perhaps somewhere in
UmsCollectionLocation::removeUrlsFromCollection()
* track metadata changes reflected in collection browser if you observe tracks
in UmsCollection and then call MapChanger::trackChanged() in
metadataChanged(). You should emit updated() when trackChanged() returns true.
Beware that it cannot currently cope with changes to track uidUrl().
ChangeLog is not updated with above entries as the changes are not user-
visible anywhere in Amarok yet.
All of the MemoryMeta changes are already used tested in iPod collection
rewrite that can be found in the ipod-rewrite branch [2], which is nearly
merge-ready.
[2]
http://quickgit.kde.org/?p=clones%2Famarok%2Flaitl%2Famarok.gita=shortlogh=refs/heads/ipod-
rewrite
Regards,
Matěj Laitl
___
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel
Re: How to get the playableUrl only when the track is about to be played?
On 13. 1. 2012 Dirley wrote: Hello, there. Hi Dirley, I've been hacking into Amarok, trying to add an internet service. Turns out that my service is a little bit... uncommon. When the tracks come from the service, they don't have a playable url. I have to ask the service a url for each track I want to play. How would I do that on Amarok? I've considered using the MultiSourceCapability, but it didnt work, since the first url is always invalid, so the engine skips the song. The MultiPlayableCapability almost works, since the engine do wait for the first source to be fetched. So I'm missing something? Is it feasible or should I give up and run a http server inside my service that redirects to the playable url? I think you can inherit Meta::Track directly and just implement virtual void prepareToPlay() Engine controller apparently calls it when it is about to play a song and it is called nowhere else in Amarok AFAICS. I would return empty playableUrl() and false in isPlayable() before prepareToPlay() is called. Regards, Matěj ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: KIO-MTP development
On 13. 1. 2012 Todd wrote: Philipp Schmidt philschm...@gmx.net wrote: this week I started developing a KIO-Slave for MTP since I now have a Galaxy Nexus and like accessing it via Dolphin and not having to use mtpfs. You can find it on github: So far it can do very little (Basically listing devices, storages and files works, if the right kio-instance gets the cache), but I am working on that. Since concurrent access to the devices using libmtp is buggy at best I would like to introduce a persistent cache that gets updated when changing operations occur or devices get plugged in/removed. Is there any preferred way in KDE to do that, or an infrastructure already in place? Also, if you have any other ideas or things to say (for example regarding the coding style), please do ;). My long term goal is to get this integrated into KDELibs, together with a Solid-Component that detects MTP-Devices and shows them in Dolphin etc. Help is also appreciated :). You might also want to forward this to the dolphin and amarok mailing lists, since they may want to be able to use this kio slave for their own mtp access. Having a single mtp access tool would probably be nice. Philip, MTP kioslave whould be certainly beneficial for Amarok too, as its current MTP- collection is pretty much unmaintained. Would copying music files in Dolphin to a MTP device using your kioslave suffice for the device to find it (and display its metadata)? Is so, Amarok's USB Mass Storage collection shouldn't be hard to extend to handle such devices. If it wouldn't be that simple, we should find a way how to set MTP-compatible metadata for transferred songs. (see KIO::Job::metaData()) Philip, also please take look at https://git.reviewboard.kde.org/r/103028/ which deals with MTP device detection in Solid. Regards, Matěj ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: How to get the playableUrl only when the track is about to be played?
On 14. 1. 2012 Dirley wrote: Ok, I tried that, but it didnt work. Right after prepareToPlay, the EngineController calls `play(track.playableUrl())`. Since the playable url is being retrieved, the track playableUrl is still empty. So Amarok stops the playback. I need a way to make the engine wait for the url to be retrieved, like in MultiPlayback: I see. I thought you would block in prepareToPlay(), but it wouldn't be nice, anyway. So I think MultiPlayback capability is the right workaround for you currently. (make sure you comment its use properly) Matěj ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Review Request: MetaFile: guess album artist as in SQL collection scanner, MemoryCollection: use also album artist as identifying key in album map (2 commits squelched just for reviewboard)
/TestUnionJob.cpp 591d695d1ea0641951e5aa7188a697f98b988b5c Diff: http://git.reviewboard.kde.org/r/103715/diff/diff Testing --- This change is tested to work with new iPod collection, UMS collection, all tests still pass. Only area that received just very basic testing are online service collections that are also affected by this change. Screenshots --- amarok-ums-collections-and-album-artists.png http://git.reviewboard.kde.org/r/103715/s/416/ Thanks, Matěj Laitl ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Amarok's iPod Collection: Volunteering to help
On 17. 1. 2012 Santiago G. wrote: Hi. My name is Santiago, I'm the poster of this bug request: https://bugs.kde.org/show_bug.cgi?id=291722 Hi Santiago! Today I have been fighting all afternoon to be able to transfer music from my music collection to my iPod touch. So far I couldn't not find a complete solution, and that annoys me. Even using Apple's iTunes (running in a virtual machine) I have the issue of not being able to transcode files as I would like. Being an Amarok lover, and knowing that what you are working on will be awesome: making the whole process simple and easy (as it should be in the first place), I wanted to volunteer to help you in any way you want. My coding skills aren't that great yet, but I can help you with testing, documentation, translation, or whatever you may need. That would be great, I do have older iPod nanos to test, but devices with iOS are accessed in a slightly different way so I would certainly use a lot of testing from you. For start, I'd like you to compile amarok from sources, start using that version (Version 2.5-GIT must appear in Amarok about dialog) and verify it works at least as good (wrt iPod support) as Amarok 2.5 from kubuntu. [1] should definitely help you doing so, but please use my git clone git://anongit.kde.org/clones/amarok/laitl/amarok.git instead of official git://anongit.kde.org/amarok.git [1] http://amarok.kde.org/wiki/2.0_Development_HowTo I have posted the output of the command you told me, and if you want I could get you the data for an iPad 2. Yup, that was already useful for me, I'll let you know when I'll need more. Also, if you want to get in touch in a more direct way---IRC, MSN Messenger, etc.---feel free to tell me, I have free time I could spend. Good, my MSN is stro...@hotmail.com and Jabber stro...@gmail.com but I generally prefer e-mails because I can handle them in batches, but don't hesitate to contact me using MSN/Jabber for shortquick things. Thank you very much for your work. Best of luck. /Santiago/ Thanks for your help, this will directly contribute to better support of iPod devices in Amarok. Matěj ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: MetaFile: guess album artist as in SQL collection scanner, MemoryCollection: use also album artist as identifying key in album map (2 commits squelched just for reviewboard)
On Jan. 17, 2012, 6:40 p.m., Ralf Engels wrote: While working on the multiple-artists feature I came to the following conclusion: Amarok should only try to guess album artists (or all artists in case of a Ralf featuring Matej) for internal usage and not try to write them back. That would mean that the album artist could still be displayed as being empty while the track artist is used in the Collection. Still, the patch is good. Ship it. Yup, I agree with the conclusion. But still I'm unable to come up with something reasonable. [brainstorm] Album::prettyAlbumArtist() (the guessed one) and Album::albumArtist() (the true one)? Also, I think that Album::hasAlbumArtist() is completely redundant (I fear to trust all Amarok code to return non-null albumArtist() if hasAlbumArtist() == true) and Album::isCompilation() is virtually !Album::hasAlbumArtist(). Perhaps we could remove both and define compilations as Album::prettyAlbumArtist() == null ptr? [/brainstorm] Speaking about the patch I've also received favourable review by email from Maximilian Kossick that will result in some docstring clarifications and a few more comments. If noone opposes, I'll push this tomorrow or so. - Matěj --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103715/#review9892 --- On Jan. 17, 2012, 2:10 p.m., Matěj Laitl wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103715/ --- (Updated Jan. 17, 2012, 2:10 p.m.) Review request for Amarok, Bart Cerneels and Ralf Engels. Description --- MetaFile: guess album artist as in SQL collection scanner, compilations Call albumArtist = ArtistHelper::bestGuessAlbumArtist( albumArtist, artist, genre, composer ); afer the data are read from file tags. This makes album artist field consistent for tracks in Local collection and tracks in file browser or drag dropped to Amarok playlist, but it has some downsides: * Amarok will essentially lie about what album artist is assigned to a track, most notably will replace empty album artist by track artist and will replace Various Artists by empty artist. * Editing album artist will be strange in corner cases: in particular if user sets album artist to Various Artists, it will be written to file tags as-is, but displayed as empty album artist. Other case would be when user sets album artist to empty string, but this is currently prevented due to a bug/feature in TagDialog. (every occurrence of Various Artists also accepts localized version of the string) Above change is needed for upcoming MemoryCollection: use also album artist as identifying key in album map change for UmsCollection, but if someone opposes, similar change can be made on UmsCollection level. Also, implement MetaFile::FileAlbum isCompilation() which is now easy as it can return albumArtist.isEmpty() DIGEST: unify album artist handling in Amarok MemoryCollection: use also album artist as identifying key in album map This has some far-reaching consequences which I believe are only positive, most notably Amarok is able to differentiate between 2 albums with same name but different album artists in USB Mass Storage collection and in new iPod collection. This unifies album artist handling across MemoryCollection and SqlCollection, 2 major collection storage implementations in Amarok. Technically, AlbumMap is changed from: typedef QMapQString, Meta::AlbumPtr AlbumMap; to: class AlbumMap : public QMapMeta::AlbumKey, Meta::AlbumPtr with insert(), remove(), value(), contains() QMap methods hidden and reimplemented for convenience and to prevent coding mistakes. (you no longer can add album under wrong key) This change is tested to work with new iPod collection, UMS collection (depends on previous MetaFile: guess album artist as in SQL collection scanner, compilations), all tests still pass. Only area that received just very basic testing are online service collections that are also affected by this change. DIGEST: Albums with same name but different album artist are now correctly separated in USB Mass Storage, iPod and various online service collections. Diffs - ChangeLog ed4e955deeebf6a506e87ca8973645b9eeadc67b src/core-impl/collections/daap/daapreader/Reader.cpp 9f749c5fe892f0cbcbacd8454d8fc80940f6bb43 src/core-impl/collections/ipodcollection/handler/IpodHandler.h ceccd7cc3028051d9b93201caea180a10af15ed3 src/core-impl/collections/ipodcollection/handler/IpodHandler.cpp
Re: Review Request: Some changes to make Amarok appearance more pretty
On 18. 1. 2012 Lucas Lira Gomes wrote: Hi everyone, Does anybody else but Myriam doesn't liked the extra info on the CollectionBrowser? If you haven't opined yet please let us know what's your opinion and explain why you prefer one way or another. I agree with Myriam. Track count is useful in Podcasts, not in collection browser. I also agree with Nikita about the alignment issues. Maybe Myriam is right, but this feature could be possibly like the album art. You can hide it or you can show it, thereby pleasing everyone. Also, if the majority doesn't liked the extra info on CollectionBrowser I can remove it easily ^^. Well, we try to keep ammount of micro-options low. Matěj ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review request: MemoryMeta changes and new iPod collection
On 19. 1. 2012 Bart Cerneels wrote: Bart, do you plan to make the UmsCollection changes or should I add it to my TODO list? I guess it will be faster if you implement it. Gives you a change to spot any mistakes in my coding as well. I'll probably have to little time the next few weeks anyway. Okay, I'll do it. I've already seen a few little bugs in UmsCollection, so I'll take these, too. Matěj ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Possibility to give an UMS-Collaction a descriptive name
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103736/ --- (Updated Jan. 20, 2012, 10:43 a.m.) Review request for Amarok and Bart Cerneels. Changes --- Added Bart. Description --- I sometimes missed the possibility to give an UMS-Collection a descriptive name. This patch adds this feature to the USM-Collection. Diffs - src/core-impl/collections/umscollection/UmsConfiguration.ui d9a6365 src/core-impl/collections/umscollection/UmsCollection.h 7c86fab src/core-impl/collections/umscollection/UmsCollection.cpp aaa9a6d Diff: http://git.reviewboard.kde.org/r/103736/diff/diff Testing --- Screenshots --- UMS-Config Dialog http://git.reviewboard.kde.org/r/103736/s/417/ UMS-Collection http://git.reviewboard.kde.org/r/103736/s/418/ Thanks, Volker Christian ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Possibility to give an UMS-Collaction a descriptive name
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103736/#review9961 --- This looks good (apart from some whitespace errors), let's see what Bart says. Ability to change name is however somehow redundant, because AFACS Amarok uses filesystem label of the partition which should be editable by users. (Utilities line gnome-disk-utility allow you to do so) src/core-impl/collections/umscollection/UmsCollection.h http://git.reviewboard.kde.org/r/103736/#comment8224 You added whitespace here. src/core-impl/collections/umscollection/UmsCollection.cpp http://git.reviewboard.kde.org/r/103736/#comment8225 whitespace error. src/core-impl/collections/umscollection/UmsCollection.cpp http://git.reviewboard.kde.org/r/103736/#comment8226 whitespace - Matěj Laitl On Jan. 20, 2012, 10:43 a.m., Volker Christian wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103736/ --- (Updated Jan. 20, 2012, 10:43 a.m.) Review request for Amarok and Bart Cerneels. Description --- I sometimes missed the possibility to give an UMS-Collection a descriptive name. This patch adds this feature to the USM-Collection. Diffs - src/core-impl/collections/umscollection/UmsConfiguration.ui d9a6365 src/core-impl/collections/umscollection/UmsCollection.h 7c86fab src/core-impl/collections/umscollection/UmsCollection.cpp aaa9a6d Diff: http://git.reviewboard.kde.org/r/103736/diff/diff Testing --- Screenshots --- UMS-Config Dialog http://git.reviewboard.kde.org/r/103736/s/417/ UMS-Collection http://git.reviewboard.kde.org/r/103736/s/418/ Thanks, Volker Christian ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Avoid the crash @ QtGroupingProxy::addSourceRow
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103741/#review9962 --- src/browsers/playlistbrowser/PlaylistBrowserModel.cpp http://git.reviewboard.kde.org/r/103741/#comment8227 I don't understand this. Do not add a new playlist if there were no playlists previously? (both new and old approach suffer from this) - Matěj Laitl On Jan. 19, 2012, 9:23 p.m., Jaime Torres Amate wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103741/ --- (Updated Jan. 19, 2012, 9:23 p.m.) Review request for Amarok. Description --- Avoid trying to add no rows (if i==0). This addresses bug 285541. https://bugs.kde.org/show_bug.cgi?id=285541 Diffs - src/browsers/playlistbrowser/PlaylistBrowserModel.cpp e507547 Diff: http://git.reviewboard.kde.org/r/103741/diff/diff Testing --- Until this little change, I was reproducing the bug. Thanks, Jaime Torres Amate ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Review Request: Rework transcoding: CollectionLocation asks user, not caller of prepareCopy()
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103752/ --- Review request for Amarok and Teo Mrnjavac. Description --- Rework transcoding: CollectionLocation asks user, not caller of prepareCopy() This moves the get transcoding configuration from callers of prapareCopy() to CollectionLocation itself, resulting in hopefully more predictable user experience. New capability, TranscodeCapability is created and supposed to be provided by collections to indicate source CollectionLocation it should display the transcoding assistant dialog. Default implementation of it has no pure virtual methods and SqlCollection is changed to provide it, making this change invisible to user for now. TranscodeCapability can also tell what formats will be playable on target collection (so that meaningless codecs can be disabled in the dialog) and mainly it allows target collections to store their preferred transcoding configuration so that the user is not bothered with the dialog every time. (the NULL_CODEC option can store a preference just copy) No collection is currently able to do this, but I will implement it for SqlCollection and rewrite of IpodCollection. IpodCollection rewrite (not yet in master) is the other coll. that provides the capability, it already implements playableFileTypes() TranscodingAssistantDialog is also tweaked so that it disables (not hides) unavailable transcoding options. (with info in a tooltip) Some core transcoding classes are cleaned up. Following bugs are still valid, but this is a first step to solving them: CCBUG: 280526 CCBUG: 264681 CCBUG: 291722 DIGEST: groundwork for more convenient transcoding in Amarok Diffs - src/CMakeLists.txt aed51026af8b4b7e826ba0f38c1bce328f78089a src/browsers/CollectionTreeView.h b70f99d811d1f7f271ec79298c3b0140fbd0ede4 src/browsers/CollectionTreeView.cpp c9069c770fd28fe16e76b1af132f3c7dff4c82d3 src/browsers/filebrowser/FileView.h 890b9f458e7ae504a52019b8f41e3e6d2ba218a5 src/browsers/filebrowser/FileView.cpp b23abc22a91bb8ca7bc8339cc873153c7cb179eb src/core-impl/collections/db/sql/SqlCollection.cpp ace308b3831deaf51c91dd892e224060d9c03461 src/core-impl/collections/support/CollectionLocationDelegateImpl.h 12b975fc7d5c5d56e314f3fce4384eb559e88274 src/core-impl/collections/support/CollectionLocationDelegateImpl.cpp fb7c18f6fef5ef634ef1a40cea604f12a379cfc6 src/core/CMakeLists.txt 6a876e842fc2551763848ebfd3f09a7d35fcc7a6 src/core/capabilities/Capability.h b38d9166797658c99a0162e6dfa7944d47b98de5 src/core/capabilities/TranscodeCapability.h PRE-CREATION src/core/capabilities/TranscodeCapability.cpp PRE-CREATION src/core/collections/CollectionLocation.h 375a858aae47b27b5cb9081567b3e384c05d97d8 src/core/collections/CollectionLocation.cpp 01d0f5b8100cd2ad7fb0d3a4fa6d6c4a61e89931 src/core/collections/CollectionLocationDelegate.h c49a7445e260665e508795ae8dbee4ac9f271f71 src/core/transcoding/TranscodingController.h e9b04df6478e1f18d1e8c7d6d9859e6c1c86d9f3 src/core/transcoding/TranscodingController.cpp e16b6fe7d3d60621ef0a237951ac038f7846b51e src/core/transcoding/TranscodingDefines.h 4ba3e2ae9de710aa7a5f446eddf565e9f5138e4c src/core/transcoding/TranscodingFormat.h 5bf4e6098be4f3e08dcd45c48fdd264418660293 src/core/transcoding/formats/TranscodingNullFormat.cpp 98f09d1853833d2fd8b8de0603612ae337c6ef52 src/transcoding/TranscodingAssistantDialog.h 718c01d40fed0113087b90e425a9102b365076cb src/transcoding/TranscodingAssistantDialog.cpp b64e74b9ee75c8b597a07c008a4e161333bb0d86 tests/core/collections/MockCollectionLocationDelegate.h a58ca4b344e98f6b75da19cdd6b38172ecc95f59 Diff: http://git.reviewboard.kde.org/r/103752/diff/diff Testing --- Works as before with SqlCollection, works with iPod collection rewrite that also disables (gays-out) unplayable transcoding options. Thanks, Matěj Laitl ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Rework transcoding: CollectionLocation asks user, not caller of prepareCopy()
On Jan. 21, 2012, 11:56 a.m., Ralf Engels wrote:
No obvious technical issues.
However, I am wondering: Is there really a transcoding capability?
I mean, we should be able to transcode everything independent of the
collection it's in.
Also, I don't like the concept of the capabilities. The only reason to have
these capabilities is to keep the API binary compatible.
But in Amarok we don't really care about that for now.
However, I am wondering: Is there really a transcoding capability?
I mean, we should be able to transcode everything independent of the
collection it's in.
Yes, I originally thought this would be the right best approach. Thanks to
CollectionLocation::copyUrlsToCollection( const QMapMeta::TrackPtr, KUrl ...
) we could transcode tracks somewhere in CollectionLocation and just replace
the KUrls with the transcoded ones. But it would have some drawbacks:
* motivation for this change was transcoding for iPod collection rewrite:
transferring tracks onto iPod is usually slow (nearly as slow as transcoding).
With such a 2-step process it would take twice the time. (because currently it
can be done in parallel)
* you would be essentially copying the files twice (witout greater
CollectionLocation redesign)
* many collection locations just copy source track metadata such as bitrate,
length, filetype etc. This would need to be dealed with.
* I'd still want to have ability to save per-destinaiton-collection preferred
transcoding configuration. Collection locations would then have to provide
something like {get,has}SavedConfiguration and saveConfiguration. (we don't
want this to be handed universally, for example UmsCollection and iPod
collection want to store this preferrence on the device)
* The same applies to playableFileTypes() (altough this would maybe fit into
CollectionLocation directly)
Therefore I came to the conslusion that each CollectionLocation should
implement transcoding in its copyUrlsToCollection() method. This is not really
hard, Transcoding::Job facilitates it. So the reason to put this into a
capability was to have it self-contained and do not add additional methods to
over-crowded CollectionLocation of Collection. But I will happily implement a
better design should some suggest such.
- Matěj
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/103752/#review9975
---
On Jan. 21, 2012, 12:47 a.m., Matěj Laitl wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/103752/
---
(Updated Jan. 21, 2012, 12:47 a.m.)
Review request for Amarok and Teo Mrnjavac.
Description
---
Rework transcoding: CollectionLocation asks user, not caller of prepareCopy()
This moves the get transcoding configuration from callers of
prapareCopy() to CollectionLocation itself, resulting in hopefully more
predictable user experience.
New capability, TranscodeCapability is created and supposed to be
provided by collections to indicate source CollectionLocation it should
display the transcoding assistant dialog. Default implementation of it
has no pure virtual methods and SqlCollection is changed to provide it,
making this change invisible to user for now.
TranscodeCapability can also tell what formats will be playable on
target collection (so that meaningless codecs can be disabled in the
dialog) and mainly it allows target collections to store their
preferred transcoding configuration so that the user is not bothered
with the dialog every time. (the NULL_CODEC option can store a
preference just copy) No collection is currently able to do this,
but I will implement it for SqlCollection and rewrite of IpodCollection.
IpodCollection rewrite (not yet in master) is the other coll. that
provides the capability, it already implements playableFileTypes()
TranscodingAssistantDialog is also tweaked so that it disables (not
hides) unavailable transcoding options. (with info in a tooltip) Some
core transcoding classes are cleaned up.
Following bugs are still valid, but this is a first step to solving
them:
CCBUG: 280526
CCBUG: 264681
CCBUG: 291722
DIGEST: groundwork for more convenient transcoding in Amarok
Diffs
-
src/CMakeLists.txt aed51026af8b4b7e826ba0f38c1bce328f78089a
src/browsers/CollectionTreeView.h b70f99d811d1f7f271ec79298c3b0140fbd0ede4
src/browsers/CollectionTreeView.cpp
c9069c770fd28fe16e76b1af132f3c7dff4c82d3
src/browsers/filebrowser/FileView.h
890b9f458e7ae504a52019b8f41e3e6d2ba218a5
src/browsers/filebrowser/FileView.cpp
b23abc22a91bb8ca7bc8339cc873153c7cb179eb
src/core-impl/collections/db/sql/SqlCollection.cpp
Re: [amarok] cmake/modules: Revert FindMySQLAmarok.cmake fix argumets when calling mysql_config
On 19. 12. 2011 Christophe Giboudeaux wrote: On Monday 19 December 2011 00:38:47 Matěj Laitl wrote: I tried to solve http://mail.kde.org/pipermail/amarok-devel/2011- December/009663.html Specifically, the problem is that mysql_config (at least 5.1.56) doesn't accept --variable=pkgincludedir - it accepts only --include argument which returns something like -I/usr/include/mysql -- Found MySQL: Usage: /usr/bin/mysql_config [OPTIONS] ok, then it only exists in higher version (the option exists in 5.5.x). I see two solutions: - only rely on find_path(MYSQL_INCLUDE_DIR...), so move it outside the else(MYSQLCONFIG_EXECUTABLE) loop [1] - Drop FindMysqlAmarok.cmake and use FindAmarok.cmake from kdelibs instead (which worked fine here). I prefer the second solution as the FindMysql from kdelibs is really cleaner than the Amarok copy: - MYSQL_CFLAGS doesn't look that important - the linked targets gathered with 'mysql_config --libs' are already explicitly added in the CMakeLists.txt files (./src/core- impl/collections/db/sql/mysqlecollection/CMakeLists.txt, ./src/core- impl/collections/db/sql/mysqlservercollection/CMakeLists.txt and some unit tests) and the output variable is different if mysql_config is not present: * with mysql_config, MYSQL_LIBRARIES=-L/usr/lib64 -lmysqlclient -lpthread - lz -lm -lrt -lssl -lcrypto -ldl * without it, MYSQL_LIBRARIES=/usr/lib64/libmysqlclient.so. The second one is enough Hmm, I've tried dropping FindMySQLAmarok.cmake, but using kdelibs one doesn't work for me (you probably don't build tests): Linking CXX executable ../../../../testsqlcollection /usr/lib64/mysql/libmysqld.so: undefined reference to `DH_new' /usr/lib64/mysql/libmysqld.so: undefined reference to `SSL_get_current_cipher' /usr/lib64/mysql/libmysqld.so: undefined reference to `SSL_CIPHER_get_name' (...) I don't know why core mysqlcollection builds fine (probably something else pulls OpenSSL? AFAIK undefined symbols are allowed in shared libs, but not final executables), but apparently the additional entries in MYSQL_LIBRARIES are needed. (and I really don't feel like hard-coding them in Amarok) So I will commit your patch if no-one disagrees. Matěj ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Draft message to packagers: Patch for Amarok 2.5.0 to fix critical usability bug with KDE 4.8
Hi list,
as Myriam repeatedly mentioned, we need to push a patch to packagers so that
Amarok can be used with KDE 4.8. Some known Amarok developer (Bart, Ralf,
Mark?) should probably send (a variant of) following message to kde-packager
list. I've tested the 2.5.0 + attached patch personally with kdelibs 4.7.4 and
4.8.0. (on Qt 4.8.0, that shouldn't matter)
Matěj
---
Hi Amarok packagers,
there is a bug in Amarok 2.5.0 [1] when run on kdelibs = 4.8.0 (no matter
what kdelibs version Amarok was compiled against) which significantly degrades
user experience. If you ship kdelibs 4.8.0 or plan to do so, please consider
applying following patch to your Amarok 2.5.0 package which fixes the problem.
The patch was also successfully tested to work with kdelibs 4.7.4.
[1] https://bugs.kde.org/show_bug.cgi?id=290123
From 7d1fcf5139b488136871fb32e361e90311db6024 Mon Sep 17 00:00:00 2001
From: Marco Martin notm...@gmail.com
Date: Thu, 5 Jan 2012 16:59:05 +0100
Subject: [PATCH] contextview taskbar not child of containment
the containment being parent can control its appearance, such as
clipping. This fixes a critical bug where Context view cannot be
manipulated because the bottom line with applet list and wrench is not
visible if Amarok is used with KDE 4.8 (including betas).
This is the squashed version of the patch intended to apply directly to
Amarok v2.5.0. This patch is already in master as commits 1b2e221a98
and b3d81df5d713.
BUG:290123
---
src/context/ToolbarView.cpp |6 --
src/context/toolbar/AppletToolbar.cpp | 13 +
src/context/toolbar/AppletToolbar.h |3 +++
3 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/src/context/ToolbarView.cpp b/src/context/ToolbarView.cpp
index f3b4916..840b99b 100644
--- a/src/context/ToolbarView.cpp
+++ b/src/context/ToolbarView.cpp
@@ -74,7 +74,9 @@ Context::ToolbarView::ToolbarView( Plasma::Containment* containment, QGraphicsSc
setVerticalScrollBarPolicy( Qt::ScrollBarAlwaysOff );
// now we create the toolbar
-m_toolbar = new AppletToolbar( containment );
+m_toolbar = new AppletToolbar(0);
+scene-addItem(m_toolbar.data());
+m_toolbar.data()-setContainment( qobject_castContext::Containment *(containment) );
m_toolbar.data()-setZValue( m_toolbar.data()-zValue() + 1000 );
m_toolbar.data()-setPos( TOOLBAR_X_OFFSET, 0 );
@@ -96,7 +98,7 @@ Context::ToolbarView::ToolbarView( Plasma::Containment* containment, QGraphicsSc
Context::ToolbarView::~ToolbarView()
{
-
+delete m_toolbar.data();
}
void
diff --git a/src/context/toolbar/AppletToolbar.cpp b/src/context/toolbar/AppletToolbar.cpp
index 0e0deb5..c51762c 100644
--- a/src/context/toolbar/AppletToolbar.cpp
+++ b/src/context/toolbar/AppletToolbar.cpp
@@ -67,6 +67,19 @@ Context::AppletToolbar::~AppletToolbar()
}
void
+
+Context::AppletToolbar::setContainment( Containment * containment )
+{
+m_cont = containment;
+}
+
+Context::Containment *
+Context::AppletToolbar::containment() const
+{
+return m_cont;
+}
+
+void
Context::AppletToolbar::resizeEvent( QGraphicsSceneResizeEvent * event )
{
debug() setting layout to QRectF( QPointF( 0, 0 ), event-newSize() );
diff --git a/src/context/toolbar/AppletToolbar.h b/src/context/toolbar/AppletToolbar.h
index ee8a208..058baae 100644
--- a/src/context/toolbar/AppletToolbar.h
+++ b/src/context/toolbar/AppletToolbar.h
@@ -55,6 +55,9 @@ class AppletToolbar : public QGraphicsWidget
void appletRemoved( Plasma::Applet* applet );
+void setContainment( Containment * containment );
+Containment* containment() const;
+
signals:
void showApplet( Plasma::Applet* );
void appletAddedToToolbar( Plasma::Applet* applet, int loc );
--
1.7.4.5
signature.asc
Description: This is a digitally signed message part.
___
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel
Review Request: Another approach to fix bug 291068, be more permissive
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103856/ --- Review request for Amarok and Bart Cerneels. Description --- Another approach to fix bug 291068, be more permissive Bart originally solved the bug by enabling track dropping only precisely on root collection rows. This is IMO too much restrictive as it prevents you to drag tracks between 2 large expanded collections without excessive scrolling. (something I would miss) Additionally, there was no visual indication that the drop will not be performed. This is my try to rework it in a way that: * keeps drops onto artists/albums (whatever you first level entity in collection browser is) allowed. There is a drop indicator that clearly shows that the drop will go _between_ the entities, not to them. * disables drops to read-only collections * disabled drops are indicated visually using the not-allowed mouse cursor (the tricky part, but commented well in code) BUG: 291068 This addresses bug 291068. https://bugs.kde.org/show_bug.cgi?id=291068 Diffs - ChangeLog ed4e955deeebf6a506e87ca8973645b9eeadc67b src/browsers/CollectionTreeItemModel.cpp d2bab082f44b37fa8945c827006439e7deb0017e src/browsers/CollectionTreeItemModelBase.h c6cca46fff956f9123299cb29c81e6a792abbc3b src/browsers/CollectionTreeItemModelBase.cpp e7f8e620a0ae2f7546c879dfd5d67a13fcb0f34a src/browsers/CollectionTreeView.h b70f99d811d1f7f271ec79298c3b0140fbd0ede4 src/browsers/CollectionTreeView.cpp c9069c770fd28fe16e76b1af132f3c7dff4c82d3 Diff: http://git.reviewboard.kde.org/r/103856/diff/diff Testing --- Works as described here. Thanks, Matěj Laitl ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Review Request: Change GUI strings in Configuration dialog to reduce user confusion
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103871/ --- Review request for Amarok. Description --- Change GUI strings in Configuration dialog to reduce user confusion This is a result of a dialogue with an Amarok user (Leonidas Tsampros) on #amarok who (logically) thought that if he enables NFS Device in Devices section of Amarok plugins, NFS will pop-up as another collection. This is not how SMB, NFS, MassStorage Devices work currently, they are meant to enable users to have a part of their collection on respective devices. I believe that the old plugin names and descriptions are confusing to the extent that their translations should be dropped; it should be a plenty of time till Amarok 2.6 so translators should have opportunity to pickup these. (or, do .desktop translations get erased automatically when original string changes?) CHANGES: * Devices in Amarok configuration - Plugins is with other related strings renamed to Local Collection Backends to reduce user confusion. BUG: 293277 FIXED-IN: 2.6 This addresses bug 293277. https://bugs.kde.org/show_bug.cgi?id=293277 Diffs - ChangeLog ed4e955deeebf6a506e87ca8973645b9eeadc67b src/configdialog/ConfigDialog.cpp 1e8e30a7f02709fb8d02b705faf01de58f52ec94 src/configdialog/dialogs/PluginsConfig.cpp 0e0ff1852ab192be2911cce3ed73dd42f38ab6f3 src/core-impl/collections/db/sql/device/massstorage/amarok_device_massstorage.desktop 34dda8cda57e4ea8eb8980e5b6e3e44019718004 src/core-impl/collections/db/sql/device/nfs/amarok_device_nfs.desktop 66a0b563c1e0daea8c77dd3fb6740c29897182de src/core-impl/collections/db/sql/device/smb/amarok_device_smb.desktop 82d5715971e6001d4b1a624f256b48ea121a8155 Diff: http://git.reviewboard.kde.org/r/103871/diff/diff Testing --- Thanks, Matěj Laitl ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Change GUI strings in Configuration dialog to reduce user confusion
On Feb. 6, 2012, 9:06 a.m., Bart Cerneels wrote: Why do we even show these plugins at all? It's feels like a big implementation detail that should not be expose to the user to me. Hmm, with a bit of thinking I must fully agree with you. If no one else opposes, I will implement this. (taking care that the plugins will be _always_ enabled regardless of settings in amarokrc to prevent disabled plugins that can no longer be enabled in gui) - Matěj --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103871/#review10369 --- On Feb. 5, 2012, 11:56 a.m., Matěj Laitl wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103871/ --- (Updated Feb. 5, 2012, 11:56 a.m.) Review request for Amarok. Description --- Change GUI strings in Configuration dialog to reduce user confusion This is a result of a dialogue with an Amarok user (Leonidas Tsampros) on #amarok who (logically) thought that if he enables NFS Device in Devices section of Amarok plugins, NFS will pop-up as another collection. This is not how SMB, NFS, MassStorage Devices work currently, they are meant to enable users to have a part of their collection on respective devices. I believe that the old plugin names and descriptions are confusing to the extent that their translations should be dropped; it should be a plenty of time till Amarok 2.6 so translators should have opportunity to pickup these. (or, do .desktop translations get erased automatically when original string changes?) CHANGES: * Devices in Amarok configuration - Plugins is with other related strings renamed to Local Collection Backends to reduce user confusion. BUG: 293277 FIXED-IN: 2.6 This addresses bug 293277. https://bugs.kde.org/show_bug.cgi?id=293277 Diffs - ChangeLog ed4e955deeebf6a506e87ca8973645b9eeadc67b src/configdialog/ConfigDialog.cpp 1e8e30a7f02709fb8d02b705faf01de58f52ec94 src/configdialog/dialogs/PluginsConfig.cpp 0e0ff1852ab192be2911cce3ed73dd42f38ab6f3 src/core-impl/collections/db/sql/device/massstorage/amarok_device_massstorage.desktop 34dda8cda57e4ea8eb8980e5b6e3e44019718004 src/core-impl/collections/db/sql/device/nfs/amarok_device_nfs.desktop 66a0b563c1e0daea8c77dd3fb6740c29897182de src/core-impl/collections/db/sql/device/smb/amarok_device_smb.desktop 82d5715971e6001d4b1a624f256b48ea121a8155 Diff: http://git.reviewboard.kde.org/r/103871/diff/diff Testing --- Thanks, Matěj Laitl ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Continuous integration for Amarok on build.kde.org
Hi Torgny, we've been discussing Amarok CI possibilities with other Amarok developers lately and I've been pointed to you by Ben Cooksley as a maintainer of build.kde.org. Would it be possible to set up a build test job for Amarok on build.k.o? With its CMake CTest buildsystem it should be fairly similar to other KDE projects. These are Amarok required dependencies: * KDE-Libs 4.6 + KDE-Base-runtime 4.6(oxygen-icons) (or newer) * Qt 4.6 (or newer) * TagLib 1.7 (or newer) * TagLib Extras 1.0.1 (or newer) * MySQL 5.0 (or newer) Embedded: libmysqld compiled with fPIC * QtScript Generator, Qt Bindings 0.1.0 * LibQCA 2.0.2 (or newer) * gmock 1.4 (or newer) - http://code.google.com/p/googlemock/ [in fact optional, but tests need it and CI wouldn't have sense without them] Following are optional dependencies [nice-to-have, not crucial]: * libgpod 0.7.93 (or newer) [iPod plugin] * GDKPixBuf 2.0 (or newer) [artwork in iPod plugin] * libmtp 1.0.0 (or newer) [MTP device support] * OpenSSL http://www.openssl.org [Mp3tunes.com integration] * libxml2 http://xmlsoft.org [Mp3tunes.com integration] * libcurl http://curl.haxx.se [Mp3tunes.com integration] * Glib2 http://www.gtk.org [Mp3tunes.com integration] * Loudmouth, the Jabber library [Mp3tunes.com integration] * Qt compiled with Glib enabled [Mp3tunes.com integration] * Liblastfm 0.3 [last.fm integration] * QJson 0.7 (or newer) [playdar collection] * MySQL 5.0 (or newer) Server (external database support) * LibOFA - http://code.google.com/p/musicip-libofa/ [musicbrainz integr.] * libmygpo-qt 1.0.5 (or newer) [ gpodder.net Podcast Provider Service] I do have some limited Hudson experience so given an account, I can try to setup/maintain Amarok jobs myself. Please let us know whether this would be possible and what can we do for it. Regards, Matěj Laitl ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Continuous integration for Amarok on build.kde.org
Hi! On 12. 2. 2012 Torgny Nyblom wrote: On Saturday 11 February 2012 19.53.59 you wrote: Hi Torgny, we've been discussing Amarok CI possibilities with other Amarok developers lately and I've been pointed to you by Ben Cooksley as a maintainer of build.kde.org. Would it be possible to set up a build test job for Amarok on build.k.o? I'm on it :) Great! With its CMake CTest buildsystem it should be fairly similar to other KDE projects. These are Amarok required dependencies: * KDE-Libs 4.6 + KDE-Base-runtime 4.6(oxygen-icons) (or newer) * Qt 4.6 (or newer) * TagLib 1.7 (or newer) * TagLib Extras 1.0.1 (or newer) * MySQL 5.0 (or newer) Embedded: libmysqld compiled with fPIC * QtScript Generator, Qt Bindings 0.1.0 * LibQCA 2.0.2 (or newer) * gmock 1.4 (or newer) - http://code.google.com/p/googlemock/ [in fact optional, but tests need it and CI wouldn't have sense without them] Do you know if there is any special install trick needed for this one? ./configure make make install doesn't work for me. Hmm, `make install` of gmock-1.6.0 fails for me, too, but it works for gmock-1.5.0, so I suggest you try that as a quick fix: http://googlemock.googlecode.com/files/gmock-1.5.0.tar.gz Following are optional dependencies [nice-to-have, not crucial]: * libgpod 0.7.93 (or newer) [iPod plugin] * GDKPixBuf 2.0 (or newer) [artwork in iPod plugin] * libmtp 1.0.0 (or newer) [MTP device support] * OpenSSL http://www.openssl.org [Mp3tunes.com integration] * libxml2 http://xmlsoft.org [Mp3tunes.com integration] * libcurl http://curl.haxx.se [Mp3tunes.com integration] * Glib2 http://www.gtk.org [Mp3tunes.com integration] * Loudmouth, the Jabber library [Mp3tunes.com integration] * Qt compiled with Glib enabled [Mp3tunes.com integration] * Liblastfm 0.3 [last.fm integration] * QJson 0.7 (or newer) [playdar collection] * MySQL 5.0 (or newer) Server (external database support) * LibOFA - http://code.google.com/p/musicip-libofa/ [musicbrainz] * libmygpo-qt 1.0.5 (or newer) [ gpodder.net Podcast Provider] I'll try and install as many of these as I can find. Good, thanks. No problem installing these gradually. I do have some limited Hudson experience so given an account, I can try to setup/maintain Amarok jobs myself. Please let us know whether this would be possible and what can we do for it. For now we try to keep the number of admins to me and the sysadmin team, this is due to change once we move the installation to the real server that Ben is setting up. The plan then is to allow all repo admins to administrator the jenkins parts in jenkins as well. Issue will still be that installing dependencies and so will require shell access to the server and that is up to the sysadmin team to handle (I'm not part of that) Okay. Apart from installing dependencies, shell access shouldn't be needed at all. Another thing came to my mind - some Amarok tests apparently require X session. Is this already dealed with on build.k.o? If not, Xvfb should do the job nicely. Future considerations include testing against multiple kdelibs, Qt and TagLib versions, but this is really low priority. Thanks for your efforts, Matěj Laitl and the Amarok development team ___ 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/#review10581 --- This change looks good to me. I cannot test right now, do I assume correctly that the shortcut appears in Settings - Configure chortcuts? src/MainWindow.cpp http://git.reviewboard.kde.org/r/103960/#comment8654 I assume this edits details of currently playing (or currently selected?) song. I would include the fact in action description. src/playlist/PlaylistDock.cpp http://git.reviewboard.kde.org/r/103960/#comment8652 Is there any specific reason this call is here? - Matěj Laitl On Feb. 13, 2012, 5:26 a.m., Jasneet Bhatti wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103960/ --- (Updated Feb. 13, 2012, 5:26 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: Continuous integration for Amarok on build.kde.org
On 13. 2. 2012 Torgny Nyblom wrote: http://build.kde.org/view/EXTRAGEAR/ Here it is Please let me know if there is anymore I can help with. Superbe, thank you very much. I will look at the test failures and determine where is the problem - the tests all pass when I run them locally. Please let us know when the migration is complete - I do have some fancy ideas for our Jenkins jobs. Matěj ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Dev query : To get the path of the current track playing from ratingValueChanged()
On 4. 3. 2012 Phalgun Guduthur wrote: Hello I'm trying to write a patch along my GSoC proposal ( I have already mailed in the first rough draft, please review it if possible, any feedback is welcome! ). What I'm trying to do is, when a user changes the rating of a song, the song resource's nao:rating should be updated along with the normal id3 tag updation carried out by Amarok. The slot for this is in src/playlist/view/listview/InlineEditorWidget.cpp which calls the function ratingValueChanged. (line 159 and 270) But I'm stuck on getting the path of the current track whose rating is being modified. I tried using Playlist::Item::track() But it didn't work. The ratingWidget disappeared after this. I suggest you hook in a completely different place. Tracks from what collection do you want to watch? If it is SqlCollection, I suggest you hook in SqlTrack::setRating(). For tracks not in any collection there is MetaFile::Track. If you need to watch tracks from various collections, you could become their observer by implementing Meta::Observer, but that would be inefficient to watch entire collections etc. Also, some tracks are not file-based (MTP collection), some could be remote.. Regards, Matěj ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Fix for bug 263693: The Delete Tracks dialog is misleading/ambiguous
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102236/#review11213 --- It is good that you factored the common code to its own method, there are however some small string issues, please resolve them: src/core-impl/collections/support/CollectionLocationDelegateImpl.cpp http://git.reviewboard.kde.org/r/102236/#comment9019 Hmm, I think you should use name() here instead of prettyName() as prettyName can give Unknown artist etc. (please check its implementation) src/core-impl/collections/support/CollectionLocationDelegateImpl.cpp http://git.reviewboard.kde.org/r/102236/#comment9020 Same here, name() could be better. (check implementations) src/core-impl/collections/support/CollectionLocationDelegateImpl.cpp http://git.reviewboard.kde.org/r/102236/#comment9015 You can (and should) use artistName and trackName here. src/core-impl/collections/support/CollectionLocationDelegateImpl.cpp http://git.reviewboard.kde.org/r/102236/#comment9016 This will give: path/to/file.mp3(Track Name) (missing space) src/core-impl/collections/support/CollectionLocationDelegateImpl.cpp http://git.reviewboard.kde.org/r/102236/#comment9017 This will give: path/to/file.mp3(Artist by ) src/core-impl/collections/support/CollectionLocationDelegateImpl.cpp http://git.reviewboard.kde.org/r/102236/#comment9018 For easier translatability (think of right-to-left scripts), I suggest you use one format string for the whole string and no strinc concatenation, for example this (in every if clause:) str = i18nc(%1 is track url, %2 track title, %3 track artist, %1 (%2 by %3), url, trackName, artistName); - Matěj Laitl On March 7, 2012, 10:30 p.m., Ryan McCoskrie wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102236/ --- (Updated March 7, 2012, 10:30 p.m.) Review request for Amarok. Description --- Fix for bug 263693. When the user is asked to confirm deleting a file from his/her music collection the prompt will use the songs meta-data in place of the path name if possible. This addresses bug 263693. https://bugs.kde.org/show_bug.cgi?id=263693 Diffs - src/core-impl/collections/support/CollectionLocationDelegateImpl.h 12b975f src/core-impl/collections/support/CollectionLocationDelegateImpl.cpp fb7c18f Diff: http://git.reviewboard.kde.org/r/102236/diff/ Testing --- Ran application and asked to delete several files from collection. Patch worked as expected. Deleted meta date from one track and asked to delete that also. Found that Meta::TrackPtr::prettyName() will return the file name of the track instead of an empty QString and that Meta::ArtistPtr::prettyName() returns 'Unknown Artist' in place of an empty QString. This will render the data checking needless under all known circumstances. Screenshots --- Uses meta-data instead of raw file path http://git.reviewboard.kde.org/r/102236/s/220/ Thanks, Ryan McCoskrie ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Fix for bug 263693: The Delete Tracks dialog is misleading/ambiguous
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102236/#review11219 --- Ship it! Good! I will push this tomorrow. - Matěj Laitl On March 7, 2012, 11:12 p.m., Ryan McCoskrie wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102236/ --- (Updated March 7, 2012, 11:12 p.m.) Review request for Amarok. Description --- Fix for bug 263693. When the user is asked to confirm deleting a file from his/her music collection the prompt will use the songs meta-data in place of the path name if possible. This addresses bug 263693. https://bugs.kde.org/show_bug.cgi?id=263693 Diffs - src/core-impl/collections/support/CollectionLocationDelegateImpl.h 12b975f src/core-impl/collections/support/CollectionLocationDelegateImpl.cpp fb7c18f Diff: http://git.reviewboard.kde.org/r/102236/diff/ Testing --- Ran application and asked to delete several files from collection. Patch worked as expected. Deleted meta date from one track and asked to delete that also. Found that Meta::TrackPtr::prettyName() will return the file name of the track instead of an empty QString and that Meta::ArtistPtr::prettyName() returns 'Unknown Artist' in place of an empty QString. This will render the data checking needless under all known circumstances. Screenshots --- Uses meta-data instead of raw file path http://git.reviewboard.kde.org/r/102236/s/220/ Thanks, Ryan McCoskrie ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Fix for bug 263693: The Delete Tracks dialog is misleading/ambiguous
On March 8, 2012, 1:14 p.m., Bart Cerneels wrote: I feel you should still show the file path in the confirmation dialog. After all, you might have duplicates you want to remove and can't be sure which copy it is. The latest version shows it in all cases. (or you talk about prettyUrl() vs. url()?) - Matěj --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102236/#review11236 --- On March 7, 2012, 11:39 p.m., Ryan McCoskrie wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102236/ --- (Updated March 7, 2012, 11:39 p.m.) Review request for Amarok. Description --- Fix for bug 263693. When the user is asked to confirm deleting a file from his/her music collection the prompt will use the songs meta-data in place of the path name if possible. This addresses bug 263693. https://bugs.kde.org/show_bug.cgi?id=263693 Diffs - src/core-impl/collections/support/CollectionLocationDelegateImpl.h 12b975f src/core-impl/collections/support/CollectionLocationDelegateImpl.cpp fb7c18f Diff: http://git.reviewboard.kde.org/r/102236/diff/ Testing --- Ran application and asked to delete several files from collection. Patch worked as expected. Deleted meta date from one track and asked to delete that also. Found that Meta::TrackPtr::prettyName() will return the file name of the track instead of an empty QString and that Meta::ArtistPtr::prettyName() returns 'Unknown Artist' in place of an empty QString. This will render the data checking needless under all known circumstances. Screenshots --- Uses meta-data instead of raw file path http://git.reviewboard.kde.org/r/102236/s/220/ Thanks, Ryan McCoskrie ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Review Request: Rework transcoding: remember encoder, transcode on move, cleaner code
890b9f458e7ae504a52019b8f41e3e6d2ba218a5 src/browsers/filebrowser/FileView.cpp 425641af15e66c2670bce719eff1615f416ad6b7 src/configdialog/dialogs/CollectionConfig.cpp 7704fb9fab5a09c4635c1ec7526ae05047df0c6f src/configdialog/dialogs/CollectionConfig.ui d0ccdb33e8e54ecf4db205e10dbd8396eac488ad src/core-impl/collections/db/sql/SqlCollection.h 3a96bea1aa21761f12d956f7905f87808e0efc4a src/core-impl/collections/db/sql/SqlCollection.cpp 79714153661e50b70885b3e82cb6529b79ff98e2 src/core-impl/collections/db/sql/SqlCollectionLocation.h 1db72726d041713158be0fe91a93be3b1044b70e src/core-impl/collections/db/sql/SqlCollectionLocation.cpp 2c33da62565a9be4b5710cce590bac99d28b47ae src/core-impl/collections/mediadevicecollection/MediaDeviceCollectionLocation.h 24bb306c5b28a6d21f66f598f4caccbbb60f5bf8 src/core-impl/collections/support/CollectionLocationDelegateImpl.h 12b975fc7d5c5d56e314f3fce4384eb559e88274 src/core-impl/collections/support/CollectionLocationDelegateImpl.cpp fb7c18f6fef5ef634ef1a40cea604f12a379cfc6 src/core-impl/collections/support/PlaylistCollectionLocation.h 967d2150cf2c76f563dac0ff5396e84448826aed src/core-impl/collections/support/TrashCollectionLocation.h a9c92495abebd13506ed52a185355f21b55ee347 src/core-impl/collections/umscollection/UmsCollectionLocation.h 3fc0af405a8d47374afacdf5c1190d112b126d21 src/core/CMakeLists.txt 4db2105f08246898585bbe38ba4fbe0d006bd138 src/core/capabilities/Capability.h 42ffd7e89fd67d1b1b1b0b6f3a8ddc35cde5943e src/core/capabilities/TranscodeCapability.h PRE-CREATION src/core/capabilities/TranscodeCapability.cpp PRE-CREATION src/core/collections/CollectionLocation.h 35480b2000cccf9ece99b7654ff3852087b3144e src/core/collections/CollectionLocation.cpp 9b258ce9a19ab22f7027b674da4b76d1f15d973b src/core/collections/CollectionLocationDelegate.h c49a7445e260665e508795ae8dbee4ac9f271f71 src/core/transcoding/TranscodingConfiguration.h 378fe12ffa86764fe3ecf0cc5f2a38d8a47a0561 src/core/transcoding/TranscodingConfiguration.cpp 40f09be71ee0c75cbeeb6d3a4702c2a6f5ec3b99 src/core/transcoding/TranscodingController.h e9b04df6478e1f18d1e8c7d6d9859e6c1c86d9f3 src/core/transcoding/TranscodingController.cpp e16b6fe7d3d60621ef0a237951ac038f7846b51e src/core/transcoding/TranscodingDefines.h 4ba3e2ae9de710aa7a5f446eddf565e9f5138e4c src/core/transcoding/TranscodingFormat.h 5bf4e6098be4f3e08dcd45c48fdd264418660293 src/core/transcoding/TranscodingProperty.h 928854baa31eb4e78b3fe80768eb4f9811a0f5bf src/core/transcoding/TranscodingProperty.cpp 2b6752cfbc39e37880b05930dd9e797d60200c97 src/core/transcoding/formats/TranscodingNullFormat.h 96393e80f9a74a34a6858fcc114f2719089fac71 src/core/transcoding/formats/TranscodingNullFormat.cpp 98f09d1853833d2fd8b8de0603612ae337c6ef52 src/services/magnatune/MagnatuneCollectionLocation.cpp d75ea9edd0af9317b66f223961d325635fb26e33 src/services/mp3tunes/Mp3tunesServiceCollectionLocation.h c168a1f7bb07e798702b01f488912fd0b2c191c5 src/transcoding/CMakeLists.txt 06d5be85075581e979dc5fd8b411286200049846 src/transcoding/TranscodingAssistantDialog.h 718c01d40fed0113087b90e425a9102b365076cb src/transcoding/TranscodingAssistantDialog.cpp b64e74b9ee75c8b597a07c008a4e161333bb0d86 src/transcoding/TranscodingAssistantDialog.ui 912a1c2d636f4b24b1efd94185c0ae41f11ee96a src/transcoding/TranscodingOptionsStackedWidget.cpp 54c936dd041d9e23820afc8dbde662b5bb0dcd46 src/transcoding/TranscodingPropertyComboBoxWidget.h ecab1583fcb27ab78497e9c56f8c26a0e9a8b05a src/transcoding/TranscodingPropertyComboBoxWidget.cpp dbb2462f526ad5b47c14efa5c0f0983db296cb20 src/transcoding/TranscodingPropertySliderWidget.cpp 0a49da06127d9fa4dde9f8df95650e1a9a5ed6bc src/transcoding/TranscodingPropertySpinBoxWidget.h f1fa7f561c518f7420fc904252e35e8c107dd707 src/transcoding/TranscodingPropertySpinBoxWidget.cpp dcc34efa2c66e1f7be2af64524be238cd4f2fe8e src/transcoding/TranscodingPropertyWidget.cpp 4767c5208542b3815a4ca57023150bc2f5c7cf42 src/transcoding/TranscodingSelectConfigWidget.h PRE-CREATION src/transcoding/TranscodingSelectConfigWidget.cpp PRE-CREATION tests/core-impl/collections/db/sql/TestSqlCollectionLocation.cpp 7e90d1cd5acc6ef101897ad277c3a2f992bf3150 tests/core/collections/MockCollectionLocationDelegate.h a58ca4b344e98f6b75da19cdd6b38172ecc95f59 Diff: http://git.reviewboard.kde.org/r/104213/diff/ Testing --- Works, both with Local Collection and in-the-works iPod collection. Screenshots --- Better Organize dialog title http://git.reviewboard.kde.org/r/104213/s/456/ Changes to the Configure Collection dialog http://git.reviewboard.kde.org/r/104213/s/457/ Revamped Transcode dialog http://git.reviewboard.kde.org/r/104213/s/458/ Thanks, Matěj Laitl ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Rework transcoding: remember encoder, transcode on move, cleaner code
On 10. 3. 2012 Julian Simioni wrote: Hi Matěj, Side question: you refer to an in-the-works ipod collection. Is this work in the main amarok git repository? If not, can you tell me where to find it? You can find it in the ipod-rewrite branch (which contains reworked transcoding) of my Amarok repository clone [1]. Then you have to enabled it (and disable the old one) in Amarok Configuration - plugins. It is almost ready now, I'd be happy if you tested it and reported back to me. [1] http://quickgit.kde.org/index.php?p=clones%2Famarok%2Flaitl%2Famarok.gita=shortlogh=refs/heads/ipod- rewrite Matěj ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Rework transcoding: remember encoder, transcode on move, cleaner code
On March 14, 2012, 11:10 a.m., Bart Cerneels wrote:
I think in this case the use of a Capability is completely justified. It's
the Capabilities that just add complexity that are problematic.
Thanks for review!
On March 14, 2012, 11:10 a.m., Bart Cerneels wrote:
Screenshot: Changes to the Configure Collection dialog
http://git.reviewboard.kde.org
If there are = 3 options, don't use a combobox.
Yup, there are 2 or 3. Should I use radio buttons?
On March 14, 2012, 11:10 a.m., Bart Cerneels wrote:
Screenshot: Revamped Transcode dialog
http://git.reviewboard.kde.org
This is hard to understand and contains some language errors. Perhaps
Use this for next tracks ?
I particularly suck at english. :) I wanted to justify that this option will be
used for both copying and moving and that the setting is used just for the
particular collection. Additionally, even the Just copy will be remembered.
All suggestions for the GUI string that will make these clear are welcome.
- Matěj
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104213/#review11394
---
On March 9, 2012, 11:31 p.m., Matěj Laitl wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104213/
---
(Updated March 9, 2012, 11:31 p.m.)
Review request for Amarok and Teo Mrnjavac.
Description
---
Rework transcoding: remember encoder, transcode on move, cleaner code
This is a major rework of transcoding feature that brings following
user-visible changes to Amarok:
* Amarok can remember preferred transcoding configuration per each
collection that supports transcoding. Therefore, the Use default
configuration work-around can go away and the Transcode or copy?
dialog can (and is) be one-step now. This preference can be changed
in configuration.
* Transcoding is now supported even during the move operation. No
worries, only successfully transcoded tracks are removed from their
original location.
* Only formats playable on the target collection are offered. Already
used tested in yet-to-be-merged iPod collection rewrite.
* The Organize Tracks dialog title and progress bar operation name
now more verbosely describe actual operation to prevent user
mistakes.
* Double-transcode when ripping audio CDs that caused failures is
avoided. (ChangeLog entry for this was miscredited to my earilier
commit)
Technically, following changes are made:
* many methods that accepted optional TranscodingConfiguration now
either have it mandatory or not at all.
* TranscodingConfiguration's NULL_CODEC was splitted to JUST_COPY and
INVALID along with convenience methods isValid() and isJustCopy().
This simplifies logic in many methods.
* CollectionLocation::prepare{Copy,Move}() now don't have optional
TranscodingConfiguration parameter. Depending on target collection,
CollectionLocation determines it automatically or asks user in
showSourceDialog() (overridable). AudioCdCollectionLocation already
overrides it.
* Collections that support transcoding now should expose
TranscodeCapability which is used to a) indicate that transcoding
is supported; b) query which file formats are playable on target
collection; c) read save unset preferred transcoding parameters.
Why the hell the new Capability?
Many Amarok devs dislike the concept of capabilities[1]. Why the hell I
introduced the new one? In ideal world Amarok would be able to transcode
everything regardless of the target collection. This is however not
doable witch current copyUrlToCollection() design - target collection
needs to do non-trivial things such as re-reading file tags, accounting
for different file name and space requirements etc. See my comments in
[1]. We therefore need a way for target collection to indicate it
supports transcoding (in order not to fool user). Some collection
locations such as TrashCollectionLocation should even intentionally
disallow transcoding. Additionally, we want to be able to query
supported destination file formats, to save preferred transcoding
paremeters etc.
I simply didn't want to pollute already over-crowded CollectionLocation
with three more methods used by only a few subclasses. On the other
hand, TranscodeCapability is not the central idea of this patch and I
can factor it into CollectionLocation should there be a voice supporting
it.
[1] https://git.reviewboard.kde.org/r/103752/
FEATURE: 280526
FEATURE: 264681
CCBUG: 291722
BUG: 263775
FIXED-IN: 2.6
REVIEW: TODO
DIGEST: Feature: much improved transcoding
Re: Review Request: Rework transcoding: remember encoder, transcode on move, cleaner code
d0ccdb33e8e54ecf4db205e10dbd8396eac488ad src/core-impl/collections/db/sql/SqlCollection.h 3a96bea1aa21761f12d956f7905f87808e0efc4a src/core-impl/collections/db/sql/SqlCollection.cpp 79714153661e50b70885b3e82cb6529b79ff98e2 src/core-impl/collections/db/sql/SqlCollectionLocation.h 1db72726d041713158be0fe91a93be3b1044b70e src/core-impl/collections/db/sql/SqlCollectionLocation.cpp 2c33da62565a9be4b5710cce590bac99d28b47ae src/core-impl/collections/mediadevicecollection/MediaDeviceCollectionLocation.h 24bb306c5b28a6d21f66f598f4caccbbb60f5bf8 src/core-impl/collections/support/CollectionLocationDelegateImpl.h 08a1762bb4c72b7ba32658651e00bf356e2cffba src/core-impl/collections/support/CollectionLocationDelegateImpl.cpp 23986570a42ad78557a394581233921abc668a8f src/core-impl/collections/support/PlaylistCollectionLocation.h 967d2150cf2c76f563dac0ff5396e84448826aed src/core-impl/collections/support/TrashCollectionLocation.h a9c92495abebd13506ed52a185355f21b55ee347 src/core-impl/collections/umscollection/UmsCollectionLocation.h 3fc0af405a8d47374afacdf5c1190d112b126d21 src/core/CMakeLists.txt 4db2105f08246898585bbe38ba4fbe0d006bd138 src/core/capabilities/Capability.h 42ffd7e89fd67d1b1b1b0b6f3a8ddc35cde5943e src/core/capabilities/TranscodeCapability.h PRE-CREATION src/core/capabilities/TranscodeCapability.cpp PRE-CREATION src/core/collections/CollectionLocation.h 35480b2000cccf9ece99b7654ff3852087b3144e src/core/collections/CollectionLocation.cpp 9b258ce9a19ab22f7027b674da4b76d1f15d973b src/core/collections/CollectionLocationDelegate.h c49a7445e260665e508795ae8dbee4ac9f271f71 src/core/transcoding/TranscodingConfiguration.h 378fe12ffa86764fe3ecf0cc5f2a38d8a47a0561 src/core/transcoding/TranscodingConfiguration.cpp 40f09be71ee0c75cbeeb6d3a4702c2a6f5ec3b99 src/core/transcoding/TranscodingController.h e9b04df6478e1f18d1e8c7d6d9859e6c1c86d9f3 src/core/transcoding/TranscodingController.cpp e16b6fe7d3d60621ef0a237951ac038f7846b51e src/core/transcoding/TranscodingDefines.h 4ba3e2ae9de710aa7a5f446eddf565e9f5138e4c src/core/transcoding/TranscodingFormat.h 5bf4e6098be4f3e08dcd45c48fdd264418660293 src/core/transcoding/TranscodingProperty.h 928854baa31eb4e78b3fe80768eb4f9811a0f5bf src/core/transcoding/TranscodingProperty.cpp 2b6752cfbc39e37880b05930dd9e797d60200c97 src/core/transcoding/formats/TranscodingNullFormat.h 96393e80f9a74a34a6858fcc114f2719089fac71 src/core/transcoding/formats/TranscodingNullFormat.cpp 98f09d1853833d2fd8b8de0603612ae337c6ef52 src/services/magnatune/MagnatuneCollectionLocation.cpp d75ea9edd0af9317b66f223961d325635fb26e33 src/services/mp3tunes/Mp3tunesServiceCollectionLocation.h c168a1f7bb07e798702b01f488912fd0b2c191c5 src/transcoding/CMakeLists.txt 06d5be85075581e979dc5fd8b411286200049846 src/transcoding/TranscodingAssistantDialog.h 718c01d40fed0113087b90e425a9102b365076cb src/transcoding/TranscodingAssistantDialog.cpp b64e74b9ee75c8b597a07c008a4e161333bb0d86 src/transcoding/TranscodingAssistantDialog.ui 912a1c2d636f4b24b1efd94185c0ae41f11ee96a src/transcoding/TranscodingOptionsStackedWidget.cpp 54c936dd041d9e23820afc8dbde662b5bb0dcd46 src/transcoding/TranscodingPropertyComboBoxWidget.h ecab1583fcb27ab78497e9c56f8c26a0e9a8b05a src/transcoding/TranscodingPropertyComboBoxWidget.cpp dbb2462f526ad5b47c14efa5c0f0983db296cb20 src/transcoding/TranscodingPropertySliderWidget.cpp 0a49da06127d9fa4dde9f8df95650e1a9a5ed6bc src/transcoding/TranscodingPropertySpinBoxWidget.h f1fa7f561c518f7420fc904252e35e8c107dd707 src/transcoding/TranscodingPropertySpinBoxWidget.cpp dcc34efa2c66e1f7be2af64524be238cd4f2fe8e src/transcoding/TranscodingPropertyWidget.cpp 4767c5208542b3815a4ca57023150bc2f5c7cf42 src/transcoding/TranscodingSelectConfigWidget.h PRE-CREATION src/transcoding/TranscodingSelectConfigWidget.cpp PRE-CREATION tests/core-impl/collections/db/sql/TestSqlCollectionLocation.cpp 7e90d1cd5acc6ef101897ad277c3a2f992bf3150 tests/core/collections/MockCollectionLocationDelegate.h a58ca4b344e98f6b75da19cdd6b38172ecc95f59 Diff: http://git.reviewboard.kde.org/r/104213/diff/ Testing --- Works, both with Local Collection and in-the-works iPod collection. Screenshots (updated) --- Better Organize dialog title http://git.reviewboard.kde.org/r/104213/s/456/ Changes to the Configure Collection dialog http://git.reviewboard.kde.org/r/104213/s/467/ Revamped Transcode dialog http://git.reviewboard.kde.org/r/104213/s/468/ Thanks, Matěj Laitl ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Fix bug 295275
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104321/#review11528 --- Hmm, my understanding why the code was there: 1) User starts to drag tracks from a collection tree view: CollectionTreeItemModelBase::mimeData() has following code: AmarokMimeData *mimeData = new AmarokMimeData(); mimeData-setTracks( tracks ); mimeData-setQueryMakers( queries ); // queries are non-empty only if you have something in search field. try: tracknumber:1 mimeData-startQueries(); 2) QueryMakers are started and processing while the user is dragging 3) User drops tracks; if she is _very_ quick, not all QueryMakes have completed. AmarokMimeData::tracks() therefore must assure that all QueryMakers complete (and the newResultReady() and queryDone() slots are activated) before returning the tracks, which is done in the loop this patch removes. I'd suggest using QEventLoop::ExcludeUserInputEvents, too. - Matěj Laitl On March 17, 2012, 11:10 p.m., Alexey Neyman wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104321/ --- (Updated March 17, 2012, 11:10 p.m.) Review request for Amarok. Description --- This patch fixes issue https://bugs.kde.org/show_bug.cgi?id=295275; here is a discussion from #amarok: [15:41:30] stilor Sentynel: could you reproduce if the processEvents() loop is removed from AmarokMimeData::tracks()? [15:42:43] stilor idea is, drag-n-drop, AFAIU is two events, drag and drop. The 1st event is fetched before CollectionTreeView::dragEnterEvent is called [15:42:58] Sentynel sounds plausible, let's have a look [15:43:10] stilor so this processEvents() loop fetches the drop event for which it doesn't have source [15:44:45] stilor I played aggressively with my mouse for a few minutes and can't get it to crash anymore [15:44:56] Sentynel yeah I can't get it to crash either [15:45:04] Sentynel so the next question is, why is that code there in the first place [15:45:39] stilor I have no idea, I didn't look into Amarok code until two days ago ;) [15:45:49] Sentynel whatever it is it's been there since 2007 [15:48:05] stilor yeah, I found the commit but the message doesn't say anything about why nested event processing is needed [15:51:15] Sentynel well, it doesn't seem to break anything as far as I can tell... [15:51:29] Sentynel my suggestion would be to submit a review request for taking that loop out [15:51:33] Sentynel and see if anybody else has any ideas why it's there [15:54:01] Sentynel the other thing that works is changing QEventLoop::AllEvents to QEventLoop::ExcludeUserInputEvents [15:54:22] Sentynel which stops the crash and should theoretically still let it process whatever events it was trying to process [15:54:51] Sentynel but I'd rather get rid of that code block entirely if it's not doing anything useful This addresses bug 295275. https://bugs.kde.org/show_bug.cgi?id=295275 Diffs - src/AmarokMimeData.cpp 226b0fa Diff: http://git.reviewboard.kde.org/r/104321/diff/ Testing --- Thanks, Alexey Neyman ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Add composer button to wikipedia applet
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104327/#review11530 --- The patch looks good, thanks for it! There are 2 smaller issues (one with the patch, one witch existing code) that I would love to see addressed before merging this. src/context/engines/wikipedia/WikipediaEngine.cpp http://git.reviewboard.kde.org/r/104327/#comment9160 Hmm, why don't we use i18nc() for creating the pattern here? Something with a very descriptive context string should do. This applies to all cases in the swich, not just this patch. src/context/engines/wikipedia/WikipediaEngine.cpp http://git.reviewboard.kde.org/r/104327/#comment9159 Who don't we tell user what exactly has happened? (the tracks has empty composer) That way you could also remove the useless debug() message. - Matěj Laitl On March 18, 2012, 6:27 a.m., Ryan McCoskrie wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104327/ --- (Updated March 18, 2012, 6:27 a.m.) Review request for Amarok. Description --- Adds a new composer button for the Wikipedia applet. Currently only works in English. This addresses bug 272982. https://bugs.kde.org/show_bug.cgi?id=272982 Diffs - src/context/applets/wikipedia/WikipediaApplet_p.h b0d6116 src/context/engines/wikipedia/WikipediaEngine.cpp a4ee870 src/context/applets/wikipedia/WikipediaApplet.h 897a32c src/context/applets/wikipedia/WikipediaApplet.cpp e326804 Diff: http://git.reviewboard.kde.org/r/104327/diff/ Testing --- Compiled Amarok, changed a file to have a different musician name to the composer name and pressed button. Everything works as expected. This patch is simply a case of copying, pasting, substituteing artist with composer so new bugs are extremely unlikely. Thanks, Ryan McCoskrie ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Removes unnecessary OpenGL constraints from the spectrum analyzer
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104330/#review11540 --- I've tried the patch. Previously the spectrum analyzer said: no sample buffer support. Now it pretends it is working but shows nothing or just some corrupted image that doesn't update. Following messages appear in debug output: QEglContext::swapBuffers(): Bad surface (0x300D) My card is Intel Sandy Bridge Integrated Graphics Controller. Perhaps lifting the limitation isn't this simple? - Matěj Laitl On March 18, 2012, 1:46 p.m., Daniel Dewald wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104330/ --- (Updated March 18, 2012, 1:46 p.m.) Review request for Amarok. Description --- Removes some unnecessary OpenGL constraints from the spectrum analyzer (See Bug# 290327). Also extended the remaining constraints error messages to clarify that its an OpenGL problem. This addresses bug 290327. https://bugs.kde.org/show_bug.cgi?id=290327 Diffs - src/context/applets/spectrumanalyzer/SpectrumAnalyzerApplet.cpp 8d5ad25 Diff: http://git.reviewboard.kde.org/r/104330/diff/ Testing --- Since my graphic card supports all the removed constraints I can't really test if the problem is gone now. Someone with an older card should test if the error message is gone now. Thanks, Daniel Dewald ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Removes unnecessary OpenGL constraints from the spectrum analyzer
On March 18, 2012, 2:10 p.m., Matěj Laitl wrote: I've tried the patch. Previously the spectrum analyzer said: no sample buffer support. Now it pretends it is working but shows nothing or just some corrupted image that doesn't update. Following messages appear in debug output: QEglContext::swapBuffers(): Bad surface (0x300D) My card is Intel Sandy Bridge Integrated Graphics Controller. Perhaps lifting the limitation isn't this simple? Daniel Dewald wrote: What kind of driver are you using in your setup for the graphic card? Can you use other OpenGL apps like glxgears? xf86-video-intel 2.18. glxgears runs fine along with more demanding 3D apps such as 0 A.D. game. - Matěj --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104330/#review11540 --- On March 18, 2012, 1:46 p.m., Daniel Dewald wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104330/ --- (Updated March 18, 2012, 1:46 p.m.) Review request for Amarok. Description --- Removes some unnecessary OpenGL constraints from the spectrum analyzer (See Bug# 290327). Also extended the remaining constraints error messages to clarify that its an OpenGL problem. This addresses bug 290327. https://bugs.kde.org/show_bug.cgi?id=290327 Diffs - src/context/applets/spectrumanalyzer/SpectrumAnalyzerApplet.cpp 8d5ad25 Diff: http://git.reviewboard.kde.org/r/104330/diff/ Testing --- Since my graphic card supports all the removed constraints I can't really test if the problem is gone now. Someone with an older card should test if the error message is gone now. Thanks, Daniel Dewald ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
GSoC Proposal: Statistics synchronization for pluggable devices and Last.fm
Hi Teo, Bart and list, as suggested by Teo, I've decided to try to take part in GSoC 2012 working as a student on Amarok. My idea is not on the KDE Ideas page, but I've been playing with (a variant of it) for months. Continues a very draft of my GSoC proposal. I'd be very grateful for any possible comments, pointed-out omissions or questions that may arise. Introduction === Amarok has an ability to store per-track play statistics such as play count, first last played date, rating and labels. It then has powerful means to generate custom-tailored playlists based on gathered statistics (e.g.: play me what I've listened to last month) that many users like to exploit. This works well when your computer is the only device you play music from. More likely situation is that you play music using Amarok at home, listen to iPod while commuting and use Amarok or another music player at work. All these 3 devices are able to keep track of what you've listened to, but each one only a third of it, which makes Amarok statistics more or less useless. This project aims to remedy the situation; Last.fm is an online service that can keep track of music a user listens to[1] and can help us with a part of this project. Amarok Users group on Last.fm has over 23 000 users.[2] Project Goals This project will implement: * track statistics synchronization between Amarok collections that support statistics; these are currently Local Collection and iPod Collection, but the framework will be general * Last.fm scrobbling from pluggable media players that support statistics (iPods, currently) using the general framework from previous point * ability to synchronize labels from Amarok to Last.fm * ability to synchronize play counts, first last played date from Last.fm to Amarok collections (other way around is already implemented by scrobbling) * GUI dialogue for performing the synchronization/resolving conflicts Bonus points (what will Amarok gain for free): * ability to synchronize statistics of Amarok and other media player that scrobbles to Last.fm * track statistics backup through Last.fm Caveats: * Last.fm has no concept of track ratings. This can be however worked around by special Last.fm-side labels such as 7/10 stars * advanced features will be only available for Last.fm users; Last.fm is free to use, but the data are public which may be unpleasant for certain users Implementation = Amarok represents audio tracks by Meta::Track abstract C++ class that provides getter methods for meta-data (title, artist, album..) and getter/setter methods for statistics (rating, play count...). These tracks are grouped into so-called Collections, where each Collection represents one source of songs (iPod, Local, USB Mass Storage..). Tracks from different collections will be matched together using their meta-data and other collection's QueryMaker to perform the search. Moreover, iPods provide additional data that can be used for conflict-resolution: app_rating and recent_playcount. [3] I plan to expose these as new capability offered by Collections. This capability will also be used to implement Last.fm scrobbling from iPods (in fact, every collection that will support this capability), exploiting recent_playcount field in the iPod case. It should be noted that I have already implemented similar synchronization in my spare time back in summer 2011 [4], but I was not satisfied with its iPod-specific design and GUI, so I decided not to strive for its inclusion. But I have the code working and ported to Amarok 2.5, so it can be used to fast-start this project. Another interesting note is that scrobble-from-iPod-to-Last.fm was functional in Amarok 1.4 days, but this feature got dropped during rewrites leading to 2.0, so this will fix one long-overdue regression. Speaking about Last.fm integration, Last.fm provides rather nice RESTful API [5] a subset of which is already used through liblastfm [6] library in Amarok to submit (scrobble) currently played songs. I plan to reuse this library and Amarok code dealing with it; the Last.fm API is powerful enough to support all claimed features. There is already even Last.fm on-line service Collection, but it focuses on playing Last.fm radio streams and doesn't handle individual tracks. In order to implement actual synchronization with Last.fm, user's Last.fm Library (that contains relevant track data) can be represented as a new (invisible) Collection or as special case in synchronizer, I have yet to decide this design choice. Timeline = [To be done when the general idea is accepted.] Generally: inter-collection synchronization will be first and will be done by the midterm evaluation, Last.fm support will be second. I'm already quite bound to Amarok community, so I can finish the design, iron-out some details and perhaps present some GUI mockups during the community bonding period. If accepted, GSoC will be my
Re: GSoC Proposal: Statistics synchronization for pluggable devices and Last.fm
On 21. 3. 2012 Christoph Obexer wrote: Thats an awesome idea! You could solve last.fm's downsides with ownCloud. Yup, thinking about this, I can hide Last.fm behind a thin abstraction layer, let's say ScrobblingService abstract class, then Last.fm, libre.fm and ownCloud could be just its implementations. It seems that ownCloud doesn't implement such interface, but this shouldn't be hard, it is just a kind of database of song plays with associated meta-data. Also it would be cool if this could work with android based devices, not sure if there are android music players that support statistics tough... This could be solved using 2 ways: a) (more likely) If the Android player can scrobble to Last.fm (or alternatives), Amarok will get its statistics through it b) (less likely) if the Android player can maintain statistics but cannot scrobble, Amarok can be taught to read its stats and synchronize and scrobble them for the device. (as it is done for iPods) Thanks for the comments, Matěj ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: GSoC Proposal: Statistics synchronization for pluggable devices and Last.fm
On 21. 3. 2012 Bart Cerneels wrote:
On Wed, Mar 21, 2012 at 00:06, Matěj Laitl ma...@laitl.cz wrote:
Project Goals
This project will implement:
* track statistics synchronization between Amarok collections that
support
statistics; these are currently Local Collection and iPod Collection, but
the framework will be general
* Last.fm scrobbling from pluggable media players that support statistics
(iPods, currently) using the general framework from previous point
* ability to synchronize labels from Amarok to Last.fm
* ability to synchronize play counts, first last played date from
Last.fm to Amarok collections (other way around is already implemented by
scrobbling) * GUI dialogue for performing the synchronization/resolving
conflicts
Implementation
=
Speaking about Last.fm integration, Last.fm provides rather nice RESTful
API [5] a subset of which is already used through liblastfm [6] library
in Amarok to submit (scrobble) currently played songs. I plan to reuse
this library and Amarok code dealing with it; the Last.fm API is powerful
enough to support all claimed features. There is already even Last.fm
on-line service Collection, but it focuses on playing Last.fm radio
streams and doesn't handle individual tracks. In order to implement
actual synchronization with Last.fm, user's Last.fm Library (that
contains relevant track data) can be represented as a new (invisible)
Collection or as special case in synchronizer, I have yet to decide this
design choice.
I very much see the need for statistics syncing between iPod and SQL
and similar. But I strongly object to a Collection for last.fm, even a
hidden one. It's corrupting the architecture (Last.fm does not
provide us distinct playable tracks, hence can not be a real
Collection) and further complicates the codebase with various special
cases. We've been suffering a similar mistake with Services already
and want to avoid making it again at all cost.
If all you really need for synchronization from last.fm to a
Collection is metadata and a QueryMaker, I don't see the need for a
Last.fm Collection, QueryMaker, etc. In order to keep it sane you'll
have to execute the sync from the last.fm service anyway and have a
config to select which Collections are synced (SQL as default). From
SQL to last.fm is already implemented with scrobbling.
When giving it more thought, I must acknowledge that creating a fake
Collection would be a bad idea. Instead, and thanks to suggestions by Markey,
Sam Christoph, Last.fm access could be accomplished through some kind of a
scrobbling service abstraction, for example:
class ScrobblingService
{
// mandatory:
virtual void scrobble( TrackMetaData track, QDateTime time = now() ) =
0;
// optional, constrained by API Last.fm provides [1] and performace:
virtual QStringList userArtists(); // ambiguity doesn't matter here
virtual QMultiMapTrackMetaData, QDateTime artistScrobbles( QString
artist
); // this seems to be the only way to get track first last played datetime
virtual QStringList trackLabels( TrackMetaData track );
virtual void setTrackLabels( TreackMetaData track, QStringList labels );
virtual int trackRating( TrackMetaData track );
virtual void setTrackRating( TrackMetaData track, int rating );
}
The calls are are possibly long-running, so the interface should be either
asynchronous (thus more complex) or blocking-but designed to be run from a
non-gui thread (which I would prefer).
Very good proposal in any case and I'm confident you can pull it off.
Oh, thanks for review and encouraging! I will update the implementation part
of the proposal; Also, I will reformulate it a bit to make clear that the
synchronization with on-line scrobbling services will not be Last.fm-specific.
(but this GSoC will still implement just the Last.fm backend)
Matěj
[1] http://www.last.fm/api/intro
___
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/#review11770 --- Hi, thanks for the patch, I'd really like to see this feature implemented. But there are issues with the patch; please resolve them. Please don't feel discouraged, no-one can make perfect patches for such big project with rather lacking developer documentation. src/amarokurls/AmarokUrl.h http://git.reviewboard.kde.org/r/104307/#comment9329 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(); src/amarokurls/AmarokUrl.cpp http://git.reviewboard.kde.org/r/104307/#comment9330 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. src/amarokurls/BookmarkModel.h http://git.reviewboard.kde.org/r/104307/#comment9333 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. src/amarokurls/BookmarkModel.cpp http://git.reviewboard.kde.org/r/104307/#comment9332 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) src/widgets/BookmarkTriangle.h http://git.reviewboard.kde.org/r/104307/#comment9331 What if slider width changes during the bookmark lifetime? Also, please also document new (or better, even the old) parameters. - Matěj Laitl 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://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: Some changes to make Amarok appearance more pretty
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103603/#review11771 --- src/browsers/AbstractTreeViewDelegate.h http://git.reviewboard.kde.org/r/103603/#comment9334 I would be more verbose here: bigFontMetrics(); smallFontMetrics(); src/browsers/CollectionTreeItemModelBase.cpp http://git.reviewboard.kde.org/r/103603/#comment9336 Sorry, I fooled you in a bad way, I wanted to say that the height should be left at 34 pixels (i.e. untouched). Could you please explain what are the benefits (direct, current - not hypothetical or philosofical) of the TreeItem - NormalTreeItem, MergedTreeItem split? Where exactly is avoids code duplication? - Matěj Laitl On Jan. 29, 2012, 6:42 p.m., Lucas Gomes wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103603/ --- (Updated Jan. 29, 2012, 6:42 p.m.) Review request for Amarok and Bart Cerneels. Description --- This is my attempt to make QTreeView subclasses items, used in Amarok, more pretty by displaying some extra information. Notice that these extra information are usually the quantity of tracks in a album, the quantity of episodes in a podcast and the quantity of episodes marked as new in a podcast. So, please help me to improve this feature even more by answering some questions: 1) Should I show the quantity of tracks on playlists listed in PlaylistBrowser too? 2) Is there any extra information that you think it's relevant to be showed somewhere (In QTreeViews)? Link for my personal repository (Look for ui-improve branch): http://quickgit.kde.org/index.php?p=clones%2Famarok%2Fgomes%2Fmaskmaster-amarok.gita=summary Diffs - src/browsers/collectionbrowser/CollectionBrowserTreeView.cpp 35a8222 src/browsers/collectionbrowser/CollectionMergedTreeItemDelegate.h PRE-CREATION src/browsers/collectionbrowser/CollectionMergedTreeItemDelegate.cpp PRE-CREATION src/browsers/collectionbrowser/CollectionNormalTreeItemDelegate.h PRE-CREATION src/browsers/collectionbrowser/CollectionNormalTreeItemDelegate.cpp PRE-CREATION src/browsers/CollectionTreeItemModelBase.cpp e7f8e62 ChangeLog 70dd420 src/CMakeLists.txt 4241e69 src/browsers/AbstractTreeViewDelegate.h PRE-CREATION src/browsers/AbstractTreeViewDelegate.cpp PRE-CREATION src/browsers/collectionbrowser/CollectionTreeItemDelegate.h 8a189e6 src/browsers/collectionbrowser/CollectionTreeItemDelegate.cpp 755be00 src/browsers/collectionbrowser/CollectionWidget.cpp ac1c26d src/browsers/playlistbrowser/PlaylistBrowserCategory.h 9198d43 src/browsers/playlistbrowser/PlaylistBrowserCategory.cpp 0c2f9c1 src/browsers/playlistbrowser/PlaylistBrowserView.cpp 9c4236d src/browsers/playlistbrowser/PlaylistMergedTreeItemDelegate.h PRE-CREATION src/browsers/playlistbrowser/PlaylistMergedTreeItemDelegate.cpp PRE-CREATION src/browsers/playlistbrowser/PlaylistNormalTreeItemDelegate.h PRE-CREATION src/browsers/playlistbrowser/PlaylistNormalTreeItemDelegate.cpp PRE-CREATION src/browsers/playlistbrowser/PlaylistTreeItemDelegate.h 3a094b0 src/browsers/playlistbrowser/PlaylistTreeItemDelegate.cpp bc76551 src/browsers/playlistbrowser/PlaylistsByProviderProxy.h 941268c src/browsers/playlistbrowser/PlaylistsByProviderProxy.cpp 12f2676 src/browsers/playlistbrowser/PlaylistsInFoldersProxy.h 9a01dbe src/browsers/playlistbrowser/PlaylistsInFoldersProxy.cpp 4268a82 src/browsers/playlistbrowser/PodcastCategory.cpp 1c353dc src/browsers/playlistbrowser/PodcastMergedTreeItemDelegate.h PRE-CREATION src/browsers/playlistbrowser/PodcastMergedTreeItemDelegate.cpp PRE-CREATION src/browsers/playlistbrowser/PodcastModel.h e88f4a1 src/browsers/playlistbrowser/PodcastModel.cpp 18334f6 src/browsers/playlistbrowser/PodcastNormalTreeItemDelegate.h PRE-CREATION src/browsers/playlistbrowser/PodcastNormalTreeItemDelegate.cpp PRE-CREATION src/browsers/playlistbrowser/UserPlaylistCategory.cpp b48a55f src/core-impl/podcasts/sql/SqlPodcastMeta.h 42ad039 src/core-impl/podcasts/sql/SqlPodcastMeta.cpp 1c3bdf4 src/core/podcasts/PodcastMeta.h 679f7ac src/core/podcasts/PodcastMeta.cpp b9851f7 src/widgets/TrackSelectWidget.cpp 5bd5059 Diff: http://git.reviewboard.kde.org/r/103603/diff/ Testing --- This patch should build. Everything is working as expected and there aren't any known issues. Screenshots --- CollectionBrowser http://git.reviewboard.kde.org/r/103603/s/420/ PodcastBrowser http://git.reviewboard.kde.org/r/103603/s/423/ Thanks, Lucas Gomes
Re: GSoC 2012 : Improvised proposal for 'Semantic Collection for Amarok'
On 2012-03-22, Phalgun Guduthur phalgun.gudut...@gmail.com wrote:
I have been working towards the 2012 GSoC idea 'Semantic Collection for
Amarok' since a month now. I have already sent in my first rough draft of
the proposal.
At that time, I promised a proof of Concept and you can it submitted on the
review board https://git.reviewboard.kde.org/r/104369/
It is a small patch demonstrating the read and write of Nepomuk index
through Amarok.
I was very positively surprised how simple the patch is, good!
I have also attached my improvised proposal after interactions with both
the mentors Teo and Vishesh.
Please review my proposal whenever you find time. Any feedback is
appreciated.
Hey, the proposal is very good. If I have a chance to implement the inter-
collection statistics synchronization, you'll get a way of synchronizing
Nepomuk and SQL collection for free, which will be good for users
transitioning betweem Nepomuk and SQL collections.
I would reword a few paragraphs of the technical details though:
The core of the project is to implement a NepomukCollection and
NepomukQueryMaker classes. The classes to handle the generated and indexed
metadata will be needed as well, eg a handler to write data back to Nepomuk,
to update the UI using the metadata from Nepomuk index etc.
These will be probably the Meta::{Track,Album,Artist,Year,Genre} subclasses.
Updating the GUI is done through their notifyObservers() methods and through
Collection's updated() signal for bigger changes. (But I agree that you may
want to have sime kind of Nepomuk helper class; something can go directly into
NepomukCollection though)
Also please note that the Meta::Track interaction with other Meta::
{Album,Artist,...} classes and Collection is a bit tricky to figure out:
1) if 2 tracks have same album (artist, ...), their album() (artist(), ...)
methods should return the exact same Album object.
2) There is a kind of a double link between Track and Album (Artist, ...)
classes: Track has album() and Album has tracks(). Given that all Meta classes
are memory-managed using reference counting trough KSharedPtr, you cannot
simply store a pointer to Album in Track and a list of track pointers in
Album, because that would essentially create a memory leak. But thanks to
Nepomuk, you might not face this problem. (for some inspiration how this is
solved in UMS and iPod Collections, you may have a look at src/core-
impl/collections/support/MemoryMeta.{h,cpp};)
3) While undocumented, all Meta classes' methods should be thread-safe
(Collection's need not)
4) Lifetime of a Meta class object is out of your control. It can happen that
their collection is deleted when they are still alive. This has been the
source of some crashes in past, so please count with it.
I'm confident you'll be able to get through this, I just wanted to point out
some things in advance that I faced when re-writing IpodCollection.
The NepomukQueryMaker can be developed into an API which can be used by
plugin developers to exploit the Nepomuk backend.
Hmm, I think it would be suboptimal to add plugin API just to
NepomukQueryMaker, better add it to the generic QueryMaker, so it will work
for all subclasses. But this feature should be IMO very low priority, this can
be easily done after the GSoC.
The existing database schema will be followed so as to not break the
existing applications and plugins. So, the propagation to a Nepomuk backend
would be seamless.
Hmm, I think I know what you wanted to say here, but database schema is an
implementation detail of the SQL collection. Perhaps you can say something
like Existing design of Meta classes [1] will be followed..
[1] http://amarok.kde.org/wiki/Development/Architecture
Cheers,
Matěj
___
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 ? 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) 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 ? I thought about the case when Amarok window is resized during playback. (plase correct me if I'm wrong) 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. Jasneet Bhatti wrote: 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. Hmm, okay. So don't include this fix in this patch, and give me some time to think more about this, because I see no quick solution right now. (Or open another review request if you come up with something) - Matěj --- 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
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/#review11800 --- src/amarokurls/AmarokUrl.h http://git.reviewboard.kde.org/r/104307/#comment9354 Please add the following documentation comment before the method: /** * Sets the url argument named @param name to @param value. Overrides any possible previous value. */ src/amarokurls/AmarokUrl.cpp http://git.reviewboard.kde.org/r/104307/#comment9355 Not needed anymore. src/amarokurls/AmarokUrl.cpp http://git.reviewboard.kde.org/r/104307/#comment9356 Ditto. src/amarokurls/AmarokUrl.cpp http://git.reviewboard.kde.org/r/104307/#comment9357 Sorry, I didn't formulate my remark well: the documenting comments should be added to method declarations in .h files (see above). src/amarokurls/BookmarkModel.h http://git.reviewboard.kde.org/r/104307/#comment9358 This method does 2 things (renaming and setting argument), but should do only one (setting argument). The signature should be just: void setBookmarkArg(const QString bookmarkName, const QString key, const QString value); Plase add documentation, too, in a format similat to what I haved showed above. src/amarokurls/BookmarkModel.h http://git.reviewboard.kde.org/r/104307/#comment9359 Ditto, should just set argument. (and the newName argument should therefore vanish) src/amarokurls/BookmarkModel.cpp http://git.reviewboard.kde.org/r/104307/#comment9360 Just to note, this debugging print should stay, because this actually points some kind of a mistake. But in order to be useful, it should be formulated more verbosely and perhaps turned into a warning(), like this: warning() Cannot set argument key of the bookmark name to value value - bookmark not found.; spaces are automatically added in operator. src/amarokurls/PlayUrlGenerator.h http://git.reviewboard.kde.org/r/104307/#comment9362 Please document what this method does and its parameters in a docstring and don't forget to note that caller must supply a valid bookmark name including the - mm:ss part. src/amarokurls/PlayUrlGenerator.cpp http://git.reviewboard.kde.org/r/104307/#comment9363 This will have to be split into 2 call wrt BookmarkModel changes. src/widgets/BookmarkTriangle.h http://git.reviewboard.kde.org/r/104307/#comment9365 Minor thing: is seems it would suffice to include MetaUtility in the .cpp file and therefore reduce compilation times. src/widgets/BookmarkTriangle.h http://git.reviewboard.kde.org/r/104307/#comment9364 Documented attributes, good! src/widgets/BookmarkTriangle.cpp http://git.reviewboard.kde.org/r/104307/#comment9366 Minor things: trailing whitespace, excessive debugging. - Matěj Laitl 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/#review11802 --- The patch greatly improved, thanks! There is just a bunch of minor issues; when there are resolved, it will be perfect. - Matěj Laitl 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/#review11839 --- Ship it! 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. - Matěj Laitl 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: GSoC 2012 : Improvised proposal for 'Semantic Collection for Amarok'
On 26. 3. 2012 Teo Mrnjavac wrote: 2) Can you please be a little more elaborate on why NepomukCollection it can't be completed before NepomukQueryMaker? What will you return in Collection's queryMaker() method? A collection without a querymaker is virtually unusable, it won't be shown in Collection browser etc. Matěj ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Prevent amarok from merging tracks with same title but different track numbers
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104423/#review11948 --- This patch looks very sane, thanks. What about adding disc number to the track key too? I can imagine a multi disc album with following tracks: CD1 01 Intro (...) CD2 01 Intro src/core/meta/support/MetaKeys.h http://git.reviewboard.kde.org/r/104423/#comment9458 very minor: this could be removed now - Matěj Laitl On March 27, 2012, 6:16 a.m., Alexey Neyman wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104423/ --- (Updated March 27, 2012, 6:16 a.m.) Review request for Amarok. Description --- In merged view, amarok merges the tracks if the tracks in an album have the same title even though they have different track number. It does not do so when shown collections separately. As far as I understand, the merged view is supposed to eliminate identical tracks - but tracks with different track numbers are obviously not identical. A couple of examples where album has more than one track with same name: - I have a concert recording where between the songs there are author's commentaries. Naturally, there are 11 tracks titled Commentary - Classical pieces often have parts titled by the tempo, e.g. track #2 Allegro. Oftentimes there's more than one part with the same tempo in a piece. This patch adds track number to Meta::TrackKey class - the ProxyCollection class which implements merged view uses Meta::TrackKey to determine if tracks are identical. Diffs - src/core/meta/support/MetaKeys.h e2227e6 src/core/meta/support/MetaKeys.cpp 7f8ce7d Diff: http://git.reviewboard.kde.org/r/104423/diff/ Testing --- Thanks, Alexey Neyman ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
GSoC Proposal v2: Statistics synchronization for pluggable devices and Last.fm
Hi, this is a second, improved version of my GSoC proposal that incorporates suggestions and comments made by Bart, Teo, Markey, Sam and Christoph, thanks for them! Keep them coming. I will upload identical version to GSoC site, too. Changes from v1: * made it clear that there will be abstraction for Last.fm access to support alternatives in future * provided more implementation details and how access to Last.fm will be carried out (ruling off any shadow collections) * detailed (hopefully enough) timeline (Hi Teo`) :-) * made my school obligations clear * brag a tiny little bit more in About me ;-) Abstract Amarok can maintain useful per-track play statistics and meta-data such as: play count, first/last played date, rating and labels; these are tracked for each collection separately. This project will implement statistics synchronization between all collections that support them (local, iPod) in its first part. Second part is to implement synchronization of the statistics from scrobbling services such as Last.fm to Amarok. (other way around already partially works) Contact === Name: Matěj Laitl Email Address: ma...@laitl.cz Freenode IRC Nick: strohel IM Service and Username: XMPP: ma...@laitl.cz Location: Prague, Czech Republic, GMT+1 Motivation for Proposal: Amarok has an ability to store per-track play statistics such as play count, first last played date, rating and labels. It then has powerful means to generate custom-tailored playlists based on gathered statistics (e.g.: play me what I've listened to last month) that many users like to exploit. This works well when your computer is the only device you play music from. More likely situation is that you play music using Amarok at home, listen to iPod while commuting and use Amarok or another music player at work. All these 3 devices are able to keep track of what you've listened to, but each one only a third of it, which makes Amarok statistics more or less useless. This project aims to remedy the situation; Last.fm is an on-line service that can keep track of music a user listens to[1] and can help us with a part of this project. Amarok Users group on Last.fm has over 23 000 users.[2] Project Goals: == This project will implement: * track statistics synchronization between Amarok collections that support statistics; these are currently Local Collection and iPod Collection, but the framework will be general * Revamp of the existing Last.fm-scrobbling mechanism including addition of a thin abstraction layer to allow alternative implementations * Last.fm scrobbling from pluggable media players that support statistics (iPods, currently) * ability to synchronize labels and ratings (see caveats below) from Amarok to Last.fm * ability to synchronize play counts, first last played date from Last.fm to Amarok collections (other way around is already implemented by scrobbling) * GUI dialogue for performing the synchronization/resolving conflicts Bonus points (what will Amarok gain for free): * ability to synchronize statistics of Amarok and other media player that scrobbles to Last.fm * track statistics backup through Last.fm * synchronization of Amarok and Nepomuk meta-data when Nepomuk collection is implemented Caveats: * Last.fm has no concept of track ratings. This can be however worked around by special Last.fm-side labels such as 7/10 stars * advanced features will be initially available only Last.fm users; implementations dealing with alternative sites can be added in future Implementation Details: === Amarok represents audio tracks by Meta::Track abstract C++ class that provides getter methods for meta-data (title, artist, album..) and getter/setter methods for statistics (rating, play count...). These tracks are grouped into so-called Collections, where each Collection represents one source of songs (iPod, Local, USB Mass Storage..). Tracks from different collections will be matched together using their meta-data and other collection's QueryMaker to perform the search. Moreover, iPods provide additional data that can be used for conflict-resolution: app_rating and recent_playcount. [3] I plan to expose these as new capability offered by Collections. This capability will also be used to implement Last.fm scrobbling from iPods (in fact, every collection that will support this capability), exploiting recent_playcount field in the iPod case. It should be noted that I have already implemented similar synchronization in my spare time back in summer 2011 [4], but I was not satisfied with its iPod-specific design and GUI, so I decided not to strive for its inclusion. But I have the code working and ported to Amarok 2.5, so it can be used to fast-start this project. Another interesting note is that scrobble-from-iPod-to-Last.fm was functional in Amarok 1.4 days, but this feature got dropped during rewrites leading
Re: Review Request: Diagnostics Dialog for Amarok.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104449/#review12109 --- Ship it! As Bart said, very good patch, thanks for it! Besides to what Bart already pointed out, I've found 2 minor things. src/MainWindow.h http://git.reviewboard.kde.org/r/104449/#comment9536 Nitpicking: perhaps unnecessary newline src/dialogs/DiagnosticDialog.h http://git.reviewboard.kde.org/r/104449/#comment9537 Any special reason why the QWeakPointer is used? If there is none, plain pointer should be used instead. - Matěj Laitl On March 31, 2012, 6:15 p.m., Andrzej Hunt wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104449/ --- (Updated March 31, 2012, 6:15 p.m.) Review request for Amarok. Description --- Adds a diagnostics dialog to Amarok. This shows versions for KDE, QT, Phonon, the Phonon backend, and all scripts and plugins. As described in https://bugs.kde.org/show_bug.cgi?id=296415. This patch also changes/corrects PluginManager::plugins() to be const. This addresses bug 296415. https://bugs.kde.org/show_bug.cgi?id=296415 Diffs - src/CMakeLists.txt 6e590e8 src/MainWindow.h b149cb9 src/MainWindow.cpp 98b1c77 src/PluginManager.h 6b9f3ca src/PluginManager.cpp c46b12f src/dialogs/DiagnosticDialog.h PRE-CREATION src/dialogs/DiagnosticDialog.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/104449/diff/ Testing --- Screenshots --- Screenshot of Dialog http://git.reviewboard.kde.org/r/104449/s/501/ Thanks, Andrzej Hunt ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Review Request: RFC patch: distinguish between mp4, m4a, m4v types in Amarok::FileType
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104481/ --- Review request for Amarok. Description --- RFC patch: distinguish between mp4, m4a, m4v types in Amarok::FileType iPod Collection (the new one) needs to distinguish somehow between MPEG-4 audio and video files; to make this generic, least bad approach it probably to use Meta::Track:type(). MetaFile::Track's type() is okay, as it just returns lowercased file extension. SqlTrack stores file type in db as numeric index to the Amarok::FileType enum, which currently has just one generic entry for MP4 files. This patch extends Amarok::FileType with M4a and M4v values and TagHelper to try to detect more specific MP4 file format. (currently just file extension based, can be extended in future) Users will need to do a full rescan for Local Collection to pick up more specialised file types. I'm running this through review board as there seems no general agreement on Meta::Track:type() semantics. (Speaking of which, I'd be most satisfied if it returned (the most specific) mime-type represented using dedicated class that would support mimetype hierarchy) BUG: 268238 FIXED-IN: 2.6 REVIEW: 104481 This addresses bug 268238. https://bugs.kde.org/show_bug.cgi?id=268238 Diffs - ChangeLog 4978be6074c5e9857085bc9b6170b9becde485a6 shared/FileType.h 5c8081fa10fe227855a8de6a31fca17ebac5f0e5 shared/FileType.cpp a6e25d5661dc60c40cd78a933c2697e26d4e8f64 shared/tag_helpers/TagHelper.cpp 4c0fb2b0e6361ead03d2b569499d6828858b1c67 Diff: http://git.reviewboard.kde.org/r/104481/diff/ Testing --- Thanks, Matěj Laitl ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Some changes to the handling of cover art reading and writing
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104513/#review12303 --- I agree that current hard-coded value of 200px is a bit suboptimal. But I doubt there is a significant group of users that makes use of multiple per-file covers. When I order Amarok to replace track cover, I want Amarok to make this picture the one and only cover. I would suggest raising the dafaut size to something more reasonable like 500px (storage is really cheap these days) and making it a hidden confing-file-only option documented in the handbook. Alternatively, I have a mockup of perhaps rather ugly extension to the config dialog that wouldn't get in the way. (I will attach soon) The work of cleaning up cover file reading and writing and fixing its bugs is of course welcome. - Matěj Laitl On April 8, 2012, 5:54 p.m., Daniel Faust wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104513/ --- (Updated April 8, 2012, 5:54 p.m.) Review request for Amarok. Description --- This patch includes a list of small changes that are supposed to give the user more control over the cover writing process and unify it for all file formats. 1. The user can set the maximum cover size in the config dialog instead of having a fixed 200px. (BR 279493) 2. The user can set how existing covers should be handled: - 'Replace existing front covers' (almost the current behavior except for m4a files and some cases where more than one 'front' cover exists) - 'Replace all existing covers' - 'Append new cover' (actually it's a prepend, doesn't work with wma files, though; the current behavior for m4a files) 3. Unify the reading of covers. Instead of searching for the biggest cover like it was implemented for some file formats from now on the first 'front' cover will be taken. If there is no 'front' cover, the first 'other' cover will be taken (if present). The 1kb limit is still present. 4. Fix a potential bug where covers couldn't be found in mp3 files if the first cover was neither a 'front' cover nor 'other' or smaller than 1kb. This addresses bug 279493. https://bugs.kde.org/show_bug.cgi?id=279493 Diffs - shared/tag_helpers/ASFTagHelper.cpp 93e6031 shared/tag_helpers/ID3v2TagHelper.cpp 27e0cf0 shared/tag_helpers/MP4TagHelper.cpp faeae0a shared/tag_helpers/VorbisCommentTagHelper.cpp 1fbb437 src/amarokconfig.kcfg 5610c4a src/core-impl/collections/db/sql/SqlMeta.cpp e663adf src/dialogs/CollectionSetup.h 3146f17 src/dialogs/CollectionSetup.cpp f1b7850 Diff: http://git.reviewboard.kde.org/r/104513/diff/ Testing --- I have tested writing covers with flac, mp3, and wma files. Screenshots --- http://git.reviewboard.kde.org/r/104513/s/508/ Thanks, Daniel Faust ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Some changes to the handling of cover art reading and writing
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104513/ --- (Updated April 10, 2012, 5:01 p.m.) Review request for Amarok. Changes --- I have added a mockup of alternative config GUI. Description --- This patch includes a list of small changes that are supposed to give the user more control over the cover writing process and unify it for all file formats. 1. The user can set the maximum cover size in the config dialog instead of having a fixed 200px. (BR 279493) 2. The user can set how existing covers should be handled: - 'Replace existing front covers' (almost the current behavior except for m4a files and some cases where more than one 'front' cover exists) - 'Replace all existing covers' - 'Append new cover' (actually it's a prepend, doesn't work with wma files, though; the current behavior for m4a files) 3. Unify the reading of covers. Instead of searching for the biggest cover like it was implemented for some file formats from now on the first 'front' cover will be taken. If there is no 'front' cover, the first 'other' cover will be taken (if present). The 1kb limit is still present. 4. Fix a potential bug where covers couldn't be found in mp3 files if the first cover was neither a 'front' cover nor 'other' or smaller than 1kb. This addresses bug 279493. https://bugs.kde.org/show_bug.cgi?id=279493 Diffs - shared/tag_helpers/ASFTagHelper.cpp 93e6031 shared/tag_helpers/ID3v2TagHelper.cpp 27e0cf0 shared/tag_helpers/MP4TagHelper.cpp faeae0a shared/tag_helpers/VorbisCommentTagHelper.cpp 1fbb437 src/amarokconfig.kcfg 5610c4a src/core-impl/collections/db/sql/SqlMeta.cpp e663adf src/dialogs/CollectionSetup.h 3146f17 src/dialogs/CollectionSetup.cpp f1b7850 Diff: http://git.reviewboard.kde.org/r/104513/diff/ Testing --- I have tested writing covers with flac, mp3, and wma files. Screenshots (updated) --- http://git.reviewboard.kde.org/r/104513/s/508/ Mockup of a possible gui for setting cover size. I must say I'm not a big fan of this. http://git.reviewboard.kde.org/r/104513/s/514/ Mockup of a possible GUI to configure cover size. I'm not a big fan of this. http://git.reviewboard.kde.org/r/104513/s/515/ Thanks, Daniel Faust ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Tidying up password storage in Amarok
On 9. 4. 2012 Andrzej J. R. Hunt wrote: I'd like to propose some changes to password storage in amarok, in particular the way KWallet is dealt with, and what is done if not available. Good. This is IMO needed. Most plugins use KWallet, but some resort to plaintext if KWallet isn't available (and some ask the user to allow this) -- meaning similar code is replicated over many plugins [some plugins only use plaintext currently]. I propose to write a wrapper class (PasswordManager ?), which uses KWallet if available, but if KWallet turns out not to be available then the user is asked once whether to use plaintext storage, with this setting being remembered across all plugins. Hmm, I may want to allow storing last.fm password in plain-text while disabling to store MySQL pass in plain-text. The confirmation should be probably per-plugin then. I would also add the option to PasswordManager to check for existing plaintext passwords, importing them to KWallet as necessary, to ease migration from older to newer versions of amarok (I could also add a panel to the amarok config allowing configuration of the password settings, i.e. to allow migration from plaintext to KWallet in case KWallet wasn't initally available, but becomes available; or the reverse -- that would be a later stage). Sane, but I suggest it is implemented using the most invisible-to-user way. (even if it would lead to some compromises) For example there are 2 or 3 popups when Amarok is first started, which I find rather embarrassing. (always think of your grandma using Amarok) Cheers, Matěj ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Tidying up password storage in Amarok
On 10. 4. 2012 Stefan Derkits wrote: Hmm, I may want to allow storing last.fm password in plain-text while disabling to store MySQL pass in plain-text. The confirmation should be probably per-plugin then. that sounds a little bit not so user-friendly (having to confirm secure storage for every plugin). I would suggest to always take the most secure storage available, without any config options or per plugin options. If I have a secure password store like KWallet why would I want to save any password in plain text? I didn't express myself correctly, for sure KWallet should be used by default without asking. I wanted to say that if KWallet isn't available, I may want to be asked for each password separately to store it in plain-text or not at all. (because some of them may be more valuable) (but this is not a big deal, I won't oppose merging even if this little thing is not implemented) Matěj ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Some changes to the handling of cover art reading and writing
On April 10, 2012, 4:57 p.m., Matěj Laitl wrote: I agree that current hard-coded value of 200px is a bit suboptimal. But I doubt there is a significant group of users that makes use of multiple per-file covers. When I order Amarok to replace track cover, I want Amarok to make this picture the one and only cover. I would suggest raising the dafaut size to something more reasonable like 500px (storage is really cheap these days) and making it a hidden confing-file-only option documented in the handbook. Alternatively, I have a mockup of perhaps rather ugly extension to the config dialog that wouldn't get in the way. (I will attach soon) The work of cleaning up cover file reading and writing and fixing its bugs is of course welcome. Daniel Faust wrote: When I order Amarok to replace track cover, I want Amarok to make this picture the one and only cover That's what I want, too. But it's not the current behavior that's why I implemented the configuration option. making it a hidden confing-file-only option The problem with hidden options is that nobody will know they are there. But still better than no option at all. Mockup of a possible gui for setting cover size. I must say I'm not a big fan of this You are right, it isn't perfect but it would be a shame to stop implementing features because the config dialog gets overcrowded. When I order Amarok to replace track cover, I want Amarok to make this picture the one and only cover That's what I want, too. But it's not the current behavior that's why I implemented the configuration option. I think we can assume that current behaviour is just wrong, or not well thought out. making it a hidden confing-file-only option The problem with hidden options is that nobody will know they are there. But still better than no option at all. Mockup of a possible gui for setting cover size. I must say I'm not a big fan of this You are right, it isn't perfect but it would be a shame to stop implementing features because the config dialog gets overcrowded. It is not a shame. I mean that well chosen default or some kind of good heuristics that works for 95% serves users better than a zillion of config options. But I'm leaving this up to you. Note that the mockup I presented cannot be used as is - it is basically untranslatable and won't work for right-to-left scripts. - Matěj --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104513/#review12303 --- On April 10, 2012, 5:01 p.m., Daniel Faust wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104513/ --- (Updated April 10, 2012, 5:01 p.m.) Review request for Amarok. Description --- This patch includes a list of small changes that are supposed to give the user more control over the cover writing process and unify it for all file formats. 1. The user can set the maximum cover size in the config dialog instead of having a fixed 200px. (BR 279493) 2. The user can set how existing covers should be handled: - 'Replace existing front covers' (almost the current behavior except for m4a files and some cases where more than one 'front' cover exists) - 'Replace all existing covers' - 'Append new cover' (actually it's a prepend, doesn't work with wma files, though; the current behavior for m4a files) 3. Unify the reading of covers. Instead of searching for the biggest cover like it was implemented for some file formats from now on the first 'front' cover will be taken. If there is no 'front' cover, the first 'other' cover will be taken (if present). The 1kb limit is still present. 4. Fix a potential bug where covers couldn't be found in mp3 files if the first cover was neither a 'front' cover nor 'other' or smaller than 1kb. This addresses bug 279493. https://bugs.kde.org/show_bug.cgi?id=279493 Diffs - shared/tag_helpers/ASFTagHelper.cpp 93e6031 shared/tag_helpers/ID3v2TagHelper.cpp 27e0cf0 shared/tag_helpers/MP4TagHelper.cpp faeae0a shared/tag_helpers/VorbisCommentTagHelper.cpp 1fbb437 src/amarokconfig.kcfg 5610c4a src/core-impl/collections/db/sql/SqlMeta.cpp e663adf src/dialogs/CollectionSetup.h 3146f17 src/dialogs/CollectionSetup.cpp f1b7850 Diff: http://git.reviewboard.kde.org/r/104513/diff/ Testing --- I have tested writing covers with flac, mp3, and wma files. Screenshots --- http://git.reviewboard.kde.org/r/104513/s/508/ Mockup of a possible gui for setting cover size. I must say I'm not a big fan of this. http://git.reviewboard.kde.org/r
Re: Tidying up password storage in Amarok
On 13. 4. 2012 Bart Cerneels wrote: Incidentally the MySQL configuration interface is implemented using KConfigXT (an xml file which is translated to c++, which then writes to plaintext, if I've understood it correctly), i.e. the settings aren't stored in KWallet. I'll look into whether that can be changed when I'm migrating the plugins to use PasswordManager. You're right. I think that it suffices to score just the database login name and password in KWallet, other options should be left in KConfigXT. I don't think it's really needed to move that to KWallet. What do you mean by that here? Login name? Database name? Database password? People using an external mysql server know how to set a unique name for their own local database. I'm not aware of any complaints about that. Even there wouldbe, I can't justify raising code complexity for it. If you want to change this, do it in a separate patch after the main one is accepted. Matěj ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Tidying up password storage in Amarok
On 13. 4. 2012 Bart Cerneels wrote: I don't think it's really needed to move that to KWallet. What do you mean by that here? Login name? Database name? Database password? database credentials in general. i.e. there is not real need for the database config to use kwallet. I oppose that, I _really_ don't want to have my database password stored in plain-text even if I can create extra database user for Amarok. Matěj ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: [amarok] src/browsers/playlistbrowser: Fix invalid assertion in PlaylistBrowserView::slotActivated()
On 13. 4. 2012 Myriam Schweingruber wrote: On Thu, Apr 12, 2012 at 22:09, Matěj Laitl ma...@laitl.cz wrote: Git commit 233004d4ab4620946c7dd61e09a8ec1b146673d9 by Matěj Laitl. Committed on 12/04/2012 at 21:05. Pushed by laitl into branch 'master'. Fix invalid assertion in PlaylistBrowserView::slotActivated() Since this commit Amarok crashes on start when the disk containing the collection is not mounted. The backtrace is useless, all I get is an error message from the database with a crash of the table 'urls'. Since my playlist contains podcasts this might be related? This is very unlikely to be related to the mentioned commit. It only touches code that is executed when you (double) click an item in playlist browser. If you run external MySQL server, you perhaps need to run mysqlrepair? It might be still useful if you submitted backtrace of all threads. Matěj ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Some changes to the handling of cover art reading and writing
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104513/#review12418 --- The newest iteration looks good to me. I'd still suggest that someone at least a bit experienced in UI design should review the UI. - Matěj Laitl On April 13, 2012, 7:42 p.m., Daniel Faust wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104513/ --- (Updated April 13, 2012, 7:42 p.m.) Review request for Amarok. Description --- This patch includes a list of small changes that are supposed to give the user more control over the cover writing process and unify it for all file formats. 1. The user can set the maximum cover size in the config dialog instead of having a fixed 200px. (BR 279493) 2. The user can set how existing covers should be handled: - 'Replace existing front covers' (almost the current behavior except for m4a files and some cases where more than one 'front' cover exists) - 'Replace all existing covers' - 'Append new cover' (actually it's a prepend, doesn't work with wma files, though; the current behavior for m4a files) 3. Unify the reading of covers. Instead of searching for the biggest cover like it was implemented for some file formats from now on the first 'front' cover will be taken. If there is no 'front' cover, the first 'other' cover will be taken (if present). The 1kb limit is still present. 4. Fix a potential bug where covers couldn't be found in mp3 files if the first cover was neither a 'front' cover nor 'other' or smaller than 1kb. This addresses bug 279493. https://bugs.kde.org/show_bug.cgi?id=279493 Diffs - shared/tag_helpers/ASFTagHelper.cpp 93e6031 shared/tag_helpers/ID3v2TagHelper.cpp 27e0cf0 shared/tag_helpers/MP4TagHelper.cpp faeae0a shared/tag_helpers/TagHelper.h f8e7fd9 shared/tag_helpers/VorbisCommentTagHelper.cpp 1fbb437 src/amarokconfig.kcfg 5610c4a src/core-impl/collections/db/sql/SqlMeta.cpp e663adf src/dialogs/CollectionSetup.h 3146f17 src/dialogs/CollectionSetup.cpp f1b7850 Diff: http://git.reviewboard.kde.org/r/104513/diff/ Testing --- I have tested writing covers with flac, mp3, and wma files. Screenshots --- http://git.reviewboard.kde.org/r/104513/s/508/ Mockup of a possible gui for setting cover size. I must say I'm not a big fan of this. http://git.reviewboard.kde.org/r/104513/s/514/ Mockup of a possible GUI to configure cover size. I'm not a big fan of this. http://git.reviewboard.kde.org/r/104513/s/515/ Still not perfect but better, I hope http://git.reviewboard.kde.org/r/104513/s/527/ Thanks, Daniel Faust ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Release plan for 2.6
On 29. 4. 2012 Myriam Schweingruber wrote: since we need to decide on a release plan for 2.6 let me make a suggestion: To give Matej some more time for his tests we could release a 2.6 beta on June 4th, then do the release 2 weeks later, on June 18th. What do you all think? The iPod collection merge went rather smooth AFAIK, so I'll probably need less time. Something like beta on May 21st or a few days earlier would work fine with me. (GSoC students could then base their work on 2.6-beta) Are 2 weeks enough for translators, testers packagers to catch up? I personally would give them a week or two more, but I have little experience with releasing Amarok. Cheers, Matěj ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
[amarok] src: Meta::Album: introduce canUpdateCompilation(), setCompilation()
Git commit 59fa0ee6d3f2cbbe1298e74b7fc56f838712fa0e by Matěj Laitl.
Committed on 01/05/2012 at 18:14.
Pushed by laitl into branch 'master'.
Meta::Album: introduce canUpdateCompilation(), setCompilation()
and convert subclasses to use these. Many many subclasses already had
equivalent methods, this reduces code duplication and eliminates one
static_cast. Some subclasses had equivalent methods but never used
them, these are deleted in order to avoid dead code.
This is a prerequisite for next commit that unifies album actions
across collections. This is rather a bigger change, so I'm CCing
list - if anyone opposes, raise your voice.
CCMAIL: amarok-devel@kde.org
M +0-1src/amarokurls/BookmarkTreeView.cpp
M +1-1src/core-impl/collections/audiocd/AudioCdCollection.cpp
M +13 -7src/core-impl/collections/audiocd/AudioCdMeta.cpp
M +3-1src/core-impl/collections/audiocd/AudioCdMeta.h
M +0-6src/core-impl/collections/daap/DaapMeta.cpp
M +0-1src/core-impl/collections/daap/DaapMeta.h
M +8-0src/core-impl/collections/db/sql/SqlMeta.cpp
M +2-11 src/core-impl/collections/db/sql/SqlMeta.h
M +13 -0src/core-impl/collections/ipodcollection/IpodMeta.cpp
M +3-0src/core-impl/collections/ipodcollection/IpodMeta.h
M +19 -1src/core-impl/collections/support/MemoryMeta.cpp
M +14 -3src/core-impl/collections/support/MemoryMeta.h
M +0-6src/core-impl/collections/upnpcollection/UpnpMeta.cpp
M +0-1src/core-impl/collections/upnpcollection/UpnpMeta.h
M +0-5src/core-impl/meta/timecode/TimecodeMeta.cpp
M +0-1src/core-impl/meta/timecode/TimecodeMeta.h
M +13 -0src/core/meta/Meta.h
M +11 -6src/services/ServiceMetaBase.cpp
M +3-1src/services/ServiceMetaBase.h
M +1-1src/services/amazon/AmazonParser.cpp
http://commits.kde.org/amarok/59fa0ee6d3f2cbbe1298e74b7fc56f838712fa0e
diff --git a/src/amarokurls/BookmarkTreeView.cpp
b/src/amarokurls/BookmarkTreeView.cpp
index f5e8502..5c5261a 100644
--- a/src/amarokurls/BookmarkTreeView.cpp
+++ b/src/amarokurls/BookmarkTreeView.cpp
@@ -388,7 +388,6 @@ void BookmarkTreeView::slotCreateTimecodeTrack() const
track-setGenre( genre );
album-setAlbumArtist( artist );
-album-setIsCompilation( false );
//make the user give us some info about this item...
diff --git a/src/core-impl/collections/audiocd/AudioCdCollection.cpp
b/src/core-impl/collections/audiocd/AudioCdCollection.cpp
index c0e9f8e..637c2d3 100644
--- a/src/core-impl/collections/audiocd/AudioCdCollection.cpp
+++ b/src/core-impl/collections/audiocd/AudioCdCollection.cpp
@@ -291,7 +291,7 @@ AudioCdCollection::infoFetchComplete( KJob *job )
trackPtr-setArtist( artistPtr );
else
{
-albumPtr-setIsCompilation( true );
+albumPtr-setCompilation( true );
Meta::AudioCdArtistPtr trackArtistPtr =
Meta::AudioCdArtistPtr( new Meta::AudioCdArtist( trackArtist ) );
trackArtistPtr-addTrack( trackPtr );
diff --git a/src/core-impl/collections/audiocd/AudioCdMeta.cpp
b/src/core-impl/collections/audiocd/AudioCdMeta.cpp
index 7ef9085..d767125 100644
--- a/src/core-impl/collections/audiocd/AudioCdMeta.cpp
+++ b/src/core-impl/collections/audiocd/AudioCdMeta.cpp
@@ -390,6 +390,19 @@ AudioCdAlbum::isCompilation() const
}
bool
+AudioCdAlbum::canUpdateCompilation() const
+{
+return true;
+}
+
+void
+AudioCdAlbum::setCompilation( bool compilation )
+{
+DEBUG_BLOCK
+m_isCompilation = compilation;
+}
+
+bool
AudioCdAlbum::hasAlbumArtist() const
{
return !m_albumArtist.isNull();
@@ -450,13 +463,6 @@ AudioCdAlbum::setAlbumArtist( AudioCdArtistPtr artist )
m_albumArtist = artist;
}
-void
-AudioCdAlbum::setIsCompilation( bool compilation )
-{
-DEBUG_BLOCK
-m_isCompilation = compilation;
-}
-
//AudioCdGenre
AudioCdGenre::AudioCdGenre( const QString name )
diff --git a/src/core-impl/collections/audiocd/AudioCdMeta.h
b/src/core-impl/collections/audiocd/AudioCdMeta.h
index 083a3e1..73ac547 100644
--- a/src/core-impl/collections/audiocd/AudioCdMeta.h
+++ b/src/core-impl/collections/audiocd/AudioCdMeta.h
@@ -164,6 +164,9 @@ class AudioCdAlbum : public Meta::Album
virtual QString name() const;
virtual bool isCompilation() const;
+virtual bool canUpdateCompilation() const;
+virtual void setCompilation( bool compilation );
+
virtual bool hasAlbumArtist() const;
virtual ArtistPtr albumArtist() const;
virtual TrackList tracks();
@@ -176,7 +179,6 @@ class AudioCdAlbum : public Meta::Album
//AudioCdAlbum specific methods
void addTrack( AudioCdTrackPtr track );
void setAlbumArtist( AudioCdArtistPtr artist );
-void setIsCompilation( bool compilation );
private:
QString m_name;
diff --git a/src/core-impl/collections/daap
Re: Review Request: Fix Transcoding to iPods/iPhone with ffmpeg 0.10
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104839/#review13390 --- Thanks for the patch, it indeed doesn't work with newer ffmpeg, I faced this too, but was too lazy to fix it. However, from what I've seen -map_meta_data was just renamed to -map_metadata and I don't know if it is enabled by default. Could you please point me to the relevant poiece of ffmpeg documentation that says so? If it is confirmed, I'll be happy to merge this. - Matěj Laitl On May 3, 2012, 11:07 a.m., Julian Simioni wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104839/ --- (Updated May 3, 2012, 11:07 a.m.) Review request for Amarok. Description --- While it seemed to work a while ago (before it was merged to trunk), I recently found that the new ipod manager cannot transcode tracks when sending them to my iPod. I spent some time debugging it and was able to find the cause: The -map_meta_data option was depricated as of ffmpeg 0.7 and removed in 0.10. Additionally, as far as I can tell the options specified did nothing but explicitly confirm the default option of copying infile metadata to outfile metadata. Please let me know if I'm mistaken about this conclusion. I also made some small changes to the debug output that should make things slightly cleaner. When pulling, please use branch fixTranscode at git://github.com/orangejulius/amarok.git as it has separate commits ready to go :) Diffs - src/transcoding/TranscodingJob.cpp 5b30c44 Diff: http://git.reviewboard.kde.org/r/104839/diff/ Testing --- Tested transcoding FLAC music files to ALAC while copying to my iPod classic 160GB Thanks, Julian Simioni ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Release plan for 2.6
On 8. 5. 2012 Myriam Schweingruber wrote: OK, do we have many new strings to translate? If not, 3 weeks should suffice, so let's agree on this schedule: I don't know the counts, but there's a bunch of new strings in iPod collection and in reworked transcoding. 28. May - 2.6 beta I think we can make the beta a week earlier. The current release blockers list seems manageable and even it it slips, the translators will have enough time. (given that translations need to be done a few days before official release) 18. June - 2.6 final Fine with me. Matěj ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Improve automatic scrolling of text in Lyrics applet
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104935/#review13805 --- Ship it! I wanted to do exactly the same for ages, thanks for the patch! Only one small point - using floating point arithmetics is slightly slower, but in infrequent cases like this the slowdown is absolutelny below measurable margin - other things like repainting are thousands times slower. But I'm fine leaving it like this if you can ensure the rounding error won't be noticeable. - Matěj Laitl On May 13, 2012, 11:42 p.m., Alexander Potashev wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104935/ --- (Updated May 13, 2012, 11:42 p.m.) Review request for Amarok, Leo Franchi and Rick W. Chen. Description --- The former scrolling algorithm was to scroll text continuously while playing a song. In this case the first lines of lyrics hide very quickly - often before they are sung. Again, with the old algorithm the scrollbar position is 0 at the beginning of the song (0:00), and it reaches the maximum possible position exactly when the song ends. The new algorithm tries to keep the currently played line at center of the Lyrics applet window. To achieve this, we target scrollbar position to be (-pageStep/2) at the beginning of the song (0:00), and to reach position (maximum + pageStep/2) exactly when the song ends. `pageStep` is the height of Lyrics applet window expressed in the units used by the scrollbar. Therefore `pageStep/2` is an offset that can be added/substracted to move down/up by half a page in the Lyrics applet. Here is a formula to transform a scrollbar position used in the old algorithm to the scrollbar position when the new algorithm is used: new_pos = pos / maximum * (maximum + pageStep) - pageStep / 2 We make the following changes to the algorithm: 1. Multiply by (maximum + pageStep) instead of `maximum` to fulfill the multiplication factor from the above formula. 2. Additionally, substract `pageStep/2`, as defined by the above formula. 3. Avoid using floating point math (the double data type) to improve performance. 4. Re-read the current position of the scrollbar when saving `oldSliderPosition`, because scrollbar positions calculated using the above formula may be out of the allowed range of the scrollbar and truncated, i.e. adjusted to fit the scrollbar range. When truncated, `vbar-value()` differs from `newSliderPosition`. Diffs - src/context/applets/lyrics/LyricsApplet.cpp 9720db0 Diff: http://git.reviewboard.kde.org/r/104935/diff/ Testing --- Works as expected. Thanks, Alexander Potashev ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: DB: change lyrics table: text url - integer url pointing to the urls table
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104966/ --- (Updated May 22, 2012, 8:04 p.m.) Review request for Amarok and Ralf Engels. Changes --- Changes in ChangeLog, remove misleading comment, update HACKING/mysql_database_schema.txt Description (updated) --- DB: change lyrics table: text url - integer url pointing to the urls table I believe that the old lyrics table structure was more or less a mistake: TABLE lyrics ( id INTEGER PRIMARY KEY, -- actually never used in code url VARBINARY(324), -- actually a rpath from urls table lyrics TEXT ) Apart from data duplication and non-conformity to the update anomaly requirement of the database design, there was additional problem that rpath itself is not unique. The (deviceId, rpath) is. This change makes the lyrics table look more sane: TABLE lyrics ( url INTEGER PRIMARY KEY, -- points to url.id column lyrics TEXT ) with an effort to transition existing entries. The transition of 5000 lyrics entries takes 16s on my 2.5 GHz Core i5 (one core used), so it should be acceptable. This is the first time I'm changing db structure, I'd be glad to receive thorough review, namely of the update13to14() method and especially the duplicate-removing query. This is critical because database-corrupting fault would leave many Amarok users in a state where they would need to drop their database to make Amarok working again. Note to reporters of bug 242350: there's an unrelated bug 299150 which now applies to lyrics, too. ChangeLog of the unrelated iPod fix is updated because DB_VERSION bump triggers full collection rescan as far as it is documented. BUG: 242350 FIXED-IN: 2.6 REVIEW: 104966 This addresses bug 242350. https://bugs.kde.org/show_bug.cgi?id=242350 Diffs (updated) - ChangeLog 67bc020387a1b4bc8988c2688a82976eb49fed2f HACKING/mysql_database_schema.txt 547ffd5385b82523ced1038606db154b5cd3f640 src/core-impl/collections/db/sql/DatabaseUpdater.h 37ccb54197a8b63c77b3bb7bcceaae838db56538 src/core-impl/collections/db/sql/DatabaseUpdater.cpp e8cbd42de15ee976de37eeff866d4596c3173328 src/core-impl/collections/db/sql/SqlMeta.cpp f8f9bdb7f0c83d3584ebfe2ce1c044bc54495885 Diff: http://git.reviewboard.kde.org/r/104966/diff/ Testing --- Thanks, Matěj Laitl ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: GSoC report
On 3. 6. 2012 Riccardo Iaconelli wrote: Hi, just a quick note to follow up the weekly schedule: this week i've been busy with upcoming exams, and so I haven't done a real lot. I will start committing to Amarok (in a branch) soonish. Hi Riccardo, in a branch in my personal repository clone (see KDE techbase how to setup one, it's easy) would be even greater. Cheers, Matěj ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: [amarok] /: EditablePlaylistCapability is IMO nonsense, remove it altogether
On 6. 6. 2012 Bart Cerneels wrote: Agreed. The whole Capability idea needs to be re-evaluated. Makes little sense outside of Loving tracks in Last.fm streams I think. What design pattern is that anyway? Agreed. Well, I don't think it should go away, but it is currently designed in a rather unfortunate way: * memory management should be responsibility of the entity, not the caller: MetaCapability::has() should be is() and ::create() should be ::as(). Entities then could multiple-inherit the capabilities and return themselves. See Solid::Device. * Capabilities are too generic now, they now serve all MetaBase entities and Collections. I think caps for just Collection and Meta::Track would suffice. Albums have just ActionsCapability that can be now the same for all albums, so it is unnecessary generalization. Something for the agenda for Randa. Definitely. Matěj ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Amarok totally broken since yesterday's commits
On 7. 6. 2012 Myriam Schweingruber wrote: Hi everyone, folks, something is going totally wrong here, the Collection Browser has become totally unusable: - impossible to see anything but in the Merged View: the space below the header when unfolded briefly flashes up and disappears This is a visual problem that I have occasionally seen form months or more. For me, just starting a drag inside the collection browser fixes the visual glitches. - all album and track related options are either gone or grayed out as if the tracks were not there anymore: * all album related options in the context menu are gone I can reproduce this _only_ if that album has a track that appears in multiple collections. It's easy to guess that album options are not shown due to a check all tracks from one album. * Edit track details show everything grayed out Not for me, but edit tags dialog does not show up for a track that is merged from multiple collections. Additionally, all tracks claim then are from the collection: None in Merged View. As a result, Move to trash is not displayed for them. While all these are problems, are these actually regressions? I think not. (but I have never used Merged View much) Matěj ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Build failed in Jenkins: amarok_master #132
On 8. 6. 2012 Jenkins CI Daemon wrote: See http://build.kde.org/job/amarok_master/132/ Wow, I always wanted build failures to be mailed to amarok-devel (and in future, test failure regressions, too). Thanks to whoever who have set this up. Matěj ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: MediaDeviceCache: remove polling, solid events should suffice
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105221/#review14624
---
From the code:
297 if ((*it)-mountType() == nfs || (*it)-mountType() == nfs4
||
298 (*it)-mountType() == smb || (*it)-mountType() ==
cifs) {(...)
This was supposedly just for the network filesystems. Can anybody test with a
collection folder on one of those network fs's? With positive results, we could
even push to 2.6 final.
- Matěj Laitl
On June 11, 2012, 3:14 p.m., Matěj Laitl wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105221/
---
(Updated June 11, 2012, 3:14 p.m.)
Review request for Amarok.
Description
---
MediaDeviceCache: remove polling, solid events should suffice
This fixes a bug where Amarok (very probably needlessly) polls solid
for all devices every single second (!!!) just to detect whether some
unmounted paths become mounted or vice versa. This should not be needed
at all, solid should notify us about everything.
However, I am not sure, so this is definitely not a material for 2.6
final but rather 2.7 if no problems show up.
BUG: 289462
FIXED-IN: 2.7
REVIEW: 105221
This addresses bug 289462.
https://bugs.kde.org/show_bug.cgi?id=289462
Diffs
-
src/MediaDeviceCache.h a48d453213e684d10b0a38b5b8ac01ae39680b52
src/MediaDeviceCache.cpp 15583b8d4eb14f842242deaab18bc2d7033b5991
Diff: http://git.reviewboard.kde.org/r/105221/diff/
Testing
---
little
Thanks,
Matěj Laitl
___
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Do not return collectionFolders() unless ready.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105246/#review14729 --- src/MountPointManager.cpp http://git.reviewboard.kde.org/r/105246/#comment11633 Hmmm, IMO this can be a source of subtle bugs where calling this important method is dependent on when it is called. I fear that callers of ::collectionFolders() cache the result so that could have long-ranging consequences. I'd rather see a fix that touches the _caller_ to ensure that MountPointManager is ready. - Matěj Laitl On June 13, 2012, 8:44 p.m., Edward Hades Toroshchin wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105246/ --- (Updated June 13, 2012, 8:44 p.m.) Review request for Amarok. Description --- If a collectionFolders() method of MountPointManager is called before it has completed identifying mount points, it adds the default path like $HOME/Music. This addresses bug 286219. https://bugs.kde.org/show_bug.cgi?id=286219 Diffs - src/MountPointManager.h 910f777 src/MountPointManager.cpp d3238c0 Diff: http://git.reviewboard.kde.org/r/105246/diff/ Testing --- Thanks, Edward Hades Toroshchin ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
