Re: getamarok.com mail forwarding broken

[Bart Cerneels]

I can't log in using save credentials that used to work. Could it be
google terminated our apps in the recent move to none-free?

On Tue, Jul 16, 2013 at 1:13 PM, Sven Krohlas  wrote:
> Heya,
>
> I tried to contact eean but got no reply. It seems as if mail forwarding
> from our getamarok.com mail addresses is broken. There is no MX entry in
> DNS:
>
> dig -t MX getamarok.com
>
> ; <<>> DiG 9.9.2-P2 <<>> -t MX getamarok.com
> ;; global options: +cmd
> ;; Got answer:
> ;; ->>HEADER<<- opcode: QUERY, status: SERVFAIL, id: 5072
> ;; flags: qr rd ra; QUERY: 1, ANSWER: 0, AUTHORITY: 0, ADDITIONAL: 1
>
> ;; OPT PSEUDOSECTION:
> ; EDNS: version: 0, flags:; udp: 512
> ;; QUESTION SECTION:
> ;getamarok.com. IN  MX
>
> ;; Query time: 537 msec
> ;; SERVER: 8.8.8.8#53(8.8.8.8)
> ;; WHEN: Tue Jul 16 13:09:55 2013
> ;; MSG SIZE  rcvd: 42
> ___
> 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: CUE sheets in collection still not working :(

[Bart Cerneels]

On Sun, Apr 14, 2013 at 12:11 PM, Matěj Laitl  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

[Bart Cerneels]

---
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/TestMet

Re: Review Request 108686: hidden items in context menu: usability question

[Bart Cerneels]

> 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 
>

Re: Review Request 108686: hidden items in context menu: usability question

[Bart Cerneels]

> 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 
>

Re: [amarok] src: Remove unused AudioCdTrackProvider

[Bart Cerneels]

Wasn't this part of AudioCdCollection? How did that ever work?

On Sun, Feb 17, 2013 at 6:39 PM, Matěj Laitl  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  
>*
> - *   
>*
> - * 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 .
>*
> - 
> /
> -
> -#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  
>*
> - *   
>*
> - * 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

[Bart Cerneels]

On Mon, Feb 18, 2013 at 3:29 PM, Edward Hades Toroshchin
 wrote:
> On Mon, Feb 18, 2013 at 02:22:14PM -0000, 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: Review Request 108686: hidden items in context menu: usability question

[Bart Cerneels]

> 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 108686: hidden items in context menu: usability question

[Bart Cerneels]

> 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.

Shift for delete/move was implemented in kde's file browser already before I 
added this to Amarok. It's not an uncommon feature.


- 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 108906: Add ability to drag titles to re-arrange them in queue manager

[Bart Cerneels]

> On Feb. 12, 2013, 2:49 p.m., Matěj Laitl wrote:
> > I don't think creating a custom subclass is the right way to implement drag 
> > & drop support. The right way should be implementing QAbstractItemModel for 
> > the queue (with drag&drop methods) and then the GUI would automatically 
> > support it with none or minimal changes. Similar to what Bart said, we 
> > prefer using the .ui files as much as possible. Also, when the 
> > QAbstractItemModel approach is used, there should be no need to split the 
> > patch into 2 review requests.

I didn't even consider what the patch is actually supposed to do. Not 
completely sure that a QAIM subclass is the best option. D'n'D for moving is a 
bit harder then regular dropMimeData. But a model for a list of simple list of 
tracks that can be ordered like that could be reused a lot, even after the big 
PlaylistQueue refactor.


- Bart


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


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 108905: Move UI code of PlaylistQueueEditor from .ui to .cpp file

[Bart Cerneels]

> On Feb. 11, 2013, 5:40 p.m., Bart Cerneels wrote:
> > 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.

http://qt-project.org/doc/qt-4.8/designer-using-custom-widgets.html


- Bart


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


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

[Bart Cerneels]

---
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 108905: Move UI code of PlaylistQueueEditor from .ui to .cpp file

[Bart Cerneels]

---
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 107473: Changes in processing playlist files

[Bart Cerneels]

> On Feb. 9, 2013, 12:55 a.m., Matěj Laitl wrote:
> > src/core-impl/playlists/types/file/PlaylistFile.h, lines 71-73
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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 bbce

Re: Review Request: Changes in processing playlist files

[Bart Cerneels]

---
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: Changes in processing playlist files

[Bart Cerneels]

> 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/playlist

Re: Review Request: Changes in processing playlist files

[Bart Cerneels]

---
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: Fix one small memoy leak when switch track in amarok

[Bart Cerneels]

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

Ship it!


I don't know what that code is supposed to do, but if it's executed on each 
track change it likely is a real leak. Test if it doesn't cause dangling 
pointer crashes and ship it.

- Bart Cerneels


On Dec. 31, 2012, 2:06 p.m., Jekyll Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108050/
> ---
> 
> (Updated Dec. 31, 2012, 2:06 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> ---
> 
> I notice this when invesigating bug 298627. Amarok still leaks memory for me 
> , but at least it seems to leaks less now.
> 
> 
> Diffs
> -
> 
>   src/playlist/PlaylistBreadcrumbItemSortButton.cpp 95dba20 
> 
> Diff: http://git.reviewboard.kde.org/r/108050/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jekyll Wu
> 
>

___
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

[Bart Cerneels]

---
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: Playlists::Playlist: add metadataChanged() to observer, clean-up, docs

[Bart Cerneels]

---
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: Review Request: Playlists::Playlist: add metadataChanged() to observer, clean-up, docs

[Bart Cerneels]

---
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: Changes in processing playlist files

[Bart Cerneels]

---
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.git&a=blob&h=3eda255d944f61c3f975031b69b342cf0899fc2b&hb=57e5bf466b364a0e086000b2d24f00e1e336b22e&f=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: Changes in processing playlist files

[Bart Cerneels]

> 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.
> 
>

Re: Review Request: Changes in processing playlist files

[Bart Cerneels]

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


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.


src/core-impl/playlists/types/file/LazyPlaylistFile.cpp
<http://git.reviewboard.kde.org/r/107473/#comment17304>

I'm surprised your compiler does not trip up on this. Better remove the ';' 
to be compatible with the less forgiving compilers out there, like MSVC.



src/core-impl/playlists/types/file/LazyPlaylistFile.cpp
<http://git.reviewboard.kde.org/r/107473/#comment17305>

downloadPlaylist just needs to go, it's legacy that I just didn't remove 
yet. I don't think it's still used. If you do find a use case from it, put the 
loading outside of the Playlist code and in the user itself.



src/core-impl/playlists/types/file/PlaylistFile.h
<http://git.reviewboard.kde.org/r/107473/#comment17306>

This comment has not been valid for a while. loading from a stream just 
makes tracks available in *this* playlist, doesn't add to the current playlist 
(a.k.a. the Playback Queue).
Just remove it and write new docs once you know what they need to do.



src/core-impl/playlists/types/file/PlaylistFile.h
<http://git.reviewboard.kde.org/r/107473/#comment17307>

I don't think we should have load() in the base class. When loading a 
stream one needs to know what type of playlist is getting loaded, hence need to 
use one of the specialized PlaylistFile classes (M3U, XSPF, PLS, ...). 
Polymorphism is not required nor useful here.



src/core-impl/playlists/types/file/xspf/XSPFPlaylist.h
<http://git.reviewboard.kde.org/r/107473/#comment17308>

This needs to be removed as mentioned earlier. Implement the loading in the 
user or PlaylistManager (might already be).



src/core-impl/playlists/types/file/xspf/XSPFPlaylist.cpp
<http://git.reviewboard.kde.org/r/107473/#comment17309>

This is not good. There should be a default constructor that does minimal 
setup. Move the DOM creation to the save() function instead or create it on 
demand.



src/core-impl/playlists/types/file/xspf/XSPFPlaylist.cpp
<http://git.reviewboard.kde.org/r/107473/#comment17310>

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.


- 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 &am

Re: Access data for the new server needed

[Bart Cerneels]

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  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

Re: Nepomuk Collection

[Bart Cerneels]

On Fri, Nov 9, 2012 at 4:35 PM, Matěj Laitl  wrote:
> On 9. 11. 2012 Phalgun Guduthur wrote:
>> Hello all
>
> Hi,
>
>> After a period of inactivity, I want to continue working on the Nepomuk
>> plugin for Amarok and implement a few more features before it is shipped.
>
> Good!
>
>> These are the major problems of the plugin currently -
>>
>>- The collection takes some time to load  (10s for 2000 tracks). This is
>>because of the use of MemoryCollection.
>>To write my own QueryMaker, I need considerable amout of time as I'm
>>completely clueless about it. Any help on how to go about this would be
>>great.
>>Stecchino once mentioned to have a look at UPnP collections
>>implementation.
>
> Yes, and also the SqlQueryMaker. The idea is to build the SPARQL query in the
> calls like addMatch(), addFilder() etc, and to run it asynchronously in the
> run method. You'll probably need a cache of "live" NepomukMeta entities (a map
> from Nepomuk URIS to Meta::* instances), something like SqlRegistry, although
> hopefully not that complicated.
>
>>- After a restart of Amarok, the tracks in the playlist view get fetched
>>from the Local Collection instead of Nepomuk collection. This is because
>>the Nepomuk collection slow loading of tracks.
>>
>>Is there a way to delay the fetching of meta data for tracks in the
>>playlist view until the Nepomuk collection is fully loaded?
>>The function Playlist::Model::metadataChanged(Meta::TrackPtr) must be
>>delayed.
>
> Probably not. There are more flaws in our restore-tracks-from-urls code, so it
> maybe needs to be reworked. On the other hand, after you have your own
> QueryMaker, only thing a Nepomuk::Tracks needs to store will be the Nepomuk
> URI, other fields would be fetched using Nepomuk on demand. (perhaps with
> caching?)
>
> Then you can create Nepomuk::Tracks really quickly in trackForUirUrl() and the
> problem goes away.

Some more details: Tracks in the playlist are proxy loaded. This means
the tag values are filled in from data in current.xspf already and at
the same time a job is started looking for the actual track in
collections. If a new collection is added it also attempts to resolve
the track. So if your nepomuk collection starts late, it should still
work.
All this is theoretical architecture though and it's likely bug or
missing implementation break this behaviour, like Matêj mentions.

>
>>- Implementing Nepomuk::ResourceWatcher, so that tracks when changed by
>>other applications are reflected immediately in Amarok. This includes
>>addition of new tracks, deletion, change in metadata etc.
>>But this needs the code to be ported to Nepomuk2 ( The newer, sleeker
>>API of Nepomuk ). I'm finding it tough to get Nepomuk2 work on my
>> machine.
>
> Yes, please do the porting ASAP, reportedly it is mainly just a namespace
> change. Connecting Nepomuk::ResourceWatcher to Track::metadataChanged() should
> be trivial.
>
>>- Write back support - let the user edit meta data of his tracks through
>>Amarok. Right now the edit fields are all grayed out.
>
> Yep. I suggest you assign this a lower priority than the NepomukQueryMaker
> though. Not strictly needed for Amarok 2.7, where we can label Nepomuk
> collection as "technology preview" if it lacks iportant features.
>
>>- Support for creating playlists
>
> Nice, again lower priority than QueryMaker IMO, not needed for Amaork 2.7.
>
>>- Support for storing scores of tracks - needs ontology support which is
>>a whole different thing.
>
> Score is IMO much less used than rating, first & last play, so low prio from 
> my
> PoV.
>
>> I write this seeking any sort of feedback on the above problems either on
>> how to tackle them or their importance.
>>
>> What do you think is the most important feature that should be implemented
>> before the plugin is shipped?
>
> QueryMaker and related refactor, if you think you can manage it by the end of
> November.
>
> Cheers, high five and good luck,
> 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: GPL-incompatibility of Spotify's libspotify API ToU (was Re: Getting a Spotify Premium account for Amarok)

[Bart Cerneels]

On Mon, Nov 12, 2012 at 9:02 PM, Bradley M. Kuhn
 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

Fwd: GPL-incompatibility of Spotify's libspotify API ToU (was Re: Getting a Spotify Premium account for Amarok)

[Bart Cerneels]

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 
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 , ama...@sfconservancy.org
Cc: Tony Sebro 


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.  

Fwd: FOSDEM CrossDesktop DevRoom 2013 - Call for Talks

[Bart Cerneels]

-- Forwarded message --
From: Pau Garcia i Quiles 
Date: Wed, Oct 31, 2012 at 5:50 PM
Subject: FOSDEM CrossDesktop DevRoom 2013 - Call for Talks
To: kde-de...@kde.org, kdelibs , KDE on
Windows , Calligra Suite developers and users
mailing list 


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

[Bart Cerneels]

---
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: Fix DAAP collections published by Rhythmbox not showing up in Amarok.

[Bart Cerneels]

---
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: Review Request: Refactoring of Collection config UI code

[Bart Cerneels]

---
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: [UI] QML CV usability feedback

[Bart Cerneels]

On Fri, Sep 28, 2012 at 3:47 PM, Thomas Pfeiffer  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

Re: Review Request: phonon phive core frontend api

[Bart Cerneels]

> 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

[Bart Cerneels]

---
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/AbstractAudi

[amarok] src: Give proper place for inactive authors.

[Bart Cerneels]

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", abo

Re: Authorship

[Bart Cerneels]

On Mon, Sep 24, 2012 at 7:31 PM, Jeff Mitchell  wrote:
> On 09/24/2012 09:51, Valorie Zimmerman wrote:
>>
>> On Sat, Sep 22, 2012 at 11:17 PM, Bart Cerneels
>>  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

Re: Review Request: MetaFile::Track: only provide statistics stored in tags

[Bart Cerneels]

---
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: Status update: KIO-MTP

[Bart Cerneels]

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  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: Status update: KIO-MTP

[Bart Cerneels]

On Sun, Sep 23, 2012 at 11:55 PM, Philipp Schmidt  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: Authorship

[Bart Cerneels]

On Sun, Sep 23, 2012 at 10:49 AM, Alex Fiestas  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

Authorship

[Bart Cerneels]

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=dorem&remaction=run&namedcmd=Amarok%3A%20regression&sharer_id=70102&list_id=224345
Amarok release blockers:
https://bugs.kde.org/buglist.cgi?cmdtype=dorem&remaction=run&namedcmd=Amarok%3A%20release_blocker&sharer_id=70102&list_id=224346
Amarok most hated:
https://bugs.kde.org/buglist.cgi?cmdtype=dorem&remaction=run&namedcmd=Amarok%3A%20most%20hated&sharer_id=70102&list_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: Review Request: add cli argument to activate phonon debugging

[Bart Cerneels]

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

Ship it!


Looks good. I didn't realize that it was separate from the regular debug. Less 
risk of it getting lost in the noise but might miss EngineController context. 
So I agree with matej's suggestion. Ship this one already though.

- Bart Cerneels


On Sept. 12, 2012, 11:35 a.m., Harald Sitter wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106431/
> ---
> 
> (Updated Sept. 12, 2012, 11:35 a.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> ---
> 
> add new argument to activate audio (phonon) debugging
> 
> it simply injects the appropriate phonon 4.6+ environment variables in 
> main.cpp
> 
> this is very helpful as 99% of all phonon bug reports get a first comment 
> that asks the reporter to get an actual phonon relevant debug log, by making 
> this simpler we can help reporters get relevant data quicker and since it is 
> visually exposed they may even provide initial report logs with phonon debug 
> active thus making bug triage twice as fast
> 
> 
> Diffs
> -
> 
>   src/App.cpp 9b8bbe51e6fb93aef2e528b57ad3f5056f09c85a 
>   src/main.cpp 4a48233ece598c90c6dd080473ec23a91ab2fe05 
> 
> Diff: http://git.reviewboard.kde.org/r/106431/diff/
> 
> 
> Testing
> ---
> 
> [x] start with --debug-audio
> [x] start with --debug
> [x] start with --debug --debug-audio
> [x] start without any argument
> 
> 
> Thanks,
> 
> Harald Sitter
> 
>

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

Re: TODO for 2.7: android MTP

[Bart Cerneels]

On Mon, Sep 10, 2012 at 9:56 PM, Philipp Schmidt  wrote:
> Am Montag, 10. September 2012, 17:06:57 schrieb Matěj Laitl:
>> On 10. 9. 2012 Bart Cerneels wrote:
>> > 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.
>>
>> Yeah, I've thought about it. MTP collection rewrite should be a rather
>> straightforward adaptation of the iPod collection.
>
> I would volunteer, since I am already a little familiar with libmtp due to my
> (currently stagnant work, kudos to my master thesis) on the kio-mtp slave.
> Since I will be travelling after completing said thesis I would only be able
> to start around Christmas.
>
> One Problem that I see is that many Android Devices as of now do not support
> the tracks API (or at least not properly). So one would have to work around
> that, as that should be the preferred way to access misuci files on a MTP
> device.
>
> Cheers,
> Philipp

I would prefer if we got it in 2.7, which would ideally be released
around Christmas. I wonder if between the 3 of us there is enough
spare time and motivational energy to get it done soon. Matěj: how
about we spend a day on this in Randa? I indeed have an old-ish
android lying around, but I'm not sure this one does MTP like the 4.0
devices that are currently sold. I'll check later.
___
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel

Re: API proposal: Phonon BackendInfo

[Bart Cerneels]

On Sun, Sep 9, 2012 at 3:04 PM, Harald Sitter  wrote:
> On Sun, Sep 9, 2012 at 2:14 PM, Matěj Laitl  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

TODO for 2.7: android MTP

[Bart Cerneels]

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: Review Request: Use multiple inheritance to get an implementation for statistics handling

[Bart Cerneels]

 
> to be implemented for a full statistics provider.
> 
> It wouldn't be bold, it would be silly. We for sure don't want our core 
> to depend on sql. Moreover, I don't want any code (apart from SqlCollection) 
> to depend on sql.
> 
> > 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.
> 
> My proposal (partially implemented in my gsoc branch, rest is coming) is 
> to move all the setRating(), setScore() and add setFirst/LastPlayed(), 
> setPlayCount() to EditCapability (or rather the interface that will replace 
> it). Other option is to add extra WriteStatsInterface for the 5 methods alone 
> and decide wheter the getters should be in Meta::Track or in StatsInterface.
> 
> > 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.
> 
> 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. PermanentUrlStatisticsProvide

Re: Review Request: Use multiple inheritance to get an implementation for statistics handling

[Bart Cerneels]

> 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.
> 
>  

Re: Review Request: Fix DAAP collections published by Rhythmbox not showing up in Amarok.

[Bart Cerneels]

---
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: Fix DAAP collections published by Rhythmbox not showing up in Amarok.

[Bart Cerneels]

> 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: Fix DAAP collections published by Rhythmbox not showing up in Amarok.

[Bart Cerneels]

---
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 Permission denied when playing a track from a Rhythmbox DAAP collection

[Bart Cerneels]

---
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.

[Bart Cerneels]

---
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: GSoC : Nepomuk plugin for Amarok

[Bart Cerneels]

> On Sept. 4, 2012, 6:11 p.m., Matěj Laitl wrote:
> > src/core-impl/collections/nepomukcollection/NepomukCollection.h, line 42
> > 
> >
> > Nicpick: instead -> "in addition to"

It actually is the goal of the nepomuk collection to replace the main 
MySql-embedded default. Even though that might not be possible currently and an 
SQL storage will always be needed.
I would be happy if we ever get there, with very good performance and 
easy-of-use.

For now I agree with this nitpick, but you could add it's the goal to actually 
be a replacement.


> On Sept. 4, 2012, 6:11 p.m., Matěj Laitl wrote:
> > src/core-impl/collections/nepomukcollection/NepomukCollection.cpp, line 124
> > 
> >
> > This works for now, but cannot one configure which folders Nepomuk 
> > indexes? You should use these for this call. (not a merge-blocker)
> 
> Edward Hades Toroshchin wrote:
> Phalgun, add a // TODO comment here, so that this can be investigated in 
> the future.

You shouldn't try to change nepomuk's indexed folders. Instead create an opt-in 
or opt-out list for the content that it does need to include in the collection. 
i.e. add an additional query join (or equivalent, I'm not well versed in 
SPARQL) including/excluding based on file location.


- Bart


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


On Sept. 4, 2012, 4:51 p.m., Phalgun Guduthur wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106042/
> ---
> 
> (Updated Sept. 4, 2012, 4:51 p.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. 
> 
> ===
> Commit messages : 
> 
> 
> Code formatting changes
> 
> 
> Removed unnecessary documentation
> 
> 
> Set right Logger
> 
> The logger shows the progres of the collection updation correctly now.
> The number of tracks were calculated using a small sparql query
> 
> removed local variable
> 
> 
> Removed secondary Nepomuk meta hashmaps.
> 
> Left all the duplicate object checking to MapChanger.
> The CMakeLists logs the presence of Nepomuk
> Initialized the labellists in NepomukTrack constructor
> 
> Code formatting changes
> 
> 
> fixing strohels nitpicks
> 
> 
> Code formatting changes
> 
> 
> vHanda nitpicks in review board
> 
> 1. Moved soprano model instanciation after query string construction
> 2. QString -> QString::fromLatin1() for sparql query string
> 3. Nested album gains into album sub query
> 4. removed extraneous code that cropped up. (nao::has)
> 5. revamped tags query as I was only querying uri and not names of tags
> 
> If nepomuk is not found, the user gets a logger message informing the same
> 
> 
> Code formatting changes to fit 90chars per line
> 
> 
> The progress bar says which track is being updated
> 
> 
> The Includes order for NepomukCollection was fixed
> 
> 
> Make Nepomuk dependency optional
> 
> 
> Modified debug string to be concise and meaningful
> 
> 
> Formatting changes and documentation - part 2
> 
> Also deleted the .autosave file that creeped along the other commits
> 
> Documentation + Formatting - Part 1 ( meta classes )
> 
> 
> Added year implementation
> 
> The sparql metadata query is complete with the major metadata.
> Lyrics and Album art is yet to be done.
> 
> Code builds.
> 
> Code still in Nepomuk 1
> 
> Implemented label metadata ( tags ) in sparql enumeration
> 
> 
> Code formatting changes
> 
> 
> SPARQL query implemented instead of Nepomuk::Resource API
> 
> Hefty changes in the core of the background job of the query of tracks
> and subsequent enumeration.
> 
> Initially a manual sparql query which queries for all properties of the
> track and other meta data like artist, album etc.
> 
> The meta HashMaps are changed to incorporate Resource URIs and Labels
> instead of resources themselves, as keys.
> 
> Year and labels ( tags in nepomuk ) are yet t

Re: Review Request: Extend the scope of the playground

[Bart Cerneels]

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


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.

- Bart Cerneels


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: Extend the scope of the playground

[Bart Cerneels]

> 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: Use multiple inheritance to get an implementation for statistics handling

[Bart Cerneels]

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


Code complexity reduction sounds good to me. But I'll have to look into 
StatisticsProvider in general before I can comment further.

- Bart Cerneels


On Aug. 30, 2012, 12:03 p.m., Ralf Engels wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106276/
> ---
> 
> (Updated Aug. 30, 2012, 12:03 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> ---
> 
> This is try number three to get a consistent implementation of statistics 
> handling.
> This time with multiple inheritance.
> 
> This change will provide a complete implementation of statistics for a couple 
> of collections.
> That means that collections will get missing functional implementations e.g. 
> setPlaycount.
> 
> For the future we will need a single sql table for all statistics and 
> coverage for the ugly cases which are:
> - PodcastMeta which currently is not covered.
> - SqlMeta which uses it's own table and has problems with restoring 
> statistics.
> - A couple of collections that create tracks where the url is changed later 
> on.
> 
> Oh, and it's 500 lines less code.
> 
> 
> Diffs
> -
> 
>   src/core-impl/collections/audiocd/AudioCdMeta.h 73ac547 
>   src/core-impl/collections/audiocd/AudioCdMeta.cpp d767125 
>   src/core-impl/collections/daap/DaapMeta.h 5278b57 
>   src/core-impl/collections/daap/DaapMeta.cpp a2429d7 
>   src/core-impl/collections/db/sql/CapabilityDelegateImpl.cpp e344e0f 
>   src/core-impl/collections/db/sql/SqlMeta.h 4450dab 
>   src/core-impl/collections/db/sql/SqlMeta.cpp c05e563 
>   src/core-impl/collections/db/sql/SqlRegistry.h f64b30c 
>   src/core-impl/collections/db/sql/SqlRegistry.cpp 1444f94 
>   src/core-impl/collections/db/sql/SqlRegistry_p.h a7d4e26 
>   src/core-impl/collections/db/sql/SqlRegistry_p.cpp 68cc871 
>   src/core-impl/collections/mediadevicecollection/MediaDeviceMeta.h 2d0706e 
>   src/core-impl/collections/mediadevicecollection/MediaDeviceMeta.cpp 3946b8f 
>   src/core-impl/collections/playdarcollection/PlaydarMeta.h 93d1c2e 
>   src/core-impl/collections/playdarcollection/PlaydarMeta.cpp c9b07f3 
>   src/core-impl/collections/proxycollection/ProxyCollectionMeta.h e5d7266 
>   src/core-impl/collections/proxycollection/ProxyCollectionMeta.cpp 091f5f6 
>   src/core-impl/collections/support/MemoryMeta.h 675cf42 
>   src/core-impl/collections/upnpcollection/UpnpMeta.h feabc1e 
>   src/core-impl/collections/upnpcollection/UpnpMeta.cpp 5ffee54 
>   src/core-impl/meta/file/File.h df96a2a 
>   src/core-impl/meta/file/File.cpp 46f672b 
>   src/core-impl/meta/file/File_p.h f756074 
>   src/core-impl/meta/multi/MultiTrack.h 63a8651 
>   src/core-impl/meta/proxy/MetaProxy.h 0579d08 
>   src/core-impl/meta/proxy/MetaProxy.cpp 51299f8 
>   src/core-impl/meta/stream/Stream.h 8b64d89 
>   src/core-impl/meta/stream/Stream.cpp 3eb54c7 
>   src/core-impl/meta/stream/Stream_p.h 390109a 
>   src/core-impl/meta/timecode/TimecodeMeta.h ceb4d66 
>   src/core-impl/meta/timecode/TimecodeMeta.cpp f85476d 
>   src/core-impl/statistics/providers/tag/TagStatisticsProvider.h df79a00 
>   src/core-impl/statistics/providers/url/PermanentUrlStatisticsProvider.h 
> 7b6b3bb 
>   src/core-impl/statistics/providers/url/PermanentUrlStatisticsProvider.cpp 
> 9855954 
>   src/core/CMakeLists.txt 29a215e 
>   src/core/capabilities/Capability.h 8ec6d16 
>   src/core/capabilities/StatisticsCapability.h 690ef12 
>   src/core/capabilities/StatisticsCapability.cpp 34af392 
>   src/core/meta/Meta.h bf2184b 
>   src/core/meta/Meta.cpp df45908 
>   src/core/podcasts/PodcastMeta.h 0cfaea1 
>   src/core/statistics/StatisticsProvider.h 6f54912 
>   src/core/statistics/StatisticsProvider.cpp 0b5a788 
>   src/databaseimporter/amarok14/FastForwardWorker.cpp 56e319b 
>   src/databaseimporter/itunes/ITunesImporterWorker.cpp e5ad5e6 
>   src/services/ServiceMetaBase.h 793eb9e 
>   src/services/ServiceMetaBase.cpp 039146b 
>   src/services/magnatune/MagnatuneMeta.cpp 523617b 
>   tests/core-impl/collections/db/sql/TestSqlScanManager.cpp c449216 
>   tests/core-impl/meta/file/TestMetaFileTrack.cpp 7549a58 
>   tests/core/meta/TestMetaTrack.cpp 0adb763 
>   tests/mocks/MetaMock.h b478c87 
>   tests/mocks/MockTrack.h 57bc344 
> 
> Diff: http://git.reviewboard.kde.org/r/106276/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ralf Engels
> 
>

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

Re: Review Request: GSoC : Nepomuk plugin for Amarok

[Bart Cerneels]

> On Aug. 20, 2012, 10:46 p.m., Matěj Laitl wrote:
> > src/core-impl/collections/nepomukcollection/NepomukCollection.cpp, line 281
> > 
> >
> > 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/c

Re: Review Request: GSoC : Nepomuk plugin for Amarok

[Bart Cerneels]

> On Aug. 20, 2012, 10:46 p.m., Matěj Laitl wrote:
> > src/core-impl/collections/nepomukcollection/NepomukConstructMetaJob.h, line 
> > 71
> > 
> >
> > 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 perfect

Re: [amarok] src/services/amazon: Detect Amazon country automatically.

[Bart Cerneels]

On Sat, Aug 18, 2012 at 12:13 PM, Matěj Laitl  wrote:
> On 17. 8. 2012 Edward Toroshchin wrote:
>> > Don't take it personally, but if you don't revert I'm going to.
>>
>> Don't take it personally, but I believe you need at least someone else
>> to back you up on this to revert anything. Currently, no one except us
>> has expressed their point of view :).
>
> My point of view:
>  * no Internet Service (in Amarok sense) should pop-up a dialog during first
> start, worse if it is modal
>  * no Internet Service should even be loaded (and thus sending any data) until
> necessary. They should be loaded when user first explicitly clicks on the 
> store
> in Internet Services. Programatically, the services could be "registered" at
> startup - their object created. "Loading" a service could be calling its
> init() method or similar (I think that many services have a polish() method
> that's really abused right now). Before "loading", the services should have an
> opportunity to show a config dialog (preferably embedded in the view) that
> could request consent for sending data.
>
> For scripted services it would be a bonus point if the actual script (and Qt
> Script Bindings) could be loaded as late as in the init() method - registering
> a service should only require its icon, name & description. I don't know how
> easily doable this is however.
>

I seem to be in full agreement with the above, but it's probably best
if we have a session about Services at Randa. I hope all will
contribute some input pre-Randa [1] and if you can't make it there,
please try to join via video chat.

[1] http://amarok.kde.org/wiki/Proposals/ServicesReArchitecture
___
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

[Bart Cerneels]

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


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.

- Bart Cerneels


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: magnatune: first update related tweaks

[Bart Cerneels]

> 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: GSoC update from Riccardo - his KMail is b0rked again

[Bart Cerneels]

On Mon, Aug 20, 2012 at 2:53 PM, Matěj Laitl  wrote:
> On 20. 8. 2012 Teo Mrnjavac wrote:
>> http://wstaw.org/m/2012/08/20/plasma-desktopAH1228.png
>
> Good progress, but again I have to repeat myself: I will oppose merging any
> rewrite that will introduce significant feature-loss regressions. Sorry.
>
> Such code should live in a branch until it has general feature parity (in this
> case: functionality of all meaningful applets that work reliably currently
> should be provided by the new CV: wikipedia, labels, lyrics, similar artists,
> tabs, events, photos...), as for example my iPod Collection rewrite did. Note
> that code living in a branch isn't something bad, it receives proper testing
> and review from interested parties (e.g. me), but we shouldn't disappoint
> uninterested parties with regressions.
>
> Teo, please forward this mail to Riccardo if he cannot read the list. (even if
> your opinion is different, this is just a notice of my point of view)
>
> Cheers and keep up the good work,
> Matěj

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

Re: Review Request: {Smb, Nfs}DeviceHandler: use Solid instead of KMountPoint

[Bart Cerneels]

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


I'll test if this solves my blocking problem when mounted NFS is inaccessible 
as well.

- Bart Cerneels


On Aug. 19, 2012, 10:52 p.m., Matěj Laitl wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106094/
> ---
> 
> (Updated Aug. 19, 2012, 10:52 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> ---
> 
> {Smb,Nfs}DeviceHandler: use Solid instead of KMountPoint
> 
> KMountPoint seems to be unreliable and not working at least for smb
> shares. Use Solid's NetworkShare for the same task, which is much more
> *cough* ... solid. :-)
> 
> Server name & share name extraction changed a bit, but I've tested it
> to give the same names as the old implementation. (not for some corner
> cases, but old behaviour could be considered bogus there)
> 
> BUG: 302837
> FIXED-IN: 2.7
> REVIEW: 106094
> 
> 
> This addresses bug 302837.
> https://bugs.kde.org/show_bug.cgi?id=302837
> 
> 
> Diffs
> -
> 
>   src/core-impl/collections/db/sql/device/nfs/NfsDeviceHandler.cpp 
> 74e19bc643d15f0f1473da7a6ca52a31583ffd83 
>   src/core-impl/collections/db/sql/device/smb/SmbDeviceHandler.cpp 
> 12d6c2a051f58579f1bdc69abeb7a1b0a049c5fa 
> 
> Diff: http://git.reviewboard.kde.org/r/106094/diff/
> 
> 
> Testing
> ---
> 
> Medium, needs confirmation from bug reporter.
> 
> 
> Thanks,
> 
> Matěj Laitl
> 
>

___
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)

[Bart Cerneels]

> 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

Amarok startup blocks when NFS mounts are not accessible

[Bart Cerneels]

first on: NfsDeviceHandlerFactory::canHandle()

NfsDeviceHandler.cpp:153 KMountPoint::Ptr m =
KMountPoint::currentMountPoints().findByPath( access->filePath() );


Then after forcefully unmounting the shares, m is valid and
canHandle() returns true, then it blocks on the next call to
NfsDeviceHandler where exactly the same function is excecuted:

MountPointManager.cpp:503 DeviceHandler *handler =
factory->createHandler( device, udi, m_storage );

This might have been a fluke caused by unmouting while it was blocked
on the previous


Seems MountPointManager is another one of those rotten codebases that
need review and cleanup.


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

[amarok] src: Fix typo in plugin query string. Missing space.

[Bart Cerneels]

Git commit 66d5746e5086ac6667ebc8ad255cf6d6df9f6aab by Bart Cerneels.
Committed on 16/08/2012 at 08:57.
Pushed by shanachie into branch 'master'.

Fix typo in plugin query string. Missing space.

Not sure how/why this worked before. Can anyone familiar with KDE plugin
explain. Are we even doing it correctly?

CCMAIL:amarok-devel@kde.org

M  +2-1src/PluginManager.cpp

http://commits.kde.org/amarok/66d5746e5086ac6667ebc8ad255cf6d6df9f6aab

diff --git a/src/PluginManager.cpp b/src/PluginManager.cpp
index d5dc83c..baa9c2e 100644
--- a/src/PluginManager.cpp
+++ b/src/PluginManager.cpp
@@ -221,7 +221,8 @@ Plugins::PluginManager::findAllPlugins()
 {
 DEBUG_BLOCK
 QString query = QString::fromLatin1( "[X-KDE-Amarok-framework-version] == 
%1"
- "and [X-KDE-Amarok-rank] > 0" ).arg( 
s_pluginFrameworkVersion );
+ " and [X-KDE-Amarok-rank] > 0" )
+.arg( s_pluginFrameworkVersion );
 
 KConfigGroup pluginsConfig = Amarok::config(QLatin1String("Plugins"));
 KService::List services = KServiceTypeTrader::self()->query( 
"Amarok/Plugin", query );

___
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

[Bart Cerneels]

---
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)

[Bart Cerneels]

---
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: GSoC report: Integrate Spotify into Amarok #4 (squashed commits, recent on top)

[Bart Cerneels]

> On Aug. 13, 2012, 4:43 p.m., Edward Hades Toroshchin wrote:
> > src/core-impl/collections/spotifycollection/SpotifyCollection.cpp, line 57
> > 
> >
> > 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: [amarok/v2.6.0] src: Bump plugin version for the 2.6 release to 69

[Bart Cerneels]

On Mon, Aug 13, 2012 at 1:14 PM, Matěj Laitl  wrote:
> Git commit 7350833639f56105c333d59eed1b821b3dfd06fb by Matěj Laitl.
> Committed on 11/08/2012 at 11:07.
> Pushed by laitl into tag 'v2.6.0'.
>
> Bump plugin version for the 2.6 release to 69
>
> ...or, is it really needed to sing this song when all known Amarok
> plugins come with Amarok itself?

I don't think you needed to do this even now. Theoretically only from
minor release to minor release (where we can assume API/ABI changes)
this is needed. I've bumped the plugin version prior to beta 1.

Since we won't commit to stable API's anyway, this might indeed be a
waste of time, unless the KDE plugin system needs it somehow.

>
> M  +1-1src/PluginManager.cpp
> M  +1-1
> src/core-impl/collections/audiocd/amarok_collection-audiocdcollection.desktop
> M  +1-1
> src/core-impl/collections/daap/amarok_collection-daapcollection.desktop
> M  +1-1
> src/core-impl/collections/db/sql/device/massstorage/amarok_device_massstorage.desktop
> M  +1-1
> src/core-impl/collections/db/sql/device/nfs/amarok_device_nfs.desktop
> M  +1-1
> src/core-impl/collections/db/sql/device/smb/amarok_device_smb.desktop
> M  +1-1
> src/core-impl/collections/db/sql/mysqlecollection/amarok_collection-mysqlecollection.desktop
> M  +1-1
> src/core-impl/collections/db/sql/mysqlservercollection/amarok_collection-mysqlservercollection.desktop
> M  +1-1
> src/core-impl/collections/ipodcollection/amarok_collection-ipodcollection.desktop
> M  +1-1
> src/core-impl/collections/mediadevicecollection/amarok_collection-mediadevicecollection.desktop
> M  +1-1
> src/core-impl/collections/mtpcollection/amarok_collection-mtpcollection.desktop
> M  +1-1
> src/core-impl/collections/nepomukcollection/amarok_collection-nepomukcollection.desktop
> M  +1-1
> src/core-impl/collections/playdarcollection/amarok_collection-playdarcollection.desktop
> M  +1-1
> src/core-impl/collections/umscollection/amarok_collection-umscollection.desktop
> M  +1-1
> src/core-impl/collections/upnpcollection/amarok_collection-upnpcollection.desktop
> M  +1-1src/services/amazon/amarok_service_amazonstore.desktop
> M  +1-1src/services/ampache/amarok_service_ampache.desktop
> M  +1-1src/services/gpodder/amarok_service_gpodder.desktop
> M  +1-1src/services/jamendo/amarok_service_jamendo.desktop
> M  +1-1src/services/lastfm/amarok_service_lastfm.desktop
> M  +1-1src/services/magnatune/amarok_service_magnatunestore.desktop
> M  +1-1src/services/mp3tunes/amarok_service_mp3tunes.desktop
> M  +1-1src/services/opmldirectory/amarok_service_opmldirectory.desktop
>
> http://commits.kde.org/amarok/7350833639f56105c333d59eed1b821b3dfd06fb
>
> diff --git a/src/PluginManager.cpp b/src/PluginManager.cpp
> index 5124c38..d5dc83c 100644
> --- a/src/PluginManager.cpp
> +++ b/src/PluginManager.cpp
> @@ -35,7 +35,7 @@
>
>  #include 
>
> -const int Plugins::PluginManager::s_pluginFrameworkVersion = 68;
> +const int Plugins::PluginManager::s_pluginFrameworkVersion = 69;
>  Plugins::PluginManager* Plugins::PluginManager::s_instance = 0;
>
>  Plugins::PluginManager*
> diff --git 
> a/src/core-impl/collections/audiocd/amarok_collection-audiocdcollection.desktop
>  
> b/src/core-impl/collections/audiocd/amarok_collection-audiocdcollection.desktop
> index e4c0859..0c14a90 100644
> --- 
> a/src/core-impl/collections/audiocd/amarok_collection-audiocdcollection.desktop
> +++ 
> b/src/core-impl/collections/audiocd/amarok_collection-audiocdcollection.desktop
> @@ -102,7 +102,7 @@ ServiceTypes=Amarok/Plugin
>
>  X-KDE-Amarok-authors=Nikolaj Hald Nielsen
>  X-KDE-Amarok-email=nhnfreespi...@gmail.com
> -X-KDE-Amarok-framework-version=68
> +X-KDE-Amarok-framework-version=69
>  X-KDE-Amarok-name=audiocd-collection
>  X-KDE-Amarok-plugintype=collection
>  X-KDE-Amarok-rank=100
> diff --git 
> a/src/core-impl/collections/daap/amarok_collection-daapcollection.desktop 
> b/src/core-impl/collections/daap/amarok_collection-daapcollection.desktop
> index 19ba276..3ef24bc 100644
> --- a/src/core-impl/collections/daap/amarok_collection-daapcollection.desktop
> +++ b/src/core-impl/collections/daap/amarok_collection-daapcollection.desktop
> @@ -116,7 +116,7 @@ ServiceTypes=Amarok/Plugin
>
>  X-KDE-Amarok-authors=Maximilian Kossick
>  X-KDE-Amarok-email=maximilian.koss...@googlemail.com
> -X-KDE-Amarok-framework-version=68
> +X-KDE-Amarok-framework-version=69
>  X-KDE-Amarok-name=daap-collection
>  X-KDE-Amarok-plugintype=collection
>  X-KDE-Amarok-rank=100
> diff --git 
> a/src/core-impl/collections/db/sql/device/massstorage/amarok_device_massstorage.desktop
>  
> b/src/core-impl/collections/db/sql/device/massstorage/amarok_device_massstorage.desktop
> index c4fbd61..bf9e467 100644
> --- 
> a/src/core-impl/collections/db/sql/device/massstorage/amarok_device_massstorage.desktop
> +++ 
> b/src/core-impl/

Re: Review Request: GSoC report: Integrate Spotify into Amarok #4 (squashed commits, recent on top)

[Bart Cerneels]

---
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/spotifycol

Re: Review Request: Fix crash when closing Amarok with running CoverFetcher

[Bart Cerneels]

> On Aug. 9, 2012, 10:45 a.m., Matěj Laitl wrote:
> > src/statusbar/CompoundProgressBar.cpp, line 46
> > 
> >
> > 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.

[Bart Cerneels]

> 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: Review Request: Add playlist export action to Playlist Dock save action.

[Bart Cerneels]

---
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: GSoC Report: Integrate Spotify into Amarok

[Bart Cerneels]

On Tue, Jul 31, 2012 at 11:28 PM, Ryan Feng  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.git&a=shortlog&h=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: Review Request: Multiple EngineController and related fixes, incl. fix for release_blocker bug 299890 (squashed commits, recent on top)

[Bart Cerneels]

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

Ship it!


I see no issues. Didn't test, but I assume you did extensively anyway.

Now go and roll 2.6 RC1 tarball or it will never happen!

- Bart Cerneels


On July 24, 2012, 4:07 p.m., Matěj Laitl wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105610/
> ---
> 
> (Updated July 24, 2012, 4:07 p.m.)
> 
> 
> Review request for Amarok and Bart Cerneels.
> 
> 
> Description
> ---
> 
> EngineController: don't do serious work in slotAboutToFinish()
> 
> ...because slotAboutToFinish() may be called twice (or not at all) per
> track by some Phonon backends (hi, vlc) - increase play count rather in
> slotNewTrackPlaying() or in slotFinished(). This also needs to change
> how m_currentTrack is handled, because slotNewTrackPlaying() needs to
> have the old one in m_currentTrack.
> 
> Also, PlaylistActions::requestNextTrack() is changed to be a read-only
> method that shouldn't change playlist state especially when there is no
> next track. PlaylistActions::reflectPlaybackFinished() is introduced to
> do the thing and is called from EngineController::slotFinished(), which
> is a much better place for it than slotAboutToFinish().
> 
> Reporters of CCed bugs, please re-test your bug with this commit
> applied, it is possible it has been resolved by this patch.
> 
> BUG: 299890
> CCBUG: 268892
> CCBUG: 277197
> CCBUG: 302652
> CCBUG: 303580
> CCBUG: 302240
> FIXED-IN: 2.6
> 
> MetaStream: match track by QUrl, not by QString
> 
> Fixes a bug where MetaStram::Track didn't update tags here because
> pretty url was used in signal, but it compared against encoded url
> (the one with %20 instead of spaces).
> 
> MetaStream: big clean-up, implement play statistics methods
> 
>  * remove many unused and useless methods such as setTitle() and
>friends
>  * additionally take genre, comment, track number from Engine
>Controller's signal
>  * implement rating, score, first & last play and play count. These are
>rather debugging tool for EngineController, but they also give you
>nice info on how many songs have played in the stream
>  * no need to reimplement observer pattern, use MetaBase implementation
>  * implement length, EngineController will emit length info soon
> 
> EngineController: introduce trackFinishedPlaying() signal
> 
> Used do call track->finishedPlaying() now, to be used in Last.fm
> scrobbling when it is emitted for streams, too.
> 
> EngineController: reduce code duplication among play() and stop()
> 
> 
> EngineController: rework slotMetaDataChanged(), nearly no functional change
> 
>  * get data from Phonon to meta QVariantMap more intelligently
>  * m_lastTrack and trackChanged had effect only in debug message, ditch
>them
>  * note that m_currentTrack may be inaccurate there
>  * rename isMetadataSpam() to isInRecentMetaDataHistory() to describe
>the functionality better
>  * less debug() spam, specifically be rather quiet for duplicate
>signals from phonon
>  * Phonon doesn't provide track length, but it does provide track
>description that we save as "comments" now.
>  * trackData() method was unused and likely meant for
>slotMetaDataChanged(); ditch it
> 
> 
> This addresses bug 299890.
> https://bugs.kde.org/show_bug.cgi?id=299890
> 
> 
> Diffs
> -
> 
>   ChangeLog 1d4f1e92fb5a033500c0f18d3dca257a89f1a139 
>   src/EngineController.h ad2e0c4a5e7c80c79bf448bf74cd6b52cd1f0ed3 
>   src/EngineController.cpp 83f0a6ed0a92ae992e1809800cee65d9349dc680 
>   src/core-impl/meta/stream/Stream.h 2b0a5824eae49345807eef94a465e133996624a1 
>   src/core-impl/meta/stream/Stream.cpp 
> b0fcfad5808b9ae428cb6612bec1737b3af82d3b 
>   src/core-impl/meta/stream/Stream_p.h 
> 58b715f27cb43d014d5837f1afec9d60cb71cc48 
>   src/playlist/PlaylistActions.h 4f27658f48fa7af65c1fbc26d5c184c3f6070f78 
>   src/playlist/PlaylistActions.cpp 1be1d5af6313eb0058c7c65ba7304fa2bd1c00a1 
> 
> Diff: http://git.reviewboard.kde.org/r/105610/diff/
> 
> 
> Testing
> ---
> 
> Works for me, fixes mentioned bug and more smaller glitches.
> 
> 
> Thanks,
> 
> Matěj Laitl
> 
>

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

Re: Moving Tab-toolbars to the top

[Bart Cerneels]

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  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: Review Request: EngineController: ditch canDecode(), fix supportedMimeTypes(): make it non-static, thread-safe even on first call. (squached patches, recent on top)

[Bart Cerneels]

---
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: 2.6 release and release candidate considerations

[Bart Cerneels]

On Wed, Jul 18, 2012 at 10:28 PM, Matěj Laitl  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: GSoC Report: Integrate Spotify into Amarok

[Bart Cerneels]

On Sat, Jul 14, 2012 at 4:46 PM, Ryan Feng  wrote:
> Hi,
>
>The last report already showed that the Spotify plugin has been loaded
> correctly and the collection is working, but sometimes the search results
> are inserted into the collection but fail showing up in the collection, now
> my next move is to fix that problem.
>
> Here's a list of already known issues and todos:
>
> Slot SpotifyQueryMaker::collectResults() is connected but will never be
> executed, I think it is because the SpotifyQueryMaker is deleted before the
> results are returned. I added addToCollection() in Query::tracksAdded() to
> work around this.  Currently the SpotifyCollection::querymaker() creates and
> returns a new SpotifyQueryMaker object when called, but it's not encouraged
> to do so.
> Track length is always zero
> Track tags are missing after Amarok reload
>
> Todos:
>
> Create a SpotifyQueryMaker object, reuse it and try to fix the problem.
> Fix SpotifyCollection::trackForUrl() to load track tags when starting
> Amarok.
> Create a configuration dialog to input Spotify username and password.
>
> Repos:
> http://goo.gl/ewd1n in the `gsoc-spotify` branch
>
> Diff:
> https://git.reviewboard.kde.org/r/105285/diff/2/
>

gsoc-spotify in that repo and the patch don't match. Could you do a
git push kde:clones/amarok/zhengliangfeng/amarok-spotify.git
gsoc-spotify:gsoc-spotify to make sure it's up to date. If that
command does not work, as on IRC.
___
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel

Re: [amarok] src: EngineController: revive a part of Ralf's patch to fix 4 failing tests

[Bart Cerneels]

I've got a patch ready that removes the codec install code in Amarok.
It's since long ago been implemented in phonon.
I was holding it off till post 2.6 because you never know what it
might break non the less.

Perhaps I should push it and release a second beta just tot test?

On Sat, Jul 14, 2012 at 10:07 PM, Matěj Laitl  wrote:
> Git commit da92d401b629b75ad539ad84263c1745e88e2166 by Matěj Laitl.
> Committed on 14/07/2012 at 22:04.
> Pushed by laitl into branch 'master'.
>
> EngineController: revive a part of Ralf's patch to fix 4 failing tests
>
> Ralf, you were right, I know no sane way of avoiding if( logger )
> there, so un-revert that part of your patch.
>
> M  +12   -4src/EngineController.cpp
>
> http://commits.kde.org/amarok/da92d401b629b75ad539ad84263c1745e88e2166
>
> diff --git a/src/EngineController.cpp b/src/EngineController.cpp
> index 12f60af..00cff1f 100644
> --- a/src/EngineController.cpp
> +++ b/src/EngineController.cpp
> @@ -294,10 +294,18 @@ EngineController::supportedMimeTypes() //static
>  {
>  if ( !installDistroCodec() )
>  {
> -Amarok::Components::logger()->longMessage(
> -i18n( "Phonon claims it cannot play MP3 files. 
> You may want to examine "
> -  "the installation of the backend that phonon 
> uses."
> -  "You may find useful information in the 
> FAQ section of the Amarok Handbook." ), 
> Amarok::Logger::Error );
> +QString text = i18n(
> +"Phonon claims it cannot play MP3 files. You 
> may want to examine "
> +"the installation of the backend that phonon uses."
> +"You may find useful information in the FAQ 
> section of the Amarok Handbook." );
> +// logger is not available at this point in tests; we hesitate 
> to create
> +// logger in 4 tests just because of the next line, so check for 
> it. We cannot
> +// use QApplication::type() != Tty because the tests are 
> GUI-enabled
> +Logger *logger = Amarok::Components::logger();
> +if( logger )
> +logger->longMessage( text, Amarok::Logger::Error );
> +else
> +warning() << text.toLocal8Bit().constData();
>  }
>  mimeTable << "audio/mp3" << "audio/x-mp3";
>  }
___
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel

Re: GSoC update

[Bart Cerneels]

On Fri, Jul 13, 2012 at 11:36 AM, Phalgun Guduthur
 wrote:
> On Fri, Jul 13, 2012 at 1:02 PM, Bart Cerneels 
> wrote:
>>
>> On Wed, Jul 4, 2012 at 4:37 PM, Phalgun Guduthur
>>  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

Re: [Tomahawk Integration] GSoC Report (plus: RFC on git work-flow guideline)

[Bart Cerneels]

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  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.git&a=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.git&a=commit&h=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: Review Request: EngineController: fixes to canDecode() and supportedMimeTypes(): make them non-static, thread-safe even on first call. (squached patches, recent on top)

[Bart Cerneels]

---
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: GSoC update

[Bart Cerneels]

On Wed, Jul 4, 2012 at 4:37 PM, Phalgun Guduthur
 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
___
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel

spotify collection post mid-term TODO

[Bart Cerneels]

* 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

[Bart Cerneels]

On Mon, Jul 9, 2012 at 11:15 PM, Lucas Lira Gomes  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.git&a=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  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

[Bart Cerneels]

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  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: GSoC report: Integrate Spotify into Amarok #3 (squashed commits, recent on top)

[Bart Cerneels]

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


I'm assuming this is mostly code copied and edited (but not completely) from 
the playdar work done by Andy for GSoC 2010.
As far as I understand it the protocol used by the Tomahawk resolver is still 
the same (playdar API), so that should work. There are a few things you should 
look at before going on with this design though.
First do some namespace cleanup. ScriptResolver for instance is a confusing 
name. Here you won't be dealing with a resolver script but the spotify-resolver 
application. ResolverProcessInterface looks a little long, but does cover it's 
function pretty well. Up to you to figure out a good name.

I've not gone through the architecture of the playdar QueryMaker. It probably 
works, but I wonder if it has to be so complicated. QM is not a simple API to 
begin with though. Spotify does not support search strings for specific types 
(artist, album, genre, etc) AFAIU, so a mapping might be more straight forward. 
If this code is working keep it for now, as long as no performance issues pop 
up.


src/core-impl/collections/spotifycollection/SpotifyMeta.h
<http://git.reviewboard.kde.org/r/105201/#comment11543>

This whitespace at end of line can be prevented by using the option "Clean 
whitespaces" in Text Editor > Behavior of the QtCreator settings.


- Bart Cerneels


On June 10, 2012, 7:10 a.m., Zhengliang Feng wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105201/
> ---
> 
> (Updated June 10, 2012, 7:10 a.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> ---
> 
> Add Spotify collection code
> 
> Currently implemented SpotifyCollection, SpotifyQueryMaker and
> SpotifyMeta. The ScriptResolver is the class handles communcation with
> standalone Spotify resolver, the code is mainly from original
> ScriptResolver, but added more functions to handle messages separately.
> 
> The controller class is used to start a ScriptResolver in a separate
> thread and handles queries.
> 
> 
> Diffs
> -
> 
>   src/core-impl/collections/CMakeLists.txt 
> c78b9202ece71b51189c4e47d85acfa4a74ef8d6 
>   src/core-impl/collections/spotifycollection/CMakeLists.txt PRE-CREATION 
>   src/core-impl/collections/spotifycollection/SpotifyCollection.h 
> PRE-CREATION 
>   src/core-impl/collections/spotifycollection/SpotifyMeta.h PRE-CREATION 
>   src/core-impl/collections/spotifycollection/SpotifyMeta.cpp PRE-CREATION 
>   src/core-impl/collections/spotifycollection/SpotifyQueryMaker.h 
> PRE-CREATION 
>   src/core-impl/collections/spotifycollection/SpotifyQueryMaker.cpp 
> PRE-CREATION 
>   src/core-impl/collections/spotifycollection/support/Controller.h 
> PRE-CREATION 
>   src/core-impl/collections/spotifycollection/support/Controller.cpp 
> PRE-CREATION 
>   src/core-impl/collections/spotifycollection/support/QMFunctionTypes.h 
> PRE-CREATION 
>   src/core-impl/collections/spotifycollection/support/Query.h PRE-CREATION 
>   src/core-impl/collections/spotifycollection/support/ScriptResolver.h 
> PRE-CREATION 
>   src/core-impl/collections/spotifycollection/support/ScriptResolver.cpp 
> PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/105201/diff/
> 
> 
> Testing
> ---
> 
> Communication between ScriptResolver and Spotify resolver( from Tomahawk 
> resolver repo https://github.com/ofan/tomahawk-resolvers ).
> Logging into Spotify using a username and password.
> 
> 
> Thanks,
> 
> Zhengliang Feng
> 
>

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

Re: Review Request: Unit test : ActionsCapability

[Bart Cerneels]

---
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

[Bart Cerneels]

---
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: GSoC update: Integrate Spotify into Amarok

[Bart Cerneels]

On Sun, Jun 3, 2012 at 5:48 PM, Ryan Feng  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 
>> From: Bart Cerneels 
>> Subject: Re: GSoC update: Integrate Spotify into Amarok
>>
>> On Sun, Jun 3, 2012 at 10:12 AM, Bart Cerneels  
>> wrote:
>> > On Sun, Jun 3, 2012 at 6:32 AM, Ryan Feng  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.git&a=commitdiff&h=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.git&a=summary
___
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel

Re: GSoC update: Integrate Spotify into Amarok

[Bart Cerneels]

On Sun, Jun 3, 2012 at 10:12 AM, Bart Cerneels  wrote:
> On Sun, Jun 3, 2012 at 6:32 AM, Ryan Feng  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.git&a=commitdiff&h=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

[Bart Cerneels]

On Sun, Jun 3, 2012 at 10:12 AM, Bart Cerneels  wrote:
> On Sun, Jun 3, 2012 at 6:32 AM, Ryan Feng  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.git&a=commitdiff&h=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

[Bart Cerneels]

On Sun, Jun 3, 2012 at 6:32 AM, Ryan Feng  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.git&a=commitdiff&h=fb246b607f99a85fdff476ae189c53ad8a5fc4aa
___
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

[Bart Cerneels]

On Sun, Jun 3, 2012 at 12:05 AM, Modestas Vainius  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: Amarok 2.6 string freeze starting now!

[Bart Cerneels]

On Wed, May 30, 2012 at 5:24 PM, Frederik Schwarzer  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

[amarok] release_scripts: Update release HOWTO to current procedure.

[Bart Cerneels]

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!
 
---

Amarok 2.6 string freeze starting now!

[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
___
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel