Re: CUE sheets in collection still not working :(
On Sun, Apr 14, 2013 at 12:11 PM, Matěj Laitl ma...@laitl.cz wrote: On 11. 4. 2013 Myriam Schweingruber wrote: Hi all, This is just a reminder for https://bugs.kde.org/show_bug.cgi?id=187587 This is a long standing bug and the one with the highest amount of votes: 751 It would be really nice if we could concentrate to solve this for the next release, as it is really annoying. Bart, what about turning this into Unified CUE file and Audiobook chapter support in Amarok GSoC project? I've thought about it and CUE files and audiobooks with chapters are essentially the same, just the medium to store the the chapters/parts is different, but inside Amarok these can share the very same code paths. Was about to suggest doing it as a GSoC. I can give some suggestions but can't mentor unfortunately. If someone steps up to mentor, contact me. P.S. My amarok emails are filtered out of my inbox ATM, please CC me if you want to make sure I see it. ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request 107473: Changes in processing playlist files
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107473/#review30244 --- A suggestion: start a feature branch to work on this. Mucking about with patches is not very efficient. I think you 2 can discuss the code in details without the help of reviewboard by now. And before merging you can upload it for wider review. I can't comment on the implementation (can't make the time to review) but would like to see the result. That is easy in a feature branch. - Bart Cerneels On April 1, 2013, 6:57 p.m., Tatjana Gornak wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107473/ --- (Updated April 1, 2013, 6:57 p.m.) Review request for Amarok. Description --- I've started my changes with an implementation of a ASXPlaylist class, due this work I've found that implementation of different playlist file types has different logic, as result I end up with a rewriting playlist's implementations. As example of difference: -- Constructor M3UPlaylist::M3UPlaylist( const KUrl url ) sets a url, but does not load playlist, therefore loading of playlist are posponed till data from playlist are actualy needed (lazy loading) On the other hand constructor XSPFPlaylist::XSPFPlaylist( const KUrl url, bool autoAppend ) loads playlist. The main idea of proposed changes is to create a unify code for processing playlist files: -- lazy loading through LazyLoadPlaylist -- common functionality was moved to PlaylistFile. It is worth to be noticed that this patch changes structure of playlist's classes, bit does not changes their behavior, even if this behavior is inconsistent in some cases. In following patch-requests I plan to submit ASX Playlist parser and organize playlists processing in more consistent way. This addresses bug 291934. https://bugs.kde.org/show_bug.cgi?id=291934 Diffs - src/CMakeLists.txt 4cdee5e src/DirectoryLoader.h e53a30d src/DirectoryLoader.cpp bd7fa34 src/browsers/playlistbrowser/PlaylistBrowserModel.h 1b7a44d src/browsers/playlistbrowser/PlaylistBrowserModel.cpp 213af3b src/context/applets/info/InfoApplet.cpp f215885 src/core-impl/collections/ipodcollection/IpodPlaylistProvider.h 4dd79f3 src/core-impl/collections/umscollection/podcasts/UmsPodcastProvider.cpp 1a4a934 src/core-impl/playlists/types/file/PlaylistFile.h 43dfdeb src/core-impl/playlists/types/file/PlaylistFile.cpp 2730354 src/core-impl/playlists/types/file/PlaylistFileLoaderJob.h PRE-CREATION src/core-impl/playlists/types/file/PlaylistFileLoaderJob.cpp PRE-CREATION src/core-impl/playlists/types/file/PlaylistFileSupport.h b1a1c26 src/core-impl/playlists/types/file/PlaylistFileSupport.cpp 2dcc0cd src/core-impl/playlists/types/file/m3u/M3UPlaylist.h 7b982fe src/core-impl/playlists/types/file/m3u/M3UPlaylist.cpp 2395260 src/core-impl/playlists/types/file/pls/PLSPlaylist.h e69e133 src/core-impl/playlists/types/file/pls/PLSPlaylist.cpp 8fe4b51 src/core-impl/playlists/types/file/xspf/XSPFPlaylist.h 6cf33e9 src/core-impl/playlists/types/file/xspf/XSPFPlaylist.cpp bbcecc3 src/core-impl/podcasts/sql/SqlPodcastMeta.cpp 1a025ce src/core-impl/podcasts/sql/SqlPodcastProvider.cpp 0da9b51 src/core/playlists/Playlist.h 018d2af src/core/playlists/Playlist.cpp 36879a1 src/core/playlists/PlaylistProvider.h 0fb5818 src/playlist/PlaylistActions.cpp 00bf13a src/playlist/PlaylistController.cpp e5bde8b src/playlist/PlaylistRestorer.h PRE-CREATION src/playlist/PlaylistRestorer.cpp PRE-CREATION src/playlistmanager/PlaylistManager.h cbf65b0 src/playlistmanager/PlaylistManager.cpp 89c754b src/playlistmanager/SyncedPlaylist.h fce3d14 src/playlistmanager/SyncedPlaylist.cpp 4f7c0b7 src/playlistmanager/file/PlaylistFileProvider.cpp e4e7284 src/playlistmanager/sql/SqlPlaylist.cpp 88e685b src/playlistmanager/sql/SqlUserPlaylistProvider.cpp 5eb6987 src/services/gpodder/GpodderProvider.h 8e856e3 tests/TestDirectoryLoader.h 0583a9e tests/TestDirectoryLoader.cpp b98247d tests/core-impl/meta/multi/CMakeLists.txt bda6387 tests/core-impl/meta/multi/TestMetaMultiTrack.h 8379bf4 tests/core-impl/meta/multi/TestMetaMultiTrack.cpp 8e9e47d tests/core-impl/playlists/types/file/m3u/TestM3UPlaylist.cpp b25d059 tests/core-impl/playlists/types/file/pls/TestPLSPlaylist.h 1183599 tests/core-impl/playlists/types/file/pls/TestPLSPlaylist.cpp 5eab2f3 tests/core-impl/playlists/types/file/xspf/TestXSPFPlaylist.cpp 5ea3a41 tests/core/playlists/CMakeLists.txt e2e0ce4 tests/core/playlists
Re: Review Request 108686: hidden items in context menu: usability question
On Mon, Feb 18, 2013 at 3:29 PM, Edward Hades Toroshchin edward.ha...@gmail.com wrote: On Mon, Feb 18, 2013 at 02:22:14PM -, Bart Cerneels wrote: Yes I am, it's nice and cool here. Don't have much chance to check the commits though. Shouldn't this review have been closed already? Guys, I propose using our mailing list more, and IRC less. Otherwise such things happen: we discussed something on IRC, and Bart wasn't aware of that. Good plan. Bart, I've committed the hint before the review. Then strohel asked me to open this review to ask the usability folks to help. Which I did. Did you get a reply? As a side note: I think there are higher priority tasks for the amarok team then usability right now. Sorry for keeping you from it by my spamming. ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: [amarok] src: Remove unused AudioCdTrackProvider
Wasn't this part of AudioCdCollection? How did that ever work?
On Sun, Feb 17, 2013 at 6:39 PM, Matěj Laitl ma...@laitl.cz wrote:
Git commit d6251e08012056b1e3920edb7ae2d451bcdf9218 by Matěj Laitl.
Committed on 17/02/2013 at 18:38.
Pushed by laitl into branch 'master'.
Remove unused AudioCdTrackProvider
Amount of crap we have never stops to surprise me.
M +0-11 src/CMakeLists.txt
D +0-49 src/core-impl/meta/cdda/AudioCdTrackProvider.cpp
D +0-46 src/core-impl/meta/cdda/AudioCdTrackProvider.h
D +0-123 src/core-impl/meta/cdda/AudioCdTrackProvider_p.cpp
D +0-42 src/core-impl/meta/cdda/AudioCdTrackProvider_p.h
http://commits.kde.org/amarok/d6251e08012056b1e3920edb7ae2d451bcdf9218
diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index 043dc64..1152ca9 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -428,17 +428,6 @@ kde4_add_ui_files(libplaylist_SRCS
)
#
-# AUDIO CD SUPPORT
-#
-
-if(KDEMULTIMEDIA_FOUND)
-set(audiocdsupport_SRCS
-core-impl/meta/cdda/AudioCdTrackProvider.cpp
-core-impl/meta/cdda/AudioCdTrackProvider_p.cpp
- )
-endif(KDEMULTIMEDIA_FOUND)
-
-#
# DYNAMIC
#
set(libdynamic_SRCS
diff --git a/src/core-impl/meta/cdda/AudioCdTrackProvider.cpp
b/src/core-impl/meta/cdda/AudioCdTrackProvider.cpp
deleted file mode 100644
index 702e324..000
--- a/src/core-impl/meta/cdda/AudioCdTrackProvider.cpp
+++ /dev/null
@@ -1,49 +0,0 @@
-/
- * Copyright (c) 2008 Maximilian Kossick maximilian.koss...@googlemail.com
*
- *
*
- * This program is free software; you can redistribute it and/or modify it
under*
- * the terms of the GNU General Public License as published by the Free
Software*
- * Foundation; either version 2 of the License, or (at your option) any
later *
- * version.
*
- *
*
- * This program is distributed in the hope that it will be useful, but
WITHOUT ANY *
- * WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
FOR A *
- * PARTICULAR PURPOSE. See the GNU General Public License for more details.
*
- *
*
- * You should have received a copy of the GNU General Public License along
with *
- * this program. If not, see http://www.gnu.org/licenses/.
*
-
/
-
-#include CDDAManager.h
-#include CDDAManager_p.h
-
-CDDAManager::CDDAManager( QObject *parent )
-: QObject( parent )
-, d( new Private() )
-{
-}
-
-CDDAManager::~CDDAManager()
-{
-delete d;
-}
-
-QStringList
-CDDAManager::audioCdUdis() const
-{
-return QStringList();
-}
-
-QString
-CDDAManager::audioCdName( const QString udi ) const
-{
-return QString();
-}
-
-void
-CDDAManager::playAudioCd( const QString udi ) const
-{
-}
-
-#include CDDAManager.moc
-
diff --git a/src/core-impl/meta/cdda/AudioCdTrackProvider.h
b/src/core-impl/meta/cdda/AudioCdTrackProvider.h
deleted file mode 100644
index 2f44c63..000
--- a/src/core-impl/meta/cdda/AudioCdTrackProvider.h
+++ /dev/null
@@ -1,46 +0,0 @@
-/
- * Copyright (c) 2008 Maximilian Kossick maximilian.koss...@googlemail.com
*
- *
*
- * This program is free software; you can redistribute it and/or modify it
under*
- * the terms of the GNU General Public License as published by the Free
Software*
- * Foundation; either version 2 of the License, or (at your option) any
later *
- * version.
*
- *
*
- * This program is distributed in the hope that it will be useful, but
WITHOUT ANY *
- * WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
FOR A *
- * PARTICULAR PURPOSE. See the GNU General Public License for more details.
*
- *
Re: Review Request 108686: hidden items in context menu: usability question
On Feb. 1, 2013, 2:58 a.m., Wyatt Epp wrote: What would be the best approach here? Frankly? Scrap it; this is not good interaction. Context menus are rarely modifier modal and that's being generous (I have never seen one before now). Excepting a very few special cases (e.g. vim), modality is something to be avoided. Neither is this sort of menu something Amarok trains users for: it only happens with this one of the eight context menu arrangements I was able to find in my current layout. Short term, revert to showing the options as before, with confirmation as you see fit. If you're concerned about accidental action, segregate them or even put them in a submenu for permanent actions or something to that effect. Long term, the aim should be that nothing is possible with a context entry that isn't reversible with a simple undo. If, for some reason, things cannot be undone, that should be very clear up-front. Is there a fundamental reason that move operations aren't able to be undone? On the topic of permanent deletion, though I've personally used it in the past, I question the necessity of having it in this list. It's not available anywhere else in the interface, so its inclusion is both dangerous and without precedent. Conversely, moving to trash can be undone quite easily. Is this not sufficient? Regards, Wyatt Bjoern Balazs wrote: +1 Ralf Engels wrote: I agree. Hidden menu entries is a new UI concept that I have never seen before. If that is used wider (e.g. in whole KDE) then I am cool with it. Currently exactly two context menues are using it. So even we are not consistent in it's use. Bart Cerneels wrote: Shift for delete/move was implemented in kde's file browser already before I added this to Amarok. It's not an uncommon feature. Matěj Laitl wrote: Bart, this is not about Shift to delete/move during drop, but Shift to show different context menu - a very different thing. Bart Cerneels wrote: That is what I was talking about. Can someone check dolphin's behavior? Ralf Engels wrote: Bart, you are right. Dolphin has it and I never noticed it. So, it's not without precedence. Then let's add it in all places where it makes sense. Matěj Laitl wrote: Bart, Ralf, I fear you are talking about something completely unrelated with this review request. Ctrl to copy and Shift to move are standard Drag *DROP* (I cannot stress it enough) modifiers. And Shift is a standard modifier for Delete *keyboard shortcut* meaning to bypass the trash. This review request has nothing to do with Drag drop or keyboard shortcuts. It is about *context menus*. @Ralf, we already have hold shift to move instead of copying for dropping items onto Collections pane. Edward Hades Toroshchin wrote: This review [...] is about *context menus*. Hiding context menu items until the user holds Shift is not uncommon. In an operating system called Windows™ (which you might have heard of), a file context menu would contain an Open with... item only if the Shift key was being held. I think (but 'm not sure) it would also bypass Trash™ if the user held Shift while clicking Delete. Wyatt Epp wrote: @Bart It's not an uncommon feature. Sorry, but it really is. Can you name more than the file browser and Amarok that have context menu entries that change depending on whether a modifier key is held? I'd be particularly interested in hearing about non-KDE applications that have this (so I also can take them to task for doing it; this is not a behaviour to emulate). Amarok isn't even consistent about applying it; hit ^o and try it in the Play Media window. I _will_ acknowledge that this behaviour is present in Dolphin, but I'm afraid I have some rather pointed questions for whoever signed off on it. Its discovery is dependent on a user opening a context menu AND having the shift key pressed before closing the menu AND noticing that the entry has changed (especially in Dolphin which, you'll note, doesn't appear to even have the tooltip!). It completely violates the user's expectation of transparency, and does so in a way that's unlikely to happen simply because of the mental acrobatics involved in mousing and keyboarding simultaneously. Heiko Tietze wrote: Edward Hades Toroshchin: Hiding context menu items until the user holds Shift is not uncommon. In an operating system called Windows... Actually this behaviour is rejected by MS styleguide. Don't change menu item names dynamically. (http://msdn.microsoft.com/en-us/library/windows/desktop/aa511502.aspx#general) A menu is used to collect all actions to allow users to find any operation with the program there. If you hide something or change it dynamically he or she will not know
Re: Review Request 108686: hidden items in context menu: usability question
On Feb. 1, 2013, 2:58 a.m., Wyatt Epp wrote: What would be the best approach here? Frankly? Scrap it; this is not good interaction. Context menus are rarely modifier modal and that's being generous (I have never seen one before now). Excepting a very few special cases (e.g. vim), modality is something to be avoided. Neither is this sort of menu something Amarok trains users for: it only happens with this one of the eight context menu arrangements I was able to find in my current layout. Short term, revert to showing the options as before, with confirmation as you see fit. If you're concerned about accidental action, segregate them or even put them in a submenu for permanent actions or something to that effect. Long term, the aim should be that nothing is possible with a context entry that isn't reversible with a simple undo. If, for some reason, things cannot be undone, that should be very clear up-front. Is there a fundamental reason that move operations aren't able to be undone? On the topic of permanent deletion, though I've personally used it in the past, I question the necessity of having it in this list. It's not available anywhere else in the interface, so its inclusion is both dangerous and without precedent. Conversely, moving to trash can be undone quite easily. Is this not sufficient? Regards, Wyatt Bjoern Balazs wrote: +1 Ralf Engels wrote: I agree. Hidden menu entries is a new UI concept that I have never seen before. If that is used wider (e.g. in whole KDE) then I am cool with it. Currently exactly two context menues are using it. So even we are not consistent in it's use. Bart Cerneels wrote: Shift for delete/move was implemented in kde's file browser already before I added this to Amarok. It's not an uncommon feature. Matěj Laitl wrote: Bart, this is not about Shift to delete/move during drop, but Shift to show different context menu - a very different thing. That is what I was talking about. Can someone check dolphin's behavior? - Bart --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108686/#review26491 --- On Jan. 31, 2013, 8:09 p.m., Edward Hades Toroshchin wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108686/ --- (Updated Jan. 31, 2013, 8:09 p.m.) Review request for Amarok and KDE Usability. Description --- Some of the actions in context menu are shown only when the Shift key is pressed. We were wondering, if this were okay at all, and if yes, which hint would be better. To explain a bit more: in Amarok 2.5, the context menu (of a track, album etc.) used to have all the options (among others): * Copy to Collection - * Move to Collection - * Move to Trash * Delete With goal to (a) make accidental data-loss (or unwanted effect) harder for novice users (b) make the context menu simpler, a fancy hold Shift to swich to move/dangerous operations behaviour was implemented: - without Shift held: * Copy to Collection - [with Press Shift key for ... tooltip] * Move to Trash [with Press Shift key for ... tooltip] - with Shift key held (updates itself immediately after pressing the key): * Move to Collection - * Delete However, we discovered that discoverability (hehe) is really a problem when even experienced long-term Amarok users didn't know about the way to trigger Move/Delete. What would be the best approach here? Diffs - Diff: http://git.reviewboard.kde.org/r/108686/diff/ Testing --- File Attachments Behaviour of Amaork 2.7 with Shift key held http://git.reviewboard.kde.org/media/uploaded/files/2013/01/31/hidden_actions.png Suggestion to improve discoverability http://git.reviewboard.kde.org/media/uploaded/files/2013/01/31/hint_1.png Behaviour of Amarok 2.7 without any key held http://git.reviewboard.kde.org/media/uploaded/files/2013/01/31/hint_2.png Thanks, Edward Hades Toroshchin ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request 108905: Move UI code of PlaylistQueueEditor from .ui to .cpp file
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108905/#review27217 --- Use a promoted widget instead. UI forms superior to this, we prefer to use them more, not less. FYI the QueueEditor feature is not something we'll keep around for long. It's scheduled to be completely replaced one or two releases from now. Consider that before you spend days of useful coding time on it. - Bart Cerneels On Feb. 11, 2013, 10:39 a.m., Bartosz Szreder wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108905/ --- (Updated Feb. 11, 2013, 10:39 a.m.) Review request for Amarok. Description --- This is the first patch (of two) resolving bug #263563 I need to create a specialized QListWidget implementation. Thus it seems easier to move the rather limited part of UI into the .cpp file and cleanly add another class with the desired modifications than to dig into .ui files with nonstandard classes. This patch shouldn't introduce any changes in behavior yet. It's only purpose is to prepare ground for the next patch. This addresses bug 263563. https://bugs.kde.org/show_bug.cgi?id=263563 Diffs - src/playlist/PlaylistQueueEditor.h 40b8cdf src/playlist/PlaylistQueueEditor.cpp f647e37 src/playlist/PlaylistQueueEditor.ui a05cafd src/CMakeLists.txt 043dc64 Diff: http://git.reviewboard.kde.org/r/108905/diff/ Testing --- Tested on two different instalations, no regressions noticed. Thanks, Bartosz Szreder ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request 108906: Add ability to drag titles to re-arrange them in queue manager
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108906/#review27218 --- src/playlist/PlaylistQueueEditor.cpp http://git.reviewboard.kde.org/r/108906/#comment20495 Like I suspected in the other review, there is no reason not to use a promoted widget. - Bart Cerneels On Feb. 11, 2013, 10:43 a.m., Bartosz Szreder wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108906/ --- (Updated Feb. 11, 2013, 10:43 a.m.) Review request for Amarok. Description --- The new ListWidget (PlaylistQueueEditorListWidget - feeling enterprisey today) supports drag'n'drop of self-contained ListItems to itself. On successful dropEvent a signal is emitted, which is intercepted by a new slot reloadQueue() in PlaylistQueueEditor. This slot in turn recreates the queue via The::playlistActions(). Things to consider in future: - dragging from the playlist into the queue editor - making the whole operation more efficient than clearing and recreating whole queue on each dropEvent - possibly less hacky way of implementing things? maybe some backend interface would need to be added/exposed for the queue editor? any advice from a seasoned Amarok developer would be very valuable If this isn't going to be accepted as-is into Amarok codebase then I'm willing to put some time into reworking the solution. This addresses bug 263563. https://bugs.kde.org/show_bug.cgi?id=263563 Diffs - src/CMakeLists.txt 043dc64 src/playlist/PlaylistQueueEditor.h 40b8cdf src/playlist/PlaylistQueueEditor.cpp f647e37 Diff: http://git.reviewboard.kde.org/r/108906/diff/ Testing --- Tested on two different installations, no bugs, regressions or instabilities noticed. Thanks, Bartosz Szreder ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request 107473: Changes in processing playlist files
On Feb. 9, 2013, 12:55 a.m., Matěj Laitl wrote: src/core-impl/playlists/types/file/PlaylistFile.h, lines 71-73 http://git.reviewboard.kde.org/r/107473/diff/5/?file=112906#file112906line71 What is the difference between save() and savePlaylist()? Also, if we don't allow constructor w/out url specified, the url arguments here can go away. (which I'd like very much) It seems that save() is the public method for other code to use and savePlaylist() is the method for subclasses to implement. If this is the case, make it so: * make save() non-virtual * make savePlaylist() protected In each case, both need to be documented. The url argument in the c'tor can go away, except for M3U playlist, which uses the filename as it's name() because it doesn't have that metadata in the spec. On Feb. 9, 2013, 12:55 a.m., Matěj Laitl wrote: src/core-impl/playlists/types/file/PlaylistFile.h, line 94 http://git.reviewboard.kde.org/r/107473/diff/5/?file=112906#file112906line94 Thinking about it more, I think it is ridiculous to require calling triggerTrackLoad() manually. Just start the loading job in constructor and ditch triggerTrackLoad() altogether. [you may want to postpone working on this for the case other Amarok developers don't agree] Use case: Browsing playlist treeview with closed playlists. QAbscractItemModel::fetchMore() and so on. The whole idea is delayed loading right? On Feb. 9, 2013, 12:55 a.m., Matěj Laitl wrote: src/core-impl/playlists/types/file/PlaylistFile.h, lines 99-101 http://git.reviewboard.kde.org/r/107473/diff/5/?file=112906#file112906line99 Please document these more - including their relationship. Does calling setName() change filename? vice versa? (or perhaps setFileName() is redundant and can be removed (+1 from me)?) Here your point of removing filename as c'tor argument and as a member becomes even more valid. On Feb. 9, 2013, 12:55 a.m., Matěj Laitl wrote: src/core-impl/playlists/types/file/PlaylistFile.cpp, lines 227-249 http://git.reviewboard.kde.org/r/107473/diff/5/?file=112907#file112907line227 This should probably take some code from Playlists::loadPlaylistFile() so that it supports also remote KUrls. Do we really want to load remote (ex. http) playlist-files directly? Opens a whole other world of potential break points that need to be handled. - Bart --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107473/#review27042 --- On Feb. 7, 2013, 3:57 p.m., Tatjana Gornak wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107473/ --- (Updated Feb. 7, 2013, 3:57 p.m.) Review request for Amarok. Description --- I've started my changes with an implementation of a ASXPlaylist class, due this work I've found that implementation of different playlist file types has different logic, as result I end up with a rewriting playlist's implementations. As example of difference: -- Constructor M3UPlaylist::M3UPlaylist( const KUrl url ) sets a url, but does not load playlist, therefore loading of playlist are posponed till data from playlist are actualy needed (lazy loading) On the other hand constructor XSPFPlaylist::XSPFPlaylist( const KUrl url, bool autoAppend ) loads playlist. The main idea of proposed changes is to create a unify code for processing playlist files: -- lazy loading through LazyLoadPlaylist -- common functionality was moved to PlaylistFile. It is worth to be noticed that this patch changes structure of playlist's classes, bit does not changes their behavior, even if this behavior is inconsistent in some cases. In following patch-requests I plan to submit ASX Playlist parser and organize playlists processing in more consistent way. This addresses bug 291934. https://bugs.kde.org/show_bug.cgi?id=291934 Diffs - src/core-impl/playlists/types/file/PlaylistFile.h 4322da9 src/core-impl/playlists/types/file/PlaylistFile.cpp 2730354 src/core-impl/playlists/types/file/PlaylistFileSupport.cpp 2dcc0cd src/core-impl/playlists/types/file/m3u/M3UPlaylist.h 3176c45 src/core-impl/playlists/types/file/m3u/M3UPlaylist.cpp 2395260 src/core-impl/playlists/types/file/pls/PLSPlaylist.h e974539 src/core-impl/playlists/types/file/pls/PLSPlaylist.cpp 3ba154a src/core-impl/playlists/types/file/xspf/XSPFPlaylist.h 6cf33e9 src/core-impl/playlists/types/file/xspf/XSPFPlaylist.cpp bbcecc3 src/core/playlists/Playlist.h b4e4f40 src/playlist/PlaylistActions.h e8e541b src/playlist/PlaylistActions.cpp 00bf13a
Re: Review Request: Changes in processing playlist files
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107473/#review24407 --- Looks like I still had some unpublished comment from the last version. Clicking publish because I don't know what was not pushed yet. - Bart Cerneels On Dec. 29, 2012, 12:46 p.m., Tatjana Gornak wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107473/ --- (Updated Dec. 29, 2012, 12:46 p.m.) Review request for Amarok. Description --- I've started my changes with an implementation of a ASXPlaylist class, due this work I've found that implementation of different playlist file types has different logic, as result I end up with a rewriting playlist's implementations. As example of difference: -- Constructor M3UPlaylist::M3UPlaylist( const KUrl url ) sets a url, but does not load playlist, therefore loading of playlist are posponed till data from playlist are actualy needed (lazy loading) On the other hand constructor XSPFPlaylist::XSPFPlaylist( const KUrl url, bool autoAppend ) loads playlist. The main idea of proposed changes is to create a unify code for processing playlist files: -- lazy loading through LazyLoadPlaylist -- common functionality was moved to PlaylistFile. It is worth to be noticed that this patch changes structure of playlist's classes, bit does not changes their behavior, even if this behavior is inconsistent in some cases. In following patch-requests I plan to submit ASX Playlist parser and organize playlists processing in more consistent way. Diffs - src/core-impl/playlists/types/file/PlaylistFile.h 4322da9 src/core-impl/playlists/types/file/PlaylistFile.cpp 2730354 src/core-impl/playlists/types/file/PlaylistFileSupport.cpp 2dcc0cd src/core-impl/playlists/types/file/m3u/M3UPlaylist.h 3176c45 src/core-impl/playlists/types/file/m3u/M3UPlaylist.cpp 2395260 src/core-impl/playlists/types/file/pls/PLSPlaylist.h e974539 src/core-impl/playlists/types/file/pls/PLSPlaylist.cpp 3ba154a src/core-impl/playlists/types/file/xspf/XSPFPlaylist.h ebf3235 src/core-impl/playlists/types/file/xspf/XSPFPlaylist.cpp bbcecc3 src/core/playlists/Playlist.h 8fd1ffb src/playlistmanager/PlaylistManager.h cbf65b0 src/playlistmanager/PlaylistManager.cpp 89c754b src/playlistmanager/file/PlaylistFileProvider.cpp e4e7284 tests/core-impl/playlists/types/file/CMakeLists.txt ef69236 tests/core-impl/playlists/types/file/TestPlaylistObserver.h PRE-CREATION tests/core-impl/playlists/types/file/TestPlaylistObserver.cpp PRE-CREATION tests/core-impl/playlists/types/file/m3u/TestM3UPlaylist.cpp 0f19e1b tests/core-impl/playlists/types/file/xspf/TestXSPFPlaylist.cpp f4643ca Diff: http://git.reviewboard.kde.org/r/107473/diff/ Testing --- 1) All unit-tests were passed. 2) For all playlists I've also checked loading and saving through gui. Thanks, Tatjana Gornak ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Changes in processing playlist files
On Nov. 27, 2012, 8:08 a.m., Bart Cerneels wrote: src/core-impl/playlists/types/file/xspf/XSPFPlaylist.cpp, line 218 http://git.reviewboard.kde.org/r/107473/diff/1/?file=96233#file96233line218 Here is the real reason why lazy loading is needed. Reading playlist-file contents from disk is too fast to bother, but getting the right Track objects from CollectionManager::trackForUrl() is problematic. It should either block until the track is ready (depends in TrackProviders) or using proxy loading like it is now. The proxy is temporary and gets initialized with metadata we know from the contents of this playlist file. Myriam Schweingruber wrote: That is a totally wrong assumption, don't you remember that we introduced it because reading playlists on start-up is a PITA? Please don't assume that all users have their collection and playlists on an SSD. Playlists should never be read unless you actually load it in the play queue, not a second earlier. We reduced start up time by over a minute, and some users reported start up delays of several minutes because playlist were read on start. See also https://bugs.kde.org/show_bug.cgi?id=245654, you introduced that yourself :) Bart Cerneels wrote: The delay is coming from loading the tracks from NFS, not the text content of the playlist file. I guess I needed to clarify that lazy loading of tracks using ProxyTrack is what we currently have in SQL and XSPF playlists and is what all playlist implementations need to do. Matěj Laitl wrote: The delay is coming from loading the tracks from NFS, not the text content of the playlist file. I guess I needed to clarify that lazy loading of tracks using ProxyTrack is what we currently have in SQL and XSPF playlists and is what all playlist implementations need to do. No. We need *both*: lazy-loading of tracks (MetaProxy::Track), and lazy-loading of playlists themselves. Bart, please mount an USB 1.1 stick with 1000 playlists, then speak. Why not create the playlists (i.e. constructor and/or calling PlaylistFile::load()) in batches so it won't block the UI thread? I don't see how delayed loading of playlist contents (as opposed to loading it's tracks) will solve the scaling problem. We need the full playlist contents before anything can be done with them (source of title, #tracks estimate, etc) hence, this can't be deferred. At one point this was implemented and working. It's possible playlist syncing is causing a premature load anyway. If so it's a regression. - Bart --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107473/#review22621 --- On Dec. 29, 2012, 12:46 p.m., Tatjana Gornak wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107473/ --- (Updated Dec. 29, 2012, 12:46 p.m.) Review request for Amarok. Description --- I've started my changes with an implementation of a ASXPlaylist class, due this work I've found that implementation of different playlist file types has different logic, as result I end up with a rewriting playlist's implementations. As example of difference: -- Constructor M3UPlaylist::M3UPlaylist( const KUrl url ) sets a url, but does not load playlist, therefore loading of playlist are posponed till data from playlist are actualy needed (lazy loading) On the other hand constructor XSPFPlaylist::XSPFPlaylist( const KUrl url, bool autoAppend ) loads playlist. The main idea of proposed changes is to create a unify code for processing playlist files: -- lazy loading through LazyLoadPlaylist -- common functionality was moved to PlaylistFile. It is worth to be noticed that this patch changes structure of playlist's classes, bit does not changes their behavior, even if this behavior is inconsistent in some cases. In following patch-requests I plan to submit ASX Playlist parser and organize playlists processing in more consistent way. Diffs - src/core-impl/playlists/types/file/PlaylistFile.h 4322da9 src/core-impl/playlists/types/file/PlaylistFile.cpp 2730354 src/core-impl/playlists/types/file/PlaylistFileSupport.cpp 2dcc0cd src/core-impl/playlists/types/file/m3u/M3UPlaylist.h 3176c45 src/core-impl/playlists/types/file/m3u/M3UPlaylist.cpp 2395260 src/core-impl/playlists/types/file/pls/PLSPlaylist.h e974539 src/core-impl/playlists/types/file/pls/PLSPlaylist.cpp 3ba154a src/core-impl/playlists/types/file/xspf/XSPFPlaylist.h ebf3235 src/core-impl/playlists/types/file/xspf/XSPFPlaylist.cpp bbcecc3 src/core/playlists/Playlist.h 8fd1ffb src/playlistmanager/PlaylistManager.h
Re: Review Request: Changes in processing playlist files
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107473/#review24408 --- src/core-impl/playlists/types/file/PlaylistFile.h http://git.reviewboard.kde.org/r/107473/#comment18706 name == filename is an implementation detail (ex. not the case in XSPF) so it should not be documented as such. When implementing sub-classes this comment is taken for true and this way messiness such as the current PlaylistFile implementations can happen. src/core-impl/playlists/types/file/PlaylistFile.h http://git.reviewboard.kde.org/r/107473/#comment18705 Having to use mutable always makes me suspicious of architectural mistakes. But I'll read on and open an issue if I think it's use is invalid. src/core-impl/playlists/types/file/PlaylistFile.cpp http://git.reviewboard.kde.org/r/107473/#comment18707 Have you looked at loading playlists in batches in PlaylistFileProvider instead of threading? Threading does not necessarily prevent UI lockup, batching certainly does. src/core-impl/playlists/types/file/xspf/XSPFPlaylist.cpp http://git.reviewboard.kde.org/r/107473/#comment18708 I realize this is current behavior, but just as a note to ourselves: this delete has to go, a function to save the filename should never delete anything. src/core/playlists/Playlist.h http://git.reviewboard.kde.org/r/107473/#comment18709 This comment is just confusing. Mentions implementation. - Bart Cerneels On Dec. 29, 2012, 12:46 p.m., Tatjana Gornak wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107473/ --- (Updated Dec. 29, 2012, 12:46 p.m.) Review request for Amarok. Description --- I've started my changes with an implementation of a ASXPlaylist class, due this work I've found that implementation of different playlist file types has different logic, as result I end up with a rewriting playlist's implementations. As example of difference: -- Constructor M3UPlaylist::M3UPlaylist( const KUrl url ) sets a url, but does not load playlist, therefore loading of playlist are posponed till data from playlist are actualy needed (lazy loading) On the other hand constructor XSPFPlaylist::XSPFPlaylist( const KUrl url, bool autoAppend ) loads playlist. The main idea of proposed changes is to create a unify code for processing playlist files: -- lazy loading through LazyLoadPlaylist -- common functionality was moved to PlaylistFile. It is worth to be noticed that this patch changes structure of playlist's classes, bit does not changes their behavior, even if this behavior is inconsistent in some cases. In following patch-requests I plan to submit ASX Playlist parser and organize playlists processing in more consistent way. Diffs - src/core-impl/playlists/types/file/PlaylistFile.h 4322da9 src/core-impl/playlists/types/file/PlaylistFile.cpp 2730354 src/core-impl/playlists/types/file/PlaylistFileSupport.cpp 2dcc0cd src/core-impl/playlists/types/file/m3u/M3UPlaylist.h 3176c45 src/core-impl/playlists/types/file/m3u/M3UPlaylist.cpp 2395260 src/core-impl/playlists/types/file/pls/PLSPlaylist.h e974539 src/core-impl/playlists/types/file/pls/PLSPlaylist.cpp 3ba154a src/core-impl/playlists/types/file/xspf/XSPFPlaylist.h ebf3235 src/core-impl/playlists/types/file/xspf/XSPFPlaylist.cpp bbcecc3 src/core/playlists/Playlist.h 8fd1ffb src/playlistmanager/PlaylistManager.h cbf65b0 src/playlistmanager/PlaylistManager.cpp 89c754b src/playlistmanager/file/PlaylistFileProvider.cpp e4e7284 tests/core-impl/playlists/types/file/CMakeLists.txt ef69236 tests/core-impl/playlists/types/file/TestPlaylistObserver.h PRE-CREATION tests/core-impl/playlists/types/file/TestPlaylistObserver.cpp PRE-CREATION tests/core-impl/playlists/types/file/m3u/TestM3UPlaylist.cpp 0f19e1b tests/core-impl/playlists/types/file/xspf/TestXSPFPlaylist.cpp f4643ca Diff: http://git.reviewboard.kde.org/r/107473/diff/ Testing --- 1) All unit-tests were passed. 2) For all playlists I've also checked loading and saving through gui. Thanks, Tatjana Gornak ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Layout changes to organize collection, guess metadata, edit filter and edit playlist layout dialogs
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107624/#review23125 --- Screenshot: New Guess Tag Dialog http://git.reviewboard.kde.org//r/107624/#scomment117 The colors here cause a readability issue (yellow on white in this case). Since the background color of the view depends on the style and color settings it's impossible to find colors that will work in all cases. Just use the color settings forground-text, no special colors. Screenshot: New Edit Filter Dialog http://git.reviewboard.kde.org//r/107624/#scomment118 What does this line edit do? Needs a label. Screenshot: New Edit Filter Dialog http://git.reviewboard.kde.org//r/107624/#scomment119 If the option count 4, don't use a combo. And don't make it this wide in any case. - Bart Cerneels On Dec. 7, 2012, 11:44 a.m., Ralf Engels wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107624/ --- (Updated Dec. 7, 2012, 11:44 a.m.) Review request for Amarok. Description --- Several refactoring changes centering around token pool, token, token drop target and so on. 1. splitting up the FilenameLayoutDialog (which was not a dialog at all) into two widgets to be used by the actual dialog. 2. changing the token pool from an icon list to a normal list to prevent the use from having to scroll so much. 3. simplifying the drop target so that it does not need to install event filters for it's parents. 4. fixing small issues in the token pool, token and token drop target so that they have sensible minimumSizeHint and sizeHints 5. aligning texts between the different token users. No longer different texts. 6. for the edit filter dialog changes to the layout to get rid of the space waste in the result area. 7. for the guess tag dialog I got rid of some empty areas and some useless settings (settings that the dialog could determine itself) 8. for the playlist layout dialog not much has changed except that we don't need the event-filter parent mechanism. Have a look at the attached screenshots to see the differences. The actual code changes are in the rengels-filenameLayoutDialog branch. Further work: Settings and presets need a reworking, as we have several sets currently which is confusing. Diffs - ChangeLog 15a698c Diff: http://git.reviewboard.kde.org/r/107624/diff/ Testing --- Screenshots --- Old Layout editor dialog http://git.reviewboard.kde.org/r/107624/s/878/ New Layout editor dialog http://git.reviewboard.kde.org/r/107624/s/879/ Old Guess Tag Dialog http://git.reviewboard.kde.org/r/107624/s/880/ New Guess Tag Dialog http://git.reviewboard.kde.org/r/107624/s/881/ Old Edit Filter Dialog http://git.reviewboard.kde.org/r/107624/s/882/ New Edit Filter Dialog http://git.reviewboard.kde.org/r/107624/s/883/ Thanks, Ralf Engels ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Changes in processing playlist files
On Nov. 27, 2012, 8:08 a.m., Bart Cerneels wrote: I think you started with the wrong assumption that lazy loading of the contents of the playlist file is required. We have some use cases for PlaylistFile: 1) Make playlists found in the collection folders available through the PlaylistBrowser using PlaylistFileProvider. 2) Restore the session with contents of the playback queue using XSPFPlaylist saved to $KDEHOME/share/apps/amarok/current.xspf. 3) Wherever we need to load or save a playlist file, locally or remote. Like the Add to playlist... or Export playlist... actions in the menu. Like you can tell from my inline comments, the support for remote loading needs to be removed from these classes. Fetching the content can be done outside of these classes using load(). The name and other playlist metadata is either in the content (XSPF) or derived from the filename (M3U, PLS). New PlaylistFile derived classes should use whatever the format has available, preferably from the content. Since you want to add a new Playlist class, what is you use case for it. Is there any need to load remote content in use case 1) or 2) (might be specific about the format)? I propose you don't refactor PlaylistFile just yet and implement ASXPlaylist so it works as needed for the above use cases. After that we'll have a better understanding for refactoring. I'm sure clean-up and refactor will be needed. Matěj Laitl wrote: I think you started with the wrong assumption that lazy loading of the contents of the playlist file is required. I don't agree, Myriam doesn't agree. Remember we have a bug Amarok starts up several minutes when playlists on NFS... Like you can tell from my inline comments, the support for remote loading needs to be removed from these classes. Why? I would do it differently, (No need to call to playlistcontroller) but why not have it? Fetching the content can be done outside of these classes using load(). Yes, but no reason to do that from my PoV. The name and other playlist metadata is either in the content (XSPF) or derived from the filename (M3U, PLS). New PlaylistFile derived classes should use whatever the format has available, preferably from the content. Yes. Since you want to add a new Playlist class, what is you use case for it. Is there any need to load remote content in use case 1) or 2) (might be specific about the format)? This was just a helper class. If you read my comments, I already proposed doing it in PlaylistFile directly and I'm working on a patch that will enable it. I propose you don't refactor PlaylistFile just yet and implement ASXPlaylist so it works as needed for the above use cases. After that we'll have a better understanding for refactoring. I'm sure clean-up and refactor will be needed. Who not deduplicating code first? Your approach seems like a wase of time to me. Remember, this is about deduplicating code, which I always find beneficial. Tatjana, plese wait for me to finish my preparatory patch. (today hopefully) Who not deduplicating code first? Your approach seems like a wase of time to me. Remember, this is about deduplicating code, which I always find beneficial. Experience has taught me otherwise. It's faster to implement and then refactor for code reuse and consistency then to assume possible refactoring beforehand. With limited time/problem-space understanding/experience/etc smaller functional patches are better than attempting large refactorings. It's better for the review process as well. On Nov. 27, 2012, 8:08 a.m., Bart Cerneels wrote: src/core-impl/playlists/types/file/xspf/XSPFPlaylist.cpp, line 218 http://git.reviewboard.kde.org/r/107473/diff/1/?file=96233#file96233line218 Here is the real reason why lazy loading is needed. Reading playlist-file contents from disk is too fast to bother, but getting the right Track objects from CollectionManager::trackForUrl() is problematic. It should either block until the track is ready (depends in TrackProviders) or using proxy loading like it is now. The proxy is temporary and gets initialized with metadata we know from the contents of this playlist file. Myriam Schweingruber wrote: That is a totally wrong assumption, don't you remember that we introduced it because reading playlists on start-up is a PITA? Please don't assume that all users have their collection and playlists on an SSD. Playlists should never be read unless you actually load it in the play queue, not a second earlier. We reduced start up time by over a minute, and some users reported start up delays of several minutes because playlist were read on start. See also https://bugs.kde.org/show_bug.cgi?id=245654, you introduced that yourself :) The delay
Re: Review Request: Changes in processing playlist files
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107473/#review22631 --- This architecture document should help and avoid me repeating myself: http://quickgit.kde.org/?p=amarok.gita=blobh=3eda255d944f61c3f975031b69b342cf0899fc2bhb=57e5bf466b364a0e086000b2d24f00e1e336b22ef=HACKING%2Farchitecture%2FPlaylists.txt It's in HACKING/architecture/ - Bart Cerneels On Nov. 26, 2012, 12:10 p.m., Tatjana Gornak wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107473/ --- (Updated Nov. 26, 2012, 12:10 p.m.) Review request for Amarok. Description --- I've started my changes with an implementation of a ASXPlaylist class, due this work I've found that implementation of different playlist file types has different logic, as result I end up with a rewriting playlist's implementations. As example of difference: -- Constructor M3UPlaylist::M3UPlaylist( const KUrl url ) sets a url, but does not load playlist, therefore loading of playlist are posponed till data from playlist are actualy needed (lazy loading) On the other hand constructor XSPFPlaylist::XSPFPlaylist( const KUrl url, bool autoAppend ) loads playlist. The main idea of proposed changes is to create a unify code for processing playlist files: -- lazy loading through LazyLoadPlaylist -- common functionality was moved to PlaylistFile. It is worth to be noticed that this patch changes structure of playlist's classes, bit does not changes their behavior, even if this behavior is inconsistent in some cases. In following patch-requests I plan to submit ASX Playlist parser and organize playlists processing in more consistent way. Diffs - src/CMakeLists.txt b70147d src/core-impl/playlists/types/file/LazyPlaylistFile.h PRE-CREATION src/core-impl/playlists/types/file/LazyPlaylistFile.cpp PRE-CREATION src/core-impl/playlists/types/file/PlaylistFile.h 4322da9 src/core-impl/playlists/types/file/PlaylistFile.cpp 2730354 src/core-impl/playlists/types/file/PlaylistFileSupport.cpp 2dcc0cd src/core-impl/playlists/types/file/m3u/M3UPlaylist.h 3176c45 src/core-impl/playlists/types/file/m3u/M3UPlaylist.cpp 2395260 src/core-impl/playlists/types/file/pls/PLSPlaylist.h e974539 src/core-impl/playlists/types/file/pls/PLSPlaylist.cpp 3ba154a src/core-impl/playlists/types/file/xspf/XSPFPlaylist.h ebf3235 src/core-impl/playlists/types/file/xspf/XSPFPlaylist.cpp bbcecc3 src/core/playlists/Playlist.h 5cf11f2 src/playlistmanager/file/PlaylistFileProvider.cpp e4e7284 tests/core-impl/playlists/types/file/CMakeLists.txt ef69236 tests/core-impl/playlists/types/file/TestLazyPlaylistFile.h PRE-CREATION tests/core-impl/playlists/types/file/TestLazyPlaylistFile.cpp PRE-CREATION tests/core-impl/playlists/types/file/m3u/TestM3UPlaylist.cpp 0f19e1b Diff: http://git.reviewboard.kde.org/r/107473/diff/ Testing --- 1) All unit-tests were passed. 2) For all playlists I've also checked loading and saving through gui. Thanks, Tatjana Gornak ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Playlists::Playlist: add metadataChanged() to observer, clean-up, docs
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107484/#review22636 --- src/core/playlists/Playlist.cpp http://git.reviewboard.kde.org/r/107484/#comment17318 You missed the opportunity to move this to Meta namespace. Much work elsewhere though. There, now I have a issue for an otherwise perfect patch ;) - Bart Cerneels On Nov. 27, 2012, 12:01 p.m., Matěj Laitl wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107484/ --- (Updated Nov. 27, 2012, 12:01 p.m.) Review request for Amarok, Bart Cerneels, Edward Hades Toroshchin, and Tatjana Gornak. Description --- Playlists::Playlist: add metadataChanged() to observer, clean-up, docs All observers are made to react on metadataChanged() as appropriate. TODO: actually call notifyObserversMetadataqChanged() in Playlists subclasses as appropriate. Diffs - src/browsers/playlistbrowser/PlaylistBrowserModel.h 8f9febd0636b8dab99daaca6c813b449944396c3 src/browsers/playlistbrowser/PlaylistBrowserModel.cpp afbae7ff5b6cadcd3c0045bcc0b07cdda836573b src/core-impl/collections/ipodcollection/IpodPlaylistProvider.h 1eff808e91a5c9c9cba64a48eaf172a7d5f5 src/core-impl/collections/ipodcollection/IpodPlaylistProvider.cpp 6b636270077bced6fd31281926fedf2a969ba630 src/core/playlists/Playlist.h 5cf11f2128c593774d9ed5cae456ee886d09771b src/core/playlists/Playlist.cpp 86ecaa5b77d41fa95d81bff492f278f8724a835e src/playlistmanager/SyncedPlaylist.h f7716d20010bcb7c32605e606e03bd8c5425fa96 src/playlistmanager/SyncedPlaylist.cpp 1607914f9a1f461959a84b255a3bab3ad6c8a06c Diff: http://git.reviewboard.kde.org/r/107484/diff/ Testing --- Amarok still works fine incl. Saved Playlists. Thanks, Matěj Laitl ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Playlists::Playlist: add metadataChanged() to observer, clean-up, docs
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107484/#review22640 --- Ship it! Merge this. - Bart Cerneels On Nov. 27, 2012, 12:01 p.m., Matěj Laitl wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107484/ --- (Updated Nov. 27, 2012, 12:01 p.m.) Review request for Amarok, Bart Cerneels, Edward Hades Toroshchin, and Tatjana Gornak. Description --- Playlists::Playlist: add metadataChanged() to observer, clean-up, docs All observers are made to react on metadataChanged() as appropriate. TODO: actually call notifyObserversMetadataqChanged() in Playlists subclasses as appropriate. Diffs - src/browsers/playlistbrowser/PlaylistBrowserModel.h 8f9febd0636b8dab99daaca6c813b449944396c3 src/browsers/playlistbrowser/PlaylistBrowserModel.cpp afbae7ff5b6cadcd3c0045bcc0b07cdda836573b src/core-impl/collections/ipodcollection/IpodPlaylistProvider.h 1eff808e91a5c9c9cba64a48eaf172a7d5f5 src/core-impl/collections/ipodcollection/IpodPlaylistProvider.cpp 6b636270077bced6fd31281926fedf2a969ba630 src/core/playlists/Playlist.h 5cf11f2128c593774d9ed5cae456ee886d09771b src/core/playlists/Playlist.cpp 86ecaa5b77d41fa95d81bff492f278f8724a835e src/playlistmanager/SyncedPlaylist.h f7716d20010bcb7c32605e606e03bd8c5425fa96 src/playlistmanager/SyncedPlaylist.cpp 1607914f9a1f461959a84b255a3bab3ad6c8a06c Diff: http://git.reviewboard.kde.org/r/107484/diff/ Testing --- Amarok still works fine incl. Saved Playlists. Thanks, Matěj Laitl ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Access data for the new server needed
Hey Myriam i had these details but assumed Bradley had send them already to Ben, GPG encrypted. I just forwarded the plain text login details to sysadmin, they'll have to set-up a secure system anyway. On Tue, Nov 20, 2012 at 11:44 AM, Myriam Schweingruber myr...@kde.org wrote: Hi Bradley, this as a short reminder that the KDE sysadmins are still waiting for the access data for the new Hetzner server you ordered. Please send those to sysad...@kde.org so they can make the move of the various services on the Kollide.net server as soon as possible to avoid unnecessary downtime. Since Kollide will be off line at the end of the year this is getting somewhat urgent right now, so that access data is sorely needed. Best regards, Myriam -- Proud member of the Amarok and KDE Community Protect your freedom and join the Fellowship of FSFE: http://www.fsfe.org Please don't send me proprietary file formats, use ISO standard ODF instead (ISO/IEC 26300) ___ Amarok-committee mailing list amarok-commit...@kde.org https://mail.kde.org/mailman/listinfo/amarok-committee ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Fwd: GPL-incompatibility of Spotify's libspotify API ToU (was Re: Getting a Spotify Premium account for Amarok)
I'm forwarding this to all GPL project that I know are offering spotify integration. AFAIKT Tomahawk and Clementine need not worry, Amarok is a member project of the SFC and hence subject to it's rules. It's not a given that the spotify-resolver way of complying with the Spotify Terms of Use is incorrect. If you have any comments let me know here. I'll make sure they are forwarded to Bradley and Tony. I'll make some comments in a followup mail. -- Forwarded message -- From: Bradley M. Kuhn bk...@sfconservancy.org Date: Mon, Nov 12, 2012 at 9:02 PM Subject: GPL-incompatibility of Spotify's libspotify API ToU (was Re: Getting a Spotify Premium account for Amarok) To: Bart Cerneels bart.cerne...@kde.org, ama...@sfconservancy.org Cc: Tony Sebro t...@sfconservancy.org Amarok Committee, Bart, Tony, and I discussed the issue with spotify on IRC today. I hadn't realized (until today when Bart told me) that a GSoC student had added support for spotify to Amarok. Conservancy only became aware of the issue on 26 October when Bart asked for a spotify account to get set up for Conservancy. As it stands, Conservancy is very worried about Amarok offering spotify support. Spotify's terms of use for the libspotify API, at https://developer.spotify.com/technologies/libspotify/terms-of-use-us/ , require the following: By using any part of the API (as defined below), you (including any organization on whose behalf you are agreeing to these Terms of Use) (collectively sometimes referred to as you and your) are deemed to have accepted these Terms of Use... Application means an authorized end user downloadable application utilizing the API developed by you solely for use by Users to access the Service. User means a registered subscriber to the Service ... ... 3.10 When distributing the Application, you shall require end users to agree to an enforceable end user license agreement containing at least the following specific minimum terms: 1. a provision stating that you, and not Spotify, are responsible for your Application; 2. a provision indicating that the API is provided as-is, without any warranties, and that expressly disclaims all implied warranties, including the implied warranties of merchantability, fitness for a particular purpose and non-infringement; 3. a prohibition against modifying or creating derivative works based on any part of the API; 4. a prohibition against decompiling, reverse-engineering, disassembling, and otherwise reducing the API to source code or other human-perceivable form, to the full extent allowed by law... The Application is Amarok (as defined in the agreement, end user downloadable application utilizing the API developed by you). You, in the agreement, is the Amarok Developers. End users and the Users are the folks that you distribute Amarok to. 3.10.3 and 3.10.4 above contradict the requirements of the GPL, so the Amarok Project can't simultaneously meet its requirements under GPL and the ToU. Now, Conservancy doesn't believe that distribution of source code in your current spotify branch violates this agreement: it's always been Conservancy position that the publication of source code for reading is Free Speech in the USA and publishing source code for people to read is akin to publishing a poem on your website. But, definitively distributing a final Application under GPL intending that the Amarok user will have a seamless experience with spotify/Amarok integration *would*, in Conservancy's opinion, violate the ToU, or GPL, or both, depending on one's perspective. It furthermore can create similar risk for downstream distributions that package Amarok. Whether an agreement regarding using an online API would hold up in court is another question, but Conservancy's risk is nonetheless great, because the issue of intention matters in Court. Thus, if Amarok intends for its end users to use the spotify API, and makes effort (which you have) for Amarok to support spotify via the API, it's going to be difficult for Conservancy to argue it was not bound by the ToU (even *if* Conservancy doesn't create a spotify account). We'll need to figure out the right solution here. Conservancy's General Counsel, Tony Sebro (cc'ed) is going to make an effort to reach out to lawyers at Spotify, since there some chance this issue is an unintended consequence of overzealous drafting on their part. I can't say for sure if we'll be successful in reaching out to them, but we're trying. We'll keep you posted. In the meantime, I have to ask Amarok not to make Spotify support official in your distribution. I realize that might harm your release schedule and I apologize for that. Conservancy just didn't have the lead time necessary to investigate this issue in time for the next release. In future, please do send us any ToU
Re: GPL-incompatibility of Spotify's libspotify API ToU (was Re: Getting a Spotify Premium account for Amarok)
On Mon, Nov 12, 2012 at 9:02 PM, Bradley M. Kuhn bk...@sfconservancy.org wrote: Amarok Committee, Bart, Tony, and I discussed the issue with spotify on IRC today. I hadn't realized (until today when Bart told me) that a GSoC student had added support for spotify to Amarok. Conservancy only became aware of the issue on 26 October when Bart asked for a spotify account to get set up for Conservancy. As it stands, Conservancy is very worried about Amarok offering spotify support. Spotify's terms of use for the libspotify API, at https://developer.spotify.com/technologies/libspotify/terms-of-use-us/ , require the following: By using any part of the API (as defined below), you (including any organization on whose behalf you are agreeing to these Terms of Use) (collectively sometimes referred to as you and your) are deemed to have accepted these Terms of Use... Application means an authorized end user downloadable application utilizing the API developed by you solely for use by Users to access the Service. User means a registered subscriber to the Service ... ... 3.10 When distributing the Application, you shall require end users to agree to an enforceable end user license agreement containing at least the following specific minimum terms: 1. a provision stating that you, and not Spotify, are responsible for your Application; 2. a provision indicating that the API is provided as-is, without any warranties, and that expressly disclaims all implied warranties, including the implied warranties of merchantability, fitness for a particular purpose and non-infringement; 3. a prohibition against modifying or creating derivative works based on any part of the API; 4. a prohibition against decompiling, reverse-engineering, disassembling, and otherwise reducing the API to source code or other human-perceivable form, to the full extent allowed by law... The Application is Amarok (as defined in the agreement, end user downloadable application utilizing the API developed by you). You, in the agreement, is the Amarok Developers. End users and the Users are the folks that you distribute Amarok to. 3.10.3 and 3.10.4 above contradict the requirements of the GPL, so the Amarok Project can't simultaneously meet its requirements under GPL and the ToU. Now, Conservancy doesn't believe that distribution of source code in your current spotify branch violates this agreement: it's always been Conservancy position that the publication of source code for reading is Free Speech in the USA and publishing source code for people to read is akin to publishing a poem on your website. But, definitively distributing a final Application under GPL intending that the Amarok user will have a seamless experience with spotify/Amarok integration *would*, in Conservancy's opinion, violate the ToU, or GPL, or both, depending on one's perspective. It furthermore can create similar risk for downstream distributions that package Amarok. Whether an agreement regarding using an online API would hold up in court is another question, but Conservancy's risk is nonetheless great, because the issue of intention matters in Court. Thus, if Amarok intends for its end users to use the spotify API, and makes effort (which you have) for Amarok to support spotify via the API, it's going to be difficult for Conservancy to argue it was not bound by the ToU (even *if* Conservancy doesn't create a spotify account). We'll need to figure out the right solution here. Conservancy's General Counsel, Tony Sebro (cc'ed) is going to make an effort to reach out to lawyers at Spotify, since there some chance this issue is an unintended consequence of overzealous drafting on their part. I can't say for sure if we'll be successful in reaching out to them, but we're trying. We'll keep you posted. In the meantime, I have to ask Amarok not to make Spotify support official in your distribution. I realize that might harm your release schedule and I apologize for that. Conservancy just didn't have the lead time necessary to investigate this issue in time for the next release. In future, please do send us any ToU and licensing information for any service/library you want to support as early as possible. According to me the spotify-resolver, shared between Amarok and Tomahawk [1], or clementine's blob is the application in this case. spotify-resolver is BSD licensed and hence not subject to the GPLv2 section 7 [2] terms and fully capable of complying with the Spotify Terms of Use [3]. Secondly, Spotify ToU section 3.10 states that and End User License Agreement needs to be presented to the user on *distribution*. We download the resolver-binary only once the terms are acknowledged by the user, complying with the Spotify ToU. After the exec() to start the
Fwd: FOSDEM CrossDesktop DevRoom 2013 - Call for Talks
-- Forwarded message -- From: Pau Garcia i Quiles pgqui...@elpauer.org Date: Wed, Oct 31, 2012 at 5:50 PM Subject: FOSDEM CrossDesktop DevRoom 2013 - Call for Talks To: kde-de...@kde.org, kdelibs kde-core-de...@kde.org, KDE on Windows kde-wind...@kde.org, Calligra Suite developers and users mailing list calligra-de...@kde.org Hello, The Call for Talks for the CrossDesktop DevRoom at FOSDEM 2013 is now officially open. Please do not wait till the last minute! --8--- FOSDEM is one of the largest gatherings of Free Software contributors in the world and happens each February in Brussels (Belgium). One of the tracks will be the CrossDesktop DevRoom, which will host Desktop-related talks. We are now inviting proposals for talks about Free/Libre/Open-source Software on the topics of Desktop development, Desktop applications and interoperativity amongst Desktop Environments. This is a unique opportunity to show novel ideas and developments to a wide technical audience. Topics accepted include, but are not limited to: Enlightenment, Gnome, KDE, Unity, XFCE, Windows, Mac OS X, general desktop matters, applications that enhance desktops and web (when related to desktop). Talks can be very specific, such as developing mobile applications with Qt Quick; or as general as predictions for the fusion of Desktop and web in 5 years time. Topics that are of interest to the users and developers of all desktop environments are especially welcome. The FOSDEM 2012 schedule might give you some inspiration: https://archive.fosdem.org/2012/schedule/track/crossdesktop_devroom.html Please include the following information when submitting a proposal: Your name The title of your talk (please be descriptive, as titles will be listed with around 250 from other projects) Short abstract of one or two paragraphs Short bio Requested time: from 15 to 45 minutes. Normal duration is 30 minutes. Longer duration requests must be properly justified. The deadline for submissions is December 14th 2012. FOSDEM will be held on the weekend of 2-3 February 2013. Please submit your proposals to crossdesktop-devr...@lists.fosdem.org (subscribtion page for the mailing list: https://lists.fosdem.org/listinfo/crossdesktop-devroom ) -- The CrossDesktop DevRoom 2013 Organization Team --8--- -- Pau Garcia i Quiles http://www.elpauer.org (Due to my workload, I may need 10 days to answer) Visit http://mail.kde.org/mailman/listinfo/kde-devel#unsub to unsubscribe ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: collectionscanner: prevent writing malformed XML
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106944/#review20889 --- Ship it! Looks OK to me. I would advise against changing the XML creator/parser or use a different serialization format. There is a lot we can do on the parser side though. Just take a look at PodcastReader.cpp. Using it's stack-parsing method not only would it be fully incremental (handling incomplete input), it can also recover from partially malformed XML. - Bart Cerneels On Oct. 25, 2012, 2:26 p.m., Edward Hades Toroshchin wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106944/ --- (Updated Oct. 25, 2012, 2:26 p.m.) Review request for Amarok. Description --- Note, that this is by far not a permanent solution! This merely changes the collectionscanner behavior to pass incorrect data to amarok from the less acceptable behavior just error out and do nothing. This may result in some unplayable tracks, but on the other hand, the collection scan will complete successfully (more or less). To fix this completely, we should either move to other serialization medium (JSON?) or to another XML writer/reader. Thoughts, suggestions welcome. collectionscanner: prevent writing malformed XML As you probably know already, Qt deviates from the output strictly, input relaxed practice by producing shit in QXmlStreamWriter, which is then being rejected by QXmlStreamReader. This is a temporary solution, to try to escape all possible occurences of invalid characters in the XML. UPDATE: since no one objects, let's just push this guy in. This addresses bug 305527. https://bugs.kde.org/show_bug.cgi?id=305527 Diffs - shared/collectionscanner/Directory.cpp 46274ebc7fcbda9aa0123fd048ab3742f28dca64 shared/collectionscanner/Playlist.cpp f131824b3461db21f7359479710b2c3164eb6149 shared/collectionscanner/Track.h e691bb60da4f1a40dc98493630141d7ddb066383 shared/collectionscanner/Track.cpp c08dbdaafe6ed85263d22701b58641a1eb76721d shared/collectionscanner/utils.h PRE-CREATION Diff: http://git.reviewboard.kde.org/r/106944/diff/ Testing --- Scanned own collection. Looks fine. Scanned some folders with creepy characters. Looks also fine. Thanks, Edward Hades Toroshchin ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Refactoring of Collection config UI code
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106762/#review20068 --- Ship it! You might want to try the Ui::Class as member approach. But this code is already much better then what we had before. Less UI defined in C++ at least. We should do this for all our dialogs and custom widgets, have a feeling it would solve quite a few layout glitches. One clear example is the likeback dialog. src/configdialog/dialogs/CollectionConfig.h http://git.reviewboard.kde.org/r/106762/#comment15899 You could also use Ui_CollectionConfig *m_ui and do a m_ui-setupUi(this) in the constructor. The class would only derive from QWidget. It's a personal preference of mine so not obligated, but it does lead to better UI/logic separation in my experience. src/configdialog/dialogs/CollectionConfig.ui http://git.reviewboard.kde.org/r/106762/#comment15896 This should be MinimumExpanding src/configdialog/dialogs/CollectionConfig.ui http://git.reviewboard.kde.org/r/106762/#comment15897 This stretch should be the highest in the dialog (actually the only non 0) so this widget will be the only one to expand when resizing the dialog. src/configdialog/dialogs/CollectionConfig.ui http://git.reviewboard.kde.org/r/106762/#comment15898 This one probably should not expand. As long as the combo is large enough to hold the longest string it's OK. Wide comboboxes are ugly. - Bart Cerneels On Oct. 8, 2012, 11:31 a.m., Mark Kretschmann wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106762/ --- (Updated Oct. 8, 2012, 11:31 a.m.) Review request for Amarok. Description --- Refactoring the Collection config UI code, by moving move more stuff into the UI file. Also, a resizing issue of the UI was fixed, as well as some other small issues. There is one remaining issue with this patch: The TreeView in the UI takes up too much vertical space, although the SizePolicy is set to Minimum. I can't figure out why it does that, so I'd welcome help. Diffs - src/CMakeLists.txt 9efeb20 src/configdialog/dialogs/CollectionConfig.h 02c80e1 src/configdialog/dialogs/CollectionConfig.cpp 42ffa1f src/configdialog/dialogs/CollectionConfig.ui 692c3d9 src/dialogs/CollectionSetup.h 23d717a src/dialogs/CollectionSetup.cpp 5ae0294 src/widgets/CollectionSetupTreeView.h PRE-CREATION src/widgets/CollectionSetupTreeView.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/106762/diff/ Testing --- Tested visual sanity of the UI, tested connections with config. Thanks, Mark Kretschmann ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Fix DAAP collections published by Rhythmbox not showing up in Amarok.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106366/#review20069 --- I still have not gotten around to properly testing this. Tried to solve another bug with password protected shares. As far as I can tell this one could also have fixed that, but it didn't work. Will need to dive deeper in DAAP to figure it out, but man, is that a crap protocol. - Bart Cerneels On Sept. 30, 2012, 7:28 p.m., Daniel Stöckel wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106366/ --- (Updated Sept. 30, 2012, 7:28 p.m.) Review request for Amarok. Description --- This simply sends a fake revision-number to the server. Also appends the session-id field to the track URLs. See: https://bugs.kde.org/show_bug.cgi?id=306351 See: https://bugs.kde.org/show_bug.cgi?id=306351 Also implement trackForUrl and possiblyContainsTrack ind DaapCollection. This should allow reconnecting to daap servers that require authentication. However there seem to be problems with ConnectionManager which handles http urls as a special case. Also XSPFPlaylist uses the track.location instead of the track.identifier field when creating Meta::ProxyTracks. Thus using daap:// uidUrl as protocol (as in this patch) does have no effect. This addresses bug 306351. https://bugs.kde.org/show_bug.cgi?id=306351 Diffs - src/core-impl/collections/daap/DaapCollection.h f8655ec src/core-impl/collections/daap/DaapCollection.cpp 5d1bd9b src/core-impl/collections/daap/DaapMeta.h 9a9c257 src/core-impl/collections/daap/DaapMeta.cpp e66afb7 src/core-impl/collections/daap/daapreader/Reader.cpp b6196e1 Diff: http://git.reviewboard.kde.org/r/106366/diff/ Testing --- - Rhythmbox collections now correctly appear and play in Amarok. - Validated that mt-daap collections are still working. Thanks, Daniel Stöckel ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: [UI] QML CV usability feedback
On Fri, Sep 28, 2012 at 3:47 PM, Thomas Pfeiffer colo...@autistici.org wrote: Hi everyone, I've now compiled the QML branch of Amarok and will give some feedback to the new Context View, as promised about a month ago. I'm addressing the whole list since I'm not sure who will be working on the CV in the near future. First of all: I really like the look of the new CV! It's much cleaner and fits better with the rest of Amarok's appearance compared to the old one. I'd recommend changing the blue boxes at the bottom of the collection and playlist areas to some shade of grey as well though, since now without the CV's blue, they look rather out of place. I'm a bit worried about the sea-of-grey effect. The coloring of CV and toolbars might have been badly implemented in the past, but at least it prevented that uniform grey look. Perhaps we can do more background images and some color shades, but with the KDE color scheme it's always the question of what color role to use. Now for the usability feedback: It's a bit difficult to give much feedback on the current state, because the more tricky things (like the integration of additional CV modules or their configuration) are not implemented yet. Of course the good thing about it is that we can work together on interaction design for those things before they are implemented, which is less work than having to change things afterwards. My feedback to what's currently there: - The icons look nice in monochrome, but currently they lack a bit in click affordance, meaning that it isn't obvious right away that they are actual buttons vs. merely information icons, because they look very different to the other button icons in Amarok. Maybe some very very slight plasticity effect might help there. Perhaps you should ask the KDE artists team if they can create icons for you which look clearly like clickable icons while still fitting visually in the CV? I had to look up the definition of affordance. Learned something new today, thanks. Perhaps the solution is to make all the buttons in Amarok look similar/nicer? - Unless you plan to re-implement the drag-mouseover feature of the old CV, the CV should definitely be placed on the right of the UI instead of the center by default now. The mouseover options were the only argument for keeping the CV in the center before, and with that removed, it would just make no sense at all to keep it there and forcing users to drag items all the way over the CV to the playlist. You are taking about the PopUpDropper (PUD) which we intent to keep. Even if we didn't the default layout should keep the context in the middle for consitancy with earlier Amarok 2 verions. - I think placing the different content types (lyrics, Wikipedia, etc.) in one long column that can be scrolled continuously instead of switching between them would be better. Of course users could still scroll directly to each type with buttons above, but they could also scroll through them continually. The advantage would be that a user interested in everything about the current song could just read on from lyrics to Wikipedia to e.g. concert dates without having to click any buttons, bzt can still use the buttons to jump to a specific type of information. You mean without a scroll bar for the wikipedia content? It might be a bit long, but I personally loath double scrollbars, so certainly in favor. - I don't know what exactly the new CV currently pulls in from Wikipedia, but it isn't the artist's main article. The main article would make more sense here, since the things currently shown are not very informative. Ideally we'd show Artist, Album and track pages in that order. It's just a bit tricky to do since we are using heuristics to search and a bit of scraping to get the content. We should be using something like dbpedia.org And finally for the future interaction design: First of all, I'd need to know which features of the old CV you are planning to re-implement and which ones you are planning to cut. Depending on these decisions, we can start working on interaction design for the planned features. I'm looking forward to working with the Amarok team again (and now I've finally got the hang of compiling Amarok master or branches from Git easily, so I can try things out immediately)! Cool, I'll make sure you commit to that. I'll keep the long pointy stick ready ;) Thomas P.S.: I'd like to establish the [UI] tag for UI-related threads on this list so that people like me who don't understand purely technical threads anyway can concentrate on the threads relevant for them. If that's okay with you guys, I'd appreciate if you could add that tag to the subjects of UI-related mails in the future. Good idea. But reading the purely technical and social topics should not be avoided. They are the context in which the UI makes sense. UI threads certainly need screenshots. Do we have an easy place to upload them and keep
Re: Review Request: phonon phive core frontend api
On Sept. 26, 2012, 7:42 p.m., Bart Cerneels wrote: core/Player.h, line 39 http://git.reviewboard.kde.org/r/106566/diff/1/?file=87102#file87102line39 Should be addAudioOutput and addVideoOutput if you want to keep them separate. Matěj Laitl wrote: Why? The alternative is an abstract Output class, who's polymorphic sub-classes can be added. Since there is no intention just yet to make that class it's better to keep these method names rather then 2 addOutput() with different arguments. On Sept. 26, 2012, 7:42 p.m., Bart Cerneels wrote: core/Player.h, line 51 http://git.reviewboard.kde.org/r/106566/diff/1/?file=87102#file87102line51 Metadata should go in the source! Would be nice in Amarok that we can simply map Track to Phonon::Source in the metadata handling. Matěj Laitl wrote: +1. I discovered on the train back we need operator() and a global qHash(Phonon::Source) for this. On for use as a key in a QMap, the other in a QHash. On Sept. 26, 2012, 7:42 p.m., Bart Cerneels wrote: core/Player.h, line 52 http://git.reviewboard.kde.org/r/106566/diff/1/?file=87102#file87102line52 no. Total can be unknown and it's a one line calculation. Matěj Laitl wrote: But Phonon may know the length better than we in Amarok. I would personally like to use some kind fo remainingTime signals to implement bounded playback in Amarok. Phonon would internally do the same calculation (remaining = total - elapsed). If you add the convenience function, shouldn't you also add the notify signal, etc? On Sept. 26, 2012, 7:42 p.m., Bart Cerneels wrote: core/Queue.h, line 21 http://git.reviewboard.kde.org/r/106566/diff/1/?file=87104#file87104line21 Provide some convenience functions for this. iterator, index, append, prepend, etc. Matěj Laitl wrote: Do we really need em? We just use enqueue() and clear() in Amarok. Amarok uses it's own queue, other applications probably don't want to make it as complex. Besides, I'm hoping that in Phonon 5 we won't need our own queue implementation either. - Bart --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106566/#review19473 --- On Sept. 25, 2012, 11:06 a.m., Harald Sitter wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106566/ --- (Updated Sept. 25, 2012, 11:06 a.m.) Review request for Amarok and Phonon. Description --- phonon phive core frontend api Diffs - core/AudioDataOutput.h PRE-CREATION core/AudioDataOutput.cpp PRE-CREATION core/AudioOutput.h PRE-CREATION core/AudioOutput.cpp PRE-CREATION core/BackendCapabilities.h PRE-CREATION core/BackendCapabilities.cpp PRE-CREATION core/OutputEffect.h PRE-CREATION core/OutputEffect.cpp PRE-CREATION core/Player.h PRE-CREATION core/Player.cpp PRE-CREATION core/Queue.h PRE-CREATION core/Queue.cpp PRE-CREATION core/Source.h PRE-CREATION core/Source.cpp PRE-CREATION core/VideoDataOutput.h PRE-CREATION core/VideoDataOutput.cpp PRE-CREATION core/abstract/AbstractAudioOutput.h PRE-CREATION core/abstract/AbstractAudioOutput.cpp PRE-CREATION core/abstract/AbstractMediaStream.h PRE-CREATION core/abstract/AbstractMediaStream.cpp PRE-CREATION core/abstract/AbstractVideoOutput.h PRE-CREATION core/abstract/AbstractVideoOutput.cpp PRE-CREATION core/core.pro PRE-CREATION core/effects/SubtitleEffect.h PRE-CREATION core/effects/SubtitleEffect.cpp PRE-CREATION core/five_global.h PRE-CREATION Diff: http://git.reviewboard.kde.org/r/106566/diff/ Testing --- Thanks, Harald Sitter ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: phonon phive core frontend api
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106566/#review19473 --- Live review at Randa by Harald, Mark and me. Some comments mine, some group conclusions. core/AudioDataOutput.h http://git.reviewboard.kde.org/r/106566/#comment15392 I like the idea of deriving this class for advanced use cases. Simple ones (like amarok spectrum visualization) using signals. But signals are probably not appropriate for high frequency events. Your signal would either emit infrequently (once per second), but with bigger datasets or you need a direct callback into the receiver by subclassing it anyway. What about sample rate and sample format? You'll need a property to set the format and another to read the rate. core/BackendCapabilities.h http://git.reviewboard.kde.org/r/106566/#comment15394 Missing a few: VideoOutput, ... How about making this list runtime expandable. Need to think about use cases though. core/Player.h http://git.reviewboard.kde.org/r/106566/#comment15395 New name for MediaObject core/Player.h http://git.reviewboard.kde.org/r/106566/#comment15396 Should be addAudioOutput and addVideoOutput if you want to keep them separate. core/Player.h http://git.reviewboard.kde.org/r/106566/#comment15397 No, should be at the source. Or the video output can tell us. One could be convenience for the other. core/Player.h http://git.reviewboard.kde.org/r/106566/#comment15398 Metadata should go in the source! Would be nice in Amarok that we can simply map Track to Phonon::Source in the metadata handling. core/Player.h http://git.reviewboard.kde.org/r/106566/#comment15399 no. Total can be unknown and it's a one line calculation. core/Player.h http://git.reviewboard.kde.org/r/106566/#comment15400 You want to know the required resolution. And no, it's not easy to calculate when it might change! core/Queue.h http://git.reviewboard.kde.org/r/106566/#comment15401 Provide some convenience functions for this. iterator, index, append, prepend, etc. core/Queue.h http://git.reviewboard.kde.org/r/106566/#comment15402 Couldn't I put the same source in multiple times? Which one will you delete? Again, convenience functions needed. Or will the Queue be set? core/Source.h http://git.reviewboard.kde.org/r/106566/#comment15405 s/Stream/AbstractStream Shouldn't this just be a QIODevice. Sensible interface, let's not define our own. core/Source.h http://git.reviewboard.kde.org/r/106566/#comment15404 Source core/Source.h http://git.reviewboard.kde.org/r/106566/#comment15406 No, you just want to take ownership. Special case on AbstractMediaStream core/Source.h http://git.reviewboard.kde.org/r/106566/#comment15407 To be examined, but me likes. core/VideoDataOutput.h http://git.reviewboard.kde.org/r/106566/#comment15408 Use for simple usecases, derive for advanced stuff. core/abstract/AbstractVideoOutput.h http://git.reviewboard.kde.org/r/106566/#comment15410 Add same effect (object), what happens? Ordering? Could make this a QSet core/effects/SubtitleEffect.h http://git.reviewboard.kde.org/r/106566/#comment15413 Get the list of subtitles from the source. core/effects/SubtitleEffect.h http://git.reviewboard.kde.org/r/106566/#comment15412 properties needed. - Bart Cerneels On Sept. 25, 2012, 11:06 a.m., Harald Sitter wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106566/ --- (Updated Sept. 25, 2012, 11:06 a.m.) Review request for Amarok and Phonon. Description --- phonon phive core frontend api Diffs - core/AudioDataOutput.h PRE-CREATION core/AudioDataOutput.cpp PRE-CREATION core/AudioOutput.h PRE-CREATION core/AudioOutput.cpp PRE-CREATION core/BackendCapabilities.h PRE-CREATION core/BackendCapabilities.cpp PRE-CREATION core/OutputEffect.h PRE-CREATION core/OutputEffect.cpp PRE-CREATION core/Player.h PRE-CREATION core/Player.cpp PRE-CREATION core/Queue.h PRE-CREATION core/Queue.cpp PRE-CREATION core/Source.h PRE-CREATION core/Source.cpp PRE-CREATION core/VideoDataOutput.h PRE-CREATION core/VideoDataOutput.cpp PRE-CREATION core/abstract/AbstractAudioOutput.h PRE-CREATION core/abstract/AbstractAudioOutput.cpp PRE-CREATION core/abstract/AbstractMediaStream.h PRE-CREATION core/abstract/AbstractMediaStream.cpp PRE-CREATION core/abstract/AbstractVideoOutput.h PRE-CREATION core/abstract/AbstractVideoOutput.cpp PRE-CREATION core/core.pro PRE-CREATION
[amarok] src: Give proper place for inactive authors.
Git commit 927187797df79af7ea6ccf3f16563d83cafb232f by Bart Cerneels. Committed on 25/09/2012 at 08:49. Pushed by shanachie into branch 'master'. Give proper place for inactive authors. CCMAIL:amarok-devel@kde.org M +31 -15 src/main.cpp http://commits.kde.org/amarok/927187797df79af7ea6ccf3f16563d83cafb232f diff --git a/src/main.cpp b/src/main.cpp index 203e3b3..fe05d90 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -90,6 +90,37 @@ int main( int argc, char *argv[] ) ki18n(Rokymoter, Handbook (valorie)), valo...@kde.org ); ocsData.addAuthor( valorie, aboutData.authors().last() ); +//Inactive authors +/* This list should contain people who still hold major copyright on the current code + * For instance: does not include authors of 1.4 who have not contributed to 2.x */ +aboutData.addAuthor( ki18n(Inactive authors), + ki18n(Amarok authorship is not a hobby, it's a lifestyle. + But when people move on we want to keep respecting + them by mentioning them here:), ); +ocsData.addAuthor( %%category%%, aboutData.authors().last() ); + +aboutData.addAuthor( ki18n(Ian 'The Beard' Monroe), ki18n(Developer (eean)), i...@monroe.nu ); +ocsData.addAuthor( eean, aboutData.authors().last() ); + +aboutData.addAuthor( ki18n(Jeff 'IROKSOHARD' Mitchell), ki18n(Developer (jefferai)), mitch...@kde.org ); +ocsData.addAuthor( jefferai, aboutData.authors().last() ); + +aboutData.addAuthor( ki18n(Leo Franchi), ki18n(Developer (lfranchi)), lfran...@kde.org ); +ocsData.addAuthor( lfranchi, aboutData.authors().last() ); + +aboutData.addAuthor( ki18n(Lydia 'is wrong(TM)' Pintscher), ki18n(Release Vixen (Nightrose)), ly...@kde.org ); +ocsData.addAuthor( nightrose, aboutData.authors().last() ); + +aboutData.addAuthor( ki18n(Mark 'It's good, but it's not irssi' Kretschmann ), //krazy:exclude=contractions +ki18n(Project founder (markey)), kretschm...@kde.org, http://amarok.kde.org/blog/categories/1-markey; ); +ocsData.addAuthor( MarkKretschmann, aboutData.authors().last() ); + +aboutData.addAuthor( ki18n(Nikolaj Hald 'Also very hot' Nielsen), ki18n(Developer (nhn)), n...@kde.org ); +ocsData.addAuthor( nhnFreespirit, aboutData.authors().last() ); + +aboutData.addAuthor( ki18n(Maximilian Kossick), ki18n(Developer (maxx_k)), maximilian.koss...@gmail.com ); +ocsData.addAuthor( QString(), aboutData.authors().last() ); + //Contributors aboutData.addCredit( ki18n(Alejandro Wainzinger), ki18n(Developer (xevix)), aikawaraz...@gmail.com ); @@ -118,10 +149,6 @@ int main( int argc, char *argv[] ) ocsData.addCredit( oggb4mp3, aboutData.credits().last() ); aboutData.addCredit( ki18n(Harald Sitter), ki18n(Phonon, Lord-President of KDE Multimedia (apachelogger)), harald.sit...@kdemail.net ); ocsData.addCredit( apachelogger, aboutData.credits().last() ); -aboutData.addCredit( ki18n(Ian 'The Beard' Monroe), ki18n(Developer (eean)), i...@monroe.nu ); -ocsData.addCredit( eean, aboutData.credits().last() ); -aboutData.addCredit( ki18n(Jeff 'IROKSOHARD' Mitchell), ki18n(Developer (jefferai)), mitch...@kde.org ); -ocsData.addCredit( jefferai, aboutData.credits().last() ); aboutData.addCredit( ki18n(John Atkinson), ki18n(( Assorted patches )), j...@fauxnetic.co.uk ); ocsData.addCredit( fauxnetic, aboutData.credits().last() ); aboutData.addCredit( ki18n(Kenneth Wesley Wimer II), ki18n(Icons), kw...@bootsplash.org ); @@ -132,29 +159,18 @@ int main( int argc, char *argv[] ) ocsData.addCredit( zizzfizzix, aboutData.credits().last() ); aboutData.addCredit( ki18n(Lee Olson), ki18n(Artwork), leetol...@gmail.com ); ocsData.addCredit( QString(), aboutData.credits().last() ); -aboutData.addCredit( ki18n(Leo Franchi), ki18n(Developer (lfranchi)), lfran...@kde.org ); -ocsData.addCredit( lfranchi, aboutData.credits().last() ); aboutData.addCredit( ki18n(Ljubomir Simin), ki18n(Rokymoter (ljubomir)), ljubomir.si...@gmail.com ); ocsData.addCredit( ljubomir, aboutData.credits().last() ); aboutData.addCredit( ki18n(Lucas Gomes), ki18n(Developer (MaskMaster)), x8luca...@gmail.com ); ocsData.addCredit( x8lucas8x, aboutData.credits().last() ); -aboutData.addCredit( ki18n(Lydia 'is wrong(TM)' Pintscher), ki18n(Release Vixen (Nightrose)), ly...@kde.org ); -ocsData.addCredit( nightrose, aboutData.credits().last() ); -aboutData.addCredit( ki18n(Mark 'It's good, but it's not irssi' Kretschmann ), //krazy:exclude=contractions -ki18n(Project founder (markey)), kretschm...@kde.org, http://amarok.kde.org/blog/categories/1-markey; ); -ocsData.addCredit( MarkKretschmann, aboutData.credits().last() ); aboutData.addCredit( ki18n(Mathias Panzenböck), ki18n
Re: Status update: KIO-MTP
On Sun, Sep 23, 2012 at 11:55 PM, Philipp Schmidt philschm...@gmx.net wrote: Hi all, just a little status update on the kio-mtp progress. The Project is now in playground. You can find it here: https://projects.kde.org/projects/playground/base/kio-mtp The most important changes: - More stability - More speed thanks to caching, especially when copying lots of small files where the lookup for the file id (needed for copying) took too long. Since you usually list the directory they are in prior to copying they are now in the cache which now results in ludicrous speed (yeah Mel Brooks, I'm looking at you :D) increases for lots of small files (e.g. Photos), not so much for a few big files ;). Still no compiling instructions, these will follow with the first official Alpha. So, if you don't now how to cmake/make/make install, leave it be for the time being. Also: Would someone be willing to tolerate a guest post on his/her blog where I could announce it (I don't have one [yet])? Preferably Alex Fiestas, since he ist kind of involved ;), but I think once it is ready enough an announcement should be made at least somewhere, so people know it's coming. Cheers, Philipp Cloned it and compiled without effort, looking at the code now. Perhaps you can throw this on reviewboard so we can comment? ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Status update: KIO-MTP
reviewboard.kde.org (make sure to put me in the reviewers list nick: shanachie) and yes, upload the complete code. On Mon, Sep 24, 2012 at 10:14 AM, Philipp Schmidt philschm...@gmx.net wrote: Hi, Am Montag, 24. September 2012, 08:03:07 schrieb Bart Cerneels: Cloned it and compiled without effort, looking at the code now. Perhaps you can throw this on reviewboard so we can comment? Reviewboard is still in the process of being set up, so not yet. Do you mean I should put the whole code up there? Philipp ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: MetaFile::Track: only provide statistics stored in tags
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106550/#review19376 --- Ship it! Looks good. ChangeLog http://git.reviewboard.kde.org/r/106550/#comment15334 Is that Czech english? ;) instead of keeping then in the database. src/core-impl/meta/file/File.h http://git.reviewboard.kde.org/r/106550/#comment15336 These are a bit obscure. But since they are private and properly documented I guess the name does not matter much. - Bart Cerneels On Sept. 23, 2012, 11:39 p.m., Matěj Laitl wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106550/ --- (Updated Sept. 23, 2012, 11:39 p.m.) Review request for Amarok. Description --- MetaFile::Track: only provide statistics stored in tags As agreed in Randa that non-collection tracks shouldn't use external tables to store statistics. When user doesn't enable writing stats to files, we don't even read them in order not to annoy the user. Also clean up the setter methods in Meta::Track, code sharing+++ Diffs - ChangeLog 7b58b74eae5f496dc714a78398133e7c08dd96fb src/core-impl/meta/file/File.h 650c79d0f3c6af581c117f33382095db5018c63f src/core-impl/meta/file/File.cpp a3ca501449590d2f43a30a9b288cf51089515bff src/core-impl/meta/file/File_p.h d29d0948047407d5798dd98a21fd75a8f66223f7 tests/core-impl/meta/file/TestMetaFileTrack.cpp 6ee7f3c7e79dd1c18caa3549669c670dc3502686 tests/core/meta/TestMetaTrack.h 5eb49f0f73266542efb19065cda5acf57be44acc tests/core/meta/TestMetaTrack.cpp 68bc84f8e6fc3efa6d6c5fd74e41808d01edd5af Diff: http://git.reviewboard.kde.org/r/106550/diff/ Testing --- Works. This is an implementation of what has been agreed on on the requirements/architecture meetings. Running this through review to make sure we all want this. Thanks, Matěj Laitl ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Authorship
On Mon, Sep 24, 2012 at 7:31 PM, Jeff Mitchell mitch...@kde.org wrote: On 09/24/2012 09:51, Valorie Zimmerman wrote: On Sat, Sep 22, 2012 at 11:17 PM, Bart Cerneels bart.cerne...@kde.org wrote: The currently active Amarok team has defined the authors list to contain current contributors that have ownership (written it or maintaining it for a while) of amarok components or non-coding contributions to the project. As far as I can tell there is no legal reason to list all past contributors as authors. Myriam pushed a commit reflecting what we thought was the current state of that list yesterday. A few people don't seem to agree and reverted their own entries. At the time of the discussion, I asked if we were moving no-longer-active authors to a previous author category or similar, and was assured that this was what was being done. After examining the diff, I see that that is *not* true, and the former authors have been moved into Contributors. This seems disrespectful to me, and not in the spirit of teamwork that we want to foster. Legalities? I believe those are satisfied by headers. Our About dialog is about community, not legality. I very much hope we can respectfully honor the work of major contributors in the past, and not move them into the same bin in which we place those who have contributed a few euros or a couple of patches. I agree with the sentiment, although I should mention that in the past we've moved previous authors to contributors...but for lack of a better solution. So what compromise do we execute? I would rename the Thanks To tab to Contributors and add proper attribution as author (with years of service) to the appropriate people. Bart ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Authorship
The currently active Amarok team has defined the authors list to contain current contributors that have ownership (written it or maintaining it for a while) of amarok components or non-coding contributions to the project. As far as I can tell there is no legal reason to list all past contributors as authors. Myriam pushed a commit reflecting what we thought was the current state of that list yesterday. A few people don't seem to agree and reverted their own entries. I assume those who consider themselves representatives and active contributors of the Amarok project are willing to do the work that earns that distinction. Let me point you to some continually ongoing tasks: Amarok regressions: https://bugs.kde.org/buglist.cgi?cmdtype=doremremaction=runnamedcmd=Amarok%3A%20regressionsharer_id=70102list_id=224345 Amarok release blockers: https://bugs.kde.org/buglist.cgi?cmdtype=doremremaction=runnamedcmd=Amarok%3A%20release_blockersharer_id=70102list_id=224346 Amarok most hated: https://bugs.kde.org/buglist.cgi?cmdtype=doremremaction=runnamedcmd=Amarok%3A%20most%20hatedsharer_id=70102list_id=224347 And this weekend the meetings we hold at Randa will in a very significant way determine the future of Amarok: http://community.kde.org/Sprints/Randa/2012/Multimedia#Amarok_Topics As an author it's a given you'll try to contribute by being physically there, prepared materials to support the meeting or join by video chat (we hosted a G+ hangout for all the Amarok team to join, promise to be better with scheduling and lighting today). Bart ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Authorship
On Sun, Sep 23, 2012 at 10:49 AM, Alex Fiestas afies...@kde.org wrote: Hey... Sorry if this topic wasn't one to joke about, yesterday night I was told of the tensions going on because of the changes and it seemed that a little joke will chill the environment a little bit. If you take a look at the year (2020) or at few words (rey means king) it is clearly seen that I wasn't trying to actually claim any real authorship but rather only joke around. Anyway, please have my deepest excuses if I had offended anyone with it, wasn't my intention and accept a beer and my apologies in compensation. Cheerz. Hehe ,we were sort of thinking that looking at the various clues around.To bad, we were hoping to pull you in as a contributor. Feel free to join us in the meetings when you have time anyway. First one starts at 11:00, architecture is in the afternoon. In any case you owe us beers! Sparking a discussion about what authorship means is not a bad thing though. We'll certainly discuss it among the people here in Randa, trying to keep the arguments from those who could not join in mind. But unilaterally deciding to move someone from the authors list without prior discussion is not nice to say the least. I won't add fuel to the fire by immediately reverting it. We asked Mark how he feels about moving to the contributors list, noticed he did do a release this year and decided to keep him in. ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Fix DAAP collections published by Rhythmbox not showing up in Amarok.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106366/#review18778 --- Normally uidUrl() should be used. It's a bug we'll have to look into. - Bart Cerneels On Sept. 8, 2012, 7:11 p.m., Daniel Stöckel wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106366/ --- (Updated Sept. 8, 2012, 7:11 p.m.) Review request for Amarok. Description --- This simply sends a fake revision-number to the server. Also appends the session-id field to the track URLs. See: https://bugs.kde.org/show_bug.cgi?id=306351 See: https://bugs.kde.org/show_bug.cgi?id=306351 Diffs - src/core-impl/collections/daap/DaapCollection.h f8655ec src/core-impl/collections/daap/DaapCollection.cpp 5d1bd9b src/core-impl/collections/daap/DaapMeta.h 5278b57 src/core-impl/collections/daap/DaapMeta.cpp a2429d7 src/core-impl/collections/daap/daapreader/Reader.cpp b6196e1 src/core-impl/collections/support/CollectionManager.cpp d196403 Diff: http://git.reviewboard.kde.org/r/106366/diff/ Testing --- - Rhythmbox collections now correctly appear and play in Amarok. - Validated that mt-daap collections are still working. Thanks, Daniel Stöckel ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Use multiple inheritance to get an implementation for statistics handling
On Sept. 9, 2012, 10:22 a.m., Matěj Laitl wrote: There are some welcome changes in this patch: * ability to implement statistics by inheritance * StatisticsCapability removal However, there are some things I dislike: * Meta::Track inheriting AbstractStatistics * setAllStatistics() method (gosh!) * Overusing the StatisticsProvider idea. I think that statistics are implemention detail of each collection. I even dislike the statistics_permanent and statistics_tag tables and I'd like to see them ditched. If someone wants to have statistics for tracks on filesystem, put them in tags. Otherwise put the tracks into Local Collection. I'd like if the SqlCollection would be the only thing that would actually require the SQL database. What I'd like to do instead: * StatisticsCapability is already ditched in my gsoc branch, with setPlaycount() and setFirst/LastPlayed() moved to EditCapability * move also setRating(), setScore() into EditCapability * refactor the whole capability concept (for tracks to start with) along with changing Meta::Track to be passed-by-value class with explicit data sharing. This would then mean that tracks could implement Capabilities (to be named better, perhaps Interfaces) by simply implementing the methods and then they could just return this; in relevant methods. Let me present you the suggestion in a couple of days. Ralf Engels wrote: The best implementation depends mainly on how you see the statistics in relation to the tracks. 1. If you see statistics as something that only some collections have, then Capabilities are fine for it. (However I still don't like the Capabilities because they are running against the inheritance concept and add complexity, since we suddenly have tracks with vastly different capabilities) 2. If you see statistics as something that every track has, but is implemented in each collection, then it would look differently. One implementation per collection. No shared statistics tables. But also no Statistsics providers, since we don't need them if we have different implentations for the collections. 3. If statistics are a thing that every collection should have and we want to provide a default implementation, then something like my proposal is what you need. - A default implementation that covers most cases. - Sharing of statistics over the collections. - More usage of the StatisticsTagProvider Now, which of the three cases do we have in Amarok? The rating() function is build into the Meta::Track, as well as finishedPlaying(). So it seem to me that we don't want to have the first case. A lot of collections use the PermanentUrlStatisticsProvider. And the collections that don't use it should, since it adds functionality without additional cost (in code complexity). So we don't have the second case, since the implementations are not different but mostly the same. For me it seems that we have the third case. Statistics were cleanly ment to be build into the Tracks. You even get the value changed indication for the track if statistics are changed, so conceptually the statistics seem to be part of the track as e.g. the title. A clear indication that we have case three is the fact that cleaning up the code removed around 500 lines (while I added a lot of comments at the same time). Now, let's look into the stuff again: 1. I was not bold enough to flatly provide a default implementation for statistics in Meta::Track. That is even difficult since Meta::Track is in core while sql handling is in core-impl. So it would make sense to provice an abstract default implementation. Also having an abstract statistics provider class makes it very clear what functions are needed to be implemented for a full statistics provider. 2. setAllStatistics is a performance improvement. The most common statistic settings operation is finishedPlaying which sets three values. So you have three sql commits or another way to group the operations together. You can do that with a write later or block update function. But have a look at the simplistic implementation of SqlMeta and you see that a setAllStatistics is easier. 3. overuse of statistics provider. That is a clear form follows function. If we have the function that I outlined above in (3) and no way to provide a default implementation in core then we need some type of mix-ins. I have tried to implement it in two different ways but the StatisticsProvider is the best step for now. Other refactoring tries had much bigger code changes with much less cleanup effect. As further steps I propose: 1. create a statistics table to be used by everybody. 2. try to move more collections to the tag
Re: Review Request: Use multiple inheritance to get an implementation for statistics handling
then we need some type of mix-ins. I have tried to implement it in two different ways but the StatisticsProvider is the best step for now. Other refactoring tries had much bigger code changes with much less cleanup effect. This is clear consequence of you wanting the option 3. Please consider my arguments above that it would be a *really bad* idea. As further steps I propose: 1. create a statistics table to be used by everybody. I strongly oppose. Some tracks should be incapable of having statistics. Some tracks need to save it differently (iPod collection, UMS colleciton with iD3 tags, MTP collection, Nepomuk collection). 2. try to move more collections to the tag statistics provider which would ensure that a track played from the file collection remains it's rating once it's imported into the sql collection (among other things) Already implemented (by me) in SqlCollectionLocation, test it! 3. Change the name of the StatisticsProvider. Google the provider pattern for an enlightning read. This is not a provider and we shouldn't call it such. Alternative: ditch StatisticsProvider. Ralf Engels wrote: Let's first discuss about the functionality. (and not answer your specific points because that is useless unless we agree on the requirements) I would like all collections to have full statistics support. Where the underlying mechanisms don't support it I want to have a default statistics support storing the rating locally. The collections that currently use the statistics provider are: AudioCd DaapMeta UpnpMeta File TimecodeMeta (whatever that is) ServiceMeta (and by implication all collections that inherit but don't provide a different implementation) PodCastMeta has a problem, as it is in core and thus can't use any of the providers. Don't talk about implementation right now, just about the requirement. What is the requirement? Should all collections support ratings even if the underlying mechanism doesn't? I personally would like to have ratings for files on write protected network file systems, audio cds and podcasts. Matěj Laitl wrote: Hmm, thinking about it I must agree that having ratings for some of these would be nice. Please give me more time to think about it and to perhaps incorporate it to my Meta::Track Capabilities refactor suggestion. Bart Cerneels wrote: A lot of collections use the PermanentUrlStatisticsProvider. And the collections that don't use it should, since it adds functionality without additional cost (in code complexity). So we don't have the second case, since the implementations are not different but mostly the same. This is IMO our design failure. PermanentUrlStatisticsProvider has little sense IMO. If you want Amarok to track the stats of a track - put it in the Local Collection. Else you have to put the statistics into the track. Period. PermanentUrlStatisticsProvider adds little functionality (urls can easily change, not accessible outside of Amarok) and duplicates our existing Local Collection efforts. Just ditch it. I agree with this. I question the use case of wanting to keep/know the rating of a non local-collection track, or tracks on mediaplayer that support it, etc. It's probably a case of developers scratching an imaginary itch and real users probably don't even see the problem being solved. We can remove the complexity at the requirement level and improve the functionality we do really need. Ralf makes a very good argument though. AudioCD, DAAP and UPnP don't support native statistics (UPnP in theory does, but I've never seen a working implementation). File can use ID3 tags, we already have the support code. Not sure what to do with non-writable. ServiceMeta will be gone soon. We will make the ServiceCollections (Ampache MP3Tunes) real collections, the stores will use a simple TrackProvider and the rest (last.fm, opmldirectory, gpodder) is not using it currently. Dedicated podcast players on devices always have statistics (% played or in play sections even). UMS and similar bare file implementations can use ID3 tags (from MetaFile::Track). So it still comes down to: do our users care that some collections don't support statistics? I think there are decent enough implementations for both yes and no answers, with yes obviously being more complex. We just have to select which one. - Bart --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106276/#review18709 --- On Aug. 30, 2012, 12:03 p.m., Ralf Engels wrote
TODO for 2.7: android MTP
http://amarok.kde.org/en/releases/2.6#comment-14114 Having recently bought a Galaxy 3 I can confirm, we need have very good MTP support i.e. rewrite MtpCollection. For anyone taking it on (/me looks in the general direction of strohel) I might have a device you can use. Bart ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: API proposal: Phonon BackendInfo
On Sun, Sep 9, 2012 at 3:04 PM, Harald Sitter sit...@kde.org wrote: On Sun, Sep 9, 2012 at 2:14 PM, Matěj Laitl ma...@laitl.cz wrote: /** @returns \c true when this backend was provided by a platform plugin */ bool isPlatformProvided() const; Perhaps this has sense to people who known how Phonon is implemented in Qt, but I don't understand what provided by platform plugin means. Rationale being that if the backend is from the platform plugin libphonon does not actually know where it is loaded from. So that is in fact a convenience overload of path() (i.e. if the path is empty it is always a platform provided backend). #warning TODO: do we really want this here? /** @returns \c true when the backend uses PulseAudio */ bool usesPulseAudio(); While this seems very quirky, we really do want to show this in Amarok Diagnostics as it affects too many things. This in formation is actually available via the PulseSupport class, so the only rationale for having it here is to have it all in one place. However at the same time it is entirely wrong to have it in a class called BackendInfo because it does not actually reflect backend support but whether PulseSupport is active. So from a puristic POV this ought not be here. HS Couldn't the pulseaudio stuff be considered platform integration? I would be happy enough with a string indication which platform phonon and it's backends are using. example: linux (pulseaudio), linux (ALSA), Windows (DirectSound), ... And yes, I do want the OS in that string. Our diagnostics dialog is easy to copy paste by inexperienced users and it's good confirmation that they are actually using the platform they are talking about. Since we are building for windows and apple OSX as well, don't want to ifdef in the diagnostics dialog when it's not really needed, so provide a platform independent function returning platform specific info please. ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Fix DAAP collections published by Rhythmbox not showing up in Amarok.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106366/#review18649 --- Cool, about time the DAAP Collection gets some love. Hope we can expect more patches from you. Is there a published spec we can develop against? I'll test later that this still works with the mt-daap I'm running (Firefly Media Server: Version svn-1676 on a netgear readynas Duo v1) - Bart Cerneels On Sept. 7, 2012, 9:45 a.m., Daniel Stöckel wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106366/ --- (Updated Sept. 7, 2012, 9:45 a.m.) Review request for Amarok. Description --- This simply sends a fake revision-number to the server. See: https://bugs.kde.org/show_bug.cgi?id=306351 Diffs - src/core-impl/collections/daap/daapreader/Reader.cpp b6196e1 Diff: http://git.reviewboard.kde.org/r/106366/diff/ Testing --- - Rhythmbox collections now correctly appear in Amarok. - Validated that mt-daap collections are still working. Thanks, Daniel Stöckel ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Fix Permission denied when playing a track from a Rhythmbox DAAP collection
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106370/#review18650 --- You should update https://git.reviewboard.kde.org/r/106366/ with these new changes and close this one. Will review there. For help to update the review-request ask on #amarok on irc.freenode.org. - Bart Cerneels On Sept. 7, 2012, 9:51 a.m., Daniel Stöckel wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106370/ --- (Updated Sept. 7, 2012, 9:51 a.m.) Review request for Amarok. Description --- This is a followup to https://git.reviewboard.kde.org/r/106366/ Rhythmbox seems to require that the session-id field is present in a track get request. This patch adds an additional parameter to the DaapTrack class constructor and appends the session id to the Track URL. Diffs - src/core-impl/collections/daap/DaapMeta.h 5278b57 src/core-impl/collections/daap/DaapMeta.cpp a2429d7 src/core-impl/collections/daap/daapreader/Reader.cpp b6196e1 Diff: http://git.reviewboard.kde.org/r/106370/diff/ Testing --- - Tracks published by Rhythmbox now play correctly - Daap collections published by mt-daap still work Thanks, Daniel Stöckel ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Fix DAAP collections published by Rhythmbox not showing up in Amarok.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106366/#review18654 --- src/core-impl/collections/daap/DaapMeta.cpp http://git.reviewboard.kde.org/r/106366/#comment14747 Since the playable url now contains a session ID, I wonder if it will still be playable after restart. This might not be an issue if the track can be resolved using it's uidUrl by the DaapCollection the should become available not long after startup. src/core-impl/collections/daap/daapreader/Reader.cpp http://git.reviewboard.kde.org/r/106366/#comment14746 I would move the login assignment to below the connect. - Bart Cerneels On Sept. 7, 2012, 12:16 p.m., Daniel Stöckel wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106366/ --- (Updated Sept. 7, 2012, 12:16 p.m.) Review request for Amarok. Description --- This simply sends a fake revision-number to the server. Also appends the session-id field to the track URLs. See: https://bugs.kde.org/show_bug.cgi?id=306351 See: https://bugs.kde.org/show_bug.cgi?id=306351 Diffs - src/core-impl/collections/daap/DaapMeta.h 5278b57 src/core-impl/collections/daap/DaapMeta.cpp a2429d7 src/core-impl/collections/daap/daapreader/Reader.cpp b6196e1 Diff: http://git.reviewboard.kde.org/r/106366/diff/ Testing --- - Rhythmbox collections now correctly appear and play in Amarok. - Validated that mt-daap collections are still working. Thanks, Daniel Stöckel ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Fix DAAP collections published by Rhythmbox not showing up in Amarok.
On Sept. 7, 2012, 12:34 p.m., Bart Cerneels wrote: src/core-impl/collections/daap/DaapMeta.cpp, line 43 http://git.reviewboard.kde.org/r/106366/diff/2/?file=84097#file84097line43 Since the playable url now contains a session ID, I wonder if it will still be playable after restart. This might not be an issue if the track can be resolved using it's uidUrl by the DaapCollection the should become available not long after startup. Daniel Stöckel wrote: I was about to say: It definitely will not work but this is not entirely true. Apparently one can restart Amarok and still play a track published by Rhythmbox. However once Rhythmbox restarts the track can no longer be played. So it basically comes down to the level of quirkiness on the server side. On the Amarok side a quick git grep told me that a DaapTrack is instantiated at exactly one place: when downloading the TrackList from the server. Thus i do not think that any kind of resolving is actually taking place. But still, i know basically nothing about the inner workings of Amarok. Normally there is a CollectionManager::trackForUrl() call done when the playlist is restored from XSPF. The DaapCollection might not have been created yet at that point but a loader will then start listening for collectionAdded signals and try to resolve again. If there is some way to have a unique identifier for a DAAP resource that can be loaded again, replaying should be possible. If rythembox does not offer such unique ID it's their fault. - Bart --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106366/#review18654 --- On Sept. 7, 2012, 1:07 p.m., Daniel Stöckel wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106366/ --- (Updated Sept. 7, 2012, 1:07 p.m.) Review request for Amarok. Description --- This simply sends a fake revision-number to the server. Also appends the session-id field to the track URLs. See: https://bugs.kde.org/show_bug.cgi?id=306351 See: https://bugs.kde.org/show_bug.cgi?id=306351 Diffs - src/core-impl/collections/daap/DaapMeta.h 5278b57 src/core-impl/collections/daap/DaapMeta.cpp a2429d7 src/core-impl/collections/daap/daapreader/Reader.cpp b6196e1 Diff: http://git.reviewboard.kde.org/r/106366/diff/ Testing --- - Rhythmbox collections now correctly appear and play in Amarok. - Validated that mt-daap collections are still working. Thanks, Daniel Stöckel ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Extend the scope of the playground
On Aug. 22, 2012, 8:25 a.m., Bart Cerneels wrote: I would like to see the code you are working on that has a need for this change. I still think that it should go directly in core. Matěj Laitl wrote: Yup. I don't really think we should merge this. `git branch` is much better tool than CMake variables and C preprocessor. Ryan McCoskrie wrote: It's not so much that the code needs this change as I got told off last time* for not putting it under the playground. *Long story short, I lost my work Matěj Laitl wrote: Ehm, I don't understand.. What happened? Where you were told to put something into playground? Developers and practices change.. From what I remember Ryan's work is scripting support for the equalizer. Like always it will be developed in a personal branch kept locally and on git.kde.org so work can not easily be lost. If it's a multi-person collaboration that is discussed and vetted prior on amarok-devel@kde.org it can be pushed to a branch on our main repo. Once it's ready and deemed suitable it can be merged into master and maintained along with the whole of Amarok. Can't comment on the code quality of the lost work, but I do see the advantage of having scripted equalizer settings. It could be easier to maintain, enables various use cases we can't support in the C++ code and clearly there is someone willing to fix bugs and augment it for a while. Ryan: any way we can help recovering your lost work? What is your IRC nickname, I'll try to help directly. - Bart --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103999/#review17840 --- On Aug. 19, 2012, 10:09 p.m., Ryan McCoskrie wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103999/ --- (Updated Aug. 19, 2012, 10:09 p.m.) Review request for Amarok. Description --- This is infrastructure for future patches of mine (specifically equalizer scripting now that I am back on it). The intention of this patch is to allow for the /playground directory to compile code directly into the Amarok binary. The benefit this has to the Amarok project is that stable releases can include some experimental featues that power users can opt into using. Changes made to this patch: 0 Separated from a minor clean up of the playground 0 Separated from my equalizer scripting code (How on Earth did I overlook that?) 0 Added a brief tutorial on compiling in playground code 0 Now address all platforms, not just those using X11 0 Now intended for serious consideration of shipping. Diffs - CMakeLists.txt ebb8064 playground/PLAYER_BINARY.txt PRE-CREATION playground/src/CMakeLists.txt ed740ec src/CMakeLists.txt 8596144 Diff: http://git.reviewboard.kde.org/r/103999/diff/ Testing --- Checked that the code compiles with the playground option enabled. Since there is no active code, this should be sufficiant. Thanks, Ryan McCoskrie ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: GSoC : Nepomuk plugin for Amarok
On Aug. 20, 2012, 10:46 p.m., Matěj Laitl wrote: src/core-impl/collections/nepomukcollection/NepomukCollection.cpp, line 281 http://git.reviewboard.kde.org/r/106042/diff/4/?file=79201#file79201line281 Register the job with Amarok::Components::logger(), mentioning its abort() slot. Phalgun Guduthur wrote: I tried doing that, but Amarok::Components::logger() doesn't work with ThreadWeaver jobs directly right? Is there a way to use ThreadWeaver jobs with the logger? Only KJobs (include KIO::Job) and cutom QObjects which emit the right signals. You can use the last one to make it work. If there are multiple threadweaver jobs that need to do the same (perhaps multiple inside your code) you should add support for it to Logger. - Bart --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106042/#review1 --- On Aug. 20, 2012, 11:17 a.m., Phalgun Guduthur wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106042/ --- (Updated Aug. 20, 2012, 11:17 a.m.) Review request for Amarok, Edward Hades Toroshchin, Vishesh Handa, and Matěj Laitl. Description --- Nepomuk plugin for Amarok. Almost all of the code changes can be found in src/core-impl/collections/nepomukcollection/* And a minor change in src/core-impl/collections/support/MemoryMeta.cpp Code builds and after Nepomuk plugin is activated in the Settings dialog, Nepomuk Plugin comes into play and queries all the tracks in your machine. The query is not 'that' fast and might take several seconds depending on the number of tracks in your box. IMPORTANT : Make sure Nepomuk is enabled if you want to give the plugin a spin. Diffs - src/core-impl/collections/CMakeLists.txt c78b920 src/core-impl/collections/nepomukcollection/CMakeLists.txt 7cfd4b0 src/core-impl/collections/nepomukcollection/NepomukAlbum.h 185c25a src/core-impl/collections/nepomukcollection/NepomukAlbum.cpp 6a09a1b src/core-impl/collections/nepomukcollection/NepomukArtist.h 6fcedf3 src/core-impl/collections/nepomukcollection/NepomukArtist.cpp 13ddf01 src/core-impl/collections/nepomukcollection/NepomukCollection.h 928b145 src/core-impl/collections/nepomukcollection/NepomukCollection.cpp cb185e8 src/core-impl/collections/nepomukcollection/NepomukCollectionFactory.h PRE-CREATION src/core-impl/collections/nepomukcollection/NepomukCollectionFactory.cpp PRE-CREATION src/core-impl/collections/nepomukcollection/NepomukComposer.h 1b11325 src/core-impl/collections/nepomukcollection/NepomukComposer.cpp f21251e src/core-impl/collections/nepomukcollection/NepomukConstructMetaJob.h PRE-CREATION src/core-impl/collections/nepomukcollection/NepomukConstructMetaJob.cpp PRE-CREATION src/core-impl/collections/nepomukcollection/NepomukGenre.h ce0e3b7 src/core-impl/collections/nepomukcollection/NepomukGenre.cpp 945074c src/core-impl/collections/nepomukcollection/NepomukQueryMaker.h 50067de src/core-impl/collections/nepomukcollection/NepomukQueryMaker.cpp 33163ea src/core-impl/collections/nepomukcollection/NepomukRegistry.h a21347e src/core-impl/collections/nepomukcollection/NepomukRegistry.cpp 8afa199 src/core-impl/collections/nepomukcollection/NepomukTrack.h 77dd8c7 src/core-impl/collections/nepomukcollection/NepomukTrack.cpp 7db01cf src/core-impl/collections/nepomukcollection/NepomukYear.h 504cbe2 src/core-impl/collections/nepomukcollection/NepomukYear.cpp 1f13de0 src/core-impl/collections/nepomukcollection/amarok_collection-nepomukcollection.desktop 1ac9f02 src/core-impl/collections/nepomukcollection/meta/NepomukAlbum.h PRE-CREATION src/core-impl/collections/nepomukcollection/meta/NepomukAlbum.cpp PRE-CREATION src/core-impl/collections/nepomukcollection/meta/NepomukArtist.h PRE-CREATION src/core-impl/collections/nepomukcollection/meta/NepomukArtist.cpp PRE-CREATION src/core-impl/collections/nepomukcollection/meta/NepomukComposer.h PRE-CREATION src/core-impl/collections/nepomukcollection/meta/NepomukComposer.cpp PRE-CREATION src/core-impl/collections/nepomukcollection/meta/NepomukGenre.h PRE-CREATION src/core-impl/collections/nepomukcollection/meta/NepomukGenre.cpp PRE-CREATION src/core-impl/collections/nepomukcollection/meta/NepomukLabel.h PRE-CREATION src/core-impl/collections/nepomukcollection/meta/NepomukLabel.cpp PRE-CREATION src/core-impl/collections/nepomukcollection/meta/NepomukTrack.h PRE-CREATION src/core-impl/collections/nepomukcollection/meta/NepomukTrack.cpp PRE-CREATION
Re: Review Request: GSoC : Nepomuk plugin for Amarok
On Aug. 20, 2012, 10:46 p.m., Matěj Laitl wrote: src/core-impl/collections/nepomukcollection/NepomukConstructMetaJob.h, line 71 http://git.reviewboard.kde.org/r/106042/diff/4/?file=79206#file79206line71 This is an important design issue: You either need MemoryCollection or all these hashes, but not both. I'm still not convinced that you cannot avoid MemoryCollection at all, but if you use it, don't duplicate MapChanger. Edward Hades Toroshchin wrote: There are two issues here. First, it would be much better to have implemented an own QueryMaker that would translate Amarok's queries to Nepomuk's queries. This would, of course, render MemoryCollection unneeded. However, we did not go that way in this project, and you can ask me (or phalgun) personally, if you want to learn the reasons. Second, we tried to rely on MemoryCollection, and it didn't go very well. Main reason is that it mostly checks uniqueness by the uniqueness of titles (or title+artist for albums). However, it doesn't do this very reliably (or I didn't understand how to use it correctly), so some quirks did creep out here and there. So, we switched to nepomuk-uid-uniqueness, handled by NepomukCollection. TL;DR: this is quasi-temporary solution as it is, as we will move to NepomukQueryMaker eventually. So let's leave these hashes where they are. Matěj Laitl wrote: First, it would be much better to have implemented an own QueryMaker that would translate Amarok's queries to Nepomuk's queries. Total agreement. Second, we tried to rely on MemoryCollection, and it didn't go very well. Main reason is that it mostly checks uniqueness by the uniqueness of titles (or title+artist for albums). This is how MemoryCollection works. It is used for dumb back-ends that don't know tracks of an album etc. However, it doesn't do this very reliably (or I didn't understand how to use it correctly), Well, it works as it is supposed to work. E.g. you should NOT find existing album (etc.) entities for new tracks you add to MemoryColleciton through MapChanger. You should return new instance every time (see IpodMeta::Track), MapChanger does the matching. Any matching you do yourself is completly unused. so some quirks did creep out here and there. So, we switched to nepomuk-uid-uniqueness, handled by NepomukCollection. Has *no* effect at all, because the world sees the MemoryCollection maps, and it uses name-uniqueness. *No one* (apart from MapChanger for a short while) sees NepomukAlbum etc., and NepomukAlbum::tracks() is *never* called when using MapChanger. NepomukTrack is hidden behind MemoryMeta::Track. Test it! TL;DR: this is quasi-temporary solution as it is, as we will move to NepomukQueryMaker eventually. So let's leave these hashes where they are. Understand that MemoryCollection is rather good temporary solution, but it is used incorrectly. (e.g. there's a lot of completely useless code) Edward Hades Toroshchin wrote: Again: we had used MemoryCollection as you describe and it didn't work well. I didn't want to dig into it (and I don't want to now). (if you don't take my word for it, or want to dig into it, please checkout some older commits from phalgun's branch). I understand how MemoryCollection works, and I understand that there is a certain effort duplication, as well as (some) wasted memory. So, to summarize, there are two options: either accept this uncool temporary solution, or switch it back to a cool temporary solution that works worse. I don't see much sense in the latter, because it's still temporary solution, and (if we believe phalgun) promises to be quite a short-lived one. Matěj Laitl wrote: So, to summarize, there are two options: either accept this uncool temporary solution, or switch it back to a cool temporary solution that works worse. The cool temporary solution *cannot*, by definition, work worse. Read my above comment again, all that additional code has *no visible effect*. How can I convince you? (do you see that all these hashes are used only for short while when the NepomukConstructMetaJob runs and then they're discarded?) I don't see much sense in the latter, because it's still temporary solution, and (if we believe phalgun) promises to be quite a short-lived one. The cool temporary solution would be a couple of hundreds code lines less, witch nearly no effort. Worth it IMO. Edward Hades Toroshchin wrote: You're talking theory, I'm talking practice. Been there, done it, gone away. I'm just saying we shouldn't waste our time perfecting a _temporary_ solution. And it will amount to some time wasted, because we already wasted it once, trying to use
Re: Review Request: magnatune: first update related tweaks
On Aug. 18, 2012, 10:25 a.m., Matěj Laitl wrote: Yes, I definitly support this goal. By coincidence, this is very similar to what outlined in my mail. Ship it like this. I'd be even more glad if this could be somehow handled on the ServiceBrowser or Service class level in future - e.g. showing the config dialog even without loading the service fully. (unless it has been already acked by the user previously) Services need a serious refactoring anyway. Let's make that a topic we'll talk about in Randa. Edward: don't forget to give your input on this. If you really can't make it to Randa, let's make sure you can join via video chat. - Bart --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106071/#review17651 --- On Aug. 18, 2012, 10:04 a.m., Edward Hades Toroshchin wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106071/ --- (Updated Aug. 18, 2012, 10:04 a.m.) Review request for Amarok. Description --- magnatune: first update related tweaks These commits pursue single basic goal: not to let Magnatune service plugin go ahead and download bunch of stuff from Magnatune at the first Amarok run. Basically, it's just a allow auto updates checkbox in the settings (disabled by default), and a messge in Magnatune UI telling the user, that she needs to let Amarok download Magnatune database manually, or let Amarok download it automagically ever after. magnatune: added auto update database option magnatune: honor auto update settings magnatune: ask user to update database This adds a widget to the Magnatune service pane, that tells the user, that Amarok needs to download Magnatune database, and that this can also be performed automatically. Diffs - src/services/magnatune/CMakeLists.txt 524b3e8cf3d45597cd91b5f0a181ebcc0878a229 src/services/magnatune/MagnatuneConfig.h f1d25ebd3643086df4e09902a6b022a13765e810 src/services/magnatune/MagnatuneConfig.cpp 18ee8985b3cedbbf3559ef50a5a94643655d5269 src/services/magnatune/MagnatuneConfigWidget.ui 782ef25e153cee152ab4fbc35e1cd2de393bbe63 src/services/magnatune/MagnatuneNeedUpdateWidget.h PRE-CREATION src/services/magnatune/MagnatuneNeedUpdateWidget.cpp PRE-CREATION src/services/magnatune/MagnatuneNeedUpdateWidget.ui PRE-CREATION src/services/magnatune/MagnatuneSettingsModule.cpp 3d7790b2c8d0719014695fa0ea04be5b929802dd src/services/magnatune/MagnatuneStore.h 3f80e4c6e1cb350a14a3a157e76642ff48a96fcf src/services/magnatune/MagnatuneStore.cpp b1c5252f0d32bde21af6f39abcae485f8899162d Diff: http://git.reviewboard.kde.org/r/106071/diff/ Testing --- The following correct behavior is observed: 1. First run: a) Magnatune does not download the database automatically; b) The you need to download the database widget appears. 2. After update button has been clicked: a) Magnatune database is being downloaded; b) The you need to download the database widget disappears. 3. Subsequent runs (after a successful update): a) Magnatune just works; b) If the update automatically checkbox is set, it checks for updates automatically. Screenshots --- the first-run widget http://git.reviewboard.kde.org/r/106071/s/680/ Thanks, Edward Hades Toroshchin ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: GSoC report: Integrate Spotify into Amarok #3 (squashed commits, recent on top)
On Aug. 17, 2012, 12:22 p.m., Ralf Engels wrote: I can only see the changed copyright header. Is this request still valid? Can we reject it? Ryan should probably close this one, rebase his gsoc branch and upload a new, full diff to a new review board. Want feedback from more people since it's GSoC evaluation time anyway. - Bart --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105201/#review17592 --- On June 11, 2012, 1:26 p.m., Zhengliang Feng wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105201/ --- (Updated June 11, 2012, 1:26 p.m.) Review request for Amarok. Description --- Add Spotify collection code Currently implemented SpotifyCollection, SpotifyQueryMaker and SpotifyMeta. The ScriptResolver is the class handles communcation with standalone Spotify resolver, the code is mainly from original ScriptResolver, but added more functions to handle messages separately. The controller class is used to start a ScriptResolver in a separate thread and handles queries. Diffs - src/core-impl/collections/spotifycollection/support/ScriptResolver.h PRE-CREATION src/core-impl/collections/spotifycollection/support/ScriptResolver.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/105201/diff/ Testing --- Communication between ScriptResolver and Spotify resolver( from Tomahawk resolver repo https://github.com/ofan/tomahawk-resolvers ). Logging into Spotify using a username and password. Thanks, Zhengliang Feng ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Bug 261062 - JJ: Mark episodes to keep
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/100998/#review17515 --- Looks good to me to. I'll test it later, even though not even I am using podcasting in Amarok except for development testing :/ - Bart Cerneels On Feb. 8, 2012, 11:15 p.m., Lucas Gomes wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/100998/ --- (Updated Feb. 8, 2012, 11:15 p.m.) Review request for Amarok. Description --- Now downloaded episodes are kept in HD, if keep episode checkbox is checked. These kept episodes are shown in the list regardless of the purgecount. BUG Description: Currently downloaded files are not kept when the episode is no longer shown in the Podcast section. This behavior can not be configured but is tied to the purge settings of each channel. It's possible to add a keep downloaded file checkbox to episodes to prevent the deletion from happening. We should perhaps also show these kept episodes in the list regardless of the purgecount. So even if a channel has purgecount 5 and 3 episodes marked to keep it will show 5 + maximum 3 older kept episodes. Reproducible: Always This addresses bug 261062. https://bugs.kde.org/show_bug.cgi?id=261062 Diffs - src/browsers/playlistbrowser/PodcastModel.cpp 18334f6 src/core-impl/podcasts/sql/SqlPodcastMeta.h 42ad039 src/core-impl/podcasts/sql/SqlPodcastMeta.cpp 1c3bdf4 src/core-impl/podcasts/sql/SqlPodcastProvider.h c3d5e56 src/core-impl/podcasts/sql/SqlPodcastProvider.cpp 183005f src/core/podcasts/PodcastMeta.h 679f7ac src/playlistmanager/SyncedPodcast.h c186df2 Diff: http://git.reviewboard.kde.org/r/100998/diff/ Testing --- Thanks, Lucas Gomes ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: GSoC report: Integrate Spotify into Amarok #4 (squashed commits, recent on top)
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105285/#review17355 --- The resolver failed to download for me. Got an QNetworkReply::UnknownNetworkError, but normally the binary would not have been saved correctly either. src/core-impl/collections/spotifycollection/SpotifyCollection.cpp http://git.reviewboard.kde.org/r/105285/#comment13535 Use the enum value here. I assume this has to be ResolverNotFound src/core-impl/collections/spotifycollection/SpotifyCollection.cpp http://git.reviewboard.kde.org/r/105285/#comment13536 QAction is a QObject so could be parented to SpotifyCollection to be automatically deleted. Saves you some manual memory management. src/core-impl/collections/spotifycollection/SpotifyConfig.h http://git.reviewboard.kde.org/r/105285/#comment13537 API key is locked away in the resolver binary like intended. Remove these functions to avoid confusion. src/core-impl/collections/spotifycollection/SpotifyConfig.cpp http://git.reviewboard.kde.org/r/105285/#comment13540 resolverPath does not have a default value and is never set except from kwallet/configfile. What is those are empty? It causes a failure to download in Settings. src/core-impl/collections/spotifycollection/SpotifyConfig.cpp http://git.reviewboard.kde.org/r/105285/#comment13539 Does it make sense to store this in the wallet? Doesn't look like sensitive info to me. src/core-impl/collections/spotifycollection/SpotifySettings.cpp http://git.reviewboard.kde.org/r/105285/#comment13538 You need to check QNetworkReply::error() == NoError before continuing. In case of download error the write will still be attempted. - Bart Cerneels On Aug. 13, 2012, 11:24 a.m., Zhengliang Feng wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105285/ --- (Updated Aug. 13, 2012, 11:24 a.m.) Review request for Amarok. Description --- Things I've done this week: * Added a new playlist provider SpotifyPlaylistProvider * Added playlist class SpotifyPlaylist * Clear all extra whitespaces * Figured out how Collection, QueryMaker and Playlist worked Things need to be done next week: * Clean ScriptResolver's interfaces, move all query related interfaces into Query class * Replace Controller class with ScriptResolver * Test SpotifyCollection * Finish playlist and playlist provider Diffs - src/core-impl/collections/CMakeLists.txt c78b920 src/core-impl/collections/spotifycollection/CMakeLists.txt PRE-CREATION src/core-impl/collections/spotifycollection/SpotifyCollection.h PRE-CREATION src/core-impl/collections/spotifycollection/SpotifyCollection.cpp PRE-CREATION src/core-impl/collections/spotifycollection/SpotifyConfig.h PRE-CREATION src/core-impl/collections/spotifycollection/SpotifyConfig.cpp PRE-CREATION src/core-impl/collections/spotifycollection/SpotifyConfigWidget.ui PRE-CREATION src/core-impl/collections/spotifycollection/SpotifyMeta.h PRE-CREATION src/core-impl/collections/spotifycollection/SpotifyMeta.cpp PRE-CREATION src/core-impl/collections/spotifycollection/SpotifyPlaylist.h PRE-CREATION src/core-impl/collections/spotifycollection/SpotifyPlaylist.cpp PRE-CREATION src/core-impl/collections/spotifycollection/SpotifyPlaylistProvider.h PRE-CREATION src/core-impl/collections/spotifycollection/SpotifyPlaylistProvider.cpp PRE-CREATION src/core-impl/collections/spotifycollection/SpotifyQueryMaker.h PRE-CREATION src/core-impl/collections/spotifycollection/SpotifyQueryMaker.cpp PRE-CREATION src/core-impl/collections/spotifycollection/SpotifySettings.h PRE-CREATION src/core-impl/collections/spotifycollection/SpotifySettings.cpp PRE-CREATION src/core-impl/collections/spotifycollection/amarok_collection-spotifycollection.desktop PRE-CREATION src/core-impl/collections/spotifycollection/support/Controller.h PRE-CREATION src/core-impl/collections/spotifycollection/support/Controller.cpp PRE-CREATION src/core-impl/collections/spotifycollection/support/QMFunctionTypes.h PRE-CREATION src/core-impl/collections/spotifycollection/support/Query.h PRE-CREATION src/core-impl/collections/spotifycollection/support/Query.cpp PRE-CREATION src/core-impl/collections/spotifycollection/support/TrackProxy.h PRE-CREATION src/core-impl/collections/spotifycollection/support/TrackProxy.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/105285/diff/ Testing --- No test done this week. Thanks, Zhengliang Feng ___ Amarok-devel mailing list Amarok
Re: Review Request: GSoC report: Integrate Spotify into Amarok #4 (squashed commits, recent on top)
On Aug. 13, 2012, 4:43 p.m., Edward Hades Toroshchin wrote: src/core-impl/collections/spotifycollection/SpotifyCollection.cpp, line 57 http://git.reviewboard.kde.org/r/105285/diff/3/?file=77309#file77309line57 I'd rather you checked here, if m_controller is 0. Yup, can be a cause of crash. There is no guarantee that a controller object will exist yet. - Bart --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105285/#review17322 --- On Aug. 13, 2012, 11:24 a.m., Zhengliang Feng wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105285/ --- (Updated Aug. 13, 2012, 11:24 a.m.) Review request for Amarok. Description --- Things I've done this week: * Added a new playlist provider SpotifyPlaylistProvider * Added playlist class SpotifyPlaylist * Clear all extra whitespaces * Figured out how Collection, QueryMaker and Playlist worked Things need to be done next week: * Clean ScriptResolver's interfaces, move all query related interfaces into Query class * Replace Controller class with ScriptResolver * Test SpotifyCollection * Finish playlist and playlist provider Diffs - src/core-impl/collections/CMakeLists.txt c78b920 src/core-impl/collections/spotifycollection/CMakeLists.txt PRE-CREATION src/core-impl/collections/spotifycollection/SpotifyCollection.h PRE-CREATION src/core-impl/collections/spotifycollection/SpotifyCollection.cpp PRE-CREATION src/core-impl/collections/spotifycollection/SpotifyConfig.h PRE-CREATION src/core-impl/collections/spotifycollection/SpotifyConfig.cpp PRE-CREATION src/core-impl/collections/spotifycollection/SpotifyConfigWidget.ui PRE-CREATION src/core-impl/collections/spotifycollection/SpotifyMeta.h PRE-CREATION src/core-impl/collections/spotifycollection/SpotifyMeta.cpp PRE-CREATION src/core-impl/collections/spotifycollection/SpotifyPlaylist.h PRE-CREATION src/core-impl/collections/spotifycollection/SpotifyPlaylist.cpp PRE-CREATION src/core-impl/collections/spotifycollection/SpotifyPlaylistProvider.h PRE-CREATION src/core-impl/collections/spotifycollection/SpotifyPlaylistProvider.cpp PRE-CREATION src/core-impl/collections/spotifycollection/SpotifyQueryMaker.h PRE-CREATION src/core-impl/collections/spotifycollection/SpotifyQueryMaker.cpp PRE-CREATION src/core-impl/collections/spotifycollection/SpotifySettings.h PRE-CREATION src/core-impl/collections/spotifycollection/SpotifySettings.cpp PRE-CREATION src/core-impl/collections/spotifycollection/amarok_collection-spotifycollection.desktop PRE-CREATION src/core-impl/collections/spotifycollection/support/Controller.h PRE-CREATION src/core-impl/collections/spotifycollection/support/Controller.cpp PRE-CREATION src/core-impl/collections/spotifycollection/support/QMFunctionTypes.h PRE-CREATION src/core-impl/collections/spotifycollection/support/Query.h PRE-CREATION src/core-impl/collections/spotifycollection/support/Query.cpp PRE-CREATION src/core-impl/collections/spotifycollection/support/TrackProxy.h PRE-CREATION src/core-impl/collections/spotifycollection/support/TrackProxy.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/105285/diff/ Testing --- No test done this week. Thanks, Zhengliang Feng ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: GSoC report: Integrate Spotify into Amarok #4 (squashed commits, recent on top)
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105285/#review17359 --- src/core-impl/collections/spotifycollection/SpotifySettings.cpp http://git.reviewboard.kde.org/r/105285/#comment13541 You probably should do readAll() here. The data will be fully available after finished(). Unless there is some platform specific reason to do it like this? - Bart Cerneels On Aug. 13, 2012, 11:24 a.m., Zhengliang Feng wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105285/ --- (Updated Aug. 13, 2012, 11:24 a.m.) Review request for Amarok. Description --- Things I've done this week: * Added a new playlist provider SpotifyPlaylistProvider * Added playlist class SpotifyPlaylist * Clear all extra whitespaces * Figured out how Collection, QueryMaker and Playlist worked Things need to be done next week: * Clean ScriptResolver's interfaces, move all query related interfaces into Query class * Replace Controller class with ScriptResolver * Test SpotifyCollection * Finish playlist and playlist provider Diffs - src/core-impl/collections/CMakeLists.txt c78b920 src/core-impl/collections/spotifycollection/CMakeLists.txt PRE-CREATION src/core-impl/collections/spotifycollection/SpotifyCollection.h PRE-CREATION src/core-impl/collections/spotifycollection/SpotifyCollection.cpp PRE-CREATION src/core-impl/collections/spotifycollection/SpotifyConfig.h PRE-CREATION src/core-impl/collections/spotifycollection/SpotifyConfig.cpp PRE-CREATION src/core-impl/collections/spotifycollection/SpotifyConfigWidget.ui PRE-CREATION src/core-impl/collections/spotifycollection/SpotifyMeta.h PRE-CREATION src/core-impl/collections/spotifycollection/SpotifyMeta.cpp PRE-CREATION src/core-impl/collections/spotifycollection/SpotifyPlaylist.h PRE-CREATION src/core-impl/collections/spotifycollection/SpotifyPlaylist.cpp PRE-CREATION src/core-impl/collections/spotifycollection/SpotifyPlaylistProvider.h PRE-CREATION src/core-impl/collections/spotifycollection/SpotifyPlaylistProvider.cpp PRE-CREATION src/core-impl/collections/spotifycollection/SpotifyQueryMaker.h PRE-CREATION src/core-impl/collections/spotifycollection/SpotifyQueryMaker.cpp PRE-CREATION src/core-impl/collections/spotifycollection/SpotifySettings.h PRE-CREATION src/core-impl/collections/spotifycollection/SpotifySettings.cpp PRE-CREATION src/core-impl/collections/spotifycollection/amarok_collection-spotifycollection.desktop PRE-CREATION src/core-impl/collections/spotifycollection/support/Controller.h PRE-CREATION src/core-impl/collections/spotifycollection/support/Controller.cpp PRE-CREATION src/core-impl/collections/spotifycollection/support/QMFunctionTypes.h PRE-CREATION src/core-impl/collections/spotifycollection/support/Query.h PRE-CREATION src/core-impl/collections/spotifycollection/support/Query.cpp PRE-CREATION src/core-impl/collections/spotifycollection/support/TrackProxy.h PRE-CREATION src/core-impl/collections/spotifycollection/support/TrackProxy.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/105285/diff/ Testing --- No test done this week. Thanks, Zhengliang Feng ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Fix crash when closing Amarok with running CoverFetcher
On Aug. 9, 2012, 10:45 a.m., Matěj Laitl wrote: src/statusbar/CompoundProgressBar.cpp, line 46 http://git.reviewboard.kde.org/r/105942/diff/1/?file=76720#file76720line46 Nitpicky: I would personally strip all the added newlines Ralf Engels wrote: Actually I think that newlines are a good device to structure code. I use it to split different functional blocks inside a function. Often I use them to split up between the initialization, the actual function and the cleaning up inside a function. That's why there is a newline between the Locker and the rest of the code. Single line comments for those blocks (ex. //Initialization) are even better then. newlines are just newlines for anyone not aware of your intended structuring. Some structure in cpp files is actually a good thing. I usually keep it limited to having the methods in the same order as in the header file. - Bart --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105942/#review17141 --- On Aug. 9, 2012, 10:36 a.m., Ralf Engels wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105942/ --- (Updated Aug. 9, 2012, 10:36 a.m.) Review request for Amarok. Description --- childBarFinished was called with invalid m_progressDetailsWidget However also the m_progressMap needs protection. The CompoundProgressBar needs to be thread safe. This addresses bug 292553. https://bugs.kde.org/show_bug.cgi?id=292553 Diffs - src/statusbar/CompoundProgressBar.h baaab94 src/statusbar/CompoundProgressBar.cpp 371a534 Diff: http://git.reviewboard.kde.org/r/105942/diff/ Testing --- Change is verified by me to fix the problem. Thanks, Ralf Engels ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Add playlist export action to Playlist Dock save action.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105824/#review17100 --- I wouldn't do this. Toolbars become less usable the more actions are in it. What is wrong with using the menu with a function that is not supposed to be used to often? - Bart Cerneels On Aug. 2, 2012, 12:14 p.m., Ralf Engels wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105824/ --- (Updated Aug. 2, 2012, 12:14 p.m.) Review request for Amarok. Description --- Add playlist export action to Playlist Dock save action. Diffs - src/playlist/PlaylistDock.cpp ae1644f Diff: http://git.reviewboard.kde.org/r/105824/diff/ Testing --- Thanks, Ralf Engels ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Add playlist export action to Playlist Dock save action.
On Aug. 8, 2012, 11:36 a.m., Bart Cerneels wrote: I wouldn't do this. Toolbars become less usable the more actions are in it. What is wrong with using the menu with a function that is not supposed to be used to often? Matěj Laitl wrote: Did you say this with knowing that this doesn't add another toolbar enty, just one more item to the menu of the Save toolbar button? Ralf, this is really worth a screenshot. You are right, I didn't even though I had a quick glance at the code. I would have suggested to do it like that but I think it might be confusing. In any case removing it from the menu could upset existing users. - Bart --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105824/#review17100 --- On Aug. 2, 2012, 12:14 p.m., Ralf Engels wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105824/ --- (Updated Aug. 2, 2012, 12:14 p.m.) Review request for Amarok. Description --- Add playlist export action to Playlist Dock save action. Diffs - src/playlist/PlaylistDock.cpp ae1644f Diff: http://git.reviewboard.kde.org/r/105824/diff/ Testing --- Thanks, Ralf Engels ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: GSoC Report: Integrate Spotify into Amarok
On Tue, Jul 31, 2012 at 11:28 PM, Ryan Feng odayf...@gmail.com wrote: Hi all, Last week I've been working on the configuration plug-in for Spotify collection, and it's still working in progress. I have also spent some time looking back at the SpotifyCollection and SpotifyQueryMaker implementation, and tried to improve the stability of querying. Current implementation is based on MemoryCollection and MemoryQueryMaker, it caches a lot stuff including `title`, `artist`, `genre`, `year` and even `labels`, since only the first three tags are often used, so next week I will continue working on the configuration part and rewriting SpotifyCollection SpotifyQueryMaker. Currently I have a problem about the configuration plug-in for Spotify collection, I've done the coding work, the KCModule is registered correctly( checked by ktraderclient ), but the configuration button won't show up in the plugin configuration dialog, so it will be nice if someone could grab the source[1] and take a look what's maybe wrong. [1] http://quickgit.kde.org/index.php?p=clones%2Famarok%2Fzhengliangfeng%2Famarok-spotify.gita=shortlogh=refs/heads/gsoc-spotify Cheers, Ryan ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel Looks like the Collections currently don't have config dialogs. An alternative is to have an action accessible from the CollectionBrowser root item of spotify. As an example look at UmsCollection. This means that the SpotfiyCollection needs to be shown (i.e. registered with CollectionManager) but not actually doing anything before authenticated. There are various benefits and downsides to having it shown always: + discoverable for the users without having to dive into the settings. - annoying for those not using spotify or in an unsupported country. But the plugin can be disabled entirely and there should be little overhead while it's not configured. ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Moving Tab-toolbars to the top
I'm not in favor of changing the UI without a complete overhaul. I also would prefer if we avoided discussing design. On Wed, Jul 25, 2012 at 10:53 PM, Ralf Engels ralf-eng...@gmx.de wrote: Hi all, I noticed that our toolbars are all over the place. The context and the playlist view have toolbars at the bottom. The media tab has the toolbar at the top. Now I think that this is confusing and especially bad in case of the playlist where we have some options at the top and then a toolbar without icon texts at the bottom. Very hard to overlook (as always when you have two or more toolbars at different places). So I would propose to move all toolbars to the top where they are easy to notice. What do you think about this? Cheers, Ralf ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: 2.6 release and release candidate considerations
On Wed, Jul 18, 2012 at 10:28 PM, Matěj Laitl ma...@laitl.cz wrote: Hi list, Ralf Bart, after I've returned from a couple of days of off-line time, I've found out that Bart wants release a RC before releasing 2.6 - something I strongly support given the severity of some post-beta changes. However, IMO this is a great opportunity for us to release a RC with even more somehow invasive changes that fix release blockers and are currently sitting on the review board, namely: * https://git.reviewboard.kde.org/r/105488/ (fixes bug 298275, waits for re- ack by Ralf) * https://git.reviewboard.kde.org/r/105610/ (fixes bug 299890, waits for ack by Bart as he seems to understand EngineController) Have not gotten around to it but will attempt before 21:00 this evening. No one understands EngineController though ;). Bart, have you already made the 2.6 RC package? If not, would you bother to retag the RC when above fixes are pushed to master? I can merge them as soon as they're acked. I've made the package but had build problems because of KDE translations (again!). There is a v2.5.95 tag in the repo on KDE already. But we can always add a new tag. Optionally, I think we can be even more daring and merge test-fixing one to 2.6 RC, too: * https://git.reviewboard.kde.org/r/105524/ (fixes testm3uplaylist, waits for Ralf's or Bart's reply) Tests should not block a 2.6 release. I think our tests will be problematic at least until we do a proper architecture review. ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: EngineController: ditch canDecode(), fix supportedMimeTypes(): make it non-static, thread-safe even on first call. (squached patches, recent on top)
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105524/#review16125 --- Ship it! Go ahead. The RC period will expose any serious problems. - Bart Cerneels On July 19, 2012, 1:38 p.m., Matěj Laitl wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105524/ --- (Updated July 19, 2012, 1:38 p.m.) Review request for Amarok, Bart Cerneels, Ralf Engels, and Sven Krohlas. Description --- EngineController: always call Phonon::availableMimeTypes() from main thread If supportedMimeTypes() is called for the first time in a thread, Phonon::BackendCapabilities::availableMimeTypes() creates some QObject subclasses in non-main thread and that causes problems when they are deleted. Add signal/slot/QSemaphore trickery that causes Phonon::BackendCapabilities::availableMimeTypes() is called from the main thread without performance penalty for 2nd and next calls. Test is added for it (EngineController::supportedMimeTypes()) so that this fragile code (hopefully) never breaks again. EngineController: make supportedMimeTypes() non-static Static methods have no sense in a singleton class. Additionally, it was very hacky (and impossible in corner-cases) to keep it thread-safe. Making it non-static will allow us to do some tricks so that the calls are more robust. EngineController: null-pointer checking in destructor EngineController: remove unused and confusing destroy() EngineController: ditch canDecode() MetaFile::Track::isTrack() is partial replacement. Existing 3 calls to canDecode() were in fact all related to MetaFile classes, so move the method there, simplify it not to query phonon at all (and document it can return false positives). As a consequence, we show all audio and video files in file browser and in other places, even if they wouldn't be playable by the current phonon back-end. This is arguably a cleaner approach and at least lets users discover where the error is. Works quite well for me and prevents failures in many tests. This change is propelled by Bart's and Ralf's legitimate comments on http://git.reviewboard.kde.org/r/105524/ BUG: 303253 FIXED-IN: 2.6 Revert Prevent hang in testmetamultitrack. This reverts commit 115cb80f9bd94b23640ca9245c97d6c8e25d2c97. Proper fix will follow, see next commits. This addresses bug 303253. https://bugs.kde.org/show_bug.cgi?id=303253 Diffs - ChangeLog 1d4f1e92fb5a033500c0f18d3dca257a89f1a139 src/EngineController.h ad2e0c4a5e7c80c79bf448bf74cd6b52cd1f0ed3 src/EngineController.cpp 83f0a6ed0a92ae992e1809800cee65d9349dc680 src/browsers/filebrowser/FileBrowser.cpp 567ff799df7ef8bfcd93de73ee120bfc5be634b7 src/browsers/filebrowser/FileView.cpp 8eae4b6731ebfcb7310d3d719687989be125de92 src/core-impl/collections/support/CollectionManager.cpp 9085b5a27d9a7ce94d2325d94bec5fce8d126abe src/core-impl/meta/file/File.h caa7102fd901ccdac7c9f25ee0caeb554a4ee518 src/core-impl/meta/file/File.cpp 231c98521c3d6b1c609abc5799af47b88f048aee tests/CMakeLists.txt 901c716a600bca63639939f4e62dbd89d9db707f tests/TestEngineController.h PRE-CREATION tests/TestEngineController.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/105524/diff/ Testing --- Works, fixes testm3uplaylist, should fix bug 303253. (waiting for original reporter, I cannot reproduce) Thanks, Matěj Laitl ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: EngineController: fixes to canDecode() and supportedMimeTypes(): make them non-static, thread-safe even on first call. (squached patches, recent on top)
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105524/#review15772 --- src/browsers/filebrowser/FileView.cpp http://git.reviewboard.kde.org/r/105524/#comment12335 We should remove this canDecode() as well for the same reason I mention in the comment. - Bart Cerneels On July 12, 2012, 2:15 p.m., Matěj Laitl wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105524/ --- (Updated July 12, 2012, 2:15 p.m.) Review request for Amarok, Ralf Engels and Sven Krohlas. Description --- EngineController: use QFileInfo instead of apparently non-reentrant KFileItem ...remote files are not needed here, so QFileInfo does a good job. This is the cause of last crashes in testm3uplaylist. (I have about 25% probability of it crashing until this commit) EngineController: make supportedMimeTypes() thread-safe even on first call If supportedMimeTypes() is called for the first time in a thread, Phonon::BackendCapabilities::availableMimeTypes() creates some QObject subclasses in non-main thread and that causes problems later on, and is the cause of testm3uplaylist failures. Add signal/slot/QSemaphore trickery that causes Phonon::BackendCapabilities::availableMimeTypes() is called from the main thread without performance penalty for 2nd and next calls. Test is added for EngineController::supportedMimeTypes() so that this fragile code (hopefully) never breaks again. This fixes (perhaps along with the next commit) 2 failing tests ( testm3uplaylist and apparently testplaylistmodels), so the fail count is down to 4 here. EngineController: make canDecode() and supportedMimeTypes() non-static Static methods have no sense in a singleton class. Additionally, it was very hacky (and impossible in corner-cases) to keep these thread-safe, see testm3uplaylist failures. Making them non-static will allow us to do some tricks so that the calls are more robust. Never-used and confusing destory() method is removed, too. Care is taken not to break existing tests. v2: fix even more tests that have been added or fixed while this patch slept. Revert Prevent hang in testmetamultitrack. This reverts commit 115cb80f9bd94b23640ca9245c97d6c8e25d2c97. Proper fix will follow, see next commits. Diffs - src/EngineController.h ad2e0c4a5e7c80c79bf448bf74cd6b52cd1f0ed3 src/EngineController.cpp 83f0a6ed0a92ae992e1809800cee65d9349dc680 src/browsers/filebrowser/FileBrowser.cpp 567ff799df7ef8bfcd93de73ee120bfc5be634b7 src/browsers/filebrowser/FileView.cpp 8eae4b6731ebfcb7310d3d719687989be125de92 src/core-impl/collections/support/CollectionManager.cpp 9085b5a27d9a7ce94d2325d94bec5fce8d126abe tests/CMakeLists.txt 901c716a600bca63639939f4e62dbd89d9db707f tests/TestEngineController.h PRE-CREATION tests/TestEngineController.cpp PRE-CREATION tests/core-impl/playlists/types/file/m3u/TestM3UPlaylist.cpp 4bbf29be01aef9043a64075a196ac04a544134cb tests/core/collections/support/TestTrackForUrlWorker.h PRE-CREATION tests/core/collections/support/TestTrackForUrlWorker.cpp PRE-CREATION tests/core/meta/TestMetaTrack.cpp 0adb7633538533202c50487b1eaff8e4ead150e9 tests/dynamic/TestDynamicModel.h ce9934cf464172b75b1858f884a64c00cdcf4e24 tests/dynamic/TestDynamicModel.cpp 7387fbcbf28ad838fbf56076553da56d630451af tests/playlistmanager/file/TestPlaylistFileProvider.cpp 931eefba03e55f4ae2beec089369087958311013 tests/playlistmanager/sql/TestSqlUserPlaylistProvider.cpp bb09515d617e8149e7e66e804ff46aa5ddb76045 Diff: http://git.reviewboard.kde.org/r/105524/diff/ Testing --- Works, fixes testm3uplaylist Thanks, Matěj Laitl ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: [Tomahawk Integration] GSoC Report (plus: RFC on git work-flow guideline)
This command is mandatory for all amarok developers: git config branch.master.rebase true the result of this is that all git pull commands will do a rebase rather then a merge. Your local commits will be rewritten (parented to origin/master). The prevents the git log to become unreadable because of all the merge commits. Merge commits will only show on real branch merges. But since we don't use much branches in the repository on KDE it's should be an uncommon thing. I'll add a file to HACKING explaining this. On Tue, Jul 10, 2012 at 1:25 PM, Matěj Laitl ma...@laitl.cz wrote: On 9. 7. 2012 Lucas Lira Gomes wrote: Hi everyone, Hi Lucas, My amarok repo: http://quickgit.kde.org/index.php?p=clones%2Famarok%2Fgomes%2Fmaskmaster-ama rok.gita=summary (tomahawk branch) I've checked above repo and the master branch there is polluted with many Merge branch 'master' of git://anongit.kde.org/amarok commits. Shouldn't all these be fast-forwards? My only fear is that this will make merging your tomahawk branch into master tricky. I would advise having the master a carbon- copy of Amarok upstream master. The history of tomahawk branch itself looks fine. To all developers, contributors, GSoCers, SoKers, I think we should adopt following git work-flow rules: * upstream master branch is never _merged_ _to_ feature branches/personal clones etc. If you want to have new commits, _rebase_ your feature branch/clone on top of upstream master. (e.g. `git pull --ff-only`, `git pull --rebase` etc.) ^^^ why? Because if such feature branch is merged into master, it can result in strange things including silently reintroducing reverted commits in the worst case and pollutes the history in the best case. * merging feature branches to upstream master is fine and should be preferred over squashing the commits or making it a fast-forward. Ideally, a list of merged commits is embedded in the merge-commit, so the command should look like: `git merge --no-ff --log feature-branch`. See [1] for example. ^^^ why? Because tools like git bisect and git blame depend on fine-grained commit history. git blame (or its KDevelop integration) is very useful for why does this code line exist? type of questions, you should use it, really. Creating merge-commits makes the history more organized and allows for reverting or diff-ing whole merges. [1] http://quickgit.kde.org/index.php?p=amarok.gita=commith=8d8404a9a95d3a49026eac972783f66061675e53 Matěj ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: GSoC update
On Fri, Jul 13, 2012 at 11:36 AM, Phalgun Guduthur phalgun.gudut...@gmail.com wrote: On Fri, Jul 13, 2012 at 1:02 PM, Bart Cerneels bart.cerne...@gmail.com wrote: On Wed, Jul 4, 2012 at 4:37 PM, Phalgun Guduthur phalgun.gudut...@gmail.com wrote: Hey All! An update on my GSoC project : Semantic Collection for Amarok I have figured out a way to reuse a lot of existing Amarok code. So it made a part of the code I wrote till now redundant. I will be using MemoryCollection as other collections do. The Meta QMaps will be constructed using queried results from Nepomuk. This will make all the collections consistent and hopefully avoids a lot of bugs too. Similarly, I will also be reusing the MemoryQueryMaker and the other support classes that go with it. Right now, the Nepomuk Queries are taking too long. So I will have to run them in the background using Threadweaver jobs. Apart from this, the NepomukCollection looks complete with the Meta classes implemented. This worries me a little bit. MemoryCollection obviously uses a lot of memory. Where possible we should avoid having to place the complete set of tracks in memory and rather rely on the design goals of QueryMaker to create them on demand. I would think Nepomuk is the perfectly suited for that. Can you explain what blocked you and decided to switch to MemoryCollection? Bart Oh I haven't checked my memory usage when I ran Amarok Collection with Nepomuk. If all of the MetaMaps are being stored in memory, it should indeed eat up a lot, if we consider collections of size more than 2000 tracks. But then, wouldn't all the existing collections that use MemoryCollection also suffer from the same problem? Say for example the iPodCollection. They do, but it's not good either. The benefit there is that they usually don't contain as many tracks as the main collection. Any implementation that is capable of it should at least attempt to avoid keeping everything in memory. In your case I think caching of search results and auto pre-fetching are good strategies. There is an overuse of MemoryQueryMaker we should try to avoid with smarter implementations where possible. For inspiration, a mix of MemoryCollection and on-demand search based has been implemented by Nikhil Marathe for the 2010 GSoC project UPnPCollection. I did speak to Edward (dr_lepper) on the possibility of caching resource URI's (These are the unique identifiers of each resource in Nepomuk, just strings) of each track so that they can be looked up quickly. But if this caching is indeed required, I'll have to postpone it for now and work on them after completing other milestones. And as to why I wanted to reuse the MemoryCollection, I was going to implement my NepomukQueryMaker on the same lines as MemoryQueryMaker, including the *QueryMakerInternal and other intricacies. Then I realized I could make use of MemoryCollection as other collections and maintain the same consistency among the collections. Now I do start to wonder if constructing tracks on demand, say when a user clicks on the artist or a genre was indeed an option. Both approaches have their pros and cons, 'cos the 'construct at beginning' approach causes a lag only during the bootup of Amarok whereas the 'on demand' approach would cause a lag every time a query is requested. ( time lag for the query to run and enumerate ) Best, Phalgun ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
spotify collection post mid-term TODO
* SpotifyCollection Object only created if Controller status is OK and general initialization workflow * Download resolver binary from kollide from settings dialog * Pass API key on build, shepherd changes into resolver repo. * Controller GPLv3 issue - get an exception from Leo and Muesli or we'll have to relicense the Amarok codebase (not likely because of GPLv2 only files). extra's (likely post GSoC) * PlaylistProvider for user's own spotify playlists (which can be shown in the Saved Playlists a.k.a. UserPlaylists category). ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: [Tomahawk Integration] GSoC Report
On Mon, Jul 9, 2012 at 11:15 PM, Lucas Lira Gomes x8luca...@gmail.com wrote: Hi everyone, i've been fixing bugs and improving overall performance of the tomahawk service. It's a bit lazy to load big collections(50k tracks) and I haven't figured yet how I can make its loading faster. And yes, I'm running track loading and removal in the background using Threadweaver jobs. By the way, I modified the database integrator to use SqlCollection QueryMaker, instead of a full scan, to sync tomahawk db for the first time. Although Amarok is successfully sending, to other peers, the info about what the user is listening, I still need to create a menu to change privacy modes. I guess the best way to show what others peers we're listening is in an applet. But since there are more urgent matters for now, then I'll postpone its creation. Another problem is that tracks in playlists aren't playable anymore after I restart Amarok. I've been looking for possible solutions with no success, The thing is that tomahawk tracks needs their source and it's only available when the user is connected. Does anybody know how to force playlists to solve their tracks again when a new peer becomes online? This part is for after midterm, so no hurry ^^. Hint: Check if MetaProxy::Worker::slotNewTrackProvider() is getting called. It should be if you register your TrackProvider (can happen through CollectionManager::slotNewCollection()). Last but not least, I'm writing a new blog post to show the current state of the Tomahawk service. My tomahawk repo: https://github.com/x8lucas8x/tomahawk (amarokready branch) My amarok repo: http://quickgit.kde.org/index.php?p=clones%2Famarok%2Fgomes%2Fmaskmaster-amarok.gita=summary (tomahawk branch) Regards, Lucas Lira Gomes. -- Lucas Lira Gomes (llg) Linux User #533002 Tel.: (81) 9235-0916 www.about.me/lucasliragomes On 26 June 2012 23:09, Lucas Lira Gomes x8luca...@gmail.com wrote: Hi everyone, Last two weeks weren’t the most productive of all, since some tests and a project consumed a lot of my time. In spite of that, I finally managed to play tomahawk streams in Amarok. To query others collections is working too. For those curious, tomahawk service is using capabilities, since it's the less intrusive way to play tomahawk streams in the EngineController. Right now I'm coding some logic(Creating a wrapper to AclRegistry class) to allow amarok to display dialogs showing other users requests to be able to listen to your streams. Next steps include the following: - Display tomahawk service connectivity information in the amarok diagnostics dialog. - Make other peers aware of what you're listening to. - Add a menu with options to disconnect tomahawk sips, listen music in private and etc. - Use SqlCollection QueryMaker, instead of a full scan, to sync tomahawk db for the first time. (More elegant approach ^^) The four steps above are relatively easy to implement, so no need to worry about the time line ;}. Obs.: The source in my repo will be updated as soon as I finish the AclRegistry wrapper. Regards, Lucas Lira Gomes(MaskMaster). -- Lucas Lira Gomes (llg) Linux User #533002 Tel.: (81) 9235-0916 www.about.me/lucasliragomes ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: NEW: Components list on our wiki
I added me as maintainer of CUE, CD and Podcasts. I'll be working on fixing major architectural problems for the first 2 and podcasts for historical reasons. On Sun, Jun 24, 2012 at 7:05 PM, Myriam Schweingruber myr...@kde.org wrote: Hi everyone, I made a new wiki page that lists all our components with the corresponding bugs list, and the current maintainers: http://amarok.kde.org/wiki/Development/Components Just by asking nicely Sam Lade aka Sentynel agree to take on maintainership of the last.fm and Moodbar services, yay! Currently we have 60 different components with 3 not in the stable release. Sadly, we also have currently a lot of components (33 on last count) with no official maintainer, so please, all, check where you could help and what suits you. it's a first come - first serve principle, so be fast to get your preferred component ;-) Regards, Myriam -- Proud member of the Amarok and KDE Community Protect your freedom and join the Fellowship of FSFE: http://www.fsfe.org Please don't send me proprietary file formats, use ISO standard ODF instead (ISO/IEC 26300) ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Unit test : ActionsCapability
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105144/#review14420 --- I'll need to test this before I'll hit ship it. - Bart Cerneels On June 4, 2012, 10:53 a.m., Jasneet Bhatti wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105144/ --- (Updated June 4, 2012, 10:53 a.m.) Review request for Amarok. Description --- This is a patch implementing unit testing of core/capabilities/ActionsCapability Diffs - src/core/capabilities/ActionsCapability.h 08de31d tests/core/CMakeLists.txt c66d3be tests/core/capabilities/CMakeLists.txt e69de29 tests/core/capabilities/TestActionsCapability.h e69de29 tests/core/capabilities/TestActionsCapability.cpp e69de29 Diff: http://git.reviewboard.kde.org/r/105144/diff/ Testing --- Thanks, Jasneet Bhatti ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Unit test : ActionsCapability
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105144/#review14402 --- I have a few nitpicks that are more about our API, ActionsCapability in particular. As a part of the unit test patches you should improve the documentation as well. Will help to understand (and get us to properly define) the use cases of the interface. tests/core/capabilities/TestActionsCapability.cpp http://git.reviewboard.kde.org/r/105144/#comment11366 Does ActionsCapability actually claim that it has to return the same objects in the same order as creation? Perhaps you should test the order and QProprties of the returned actions instead. tests/core/capabilities/TestActionsCapability.cpp http://git.reviewboard.kde.org/r/105144/#comment11365 To verify this you could have created the capability with an empty list. Does not influence the test in any way. - Bart Cerneels On June 3, 2012, 3:45 p.m., Jasneet Bhatti wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105144/ --- (Updated June 3, 2012, 3:45 p.m.) Review request for Amarok. Description --- This is a patch implementing unit testing of core/capabilities/ActionsCapability Diffs - tests/core/capabilities/TestActionsCapability.cpp PRE-CREATION tests/core/CMakeLists.txt c66d3be tests/core/capabilities/CMakeLists.txt PRE-CREATION tests/core/capabilities/TestActionsCapability.h PRE-CREATION Diff: http://git.reviewboard.kde.org/r/105144/diff/ Testing --- Thanks, Jasneet Bhatti ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Amarok 2.5.90 does not build against KDE Platform Libraries 4.7.4
On Sun, Jun 3, 2012 at 12:05 AM, Modestas Vainius mo...@debian.org wrote: Hello, On sekmadienis 03 Birželis 2012 00:21:31 Modestas Vainius wrote: cd ../doc/nl /usr/bin/meinproc4 --check --cache /«PKGBUILDDIR»/obj-x86_64- linux-gnu/doc/nl/index.cache.bz2 /«PKGBUILDDIR»/doc/nl/index.docbook Unable to load library icui18n Cannot load library icui18n: (libicui18n.so.48: cannot open sh Unable to load library icui18n Cannot load library icui18n: (libicui18n.so.48: cannot open sh Generating BreadcrumbItemButton.moc Generating DynamicBiasWidgets.moc index.docbook:21: parser error : Entity 'ged.vertaald' not defined ged.vertaald;Rinse.Devries;Antoon.Tolboom;Freek.de.Kruijf;Thom.Casterm ans; ^ make[3]: *** [doc/nl/index.cache.bz2] Error 1 make[3]: Leaving directory `/«PKGBUILDDIR»/obj-x86_64-linux-gnu' make[2]: *** [doc/nl/CMakeFiles/nl-handbook.dir/all] Error 2 make[2]: *** Waiting for unfinished jobs FWIW, CMakeLists.txt still requires KDElibs 4.6... This brings back bad memories of amarok 2.5 failure to build with KDElibs 4.6 (only in nl language back then). Sorry for a bit wrong information, I got confused by the output of parallel building. The culprit is in doc/nl *again*. The attached patch fixes the problem. -- We've noticed it as well and already got a bug report [1]. As far as we can tell this only affects debian stable and can be fixed with a patch that is already out there. As it's only an issue with translation we are hesitant to bump our kdelibs dependency. If you ask me it's a bug in the translation system. Bart [1] https://bugs.kde.org/show_bug.cgi?id=300986 ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: GSoC update: Integrate Spotify into Amarok
On Sun, Jun 3, 2012 at 6:32 AM, Ryan Feng odayf...@gmail.com wrote: Hi, Last week I posted a scratch code of the Amarok side resolver, I'm still working on it, like Leo and Bart said, there's no need to change the standalone Spotify resolver, so I won't touch it for a time, and I added SpotifyCollection, SpotifyQuery(or should be replaced by QueryMaker later) and Spotify::Resolver as the main class to start a new thread and handle user login, logout, track searching and stream etc. Still need to dive into the code of Collections and Playlists and figure out what's the best way to sync playlists between Amarok and Spotify. Next week, I will still be working on the basic Spotify integration. The code is still in the scratch repo, it will be put together with Amarok and tested soon, and a working Spotify service with basic features should be seen by the end of the next week. Thanks, Ryan Feng The code is here: http://quickgit.kde.org/index.php?p=clones%2Famarok%2Fzhengliangfeng%2Famarok-spotify.gita=commitdiffh=fb246b607f99a85fdff476ae189c53ad8a5fc4aa ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: GSoC update: Integrate Spotify into Amarok
On Sun, Jun 3, 2012 at 10:12 AM, Bart Cerneels bart.cerne...@gmail.com wrote: On Sun, Jun 3, 2012 at 6:32 AM, Ryan Feng odayf...@gmail.com wrote: Hi, Last week I posted a scratch code of the Amarok side resolver, I'm still working on it, like Leo and Bart said, there's no need to change the standalone Spotify resolver, so I won't touch it for a time, and I added SpotifyCollection, SpotifyQuery(or should be replaced by QueryMaker later) and Spotify::Resolver as the main class to start a new thread and handle user login, logout, track searching and stream etc. Still need to dive into the code of Collections and Playlists and figure out what's the best way to sync playlists between Amarok and Spotify. Next week, I will still be working on the basic Spotify integration. The code is still in the scratch repo, it will be put together with Amarok and tested soon, and a working Spotify service with basic features should be seen by the end of the next week. Thanks, Ryan Feng The code is here: http://quickgit.kde.org/index.php?p=clones%2Famarok%2Fzhengliangfeng%2Famarok-spotify.gita=commitdiffh=fb246b607f99a85fdff476ae189c53ad8a5fc4aa Matej has been uploading the results of his weekly work to reviewboard.kde.org so we can easily see the progress and review it inline. You should do the same. ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: GSoC update: Integrate Spotify into Amarok
On Sun, Jun 3, 2012 at 10:12 AM, Bart Cerneels bart.cerne...@gmail.com wrote: On Sun, Jun 3, 2012 at 6:32 AM, Ryan Feng odayf...@gmail.com wrote: Hi, Last week I posted a scratch code of the Amarok side resolver, I'm still working on it, like Leo and Bart said, there's no need to change the standalone Spotify resolver, so I won't touch it for a time, and I added SpotifyCollection, SpotifyQuery(or should be replaced by QueryMaker later) and Spotify::Resolver as the main class to start a new thread and handle user login, logout, track searching and stream etc. Still need to dive into the code of Collections and Playlists and figure out what's the best way to sync playlists between Amarok and Spotify. Next week, I will still be working on the basic Spotify integration. The code is still in the scratch repo, it will be put together with Amarok and tested soon, and a working Spotify service with basic features should be seen by the end of the next week. Thanks, Ryan Feng The code is here: http://quickgit.kde.org/index.php?p=clones%2Famarok%2Fzhengliangfeng%2Famarok-spotify.gita=commitdiffh=fb246b607f99a85fdff476ae189c53ad8a5fc4aa I see you made the base class a Service. A Service, which is represented in Amarok's Internet tab is only to be used by sources of content that don't fit in either the CollectionBrowser (currently still called Local Music) or playlists sections. Services don't need to comply with the specific UI conventions of Collections and PlaylistProviders but have to provide their own. The last.fm service is the best example. The names have sort of been unfortunately chosen and we're talking internally about changing then for 2.7 to avoid confusion. You need to base your work on the Collection and QueryMaker classes so Spotify will show up in the CollectionBrowser. I currently see no use case that needs a special UI for Spotfiy, hence no need for Service. Bart ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: GSoC update: Integrate Spotify into Amarok
On Sun, Jun 3, 2012 at 5:48 PM, Ryan Feng odayf...@gmail.com wrote: On Sun, Jun 03, 2012 at 10:21:39AM +0200, Bart Cerneels wrote: Date: Sun, 3 Jun 2012 10:21:39 +0200 To: amarok-devel@kde.org Cc: Ryan Feng odayf...@gmail.com From: Bart Cerneels bart.cerne...@gmail.com Subject: Re: GSoC update: Integrate Spotify into Amarok On Sun, Jun 3, 2012 at 10:12 AM, Bart Cerneels bart.cerne...@gmail.com wrote: On Sun, Jun 3, 2012 at 6:32 AM, Ryan Feng odayf...@gmail.com wrote: Hi, Last week I posted a scratch code of the Amarok side resolver, I'm still working on it, like Leo and Bart said, there's no need to change the standalone Spotify resolver, so I won't touch it for a time, and I added SpotifyCollection, SpotifyQuery(or should be replaced by QueryMaker later) and Spotify::Resolver as the main class to start a new thread and handle user login, logout, track searching and stream etc. Still need to dive into the code of Collections and Playlists and figure out what's the best way to sync playlists between Amarok and Spotify. Next week, I will still be working on the basic Spotify integration. The code is still in the scratch repo, it will be put together with Amarok and tested soon, and a working Spotify service with basic features should be seen by the end of the next week. Thanks, Ryan Feng The code is here: http://quickgit.kde.org/index.php?p=clones%2Famarok%2Fzhengliangfeng%2Famarok-spotify.gita=commitdiffh=fb246b607f99a85fdff476ae189c53ad8a5fc4aa I forgot to post the link, it's still in scratch repo[1], but it will be put in my clone of Amarok soon. And if I use reviewboard to submit code, do I still need to post report mail to the mailing list? I see you made the base class a Service. A Service, which is represented in Amarok's Internet tab is only to be used by sources of content that don't fit in either the CollectionBrowser (currently still called Local Music) or playlists sections. Services don't need to comply with the specific UI conventions of Collections and PlaylistProviders but have to provide their own. The last.fm service is the best example. The names have sort of been unfortunately chosen and we're talking internally about changing then for 2.7 to avoid confusion. You need to base your work on the Collection and QueryMaker classes so Spotify will show up in the CollectionBrowser. I currently see no use case that needs a special UI for Spotfiy, hence no need for Service. The reason why I used Service is because I thought users might need to input their username and password, and adjust Spotify settings such as 'High quality streaming' in a config dialog, just like what LastFM does. That should be done through the plugin config dialog. [1] http://quickgit.kde.org/index.php?p=scratch%2Fzhengliangfeng%2Fgsoc-scratch.gita=summary ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Amarok 2.6 string freeze starting now!
Hi all We are in string freeze for release 2.6.0, the beta has been tagged (v.2.5.90). The final release (or next beta) is scheduled for the weekend starting June 16. Bart ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
[amarok] release_scripts: Update release HOWTO to current procedure.
Git commit e9bd850aac32bf30cefe16fd1b2cf9b0e0537818 by Bart Cerneels. Committed on 30/05/2012 at 14:05. Pushed by shanachie into branch 'master'. Update release HOWTO to current procedure. Don't be afraid to update and extent this people. Will make future tarball creators a lot less frustrated. CCMAIL: amarok-devel@kde.org M +44 -38 release_scripts/RELEASE_HOWTO http://commits.kde.org/amarok/e9bd850aac32bf30cefe16fd1b2cf9b0e0537818 diff --git a/release_scripts/RELEASE_HOWTO b/release_scripts/RELEASE_HOWTO index 4587575..849df80 100644 --- a/release_scripts/RELEASE_HOWTO +++ b/release_scripts/RELEASE_HOWTO @@ -1,13 +1,10 @@ - -** BIG FAT WARNING: This HowTo is very outdated and hasn't been updated ** -** to reflect Amarok 2, autotools - cmake, svn - git transition; the ** -** general ideas still hold, though. ** -**** -** Amarok now uses The Ultimate KDE Extragear Release Script located in ** -** g...@git.kde.org:releaseme.git to do the release ** - +Amarok Release process +-- -Section 0: A few days in advance +Dependancies: +- The Ultimate KDE Extragear Release Script from g...@git.kde.org:releaseme.git + +Section 0: A week in advance * Check that release date doesn't clash with KDE's schedule * Notify translators and update http://techbase.kde.org/Schedules/Extragear @@ -23,22 +20,44 @@ SECTION 1: Preparation SECTION 2: Creating and Testing the Tarball - * Run the release.rb script; OR - Follow the guide lines in appendix A + * Run the amarok release script from releaseme.git (amarok2.rb) + example: ./amarok2.rb --src ~/Code/amarok --git-branch=master -v 2.5.90 -b trunk -p ssh * Test the following: - $ ./configure make - $ ./configure --enable-final --disable-debug make + $ cmake -DCMAKE_INSTALL_PREFIX=`kde4-config --prefix` -DCMAKE_BUILD_TYPE=RelWithDebInfo ./ + $ make - * Check it works OK without any amarok files in ~/.kde + * mkdir ~/.kde-test export KDEHOME=$HOME/.kde-test + * Check it works OK without any amarok files in $KDEHOME (i.e. new user config) + * Make a backup of $KDEHOME/share/apps/amarok and $KDEHOME/share/config/amarok* + (release_scripts/backup_userdata.sh) + * Check it works with an existing $KDEHOME, including database updates SECTION 3: Once Happy with the Tarball - * SFtp the tarball to ftpama...@ktown.kde.org/stable/amarok/VERSION/src or - ftpama...@ktown.kde.org/unstable/amarok/VERSION/src - (If you don't have access, mail sysad...@kde.org) - * Notify packagers + * ftp the tarball to upload.kde.org/incoming and file a sysadmin request to have it + transfered to stable/amarok/$version/src/ or unstable/amarok/$beta-version/src + Instructions at ftp://upload.kde.org/README + * Notify packagers: +To: kde-packa...@kde.org + +Dear packagers + +Here you can find the beta tarball for the upcoming $version release: +http://download.kde.org/download.php?url=unstable/amarok/$beta-version/src/amarok-$beta-version.tar.bz2 + +MD5Sum: $(md5sum tarball.tar.bz2) +SHA1Sum: $(sha1sum tarball.tar.bz2) + +We plan to release the final about 3 weeks from now but please make +this available to users via a specialized beta archive for packaging +is possible. We hope to get as much testers for this beta. + +Thanks for packaging. +The Amarok Team + + * Write release notes and dot story (and notify KDE-press list if necessary) * Add new version to Bugzilla @@ -51,8 +70,8 @@ Section 4: The Release Day * Send a witty, intelligent and diligently crafted email to the following lists: To: kde-announce-a...@kde.org, ama...@kde.org CC: amarok-pr...@kde.org - BCC: kde-extra-g...@kde.org, kde-multime...@kde.org, - amarok-packag...@googlegroups.com, (kde-press-annou...@kde.org) + BCC: kde-extra-g...@kde.org, kde-multime...@kde.org, kde-packag...@kde.org, + (kde-press-annou...@kde.org) * Update IRC channel topics * Update social networks * Update kde-apps.org @@ -62,26 +81,13 @@ Section 4: The Release Day SECTION 5: After the Release - If you must change anything about the tarball you need to email bin...@kde.org - because otherwise he gets annoyed that the md5 has changed. + If you must change anything about the tarball you need to email sysad...@kde.org + because otherwise they gets annoyed that the md5 has changed. Generally it is not wise to change the tarball, it could annoy the entire open-source world! -APPENDIX
Re: Amarok 2.6 string freeze starting now!
On Wed, May 30, 2012 at 5:24 PM, Frederik Schwarzer schwar...@kde.org wrote: Am Mittwoch, 30. Mai 2012, 13:56:12 schrieb Bart Cerneels: Hi all We are in string freeze for release 2.6.0, the beta has been tagged (v.2.5.90). The final release (or next beta) is scheduled for the weekend starting June 16. Bart One question: There is a string: Local Collection folders on remote Samba (Windows) shares But Samba is not the name for those shares, it is? Samba is only thne name of the free implementation of the SMB protocol, which is used in Windows. Regards It means a Windows share, standard CIFS/SMB protocol. It is in fact incorrect to call them Samba shares since they don't need to be that specific implementation. ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: GSoC report #2 - Integrate Spotify into Amarok
On Sun, May 27, 2012 at 11:13 PM, Ryan Feng odayf...@gmail.com wrote: Hi, The Spotify service plugin has two parts, a standalone Spotify resolver and a Amarok side Script Resolver, it's mainly from Tomahawk. I've rewrote the Amarok side resolver, it can work without Tomahawk base classes and headers now. Here[1] is the code. In the following week, I will continue working on the resolver to make it more suited for Amarok. [1] http://quickgit.kde.org/index.php?p=scratch%2Fzhengliangfeng%2Fgsoc-scratch.gita=summary Thanks, Ryan Thanks Ryan. Certainly not a wasted effort to split of the tomahawk headers. I'll go through the code tomorrow. ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: GSoC project report: Integrate Spotify into Amarok
On Mon, May 21, 2012 at 5:30 AM, Ryan Feng odayf...@gmail.com wrote: Hi, My name is Zhengliang Feng, I'm from China, and currently studying in the US. This is my first report of the Google Summer of Code project 'Integrate Spotify into Amarok'[1]. My mentor is Bart Cerneels. The main goal of my project is to integrate Spotify service into Amarok as a plugin, so users can login, access to their playlist, and stream tracks on Spotify in Amarok. The additional goal is make Spotify radio service usable in Amarok. Currently I'm trying to get Amarok running on Mac OS X, just to get familiar with the codebase, but unfortunately some dependencies in Macports are too old and there is much work to do with KDE and Qt port on OS X. Since the coding period has already started, so I would write some demo code with libspotify[2] and Qt next week. [1] http://www.google-melange.com/gsoc/proposal/review/google/gsoc2012/ofan/1 [2] https://developer.spotify.com/technologies/libspotify/ Thanks, Zhengliang Feng Hey Zhengliang Thanks for the intro, though I have to be honest and mention that you should have send this a lot earlier. Make sure you send us weekly updates and if you want to regularly blog (ask me how to get your blog on planet.kde.org and amarok.kde.org/planet). The blogs can be less technical and with pretty picture. They should be more oriented towards the general public. In the mails to this list you can talk class names and CMake macro's ;). But don't wait to ask questions in the weekly mail. You can ask on IRC or email if you don't get an immediate answer. I'll renew my promise to be on IRC between 19:00 and 20:30 UTC every Mon-Tue-Wed and Thu and I'll answer email ASAP. Ask others in the channel as well. Our mentoring is a team effort. Bart P.S. I've noticed you're using the first name Rick on your google profile. Can we use that? I certainly is less typing and less typo prone anyway. Bart ___ 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/#review14013 --- I'm not all that familiar with lyrics the SQL schema so will leave others to call it shipable. Make sure to update HACKING/mysql_database_schema.txt when you commit. ChangeLog http://git.reviewboard.kde.org/r/104966/#comment11106 data are migrated tickles my English grammar bone. data is migrated sounds better, even though it might not technically be correct with data plural. ChangeLog http://git.reviewboard.kde.org/r/104966/#comment11105 Does this patch fix this? Or just makes it so rescanning is not required anymore? src/core-impl/collections/db/sql/DatabaseUpdater.cpp http://git.reviewboard.kde.org/r/104966/#comment11107 You don't actually check if the previous query succeeded. src/core-impl/collections/db/sql/DatabaseUpdater.cpp http://git.reviewboard.kde.org/r/104966/#comment11108 I was wondering if the url column could actually be intentional to point to a remote resource. But if it's not used currently there is no point in having it. - Bart Cerneels On May 16, 2012, 1:51 p.m., Matěj Laitl wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104966/ --- (Updated May 16, 2012, 1:51 p.m.) Review request for Amarok and Ralf Engels. Description --- 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 is documented. BUG: 242350 FIXED-IN: 2.6 REVIEW: 104966 This addresses bug 242350. https://bugs.kde.org/show_bug.cgi?id=242350 Diffs - ChangeLog 67bc020 src/core-impl/collections/db/sql/DatabaseUpdater.h 37ccb54 src/core-impl/collections/db/sql/DatabaseUpdater.cpp 163b089 src/core-impl/collections/db/sql/SqlMeta.cpp f8f9bdb 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: [amarok] /: AudioCdCollection: don't create a reference out of temporary var (crashfix)
The temporary QString returned by url() should remain valid until the
end of the local scope. At least that is what I think, but it's a
tricky one. I hope I'm proven wrong by the bug being fixed.
On Mon, May 7, 2012 at 11:42 PM, Matěj Laitl ma...@laitl.cz wrote:
Git commit a3d378de5aca583bbbc13d1966df258f27e7f174 by Matěj Laitl.
Committed on 07/05/2012 at 23:31.
Pushed by laitl into branch 'master'.
AudioCdCollection: don't create a reference out of temporary var (crashfix)
There's following code in AudioCdCollection::updateProxyTracks():
const QString urlString = url.url().remove( audiocd:/ );
Which I think is wrong, because url.url() returns a temporary QString
and QString::remove() returns a reference to itself. However, the
temporary is AFAICS destroyed as soon as this line ends. This IMO
results in urlString being an invalid reference.
I'm not really sure about this, I haven't read C++ spec, but given many
users report crash on the following line, this could be the culprit.
I was never able to reproduce the bug, so I'm shooting blindly.
Reporters, please test reproducibility with current git and reopen if
this is not fixed.
BUG: 256585
FIXED-IN: 2.6
DIGEST: fix grave crash
M +1 -0 ChangeLog
M +1 -1 src/core-impl/collections/audiocd/AudioCdCollection.cpp
http://commits.kde.org/amarok/a3d378de5aca583bbbc13d1966df258f27e7f174
diff --git a/ChangeLog b/ChangeLog
index 573c15c..5894bf3 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -84,6 +84,7 @@ VERSION 2.6-Beta 1
1.2 GB free is shown instead of 85% used; thicker capacity bar.
BUGFIXES:
+ * Fix crash on startup related to Audio CD collection. (BR 256585)
* When turning dynamic playlist on, immediately populate playlist and
clear
any possible playlist sorting. (BR 220558)
* Fix transcoding with ffmpeg = 0.10; patch by Julian Simioni.
diff --git a/src/core-impl/collections/audiocd/AudioCdCollection.cpp
b/src/core-impl/collections/audiocd/AudioCdCollection.cpp
index 637c2d3..95c4e06 100644
--- a/src/core-impl/collections/audiocd/AudioCdCollection.cpp
+++ b/src/core-impl/collections/audiocd/AudioCdCollection.cpp
@@ -575,7 +575,7 @@ AudioCdCollection::updateProxyTracks()
foreach( const KUrl url, m_proxyMap.keys() )
{
- const QString urlString = url.url().remove( audiocd:/ );
+ QString urlString = url.url().remove( audiocd:/ );
const QStringList parts = urlString.split( '/' );
if( parts.count() != 2 )
___
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Revert broken workaround previously not working solid fstab monitoring
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104878/#review13544 --- Why not use ifdef's to conditionally compile in the workaround? - Bart Cerneels On May 7, 2012, 1:59 p.m., Stefan Brüns wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104878/ --- (Updated May 7, 2012, 1:59 p.m.) Review request for Amarok. Description --- As the needed fix in solid is upstream since SC 4.8.2, the workaround should be removed. For details, see the BR. Anyone insisting on using an older version of kdelibs should apply the solid patch. This addresses bug 293926. https://bugs.kde.org/show_bug.cgi?id=293926 Diffs - src/MediaDeviceCache.h a48d453 src/MediaDeviceCache.cpp 15583b8 Diff: http://git.reviewboard.kde.org/r/104878/diff/ Testing --- Thanks, Stefan Brüns ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Tidying up password storage in Amarok
On Thu, Apr 12, 2012 at 22:22, Matěj Laitl ma...@laitl.cz wrote: On 11. 4. 2012 Andrzej J. R. Hunt wrote: I've just been looking at the way all the plugins use their passwords. It seems a redesign would be needed to allow password entry manually: currently the plugins stay disabled until a password is stored, once one is stored, they use this every startup to authenticate with their service. If you want to be able to have the user asked for login details every startup you would need to change the plugins to repeatedly ask for passwords until they can login (e.g. in case there is a typo in the password etc.), rather than just having them ask for a password once (since they assume the passwords are stored correctly), and then fail silently when the password doesn't work (this at least is the case for LastFM). Therefore I think it's probably better to work on the assumption that all passwords are stored on disk -- I wouldn't think it too unreasonable to expect those, who want a specific password not to be in plaintext, to go to the bother of setting up KWallet (or whatever other backends are added) correctly? Yes. That would add too much complexity for little gain. 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. 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. ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: KWallet for Magnatune credentials
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104480/#review12164
---
If this works for Last.fm this should to. I do remember problems with last.fm
when the wallet was not available though, so I wonder if all corner cases are
covered.
There are also so style errors in the copied code: there should be no space
between if and '('
if( bla )
{
}
I think the copyright header is ok. You probably don't even need the from
info, but in this case it's useful because there is no git trace.
- Bart Cerneels
On April 3, 2012, 8:04 p.m., Andrzej Hunt wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104480/
---
(Updated April 3, 2012, 8:04 p.m.)
Review request for Amarok.
Description
---
Modifies the Magnatune service to use KWallet for storage (instead of
plaintext).
The code has been copied from LastFmServiceConfig.cpp, with almost no
modification on my part. (KDevelop removed some whitespace and reformatted
some other bits of code meaning there are some formatting changes in the diff
-- I apologise if this is a problem for reviewing, and I can revert the
formatting changes if desired.)
A question about copyright attribution: Would this be the correct thing to
add to MagnatuneConfig.cpp below the current copyright line?
* Code copied from ../lastfm/LastFmServiceConfig.cpp:
* Copyright (c) 2007 Shane King k...@dontletsstart.com
* Copyright (c) 2009 Leo Franchi lfran...@kde.org
This addresses bug 242256.
https://bugs.kde.org/show_bug.cgi?id=242256
Diffs
-
src/services/magnatune/MagnatuneConfig.cpp 18ee898
src/services/magnatune/MagnatuneConfig.h f1d25eb
Diff: http://git.reviewboard.kde.org/r/104480/diff/
Testing
---
Thanks,
Andrzej Hunt
___
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Diagnostics Dialog for Amarok.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104449/#review12106 --- Ship it! Looks quite perfect style wise (ok, I found one thing to nitpick on ;). Code is clean and functional. There is one thing I noticed while testing: all are plugins are version 1.0 (X-KDE-PluginInfo-Version=1.0) We bump X-KDE-Amarok-framework-version an each release and use that to check if a plugin can be used or not. It's currently not relevant to include this anyway since we have never committed to ABI stability of our APIs and hence always bump the framework version. If you already have a developer account (identity.kde.org and request write access to the git repos via sysadmin request on bugs.kde.org) you can push it with the changes I mentioned. If not I can push it for you but urge you to get that account for future contributions. src/dialogs/DiagnosticDialog.cpp http://git.reviewboard.kde.org/r/104449/#comment9531 I would put the body on a newline. Just a bit more readable within the rest of the amarok codebase. src/dialogs/DiagnosticDialog.cpp http://git.reviewboard.kde.org/r/104449/#comment9535 Either separate them into running/stopped lists or append (stopped). The rational is that if that line is partially copy/pasted on IRC, we'll know that it's incomplete. src/dialogs/DiagnosticDialog.cpp http://git.reviewboard.kde.org/r/104449/#comment9534 Same as for plugins but with enabled/disabled. src/dialogs/DiagnosticDialog.cpp http://git.reviewboard.kde.org/r/104449/#comment9532 We spell Qt with lower case t and make apple QuickTime jokes about those who don't ;) src/dialogs/DiagnosticDialog.cpp http://git.reviewboard.kde.org/r/104449/#comment9533 The phonon backend also needs a version. Might help track down bugs to specific versions or builds. Hope you can get it easily. - Bart Cerneels 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
Re: GSoC Proposal v2: Statistics synchronization for pluggable devices and Last.fm
-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 introduce an abstraction layer, a class called ScrobblingService with an interface to scrobble tracks and an optional interface to query scrobbled tracks. Existing Last.fm code will be adapted and extended to be an implementation of this interface; Last.fm API is powerful enough to support all claimed features. It should be noted that there is already Last.fm on-line service Collection in Amarok, but it focuses just on playing Last.fm radio streams and won't be touched in this project. In order to implement synchronization between Last.fm and Amarok, there will be a special case for ScrobblingService in the synchronizer adapted to constraints of its web service nature. The plan for the actual synchronizer is to be N-way (e.g. to synchronize N collections at once) and to work in per-artist chunks; this is mainly because that would allow efficient use of Last.fm API [5], a part of which will be mimicked by ScrobblingService. The GUI will be implemented as a new window using Model/View pattern as supported by Qt's AbstractItemModel and QListView, custom delegates will be probably needed to display in-line controls for conflict resolution. Tentative Timeline: === week 1: Core classes (StatsSynchronizer, TrackMatcher probably) with stub implementations created; matching tracks from different collections by meta- data week 2: First iteration of the GUI; will be ugly but will show matched tracks week 3: TrackSynchronizer class implemented to actually synchronize meta-data of N tracks; at this stage unable to cope with conflicts week 4: tracks being updated and conflicts shown in the GUI; basic conflict resolution in the GUI (this collection wins) week 5: RecentStatsCapability introduced and implemented for iPod collection, plugged into StatsSynchronizer to aid with conflict resolution week 6: Introduction of the ScrobblingService class, existing Last.fm implementation turned to implementation of this class (just scrobbling at this time, no support for querying Last.fm data) week 7: making the GUI beautiful, more convenient conflict resolution (a button to set a master collection, with a way to override this for individual tracks) **: mid-term evaluation: inter-collection synchronization should be working at this time week 8: LastfmScrobblingService extended to be able to query user's Last.fm library for tracks she played, their playcounts, labels week 9: Basic ScroblingService support in StatsSynchronizer: ability to match collection tracks with remote Last.fm tracks week 10: LastfmScrobblingService extended to be able to set track labels; track rating getting and setting implemented using special labels week 11: Actual synchronization with ScroblingServices in StatsSynchronizer; may support just one instance of StatsSynchronizer if supporting multiple at the same time shows tricky week 12: final touches to the GUI, improving usability, progress bars for longer-running operations; scrobbling from just-plugged devices that offer RecentStatsCapability. ***: suggested pencils-down date week 13: resolving any remaining issues, testing the code for various cases (ability to use Last.fm as a statistics data backup in case of their loss etc.) ***: hard pencils-down week 14: preparing the code (stored in a git branch) for inclusion into mainline, proof-reading it, stripping debugging statemets week 15: review request sent, resolving any possible remarks, ended by inclusion into Amarok master hopefully. Do you have other obligations from late May to early August? If accepted, GSoC will be my main commitment during the summer. I plan to have a week-long vacation and a few 3-day trips, but I'm used to work during weekends on open source projects, so the vacation will be compensated. First 6 weeks of GSoC coding period coincide with my university examination period, but I have just 2 real exams this semester and coding is my favourite excuse- not-to-study so I expect it won't hamper my productivity. About Me: = I'm a 24-year-old student of mathematical informatics from Prague, Czech Republic. I've been passionate about FLOSS since high school and recently I've started contributing to a couple of projects (mainly KDE related), most notably Amarok where I worked on fixing various bugs singe last autumn [7] and recently I've rewritten the iPod collection from scratch [8] as suggested by Amarok's Bart Cerneels; I plan to submit a review request for it in coming weeks. I've been also
Re: Updated Proposal for QML Context View
On Fri, Mar 23, 2012 at 18:14, Saurabh Sood saurabhsoo...@gmail.com wrote: Hi, I have updated my proposal for QMLify Amarok Context View. I have changed some aspects of the timeline, and am still working on it. Please go through it, and suggest changes. Also, I made a sample class for Proof of Concepts, which I successfully integrated with the existing Amarok code. Do i submit it on the reviewboard? Regards,. Saurabh Submissions are open on GSoC's Melange, so I propose you upload the proposal there. Bart ___ 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 Mon, Mar 26, 2012 at 23:28, Matěj Laitl ma...@laitl.cz wrote: 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 ___ A solitary QueryMaker is good for unit testing though. But internally you might want to put some infrastructure code in the Collection and construct the QM with a Collection-ptr. So yeah, those 2 classes are very much interwoven. ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Proof of concept for the 2012 GSoC project idea 'Semantic Collection for Amarok'.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104369/#review11785 --- Code is clean and (probably) functional. Even though it's not supposed to be I would ship this. It also makes me feel that a conversion utility from SQL - nepomuk would not be to difficult to do. - Bart Cerneels On March 22, 2012, 4:58 p.m., Phalgun Guduthur wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104369/ --- (Updated March 22, 2012, 4:58 p.m.) Review request for Amarok and Teo Mrnjavac. Description --- I have tried to demonstrate a basic read and write of Nepomuk index through Amarok by altering how song ratings are stored. When applied, this patch stores any changed rating of a song into the Nepomuk index and not the Sql backend. To test this, change the rating of any song through Amarok and check the attributes of that song using Dolphin (song-properties). The song rating would have changed accordingly. To demonstrate the read part, Each song's rating is fetched from the Nepomuk index instead of the Sql backend. To test this, change the rating of any song through Dolphin and the same would be reciprocated in Amarok. Please note, this is only for proof of concept. This is not intended to be shipped. The code changes I have made are only temporary. The actual project will have Nepomuk classes and handlers to do the same task. Diffs - src/core-impl/collections/db/sql/CMakeLists.txt bdb3966 src/core-impl/collections/db/sql/SqlMeta.cpp e663adf Diff: http://git.reviewboard.kde.org/r/104369/diff/ Testing --- The existing test cases work. It has been tried on numerous songs successfully on my computer. Thanks, Phalgun Guduthur ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: GPodder Improvements Patch Rev1
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104335/#review11646 --- Still can't test because of 503 error on gpodder.net/api. Code is good. src/services/gpodder/GpodderProvider.cpp http://git.reviewboard.kde.org/r/104335/#comment9234 Note to self: use Solid::Networking::status() to prevent offline running of that 10 second loop. - Bart Cerneels On March 19, 2012, 8 p.m., Lucas Gomes wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104335/ --- (Updated March 19, 2012, 8 p.m.) Review request for Amarok, Stefan Derkits and Bart Cerneels. Description --- GPodder Improvements Patch Rev1 Put messages to show that gpodder service failed to get data from gpodder.net. Done some cleanup and added some comments. GPodder Service improvements Full status synchronisation implemented. Cleanup and some refactoring. GPodder Provider saves podcast subscriptions changes if the user closes amarok before a expected synchronisation to end, so as to synchronize them in the next start. Forcing GPodderService to verify if the username and password is not empty before creating a mygpo::ApiRequest. Solved some problems related to the KWallet use in GPodderService. Diffs - ChangeLog 0365651b2d4c28f278c7ec7eb7a3a26a95caa4a4 src/browsers/playlistbrowser/PlaylistsByProviderProxy.cpp 0a450ae2420de11ab221cf2dec9fc942a7118dd1 src/browsers/playlistbrowser/PodcastModel.h e88f4a1dea0b41ce6cc21cdbe3809b99c200b5b8 src/browsers/playlistbrowser/PodcastModel.cpp 18334f6eb970eed241f172cc71396cca8bcaf04b src/core-impl/podcasts/sql/SqlPodcastProvider.h c3d5e56e79c86e2be8c39f47df928bfeb291a920 src/core-impl/podcasts/sql/SqlPodcastProvider.cpp 183005f652989167952b0c8cf16e742de2fb94e1 src/services/gpodder/GpodderPodcastRequestHandler.h 8c12e5f5a5a63b91670fd8081018b7a017a00bbe src/services/gpodder/GpodderPodcastRequestHandler.cpp 66e8ea8a3aeec054c82a180f84d199aba5547f5a src/services/gpodder/GpodderProvider.h 4c724cd763b94515e9e90620ad9a4a3fb8f92e2c src/services/gpodder/GpodderProvider.cpp 6e3255e5f465287a9fad3454cca5a1d2ce47fc6a src/services/gpodder/GpodderService.cpp b518de33e01d20c7293c50e9edbb2dff78419e53 src/services/gpodder/GpodderServiceConfig.h 111455aeeb2a579caf1dd938249b8d832a775a89 src/services/gpodder/GpodderServiceConfig.cpp 750272e4f2c6c7ad411e9d12c622e97789a29bcd src/services/gpodder/GpodderServiceSettings.h 81bc218428d2b8bc78235852548a337579b35b24 src/services/gpodder/GpodderServiceSettings.cpp 8bb8b806927d1dbac47f2a06887c2e2a6dd3d504 src/services/gpodder/GpodderSortFilterProxyModel.h 638fe992cba553cc317febdf049b6b0f301018b5 src/services/gpodder/GpodderSortFilterProxyModel.cpp ed1e8301f6f415e86e5367b7827817fff45feb98 Diff: http://git.reviewboard.kde.org/r/104335/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: GSoC: Extending Remove duplicates in Amarok
On Mon, Mar 19, 2012 at 20:27, sunil kumar skstrongh...@gmail.com wrote: Hello, Amarok have an option in Playlist-Remove duplicates to remove songs which are binary identical. I would like to extend this feature so that it not only remove binary identical songs but also uses fuzzy comparison to find identical songs having different tags or formats, as my GSoC 2012 project. This tool would present user with a choice to remove those songs which are similar but have different format/tags/encoding or have slight variation in time. It would also show that by how much amount two songs are similar. This tool can help us when we have collected our songs from different people. -- My first thought is that this would not qualify as a GSoC project because of it's workload. Remember that GSoC is 15 weeks full time, even a bit longer. As a season of KDE task it's possible, along with some more cleanup. If you want to implement this for yourself and send us a patch on reviewboard.kde.org, we will certainly take note of that if you apply for another GSoC or Season of KDE project. If you want help with this feature, ask on #amarok @freenode.irc.org Bart ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: GPodder Service improvements
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104335/#review11609 --- Testing the patch. So far getting a parseError() on test login, so don't know if it's working. ChangeLog http://git.reviewboard.kde.org/r/104335/#comment9192 Most recent changes go on top. src/core-impl/podcasts/sql/SqlPodcastProvider.h http://git.reviewboard.kde.org/r/104335/#comment9193 We might want to add line: //TrackProvider methods here for readability. src/core-impl/podcasts/sql/SqlPodcastProvider.h http://git.reviewboard.kde.org/r/104335/#comment9194 Downloading episodes is so common among Podcast implementation we might want to make them into virtual methods of the base class one day. Not needed for this patch though. src/services/gpodder/GpodderProvider.cpp http://git.reviewboard.kde.org/r/104335/#comment9197 space src/services/gpodder/GpodderProvider.cpp http://git.reviewboard.kde.org/r/104335/#comment9195 extra space after if here. src/services/gpodder/GpodderProvider.cpp http://git.reviewboard.kde.org/r/104335/#comment9196 and here - Bart Cerneels On March 18, 2012, 3:37 p.m., Lucas Gomes wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104335/ --- (Updated March 18, 2012, 3:37 p.m.) Review request for Amarok, Stefan Derkits and Bart Cerneels. Description --- GPodder Service improvements Full status synchronisation implemented. Cleanup and some refactoring. GPodder Provider saves podcast subscriptions changes if the user closes amarok before a expected synchronisation to end, so as to synchronize them in the next start. Forcing GPodderService to verify if the username and password is not empty before creating a mygpo::ApiRequest. Solved some problems related to the KWallet use in GPodderService. Diffs - ChangeLog 0365651b2d4c28f278c7ec7eb7a3a26a95caa4a4 src/browsers/playlistbrowser/PlaylistsByProviderProxy.cpp 0a450ae2420de11ab221cf2dec9fc942a7118dd1 src/browsers/playlistbrowser/PodcastModel.h e88f4a1dea0b41ce6cc21cdbe3809b99c200b5b8 src/browsers/playlistbrowser/PodcastModel.cpp 18334f6eb970eed241f172cc71396cca8bcaf04b src/core-impl/podcasts/sql/SqlPodcastProvider.h c3d5e56e79c86e2be8c39f47df928bfeb291a920 src/core-impl/podcasts/sql/SqlPodcastProvider.cpp 183005f652989167952b0c8cf16e742de2fb94e1 src/services/gpodder/GpodderPodcastRequestHandler.h 8c12e5f5a5a63b91670fd8081018b7a017a00bbe src/services/gpodder/GpodderPodcastRequestHandler.cpp 66e8ea8a3aeec054c82a180f84d199aba5547f5a src/services/gpodder/GpodderProvider.h 4c724cd763b94515e9e90620ad9a4a3fb8f92e2c src/services/gpodder/GpodderProvider.cpp 6e3255e5f465287a9fad3454cca5a1d2ce47fc6a src/services/gpodder/GpodderService.cpp b518de33e01d20c7293c50e9edbb2dff78419e53 src/services/gpodder/GpodderServiceConfig.h 111455aeeb2a579caf1dd938249b8d832a775a89 src/services/gpodder/GpodderServiceConfig.cpp 750272e4f2c6c7ad411e9d12c622e97789a29bcd src/services/gpodder/GpodderServiceSettings.h 81bc218428d2b8bc78235852548a337579b35b24 src/services/gpodder/GpodderServiceSettings.cpp 8bb8b806927d1dbac47f2a06887c2e2a6dd3d504 src/services/gpodder/GpodderSortFilterProxyModel.h 638fe992cba553cc317febdf049b6b0f301018b5 src/services/gpodder/GpodderSortFilterProxyModel.cpp ed1e8301f6f415e86e5367b7827817fff45feb98 Diff: http://git.reviewboard.kde.org/r/104335/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: Review Request: Restore heuristics to guess whether album is a compilation
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104294/#review11457 --- All I can comment on is the coding style, which is good :). I did notice more false positives in various artists recently. I'm not sure if these are worse then uncatched VA's. If it were up to me VA would be implemented completely in the view so it works for all collections at once and can easily be turned off. - Bart Cerneels On March 16, 2012, 12:13 a.m., Alexey Neyman wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104294/ --- (Updated March 16, 2012, 12:13 a.m.) Review request for Amarok. Description --- Older amarok used the following heuristics to determine whether a particular album is a compilation (from a comment in amarok-2.2.1/src/collection/sqlcollection/ScanResultProcessor.cpp): //using the following heuristics: //if more than one album is in the dir, use the artist of each track as albumartist //if all tracks have the same artist, use it as albumartist //try to find the albumartist A: tracks must have the artist A or A feat. B (and variants) //if no albumartist could be found, it's a compilation However, more recent Amarok versions started to merge different albums with different artist in separate directories together, as explained above. Amarok started to assume all albums with same name to be compilations (even if in separate directories) since the following commit: dfd8b457d7094144563c51b2528afdbe23ffc344 Ralf Engels Fix all collection scanner auto tests. Now, amarok first scans all directories (sorting albums by the name) and then tries to process *album names*, one at a time. If it finds more than one instance of an album name, it assumes it to be a compilation. Thus, it lost the heuristics in employed before (if more than one album is in the dir...). While it is still possible to force the right behavior by selecting Do not show under Various Artists for each of the erroneous albums, it would still be better to restore the original heuristics as there may be lots of albums merged this way. I think the old heuristics made sense (why would albums be put into separate directories otherwise, if they are a single compilation album?). The attached patch restores the following logic: If any given directory contains tracks that were sorted into a single album and and that album was not created as a compilation (i.e. it has non-empty artists), this album is excluded from being merged with other albums to create a compilation. Diffs - src/core-impl/collections/db/ScanResultProcessor.cpp 4f02a16 Diff: http://git.reviewboard.kde.org/r/104294/diff/ Testing --- Thanks, Alexey Neyman ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Load all tracks from XSPF using MetaProxy.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104256/ --- (Updated March 14, 2012, 10:54 a.m.) Review request for Amarok. Changes --- This one should build. Description --- Load all tracks from XSPF using MetaProxy. Added a worker that does the actual trackForUrl call and subscribes to trackProviderAdded signal as needed. This addresses bug 295199. https://bugs.kde.org/show_bug.cgi?id=295199 Diffs (updated) - src/CMakeLists.txt 4241e69 src/core-impl/collections/support/CollectionManager.cpp 37fe03a src/core-impl/meta/proxy/MetaProxy.h d8329be src/core-impl/meta/proxy/MetaProxy.cpp d1577a2 src/core-impl/meta/proxy/MetaProxyWorker.h PRE-CREATION src/core-impl/meta/proxy/MetaProxyWorker.cpp PRE-CREATION src/core-impl/meta/proxy/MetaProxy_p.h 792675d src/core-impl/playlists/types/file/xspf/XSPFPlaylist.cpp 4cb49fb src/core/collections/support/TrackForUrlWorker.h 64d23bd src/core/collections/support/TrackForUrlWorker.cpp 7e8f289 src/services/ampache/AmpacheServiceCollection.h a48c8f2 src/services/ampache/AmpacheServiceCollection.cpp b684e34 Diff: http://git.reviewboard.kde.org/r/104256/diff/ Testing --- Loaded XSPF attached to https://bugs.kde.org/show_bug.cgi?id=295199 Could impact AmpacheService. Not checked yet. Thanks, Bart Cerneels ___ 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
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104213/#review11394
---
Ship it!
I think in this case the use of a Capability is completely justified. It's the
Capabilities that just add complexity that are problematic.
Screenshot: Changes to the Configure Collection dialog
http://git.reviewboard.kde.org//r/104213/#scomment32
If there are = 3 options, don't use a combobox.
Screenshot: Revamped Transcode dialog
http://git.reviewboard.kde.org//r/104213/#scomment33
This is hard to understand and contains some language errors. Perhaps Use
this for next tracks ?
- Bart Cerneels
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
--
Next commit squelched for the purpose of review board
--
Transcoding::Property: remove NUMERIC, LIST, TEXT types
These types were not used since Teo reworked all encoders to use the
TRADEOFF type. Remove them and associated code to make codebase cleaner
so that new code doesn't need to introduce case statements
Re: Review Request: Rework transcoding: remember encoder, transcode on move, cleaner code
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.
Matěj Laitl wrote:
Yup, there are 2 or 3. Should I use radio buttons?
Use a groupbox: http://qt-project.org/doc/qt-4.8/widgets-groupbox.html
It might make the dialog to tall. Might not be worth it for such an advanced
feature. Add a screenshot please so we can see what works. Please :)
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 ?
Matěj Laitl wrote:
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.
It's still not clear to me. But perhaps just Save settings ?
- Bart
---
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
Re: Amarok 2.4.3 doesn't write covers to the files
-- Forwarded message -- From: Arvind S Raj sraj.arv...@gmail.com Date: Fri, Mar 9, 2012 at 11:37 Subject: Amarok 2.4.3 doesn't write covers to the files To: Amarok Mailing List ama...@kde.org Hello everyone, I'm running Amarok 2.4.3 on Kubuntu 11.10. I've updated the covers for a lot of albums and they're displayed in amarok but they do not get written to the files. When I open the file in VLC, the old album cover is still displayed. I've check the option Write covers to file in Settings - Configure Amarok - Collection but Amarok still doesn't write it. I installed the debug symbols and ran amarok --debug --nofork and here's the output: http://paste.pocoo.org/show/563257/ I was told by Valorie in the IRC to not include the startup information but wasn't sure how to do so and so have included all startup information. If there's some more information required, do let me know and I'll definitely provide them. -- Arvind S Raj B.Tech, Computer Science and Engineering(Final Year) Amrita School of Engineering, Amritapuri http://arvindsraj.wordpress.com ___ Amarok mailing list ama...@kde.org https://mail.kde.org/mailman/listinfo/amarok On Fri, Mar 9, 2012 at 12:40, Myriam Schweingruber myr...@kde.org wrote: I asked Arvind (nick is dnivra on IRC) for more information on IRC: these files are part of the collection and already contain a cover. I see nowhere in this output a hint that at some point Amarok tried to add a cover to a file. Rick: could it be that it doesn't write the cover when there already is one? Regards, Myriam It would make a lot of sense that existing data is not overwritten. Data loss prevention is priority #1. I could not immediately find and code that prevents it though. ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Review Request: Load all tracks from XSPF using MetaProxy.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104256/ --- Review request for Amarok. Description --- Load all tracks from XSPF using MetaProxy. Added a worker that does the actual trackForUrl call and subscribes to trackProviderAdded signal as needed. This addresses bug 295199. https://bugs.kde.org/show_bug.cgi?id=295199 Diffs - src/CMakeLists.txt 4241e69 src/core-impl/collections/support/CollectionManager.cpp 37fe03a src/core-impl/meta/proxy/MetaProxy.h d8329be src/core-impl/meta/proxy/MetaProxy.cpp d1577a2 src/core-impl/meta/proxy/MetaProxyWorker.h PRE-CREATION src/core-impl/meta/proxy/MetaProxyWorker.cpp PRE-CREATION src/core-impl/meta/proxy/MetaProxy_p.h 792675d src/core-impl/playlists/types/file/xspf/XSPFPlaylist.cpp 4cb49fb Diff: http://git.reviewboard.kde.org/r/104256/diff/ Testing --- Loaded XSPF attached to https://bugs.kde.org/show_bug.cgi?id=295199 Could impact AmpacheService. Not checked yet. Thanks, Bart Cerneels ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Load all tracks from XSPF using MetaProxy.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104256/ --- (Updated March 13, 2012, 9:12 a.m.) Review request for Amarok. Description --- Load all tracks from XSPF using MetaProxy. Added a worker that does the actual trackForUrl call and subscribes to trackProviderAdded signal as needed. This addresses bug 295199. https://bugs.kde.org/show_bug.cgi?id=295199 Diffs - src/CMakeLists.txt 4241e69 src/core-impl/collections/support/CollectionManager.cpp 37fe03a src/core-impl/meta/proxy/MetaProxy.h d8329be src/core-impl/meta/proxy/MetaProxy.cpp d1577a2 src/core-impl/meta/proxy/MetaProxyWorker.h PRE-CREATION src/core-impl/meta/proxy/MetaProxyWorker.cpp PRE-CREATION src/core-impl/meta/proxy/MetaProxy_p.h 792675d src/core-impl/playlists/types/file/xspf/XSPFPlaylist.cpp 4cb49fb Diff: http://git.reviewboard.kde.org/r/104256/diff/ Testing --- Loaded XSPF attached to https://bugs.kde.org/show_bug.cgi?id=295199 Could impact AmpacheService. Not checked yet. Thanks, Bart Cerneels ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Load all tracks from XSPF using MetaProxy.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104256/ --- (Updated March 13, 2012, 9:12 a.m.) Review request for Amarok. Description --- Load all tracks from XSPF using MetaProxy. Added a worker that does the actual trackForUrl call and subscribes to trackProviderAdded signal as needed. This addresses bug 295199. https://bugs.kde.org/show_bug.cgi?id=295199 Diffs - src/CMakeLists.txt 4241e69 src/core-impl/collections/support/CollectionManager.cpp 37fe03a src/core-impl/meta/proxy/MetaProxy.h d8329be src/core-impl/meta/proxy/MetaProxy.cpp d1577a2 src/core-impl/meta/proxy/MetaProxyWorker.h PRE-CREATION src/core-impl/meta/proxy/MetaProxyWorker.cpp PRE-CREATION src/core-impl/meta/proxy/MetaProxy_p.h 792675d src/core-impl/playlists/types/file/xspf/XSPFPlaylist.cpp 4cb49fb Diff: http://git.reviewboard.kde.org/r/104256/diff/ Testing --- Loaded XSPF attached to https://bugs.kde.org/show_bug.cgi?id=295199 Could impact AmpacheService. Not checked yet. Thanks, Bart Cerneels ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: [GSoC 2012] Spotify Integration
On Wed, Mar 7, 2012 at 14:37, Fabiano Fidêncio fabi...@fidencio.org wrote: Howdy! Spotify integration in Amarok is an idea that I have a strong interest. But, firstly, before I start to take a look in the code, I need to know how many hours per week is expected than a student work? Best Regards, -- Fabiano Fidêncio ___ A full work day, i.e. at least 40 hours a week, longer if you work a bit slowly or feel particularly motivated. It really is a full time job for 3 months. Just looking at the code is rather boring. You could start trying to fix bugs we specifically selected to get into development. Those are called junior jobs: https://bugs.kde.org/buglist.cgi?cmdtype=doremremaction=runnamedcmd=amarok%20JJ%27slist_id=4625 Bart ___ 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. Matěj Laitl wrote: The latest version shows it in all cases. (or you talk about prettyUrl() vs. url()?) Ah, then it's OK. I was assuming based on the (outdated) screenshot. - Bart --- 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
