Re: A proposal to release 2.9

[Matěj Laitl]

Hi Stefano,
I totally agree, 2.9 should be released to push the work done to users.

I unfortunately won't find time to do it myself, but: try to start making
the release yourself! ;) It should be documented quite well.

If you get stuck or need help with particular tasks, speak up on ML (and CC
me), I'm quite confident somebody from us will help. (people usually
procrastinate "big" tasks, but answering a concrete questions is much
easier)

If somebody has objections against making a release now or different
suggestions, please speak up.

Best,
Matěj

On Wed, Jan 24, 2018 at 8:43 PM Stefano Pettini 
wrote:

> Hi Matej,
>
> have you received my last message to the mailing list? What's your
> opinion? Sadly the project seems really abandoned, still it doesn't mean we
> can't do a last release. I also wrote to Myriam, she didn't answer, hope
> everything is fine.
>
> Stefano
>
>
> -- Forwarded message --
> From: Stefano Pettini 
> Date: Sun, Jan 7, 2018 at 10:05 AM
> Subject: A proposal to release 2.9
> To: amarok-devel@kde.org
>
>
> Hi,
>
> it's many years now that Amarok 2.9 is about to be released. The
> saturday-morning emails remember us weekly that there are still a couple of
> regressions since years. In the meanwhile development almost stopped, but
> not completely. I, like many, contributed with small but important patches
> (otherwise we would haven't dedicated time to provide such fixes).
>
> I think it's fair if the work since 2.8 is not wasted and 2.9 is released.
>
> Current regressions are minor bugs, the only annoying thing not working
> anymore is the cover search. But it's not a newly-introduced regression,
> just the world changed and all the services used for cover search become
> not available anymore. It's not a problem not present in 2.8 that people
> would face when updating to 2.9. It's already broken now.
>
> I would disable what doesn't work to not give false impressions, removing
> the broken services from cover search, and release 2.9. This would fix the
> access to wikipedia and other bugs we dedicated time to.
>
> Cover search can be restored later, if developers find time to dedicate to
> it and somebody reviews the available services and select the suitable ones
> for the future Amarok.
>
> Regards,
> Stefano
>
>

Re: Review Request 129192: Add namespace to desktop and appdata files

[Matěj Laitl]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129192/#review102578
---



Ah, sorry for inactivity. Issue dropped.

OTOH I'm still thinking about value/risk of this change. Pro: according to 
standards. Con: existing .desktop links may stop working. Has anyone tested 
this for example with Amarok shortcut in panel, desktop in various DEs? Others, 
what do you think?

- Matěj Laitl


On Říj. 16, 2016, 4:12 odp., Luigi Toscano wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129192/
> ---
> 
> (Updated Říj. 16, 2016, 4:12 odp.)
> 
> 
> Review request for Amarok and Matthias Klumpp.
> 
> 
> Repository: amarok
> 
> 
> Description
> ---
> 
> - add the organization namespace to the desktop and then appdata files, 
> according the specification;
> - use the new directory for metadata (metainfo) files.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 327ec10 
>   src/amarok.appdata.xml c580fd5 
>   src/amarok.desktop  
>   src/amarok_containers.desktop  
>   src/dbus/mpris2/MediaPlayer2.cpp a1d1bda 
> 
> Diff: https://git.reviewboard.kde.org/r/129192/diff/
> 
> 
> Testing
> ---
> 
> Compiles, the files are installed in the expected places.
> 
> 
> Thanks,
> 
> Luigi Toscano
> 
>

Re: Review Request 129192: Add namespace to desktop and appdata files

[Matěj Laitl]

> On Říj. 16, 2016, 12:06 odp., Matěj Laitl wrote:
> > Looks good, but see one question below.
> > 
> > Also, I wonder what the consequences would be? People loosing custom-set 
> > file associations? What about shortcuts in panels etc.?

I also think MediaPlayer2::DesktopEntry() needs to be updated.


- Matěj


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129192/#review100035
---


On Říj. 15, 2016, 9:44 odp., Luigi Toscano wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129192/
> ---
> 
> (Updated Říj. 15, 2016, 9:44 odp.)
> 
> 
> Review request for Amarok.
> 
> 
> Repository: amarok
> 
> 
> Description
> ---
> 
> - add the organization namespace to the desktop and then appdata files, 
> according the specification;
> - use the new directory for metadata (metainfo) files.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 327ec10 
>   src/amarok.appdata.xml c580fd5 
>   src/amarok.desktop  
>   src/amarok_containers.desktop  
> 
> Diff: https://git.reviewboard.kde.org/r/129192/diff/
> 
> 
> Testing
> ---
> 
> Compiles, the files are installed in the expected places.
> 
> 
> Thanks,
> 
> Luigi Toscano
> 
>

Re: Review Request 129055: [amarok] Fixed Space key binded as a global shortcut (which breaks keyboard usage for the session).

[Matěj Laitl]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129055/#review100036
---


Ship it!




Looks good. Feel free to push if you have commit access. Since kf5 is a 
development branch and there are few people to review, feel free to push to it 
directly unless you explicitly want a review (perhaps for something 
controversial).

- Matěj Laitl


On Zář. 27, 2016, 10:13 odp., Alexandr Akulich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129055/
> ---
> 
> (Updated Zář. 27, 2016, 10:13 odp.)
> 
> 
> Review request for Amarok.
> 
> 
> Repository: amarok
> 
> 
> Description
> ---
> 
> There is a change at commit c3c4c7b8ebc4bc9ec94394f5d3a5569dee8b4725 :
> ```diff
>  setText( i18n( "Play/Pause" ) );
> -setShortcut( Qt::Key_Space );
> -setGlobalShortcut( KShortcut() );
> +KGlobalAccel::setGlobalShortcut(this, QKeySequence(Qt::Key_Space) );
> ```
> 
> The change globally binds Key_Space, which breaks the key usage for the 
> session.
> 
> 
> Diffs
> -
> 
>   src/ActionClasses.cpp 77334fc 
> 
> Diff: https://git.reviewboard.kde.org/r/129055/diff/
> 
> 
> Testing
> ---
> 
> Compiles.
> 
> 
> Thanks,
> 
> Alexandr Akulich
> 
>

Re: Review Request 129192: Add namespace to desktop and appdata files

[Matěj Laitl]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129192/#review100035
---



Looks good, but see one question below.

Also, I wonder what the consequences would be? People loosing custom-set file 
associations? What about shortcuts in panels etc.?


src/CMakeLists.txt (lines 955 - 958)
<https://git.reviewboard.kde.org/r/129192/#comment67203>

What about these .desktop files? I seems that these should be moved, too, 
at least for consistency.


- Matěj Laitl


On Říj. 15, 2016, 9:44 odp., Luigi Toscano wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129192/
> ---
> 
> (Updated Říj. 15, 2016, 9:44 odp.)
> 
> 
> Review request for Amarok.
> 
> 
> Repository: amarok
> 
> 
> Description
> ---
> 
> - add the organization namespace to the desktop and then appdata files, 
> according the specification;
> - use the new directory for metadata (metainfo) files.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 327ec10 
>   src/amarok.appdata.xml c580fd5 
>   src/amarok.desktop  
>   src/amarok_containers.desktop  
> 
> Diff: https://git.reviewboard.kde.org/r/129192/diff/
> 
> 
> Testing
> ---
> 
> Compiles, the files are installed in the expected places.
> 
> 
> Thanks,
> 
> Luigi Toscano
> 
>

Re: Review Request 128246: Lyrics browser switched to the same background of the every other context applet

[Matěj Laitl]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128246/#review97291
---


Ship it!




Ship It!

- Matěj Laitl


On Čec. 10, 2016, 11:13 odp., Stefano Pettini wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128246/
> ---
> 
> (Updated Čec. 10, 2016, 11:13 odp.)
> 
> 
> Review request for Amarok.
> 
> 
> Bugs: 314854
> https://bugs.kde.org/show_bug.cgi?id=314854
> 
> 
> Repository: amarok
> 
> 
> Description
> ---
> 
> This makes the lyrics browser similar to every other context applet.
> Please note that, when editing lyrics, the default textbox background
> is still used.
> 
> 
> Diffs
> -
> 
>   ChangeLog 23599f1 
>   src/context/applets/lyrics/LyricsBrowser.cpp abcfe64 
> 
> Diff: https://git.reviewboard.kde.org/r/128246/diff/
> 
> 
> Testing
> ---
> 
> Tested ONLY in the following environment:
> KDE Version: 4.14.16
> Qt Version: 4.8.7
> 
> Tested with light and dark color scheme
> Tested use case of changing KDE color scheme while Amarok is running
> Tested the edit lyrics mode
> 
> 
> Thanks,
> 
> Stefano Pettini
> 
>

Re: Review Request 128248: Remove non SSL option for Wikipedia applet

[Matěj Laitl]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128248/#review96755
---



Looks well, one minor thing below - unrelated change.

Also please mention:
BUG: 348313
..in the commit message and include a ChangeLog entry in the patch.


src/musicbrainz/MusicDNSAudioDecoder.cpp (line 226)
<https://git.reviewboard.kde.org/r/128248/#comment65359>

This looks like an unrelated change. Please strip of a probably post as a 
separate patch. Applies also to the change few lines below.


- Matěj Laitl


On Čer. 19, 2016, 11:44 dop., Olivier Churlaud wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128248/
> ---
> 
> (Updated Čer. 19, 2016, 11:44 dop.)
> 
> 
> Review request for Amarok and Myriam Schweingruber.
> 
> 
> Bugs: 349313
> https://bugs.kde.org/show_bug.cgi?id=349313
> 
> 
> Repository: amarok
> 
> 
> Description
> ---
> 
> I removed every possibility to use non SSL links
> 
> 
> Diffs
> -
> 
>   src/context/applets/wikipedia/WikipediaApplet.cpp 2ceb2b0 
>   src/context/applets/wikipedia/wikipediaGeneralSettings.ui 84cb5df 
>   src/context/engines/wikipedia/WikipediaEngine.cpp 969d8fc 
>   src/musicbrainz/MusicDNSAudioDecoder.cpp ea39a1b 
> 
> Diff: https://git.reviewboard.kde.org/r/128248/diff/
> 
> 
> Testing
> ---
> 
> It compiles. However not tested, because make install always install in 
> /usr/local, whatever I give in -DCMAKE_INSTALL_PREFIX. And I don't want to 
> mess my install.
> 
> 
> Thanks,
> 
> Olivier Churlaud
> 
>

Re: Review Request 128246: Lyrics browser switched to the same background of the every other context applet

[Matěj Laitl]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128246/#review96718
---


Ship it!




Although I'm not context view expert, this loks good to me, let's merge this 
and let wider user base test it.

- Matěj Laitl


On Čer. 19, 2016, 3:01 dop., Stefano Pettini wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128246/
> ---
> 
> (Updated Čer. 19, 2016, 3:01 dop.)
> 
> 
> Review request for Amarok.
> 
> 
> Bugs: 314854
> https://bugs.kde.org/show_bug.cgi?id=314854
> 
> 
> Repository: amarok
> 
> 
> Description
> ---
> 
> This makes the lyrics browser similar to every other context applet.
> Please note that, when editing lyrics, the default textbox background
> is still used.
> 
> 
> Diffs
> -
> 
>   src/context/applets/lyrics/LyricsBrowser.cpp abcfe64 
> 
> Diff: https://git.reviewboard.kde.org/r/128246/diff/
> 
> 
> Testing
> ---
> 
> Tested ONLY in the following environment:
> KDE Version: 4.14.16
> Qt Version: 4.8.7
> 
> Tested with light and dark color scheme
> Tested use case of changing KDE color scheme while Amarok is running
> Tested the edit lyrics mode
> 
> 
> Thanks,
> 
> Stefano Pettini
> 
>

Amarok release (was: Fetching googlemock)

[Matěj Laitl]

On 14. 12. 2014 Myriam Schweingruber wrote:
 Can we please make a release soon,  Matěj?

That's what I wanted to do for ages, but having a full-time job and furnishing 
a flat takes more time than I've ever expected. :-|

I still plan  want to do it, but if someone is faster than me (Ralf? 
Markey?), I'd be happy too. I don't want to be a blocker.

 There is one release blocker bug
 which I still can reproduce and which falls in your speciality, but else we
 are good for 2.9 since quite some time :)

Do you mean https://bugs.kde.org/show_bug.cgi?id=335217 and/or 
https://bugs.kde.org/show_bug.cgi?id=338541 or even something another?

Bug 335217 is on my TODO list, it is something that stopped working due to my 
changes. (it worked because it depended on a buggy behaviour, which has been 
fixed)

Regards,
Matěj
___
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel

Re: Fetching googlemock

[Matěj Laitl]

On 14. 12. 2014 Konrad Zemek wrote:
  Git submodule approach looks promising, however I have some concerns:
   a) this makes test depend on 'your' github repositories; we cannot
   guarantee they won't go away etc.
   b) this makes testing Amarok require internet connection, at least
   initially; this of shipping entire sources to build a distribution
   package etc.
   c) circumvents source file checksumming etc. that many distributions do
   to enhance security
   d) is it legally okay to redistribute googlemock, googletest? Using a
   git repo, shipping a tarball?
  
  Still, I like the idea. a) seems easily fixable b), c) seems fixable by
  tweaking the way we create Amarok tarballs.
  
  I guess a) can be easily fixed if this goes to our git repo.
  as for d) since googlemock is Free Software (New BSD 3 clause license, see
  also https://code.google.com/p/googlemock/), this shouldn't be a problem.
 
 As for b) and c), I was imagining that `git submodule update --init`
 would become a standard step to fetch sources for creating a tarball
 or building tests. The auto-fetch is there just for convenience.

Thinking about it more, this should work. Initially I was thinking about how 
distros ship packages, but this should not touch binary distros at all.

How big is tarballed gtest + gmock? Can we just embed them in our release 
tarballs? Else we can create something like amarok-testlibs-$version.tar.bz2, 
but that would be more work and effort.

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

Re: Fetching googlemock

[Matěj Laitl]

On 14. 12. 2014 Konrad Zemek wrote:
 Matěj Laitl wrote:
  How big is tarballed gtest + gmock? Can we just embed them in our release
  tarballs? Else we can create something like
  amarok-testlibs-$version.tar.bz2, but that would be more work and effort.
 
 tarred and bzipped, the repos take 588KB of space (tbz2 of the whole
 Amarok source is 158MB). I can look into shallow clones if needed.

Easy then, we can bundle gtest+gmock in Amarok release tarballs.

Note that we don't release repository, just the sources [1], so shallow clones 
won't affect it -- last Amarok release had ~ 38 MB. (but that includes 
translations and handbooks) So size of gtest+gmock will be even smaller.

So please go ahead! :) Please have a look if release_scripts/RELEASE_HOWTO 
and/or amarok2.rb in releaseme.git [2] should be updated. Ideally if you try 
to make an Amarok release tarball and check whether it looks as expected.

Thanks,
Matěj

[1] 
http://download.kde.org/stable/amarok/2.8.0/src/amarok-2.8.0.tar.bz2.mirrorlist
[2] http://quickgit.kde.org/?p=releaseme.gita=blobf=amarok2.rb
___
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel

Re: Fetching googlemock

[Matěj Laitl]

On 13. 12. 2014 Konrad Zemek wrote:
 gmock sources are still not packaged by distributions, and compiling
 Amarok with tests on is still troublesome (I still use a cmake-gui
 based approach where I manually set paths to my pre-compiled gmock
 lib, as I outlined in an email some months ago).
 
 I solved the problem through the use of submodules and commited the
 change to my personal scratch repo [1]. (...)
 
 If you find my approach agreeable, I will be happy to put it on reviewboard.

Git submodule approach looks promising, however I have some concerns:
 a) this makes test depend on 'your' github repositories; we cannot guarantee 
they won't go away etc.
 b) this makes testing Amarok require internet connection, at least initially; 
this of shipping entire sources to build a distribution package etc.
 c) circumvents source file checksumming etc. that many distributions do to 
enhance security
 d) is it legally okay to redistribute googlemock, googletest? Using a git 
repo, shipping a tarball?

Still, I like the idea. a) seems easily fixable b), c) seems fixable by 
tweaking 
the way we create Amarok tarballs.

 By the way, I noticed that importer tests are now guarded with
 'if(LINUX)' macro. There is no 'LINUX' platform in CMake, though, so
 these tests are effectively disabled everywhere. I guess there were
 some problems on non-linux systems?

Looks like a bug to me, feel free to investigate and fix, the test should run 
at least on Linux platforms (best if they are run everywhere).

Regards,
Matěj
___
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel

Re: Review Request 120930: Flac and Mp3 modified to FLAC and MP3 on dialog.

[Matěj Laitl]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120930/#review69630
---

Ship it!


Looks good to me, thanks! Someone with commit access please push this. (please 
don't forget BUG, REVIEW, FIXED-IN tags)

- Matěj Laitl


On Oct. 31, 2014, 11:49 p.m., Duilio Felix wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/120930/
 ---
 
 (Updated Oct. 31, 2014, 11:49 p.m.)
 
 
 Review request for Amarok.
 
 
 Bugs: 339495
 https://bugs.kde.org/show_bug.cgi?id=339495
 
 
 Repository: amarok
 
 
 Description
 ---
 
  [Bug 339495] Flac partially lowercase when ripping audio CD 
 
 
 Diffs
 -
 
   src/core-impl/collections/audiocd/FormatSelectionDialog.cpp 90cbb88 
   src/core-impl/collections/audiocd/FormatSelectionDialog.ui 9200301 
 
 Diff: https://git.reviewboard.kde.org/r/120930/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Duilio Felix
 


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

Re: Review Request 120399: Amarok: If waiting on the fadeout to pause, prevent pausing again.

[Matěj Laitl]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120399/#review67549
---


The change looks good (it is an improvement wrt current situation), but perhaps 
we can do what you envisioned on the bug, i.e. pause instantenously when pause 
is pressed within pausin fadeout?

- Matěj Laitl


On Sept. 28, 2014, 2:39 a.m., Martin Lyth wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/120399/
 ---
 
 (Updated Sept. 28, 2014, 2:39 a.m.)
 
 
 Review request for Amarok.
 
 
 Bugs: 339470
 https://bugs.kde.org/show_bug.cgi?id=339470
 
 
 Repository: amarok
 
 
 Description
 ---
 
 Don't call play or pause if we're already waiting on the timer to pause. See 
 Bug 339470 for details on the bug, and the 1.5 lines of code are pretty 
 self-explanatory.
 
 
 Diffs
 -
 
   src/EngineController.cpp 19c483d 
 
 Diff: https://git.reviewboard.kde.org/r/120399/diff/
 
 
 Testing
 ---
 
 Still compiles, and pressing pause continuously no longer extends the timer 
 infinitely.
 
 
 Thanks,
 
 Martin Lyth
 


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

Re: Review Request 118864: Don't prompt the user if the directories are not empty when moving tracks to trash

[Matěj Laitl]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118864/#review63073
---


There are a few style errors, otherwise the intent looks good. I'd have to 
think more about any possible side-effect before giving it final Ship it, 
though.


src/core/collections/CollectionLocation.cpp
https://git.reviewboard.kde.org/r/118864/#comment43807

You are using tabs to indent, which is forbidden by Amarok coding style. 
(please see the HACKING folder)



src/core/collections/CollectionLocation.cpp
https://git.reviewboard.kde.org/r/118864/#comment43803

excess whitespace, please remove


- Matěj Laitl


On July 24, 2014, 4:19 p.m., Robert Marshall wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/118864/
 ---
 
 (Updated July 24, 2014, 4:19 p.m.)
 
 
 Review request for Amarok.
 
 
 Repository: amarok
 
 
 Description
 ---
 
 If the user hasn't turned off the dialog which notifies if they want 
 directories deleting they get prompted even if the directory is not empty. 
 amarok ought to only prompt when it needs the information. Maybe I should 
 select the don't prompt in future option but I prefer to know what's going on!
 
 
 Diffs
 -
 
   src/core/collections/CollectionLocation.cpp 209d6b4 
 
 Diff: https://git.reviewboard.kde.org/r/118864/diff/
 
 
 Testing
 ---
 
 Built
 Tested deleting tracks on  empty directories - prompted
 Tested directories containing music and just containing other files - not 
 prompted
 
 Do I need a bug/enhancement report to tie to this?
 
 I wondered whether this change might be inefficient if the collection has 
 many levels but seems to be ok at least with my testing.
 
 
 Thanks,
 
 Robert Marshall
 


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

Re: Review Request 119405: Stop passing -std=iso9899:1999 to CMAKE_C_FLAGS.

[Matěj Laitl]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/119405/#review62918
---

Ship it!


Looks good to me, push it! :)   (do you already have commit access?)

- Matěj Laitl


On July 22, 2014, 2:32 p.m., Raphael Kubo da Costa wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/119405/
 ---
 
 (Updated July 22, 2014, 2:32 p.m.)
 
 
 Review request for Amarok.
 
 
 Repository: amarok
 
 
 Description
 ---
 
 This was added back when Amarok built its own SQLite copy, which was
 removed in 2008 by commit 489af24a010eaeacbaf2f82b0f07ebd3b12c0912.
 
 And even then, kdelibs itself already sets this same flag on its own.
 
 
 Diffs
 -
 
   src/core/CMakeLists.txt 5935c36bf03ff977b72f3bec74082ed15e39dea2 
 
 Diff: https://git.reviewboard.kde.org/r/119405/diff/
 
 
 Testing
 ---
 
 Still builds on Debian.
 
 
 Thanks,
 
 Raphael Kubo da Costa
 


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

Re: Review Request 119412: Fix the way PHAACG2.py is invoked in the build system.

[Matěj Laitl]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/119412/#review62919
---

Ship it!


This restores the sane beahviour (the insane one was apparently introduced by 
accident), so thanks for it and ship it!  (I haven't tested it, the the diff 
looks completely fine)

- Matěj Laitl


On July 22, 2014, 8:20 p.m., Raphael Kubo da Costa wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/119412/
 ---
 
 (Updated July 22, 2014, 8:20 p.m.)
 
 
 Review request for Amarok and Anmol Ahuja.
 
 
 Repository: amarok
 
 
 Description
 ---
 
 1. Touching the installation directory at configuration time is very
wrong, and usually fails, particularly when Amarok is being packaged,
since the source code tends to be built as a user and installed into
a chroot.
In this case, the script's call to os.makedirs() would normally fail.
 
 2. Invoking python without looking for it first and without
considering that it might be called differently is also very wrong.
 
 We now properly look for the Python binary, only call PHAACG2.py if it
 is found, store all the generated files inside ${CMAKE_BINARY_DIR} and
 install them like any other file with install().
 
 
 Diffs
 -
 
   CMakeLists.txt 1b03bda0e6769b5d8394ddab2fa39682089fc10c 
   src/CMakeLists.txt d3c74d09dce0eb03e6b67c7430515d0f9cd44f84 
 
 Diff: https://git.reviewboard.kde.org/r/119412/diff/
 
 
 Testing
 ---
 
 * The script finally stops failing to run if I configure Amarok as a user 
 without changing `CMAKE_INSTALL_PREFIX` (ie. it doesn't try writing to 
 `/usr/local` anymore).
 * `AutoComplete.txt` is properly installed after everything is built, and 
 nothing outside `CMAKE_BINARY_DIR` is touched before installation time.
 
 
 Thanks,
 
 Raphael Kubo da Costa
 


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

Re: Review Request 119429: Display 'random tracks' rather than 'random songs' in the appropriate bias in a dynamic mode playlist

[Matěj Laitl]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/119429/#review62969
---

Ship it!


Looks good to me, ship it and thanks. :)

- Matěj Laitl


On July 23, 2014, 5:07 p.m., Robert Marshall wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/119429/
 ---
 
 (Updated July 23, 2014, 5:07 p.m.)
 
 
 Review request for Amarok.
 
 
 Bugs: 337725
 https://bugs.kde.org/show_bug.cgi?id=337725
 
 
 Repository: amarok
 
 
 Description
 ---
 
  [Bug 337725] Text for RandomBiasFactory says 'random tracks' but the text 
 once added is 'random songs'
 
 
 Diffs
 -
 
   src/dynamic/Bias.cpp d0aab75 
   src/dynamic/biases/SearchQueryBias.cpp cfe4b01 
 
 Diff: https://git.reviewboard.kde.org/r/119429/diff/
 
 
 Testing
 ---
 
 Built (kubuntu 14.04)
 Checked that existing dynamic mode playlists with 'random songs' displays the 
 new text
 Checked that random track addition to a playlist displays correctly.
 Checked that on running the playlist it looks like random tracks are being 
 added
 
 
 Thanks,
 
 Robert Marshall
 


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

Re: Review Request 119429: Display 'random tracks' rather than 'random songs' in the appropriate bias in a dynamic mode playlist

[Matěj Laitl]

 On July 23, 2014, 5:12 p.m., Matěj Laitl wrote:
  Looks good to me, ship it and thanks. :)

Please don't forget to add relevant tags to commit:

REVIEW: ...
BUG: ...
FIXED-IN: ...


- Matěj


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/119429/#review62969
---


On July 23, 2014, 5:07 p.m., Robert Marshall wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/119429/
 ---
 
 (Updated July 23, 2014, 5:07 p.m.)
 
 
 Review request for Amarok.
 
 
 Bugs: 337725
 https://bugs.kde.org/show_bug.cgi?id=337725
 
 
 Repository: amarok
 
 
 Description
 ---
 
  [Bug 337725] Text for RandomBiasFactory says 'random tracks' but the text 
 once added is 'random songs'
 
 
 Diffs
 -
 
   src/dynamic/Bias.cpp d0aab75 
   src/dynamic/biases/SearchQueryBias.cpp cfe4b01 
 
 Diff: https://git.reviewboard.kde.org/r/119429/diff/
 
 
 Testing
 ---
 
 Built (kubuntu 14.04)
 Checked that existing dynamic mode playlists with 'random songs' displays the 
 new text
 Checked that random track addition to a playlist displays correctly.
 Checked that on running the playlist it looks like random tracks are being 
 added
 
 
 Thanks,
 
 Robert Marshall
 


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

Re: Amarok v2.8.1

[Matěj Laitl]

On Thu, May 8, 2014 at 8:41 PM, Dan Meltzer
parallelgrapefr...@gmail.com wrote:
 Hi Everyone,

Hi Dan,
first, thanks for all your recent work on Amarok, especially for
taking care of review requests and other similar long-overdue taks!

 I was talking With Mark this morning, and I think it would be prudent to 
 release an Amarok 2.8.1.  There aren't a lot of feature changes, but there 
 are a number of bug fixes, and just general runtime issues.  I Have a hope to 
 have amarok working better on os x before the release, but I wouldn't 
 consider it a showstopper either way.

I'm all in, and given that I've submitted by master's thesis this
Monday (yay; there's still final exams for me in ~4 weeks), I could
even have some time to do the release (but I'd happily pass it on if
someone *wants* to do it).

I've also started poking with the my MTP collection rewrite, which
would hopefully be in a releasable state in a month or two. If this
goes according to the plan, we may even think of calling it just
Amarok 2.9.

BTW, it would be nice if Tatjana's AudioCD rewrite would be readied
before 2.9 as it would allow us to remove a deprecated framework.
Tatjana? Edward?

Regards,
  Matěj
___
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel

Re: Review Request 114752: BugFix : 291400 - Maximum allowed length in CollectionBrowser filter dialog

[Matěj Laitl]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/114752/#review46844
---


I see, this patch is almost ready, please wait for Yuri to answer and then fix 
remaining problems. Also please don't forget about:
 1. Add commit tags to the description - see 
quickgit.kde.org/?p=kdelibs.gita=blobf=.commit-template - namely: BUG: , 
FIXED-IN: 2.9, REVIEW: ### - so that they end up in the commit - also please 
update the description to be current


src/widgets/MetaQueryWidget.cpp
https://git.reviewboard.kde.org/r/114752/#comment33442

So the maximum time is 23:59:59 - this calculation is wrong.



src/widgets/MetaQueryWidget.cpp
https://git.reviewboard.kde.org/r/114752/#comment33443

Please:
 1. provide context for the string - use i18nc() call with context like 
time format for specifying track length - hours, minutes, seconds
 2. respect Amarok coding style - spaces around arguments


- Matěj Laitl


On Jan. 5, 2014, 11:25 a.m., Abhay Sombanshi wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/114752/
 ---
 
 (Updated Jan. 5, 2014, 11:25 a.m.)
 
 
 Review request for Amarok and Yuri Chornoivan.
 
 
 Bugs: 291400
 https://bugs.kde.org/show_bug.cgi?id=291400
 
 
 Repository: amarok
 
 
 Description
 ---
 
 CollectionBrowser filter dialog will now allow a length of 24 hours.
 
 
 Diffs
 -
 
   ChangeLog 0eb03c8 
   src/widgets/MetaQueryWidget.cpp 58601cc 
 
 Diff: https://git.reviewboard.kde.org/r/114752/diff/
 
 
 Testing
 ---
 
 works as expected.
 
 
 Thanks,
 
 Abhay Sombanshi
 


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

Re: Review Request 114752: BugFix : 291400 - Maximum allowed length in CollectionBrowser filter dialog

[Matěj Laitl]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/114752/#review46778
---


Looks better, please see another round of suggestions below and also:
 1. Add commit tags to the description - see 
quickgit.kde.org/?p=kdelibs.gita=blobf=.commit-template - namely: BUG: , 
FIXED-IN: 2.9, REVIEW: ### - so that they end up in the commit - also please 
update the description to be current
 2. Add a line to the ChangeLog file (top-level in the repo) - under BUGFIXES 
on top.



src/widgets/MetaQueryWidget.cpp
https://git.reviewboard.kde.org/r/114752/#comment33409

Another thing on my mind: it may have sense to have the format localized - 
please see KLocale class 
http://api.kde.org/4.x-api/kdelibs-apidocs/kdecore/html/classKLocale.html and 
perhaps ask on #kde-i18n IRC channel on Freenode.



src/widgets/MetaQueryWidget.cpp
https://git.reviewboard.kde.org/r/114752/#comment33408

I see no point in allowing only 23:59:59, just make it 24:00:00 please, 
unless there is a usability problem with it. (and please update other places to 
match)


- Matěj Laitl


On Jan. 3, 2014, 6:33 p.m., Abhay Sombanshi wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/114752/
 ---
 
 (Updated Jan. 3, 2014, 6:33 p.m.)
 
 
 Review request for Amarok.
 
 
 Bugs: 291400
 https://bugs.kde.org/show_bug.cgi?id=291400
 
 
 Repository: amarok
 
 
 Description
 ---
 
 CollectionBrowser filter dialog will now allow a length of 2:59:59 hours.
 
 
 Diffs
 -
 
   src/widgets/MetaQueryWidget.cpp 58601cc 
 
 Diff: https://git.reviewboard.kde.org/r/114752/diff/
 
 
 Testing
 ---
 
 works as expected.
 
 
 Thanks,
 
 Abhay Sombanshi
 


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

Re: Review Request 110036: WIP - Simple equalizer scripting

[Matěj Laitl]

 On July 2, 2013, 12:50 p.m., Mark Kretschmann wrote:
  Any news on this? Could you please upload the patch again?

There has been no feedback for 6 months and I believe this could have been 
superseded by Scripting GSoC 2013 project - should this be discarded?


- Matěj


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/110036/#review35436
---


On April 18, 2013, 10:03 a.m., Ryan McCoskrie wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/110036/
 ---
 
 (Updated April 18, 2013, 10:03 a.m.)
 
 
 Review request for Amarok.
 
 
 Repository: amarok
 
 
 Description
 ---
 
 A new, unambitious attempt at adding equalizer functions to the scripting 
 interface.
 
 Adds to EngineController the functions:
 * QString eqPreset()
 * void eqApplyPreset(QString name)
 And the signal:
 eqPresetApplied(QString name)
 
 Adds to AmarokEngineScript the same functions
 set up as a property.
 
 
 Diffs
 -
 
   src/EngineController.cpp 28fb256 
   src/scriptengine/AmarokEngineScript.h f1cdb8c 
   src/scriptengine/AmarokEngineScript.cpp 4d52bbe 
   src/EngineController.h 5de4beb 
 
 Diff: https://git.reviewboard.kde.org/r/110036/diff/
 
 
 Testing
 ---
 
 Quick check using the script console that presets can be changed.
 The preset does get applied but it won't show in the EqualizerDialog.
 Possible other bugs.
 
 
 Thanks,
 
 Ryan McCoskrie
 


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

Re: Review Request 109369: Bug #254404: Copy files to USB storage devices in display order by sorting tracks in CollectionLocation.cpp

[Matěj Laitl]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/109369/#review46681
---


As there is no recent development, I believe this is a way too invasive 
complication for little gain - it should be discarded unless anybody plans to 
simplify this.

- Matěj Laitl


On March 12, 2013, 7:27 p.m., Anmol Ahuja wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/109369/
 ---
 
 (Updated March 12, 2013, 7:27 p.m.)
 
 
 Review request for Amarok.
 
 
 Repository: amarok
 
 
 Description
 ---
 
 1. Made a levelSort() function to sort tracks according to multiple 
 parameters ( Should i just be sorting tracks belonging to the same album? )
 2. Modified copyUrlToCollection() to use QList instead of QMap
 3. FileView's files are being copied in the order of selection, thanks to 
 using QLists ( is that acceptable? )
 
 Updates:
 Implemented changes as suggested by strohel
 1. Moved levelSort function to the prepareCopy callers (CollectionTreeView)
 2. Restored CollectionLocation.cpp to the old version, with QLists instead of 
 QMaps
 
 Still not complete though, working on it.
 
 
 Diffs
 -
 
   src/browsers/CollectionTreeView.h 3b2ca80 
   src/browsers/CollectionTreeView.cpp fd9fe66 
   src/browsers/collectionbrowser/CollectionWidget.h c281f41 
   src/core-impl/collections/audiocd/AudioCdCollectionLocation.cpp be13551 
   src/core-impl/collections/db/sql/SqlCollectionLocation.h 0bcf244 
   src/core-impl/collections/db/sql/SqlCollectionLocation.cpp 93efe97 
   src/core-impl/collections/ipodcollection/IpodCollectionLocation.h cc27e19 
   src/core-impl/collections/ipodcollection/IpodCollectionLocation.cpp f8105f9 
   src/core-impl/collections/ipodcollection/jobs/IpodCopyTracksJob.h 3c2d9f2 
   src/core-impl/collections/ipodcollection/jobs/IpodCopyTracksJob.cpp 8a40c6c 
   
 src/core-impl/collections/mediadevicecollection/MediaDeviceCollectionLocation.h
  e40529f 
   
 src/core-impl/collections/mediadevicecollection/MediaDeviceCollectionLocation.cpp
  f60aff6 
   
 src/core-impl/collections/mediadevicecollection/handler/MediaDeviceHandler.h 
 821f1b0 
   
 src/core-impl/collections/mediadevicecollection/handler/MediaDeviceHandler.cpp
  c1b76f5 
   src/core-impl/collections/mtpcollection/handler/MtpHandler.cpp a8d9f52 
   src/core-impl/collections/support/PlaylistCollectionLocation.h 10a365f 
   src/core-impl/collections/support/PlaylistCollectionLocation.cpp c885046 
   src/core-impl/collections/support/TrashCollectionLocation.h 239a977 
   src/core-impl/collections/support/TrashCollectionLocation.cpp 61c2e49 
   src/core-impl/collections/umscollection/UmsCollection.cpp 6bebd98 
   src/core-impl/collections/umscollection/UmsCollectionLocation.h 45ba596 
   src/core-impl/collections/umscollection/UmsCollectionLocation.cpp e0ba0ac 
   src/core/collections/CollectionLocation.h d37ccfb 
   src/core/collections/CollectionLocation.cpp aecc068 
   src/services/ServiceCollectionLocation.cpp d1cb0d8 
   src/services/mp3tunes/Mp3tunesServiceCollectionLocation.h 2b06cb4 
   src/services/mp3tunes/Mp3tunesServiceCollectionLocation.cpp aa61072 
 
 Diff: https://git.reviewboard.kde.org/r/109369/diff/
 
 
 Testing
 ---
 
 Seems to be copying tracks in the correct order now
 
 
 Thanks,
 
 Anmol Ahuja
 


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

Re: Review Request 114765: Bug 322016 - Apply button is always enabled in Playlist Layout Editor dialog

[Matěj Laitl]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/114765/#review46684
---


Hi, thanks for the patch. I think there could be a better approach in solving 
the bug - the one I've outlined in my review of a similar request: 
https://git.reviewboard.kde.org/r/113057/

It may turn out not possible/worth it, but it is for sure worth investigation.


src/playlist/layouts/PlaylistLayoutEditDialog.cpp
https://git.reviewboard.kde.org/r/114765/#comment33326

Please respect Amarok coding style - spaces around arguments - see the 
HACKING folder in Amarok sources. This applies to other places in your patch.

Note that existing calls
buttonBox-button(QDialogButtonBox::Apply)
do not conform to Amarok coding style - you may fix that in a separate 
review request (not in this one as we want style fixes separate from other 
fixes)


- Matěj Laitl


On Dec. 31, 2013, 6:04 p.m., Nilesh Suthar wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/114765/
 ---
 
 (Updated Dec. 31, 2013, 6:04 p.m.)
 
 
 Review request for Amarok.
 
 
 Bugs: 322016
 https://bugs.kde.org/show_bug.cgi?id=322016
 
 
 Repository: amarok
 
 
 Description
 ---
 
 Disabled Apply Button on Editor Creation and Close.on any change the apply 
 button is enabled.
 
 
 Diffs
 -
 
   src/playlist/layouts/PlaylistLayoutEditDialog.cpp 99aee2a 
 
 Diff: https://git.reviewboard.kde.org/r/114765/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Nilesh Suthar
 


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

Re: Review Request 114752: BugFix : 291400 - Maximum allowed length in CollectionBrowser filter dialog

[Matěj Laitl]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/114752/#review46685
---


Thanks for the patch, it looks good. When at it, I have some improvement 
suggestions, please incorporate them.


src/widgets/MetaQueryWidget.cpp
https://git.reviewboard.kde.org/r/114752/#comment33327

I suggest we use 24:00:00 (24 hours) as a maximum instead of 2:59:59, it 
cannot hurt. I also suggest to add
static const int maxHours = 24;
near to top of the cpp file and than use it at appropriate places. This 
improves maintainability.



src/widgets/MetaQueryWidget.cpp
https://git.reviewboard.kde.org/r/114752/#comment33328

I suggest to create local variable for the display format for better 
maintainability.


- Matěj Laitl


On Dec. 31, 2013, 11:48 a.m., Abhay Sombanshi wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/114752/
 ---
 
 (Updated Dec. 31, 2013, 11:48 a.m.)
 
 
 Review request for Amarok.
 
 
 Bugs: 291400
 https://bugs.kde.org/show_bug.cgi?id=291400
 
 
 Repository: amarok
 
 
 Description
 ---
 
 CollectionBrowser filter dialog will now allow a length of 2:59:59 hours.
 
 
 Diffs
 -
 
   src/widgets/MetaQueryWidget.cpp 58601cc 
 
 Diff: https://git.reviewboard.kde.org/r/114752/diff/
 
 
 Testing
 ---
 
 works as expected.
 
 
 Thanks,
 
 Abhay Sombanshi
 


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

Re: Review Request 114736: Bug 299431 The text in the notification area located in the lower left corner is cut when copying several tracks to the collection at the same time.

[Matěj Laitl]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/114736/#review46687
---


Hi, the patch looks very good, thanks for it!

To help us even more, please:
 1. attach a screenshot here (similar to the attachment of bug 
https://bugs.kde.org/show_bug.cgi?id=299431 ) showing how the problem is fixed
 2. Add commit tags to the description - see 
quickgit.kde.org/?p=kdelibs.gita=blobf=.commit-template - namely: BUG: , 
FIXED-IN: 2.9, REVIEW: ### - so that they end up in the commit
 3. Add a line to the ChangeLog file (top-level in the repo) - under BUGFIXES 
on top.


src/statusbar/ProgressBar.cpp
https://git.reviewboard.kde.org/r/114736/#comment0

Please respect Amarok coding style (see folder named HACKING) - spaces 
around arguments.


- Matěj Laitl


On Dec. 30, 2013, 7:20 a.m., Nilesh Suthar wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/114736/
 ---
 
 (Updated Dec. 30, 2013, 7:20 a.m.)
 
 
 Review request for Amarok.
 
 
 Bugs: 299431
 https://bugs.kde.org/show_bug.cgi?id=299431
 
 
 Repository: amarok
 
 
 Description
 ---
 
 QLabel for CompoundProgressbar was croping out the description from above and 
 below because QLabel default alignment is set to Horizontal left and vertical 
 Center.Changed to Vertical Top.Added Ellipsis for text overflow and tooltip 
 to view the description
 
 
 Diffs
 -
 
   src/statusbar/ProgressBar.cpp 400390f 
 
 Diff: https://git.reviewboard.kde.org/r/114736/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Nilesh Suthar
 


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

Re: Review Request 114563: Make sure not to use video stream when transcoding to Opus

[Matěj Laitl]

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

Ship it!


This patch looks good to me, but I wonder - other formats may need a similar 
option, don't they?

- Matěj Laitl


On Dec. 20, 2013, 11:06 a.m., Martin Brodbeck wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/114563/
 ---
 
 (Updated Dec. 20, 2013, 11:06 a.m.)
 
 
 Review request for Amarok.
 
 
 Repository: amarok
 
 
 Description
 ---
 
 I stumbled over a problem with transcoding to Opus. For example, if a MP3 
 file contains cover art and you transcode it to Opus, ffmpeg creates an Opus 
 file with two streams: Ogg Theora (because of the picture) and Opus. Thus, 
 this patch makes sure not to use video streams.
 
 
 Diffs
 -
 
   src/core/transcoding/formats/TranscodingOpusFormat.cpp dc6b743 
 
 Diff: http://git.reviewboard.kde.org/r/114563/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Martin Brodbeck
 


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

Re: Review Request 114406: Fetch cover picture from METADATA_BLOCK_PICTURE tag

[Matěj Laitl]

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

Ship it!


Yeah. Please don't forget following tags when comitting, also please add 
ChangeLog entry:

BUG: 328451
FIXED-IN: 2.9

- Matěj Laitl


On Dec. 11, 2013, 3:45 p.m., Martin Brodbeck wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/114406/
 ---
 
 (Updated Dec. 11, 2013, 3:45 p.m.)
 
 
 Review request for Amarok and Matěj Laitl.
 
 
 Bugs: 328451
 https://bugs.kde.org/show_bug.cgi?id=328451
 
 
 Repository: amarok
 
 
 Description
 ---
 
 With this patch, covers from Ogg files (for example Vorbis and Opus) using 
 the recommended METADATA_BLOCK_PICTURE tag will be displayed. This will fix 
 bug #328451. 
 
 
 Diffs
 -
 
   shared/tag_helpers/VorbisCommentTagHelper.cpp dc02a1c 
 
 Diff: http://git.reviewboard.kde.org/r/114406/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Martin Brodbeck
 


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

Re: Review Request 114282: Make the scripts translatable through KDE translation system

[Matěj Laitl]

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

Ship it!


Good to go from me, given all the circumstances.

- Matěj Laitl


On Dec. 7, 2013, 6:06 p.m., Yuri Chornoivan wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/114282/
 ---
 
 (Updated Dec. 7, 2013, 6:06 p.m.)
 
 
 Review request for Amarok.
 
 
 Bugs: 305264
 https://bugs.kde.org/show_bug.cgi?id=305264
 
 
 Repository: amarok
 
 
 Description
 ---
 
 Scripty can effectively help to translate .desktop files, so this patch tries 
 to trick scripty. The target translated files still installed as .spec by 
 renaming during installation.
 
 
 Diffs
 -
 
   src/scripts/librivox_service/CMakeLists.txt f4ef2da 
   src/scripts/librivox_service/script.desktop PRE-CREATION 
   src/scripts/librivox_service/script.spec 26719d5 
   src/scripts/lyrics_lyricwiki/CMakeLists.txt aa25589 
   src/scripts/lyrics_lyricwiki/script.desktop PRE-CREATION 
   src/scripts/lyrics_lyricwiki/script.spec d3346dc 
   src/scripts/radio_station_service/CMakeLists.txt d053697 
   src/scripts/radio_station_service/script.desktop PRE-CREATION 
   src/scripts/radio_station_service/script.spec 9e7fb0e 
   src/scripts/script_console/CMakeLists.txt 4d31f7c 
   src/scripts/script_console/script.desktop PRE-CREATION 
   src/scripts/script_console/script.spec 7d0a59d 
 
 Diff: http://git.reviewboard.kde.org/r/114282/diff/
 
 
 Testing
 ---
 
 Compiles, installs, can be translated through KDE translation system.
 
 
 Thanks,
 
 Yuri Chornoivan
 


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

Re: Review Request 114282: Make the scripts translatable through KDE translation system

[Matěj Laitl]

 On Dec. 7, 2013, 6:43 p.m., Matěj Laitl wrote:
  Good to go from me, given all the circumstances.

Please add following tags to the commit message when comitting, also please add 
a line into Changelog:

CCBUG: 305264
FUXED-IN: 2.9


- Matěj


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


On Dec. 7, 2013, 6:06 p.m., Yuri Chornoivan wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/114282/
 ---
 
 (Updated Dec. 7, 2013, 6:06 p.m.)
 
 
 Review request for Amarok.
 
 
 Bugs: 305264
 https://bugs.kde.org/show_bug.cgi?id=305264
 
 
 Repository: amarok
 
 
 Description
 ---
 
 Scripty can effectively help to translate .desktop files, so this patch tries 
 to trick scripty. The target translated files still installed as .spec by 
 renaming during installation.
 
 
 Diffs
 -
 
   src/scripts/librivox_service/CMakeLists.txt f4ef2da 
   src/scripts/librivox_service/script.desktop PRE-CREATION 
   src/scripts/librivox_service/script.spec 26719d5 
   src/scripts/lyrics_lyricwiki/CMakeLists.txt aa25589 
   src/scripts/lyrics_lyricwiki/script.desktop PRE-CREATION 
   src/scripts/lyrics_lyricwiki/script.spec d3346dc 
   src/scripts/radio_station_service/CMakeLists.txt d053697 
   src/scripts/radio_station_service/script.desktop PRE-CREATION 
   src/scripts/radio_station_service/script.spec 9e7fb0e 
   src/scripts/script_console/CMakeLists.txt 4d31f7c 
   src/scripts/script_console/script.desktop PRE-CREATION 
   src/scripts/script_console/script.spec 7d0a59d 
 
 Diff: http://git.reviewboard.kde.org/r/114282/diff/
 
 
 Testing
 ---
 
 Compiles, installs, can be translated through KDE translation system.
 
 
 Thanks,
 
 Yuri Chornoivan
 


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

Re: Review Request 114148: Clean up the leftover of strigi removal

[Matěj Laitl]

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

Ship it!


Good to go, thanks for finding  cleaning up the leftover.

- Matěj Laitl


On Nov. 27, 2013, 12:02 p.m., Jekyll Wu wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/114148/
 ---
 
 (Updated Nov. 27, 2013, 12:02 p.m.)
 
 
 Review request for Amarok.
 
 
 Repository: amarok
 
 
 Description
 ---
 
 No need to keep those strigi header paths, since amarok now doesn't need 
 strigi any more.
 
 
 Diffs
 -
 
   src/services/amazon/CMakeLists.txt 5bfd352 
   src/services/ampache/CMakeLists.txt 039e7fc 
   src/services/gpodder/CMakeLists.txt bab6941 
   src/services/jamendo/CMakeLists.txt 809b771 
   src/services/magnatune/CMakeLists.txt 91f24c0 
   src/services/mp3tunes/CMakeLists.txt dfa70b2 
   src/services/opmldirectory/CMakeLists.txt 05ff1eb 
 
 Diff: http://git.reviewboard.kde.org/r/114148/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Jekyll Wu
 


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

Re: Review Request 113389: GSoC 2013 Revamping Scripting - Part 5/6 : Misc shared/

[Matěj Laitl]

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


Not yet a Skip it from me, please answer some important questions below. I 
guess this patch could be renamed to Add support for reading lyrics from 
tags, right?


shared/MetaTagLib.h
http://git.reviewboard.kde.org/r/113389/#comment31476

I don't understand the need for this method. Isn't readTags() supposed to 
read lyrics and return them under Meta::valLyrics key?

What does this method return? Maps what to what? It would need 
documentation.



shared/MetaTagLib.cpp
http://git.reviewboard.kde.org/r/113389/#comment31477

You leak tagHelper in this case, I propose using QScopedPointer.

Also, I totally don't get the purpose - if ID3v2TagHelper needs special 
casing, it needs to be *inside* it.



shared/MetaValues.h
http://git.reviewboard.kde.org/r/113389/#comment31478

No, please add valLyrics after valImage with value (1LL  40) + 3;



shared/tag_helpers/ID3v2TagHelper.h
http://git.reviewboard.kde.org/r/113389/#comment31479

why the need for a custom method?


- Matěj Laitl


On Oct. 22, 2013, 6:06 p.m., Anmol Ahuja wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/113389/
 ---
 
 (Updated Oct. 22, 2013, 6:06 p.m.)
 
 
 Review request for Amarok.
 
 
 Repository: amarok
 
 
 Description
 ---
 
 Changes made to the tagging framework in shared/.
 
 
 Diffs
 -
 
   shared/MetaTagLib.h 2de3be2 
   shared/MetaTagLib.cpp d42dd32 
   shared/MetaValues.h 4b003ed 
   shared/tag_helpers/APETagHelper.cpp ba39a10 
   shared/tag_helpers/ASFTagHelper.cpp 3439845 
   shared/tag_helpers/ID3v2TagHelper.h a9c9ab9 
   shared/tag_helpers/ID3v2TagHelper.cpp 03a3836 
   shared/tag_helpers/MP4TagHelper.cpp 23d8879 
   shared/tag_helpers/VorbisCommentTagHelper.cpp 8ff52c5 
 
 Diff: http://git.reviewboard.kde.org/r/113389/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Anmol Ahuja
 


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

Re: phonon5 and times

[Matěj Laitl]

On 14. 11. 2013 Harald Sitter wrote:
 for phonon5 we can:
 a) keep it and rename it to timeUpdateInterval (to align with rename of
 tick)
 b) drop the interval; always use =100 msc updates
 c) drop the interval; always use ~100 msc (if VLC is ticking faster
 than that we go 100)
 d) any of b) or c) AND when nothing is connected to timeChanged() no
 ticks will be generated in general == no resources consumption
 (QObject::connectNotify) AND time() does not access cached values but
 will lead to a direct pipeline query
 
 personally I would go with b+d) as to me those make most sense. by
 default there's a sensible update interval that is not too resource
 consuming. if one wants more precise time values then you'd simply not
 use the builtin timeChanged but use your own QTimer to query time().
 which actually is superior to what we had before because it actually
 encourages avoiding a lot of our abstraction overhead when one wants
 accurate values (calling time() will result in a direct access on the
 pipeline, timeChanged() is a queued and therefore eventloop bound).
 
 thoughts? opinions?

For Amarok, b+d) would be most sensible.

We're currently setting tickInterval to 100ms and listening to tick() signal, 
but I'd like to change it to do our own QTimer with computed spatial 
interval (that we can additionally stop when the window is hidden etc.), 
querying time() each time (assuming it is reasonably precise and cheap in 
order not to introduce flicker).

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

Re: Review Request 113272: GSoC 2013 - Advanced Importers - 1/4: Changes in StatSyncing framework

[Matěj Laitl]

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

Ship it!


Looks good to me. You can either merge (fixing some trivial things below) right 
now or wait for me to review the rest of your GSoC.


src/statsyncing/Controller.h
http://git.reviewboard.kde.org/r/113272/#comment31389

I think none of these includes are needed any more.



src/statsyncing/Provider.h
http://git.reviewboard.kde.org/r/113272/#comment31390

Is this include needed?



src/statsyncing/ui/ConfigureProviderDialog.h
http://git.reviewboard.kde.org/r/113272/#comment31392

We're not really consistent in indentation of namespaced class definitions 
in Amarok code, but please keep consistency within the statsyncing component: 
indent as in statsyncing/Track.h etc.



src/statsyncing/ui/CreateProviderDialog.h
http://git.reviewboard.kde.org/r/113272/#comment31394

Please rename include guard to match class name



src/statsyncing/ui/CreateProviderDialog.h
http://git.reviewboard.kde.org/r/113272/#comment31393

ditto indentation


- Matěj Laitl


On Nov. 6, 2013, 8:19 p.m., Konrad Zemek wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/113272/
 ---
 
 (Updated Nov. 6, 2013, 8:19 p.m.)
 
 
 Review request for Amarok.
 
 
 Repository: amarok
 
 
 Description
 ---
 
 These are changes in the StatSyncing framework that I made as a part of my 
 project.
 
 
 Diffs
 -
 
   src/statsyncing/Config.h 140e647 
   src/statsyncing/Config.cpp dd82dbe 
   src/statsyncing/Controller.h 10c72a8 
   src/statsyncing/Controller.cpp bf4c5d8 
   src/statsyncing/Provider.h d9663f9 
   src/statsyncing/Provider.cpp 775fce3 
   src/statsyncing/ProviderFactory.h PRE-CREATION 
   src/statsyncing/ProviderFactory.cpp PRE-CREATION 
   src/statsyncing/SimpleTrack.h PRE-CREATION 
   src/statsyncing/SimpleTrack.cpp PRE-CREATION 
   src/statsyncing/SimpleWritableTrack.h PRE-CREATION 
   src/statsyncing/SimpleWritableTrack.cpp PRE-CREATION 
   src/statsyncing/Track.h 2f91704 
   src/statsyncing/Track.cpp 9655cc1 
   src/statsyncing/TrackTuple.h 157d604 
   src/statsyncing/TrackTuple.cpp 9442794 
   src/statsyncing/jobs/SynchronizeTracksJob.cpp b789aa3 
   src/statsyncing/ui/ConfigureProviderDialog.h PRE-CREATION 
   src/statsyncing/ui/ConfigureProviderDialog.cpp PRE-CREATION 
   src/statsyncing/ui/CreateProviderDialog.h PRE-CREATION 
   src/statsyncing/ui/CreateProviderDialog.cpp PRE-CREATION 
 
 Diff: http://git.reviewboard.kde.org/r/113272/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Konrad Zemek
 


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

Re: Review Request 113277: GSoC 2013 - Advanced Importers - 3/4: Tests for importers framework and concrete importers

[Matěj Laitl]

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

Ship it!


Tests are always welcome (thanks for them!), so this is obviously Ship it. 
However I suggest some shuffling around and adding 2 README files to ensure 
that some hypothetical $next_maintainer in the future can pick up our work 
easily.


Music
http://git.reviewboard.kde.org/r/113277/#comment31400

I think this file belongs to tests/importers/. (perhaps 
tests/importers/files/ up to you)

I also think it deserves Library.xml.README file that would very briefly 
explain raison d'être of the file and how to use it.



tests/importers/files/illFormedLibrary.xml
http://git.reviewboard.kde.org/r/113277/#comment31401

I think the tests/importers/files/ directory deserves a README file with 
very brief description of the contents and what are they used for. Perhaps also 
with recipe how to (re)generate files in the testcollection directory.

What is the size of the mysqle directory? (which I assume is an Amarok 2.x 
database (hint hint))


- Matěj Laitl


On Nov. 6, 2013, 8:24 p.m., Konrad Zemek wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/113277/
 ---
 
 (Updated Nov. 6, 2013, 8:24 p.m.)
 
 
 Review request for Amarok.
 
 
 Repository: amarok
 
 
 Description
 ---
 
 This review contains importers tests.
 
 The tests/importers/TestImporterBase.cpp contains common tests for concrete
 importers. The tests load a prepared database and read it through the 
 importer.
 It's therefore important that the prepared database contain certain data.
 The tests/importers/files/testCollection directory contains very small mp3
 files that are *not* used as a part of the tests, and instead are meant for
 creating the test database: one would just import these files into a media
 player, copy the media player's database and have it instantly ready for
 the common tests.
 
 
 Diffs
 -
 
   Music PRE-CREATION 
   tests/importers/CMakeLists.txt PRE-CREATION 
   tests/importers/ImporterMocks.h PRE-CREATION 
   tests/importers/ImporterMocks.cpp PRE-CREATION 
   tests/importers/TestAmarokImporter.h PRE-CREATION 
   tests/importers/TestAmarokImporter.cpp PRE-CREATION 
   tests/importers/TestBansheeImporter.h PRE-CREATION 
   tests/importers/TestBansheeImporter.cpp PRE-CREATION 
   tests/importers/TestClementineImporter.h PRE-CREATION 
   tests/importers/TestClementineImporter.cpp PRE-CREATION 
   tests/importers/TestFastForwardImporter.h PRE-CREATION 
   tests/importers/TestFastForwardImporter.cpp PRE-CREATION 
   tests/importers/TestITunesImporter.h PRE-CREATION 
   tests/importers/TestITunesImporter.cpp PRE-CREATION 
   tests/importers/TestImporterBase.h PRE-CREATION 
   tests/importers/TestImporterBase.cpp PRE-CREATION 
   tests/importers/TestImporterManager.h PRE-CREATION 
   tests/importers/TestImporterManager.cpp PRE-CREATION 
   tests/importers/TestImporterProvider.h PRE-CREATION 
   tests/importers/TestImporterProvider.cpp PRE-CREATION 
   tests/importers/TestRhythmboxImporter.h PRE-CREATION 
   tests/importers/TestRhythmboxImporter.cpp PRE-CREATION 
   tests/importers/TestSimpleImporterConfigWidget.h PRE-CREATION 
   tests/importers/TestSimpleImporterConfigWidget.cpp PRE-CREATION 
   tests/importers/files/banshee.db PRE-CREATION 
   tests/importers/files/clementine.db PRE-CREATION 
   tests/importers/files/collection.db PRE-CREATION 
   tests/importers/files/illFormedLibrary.xml PRE-CREATION 
   tests/importers/files/mysqle/amarok/admin.MYD PRE-CREATION 
   tests/importers/files/mysqle/amarok/admin.MYI PRE-CREATION 
   tests/importers/files/mysqle/amarok/admin.frm PRE-CREATION 
   tests/importers/files/mysqle/amarok/albums.MYD PRE-CREATION 
   tests/importers/files/mysqle/amarok/albums.MYI PRE-CREATION 
   tests/importers/files/mysqle/amarok/albums.frm PRE-CREATION 
   tests/importers/files/mysqle/amarok/amazon.MYI PRE-CREATION 
   tests/importers/files/mysqle/amarok/amazon.frm PRE-CREATION 
   tests/importers/files/mysqle/amarok/artists.MYD PRE-CREATION 
   tests/importers/files/mysqle/amarok/artists.MYI PRE-CREATION 
   tests/importers/files/mysqle/amarok/artists.frm PRE-CREATION 
   tests/importers/files/mysqle/amarok/bookmark_groups.MYI PRE-CREATION 
   tests/importers/files/mysqle/amarok/bookmark_groups.frm PRE-CREATION 
   tests/importers/files/mysqle/amarok/bookmarks.MYI PRE-CREATION 
   tests/importers/files/mysqle/amarok/bookmarks.frm PRE-CREATION 
   tests/importers/files/mysqle/amarok/composers.MYD PRE-CREATION 
   tests/importers/files/mysqle/amarok/composers.MYI PRE-CREATION 
   tests/importers/files/mysqle/amarok/composers.frm PRE-CREATION 
   tests/importers/files

Re: Review Request 113777: Lazy load scriptable service tracks

[Matěj Laitl]

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


Looks good to me, but please test that scriptable service tracks that are 
(remote) playlists do play right away when you double-click on them. (with play 
on double-click enabled)


src/services/scriptable/ScriptableServiceMeta.h
http://git.reviewboard.kde.org/r/113777/#comment31198

Please initialize (to false, probably) this basic type in constructor as it 
is random otherwise.


- Matěj Laitl


On Nov. 9, 2013, 10:21 p.m., Anmol Ahuja wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/113777/
 ---
 
 (Updated Nov. 9, 2013, 10:21 p.m.)
 
 
 Review request for Amarok.
 
 
 Repository: amarok
 
 
 Description
 ---
 
 Prevent all the scriptable services from loading playlists at Amarok launch.
 
 
 Diffs
 -
 
   src/services/scriptable/ScriptableServiceMeta.h 2db3951 
   src/services/scriptable/ScriptableServiceMeta.cpp 8e96785 
 
 Diff: http://git.reviewboard.kde.org/r/113777/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Anmol Ahuja
 


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

Review Request 113666: GSoC: MTP (Android) Collection Rewrite

[Matěj Laitl]

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

Review request for Amarok.


Repository: amarok


Description
---

This is my full GSoC project to rewrite MTP collection.

Notes:
 * Individual commits can be seen at 
http://quickgit.kde.org/?p=clones%2Famarok%2Flaitl%2Famarok.gita=shortlogh=gsoc
 * First commit to remove the old implementation is not included here for 
brevity
 * Project description can be seen at 
https://google-melange.appspot.com/gsoc/project/google/gsoc2013/strohel/32001
 * More info, progress reports and final report available at 
http://strohel.blogspot.com/search/label/gsoc


Diffs
-

  cmake/modules/FindMtp.cmake a6b7a0e 
  src/core-impl/collections/CMakeLists.txt 4785158 
  src/core-impl/collections/mtp/CMakeLists.txt PRE-CREATION 
  src/core-impl/collections/mtp/MtpCollection.h PRE-CREATION 
  src/core-impl/collections/mtp/MtpCollection.cpp PRE-CREATION 
  src/core-impl/collections/mtp/MtpCollectionFactory.h PRE-CREATION 
  src/core-impl/collections/mtp/MtpCollectionFactory.cpp PRE-CREATION 
  src/core-impl/collections/mtp/MtpCollectionLocation.h PRE-CREATION 
  src/core-impl/collections/mtp/MtpCollectionLocation.cpp PRE-CREATION 
  src/core-impl/collections/mtp/TODO PRE-CREATION 
  src/core-impl/collections/mtp/amarok-mtp-manage-music.desktop PRE-CREATION 
  src/core-impl/collections/mtp/amarok_collection-mtp.desktop PRE-CREATION 
  src/core-impl/collections/mtp/jobs/CopyMtpTracksJob.h PRE-CREATION 
  src/core-impl/collections/mtp/jobs/CopyMtpTracksJob.cpp PRE-CREATION 
  src/core-impl/collections/mtp/jobs/DeleteMtpTracksJob.h PRE-CREATION 
  src/core-impl/collections/mtp/jobs/DeleteMtpTracksJob.cpp PRE-CREATION 
  src/core-impl/collections/mtp/jobs/DownloadMtpTrackJob.h PRE-CREATION 
  src/core-impl/collections/mtp/jobs/DownloadMtpTrackJob.cpp PRE-CREATION 
  src/core-impl/collections/mtp/jobs/InitMtpDeviceJob.h PRE-CREATION 
  src/core-impl/collections/mtp/jobs/InitMtpDeviceJob.cpp PRE-CREATION 
  src/core-impl/collections/mtp/jobs/ListMtpStorageJob.h PRE-CREATION 
  src/core-impl/collections/mtp/jobs/ListMtpStorageJob.cpp PRE-CREATION 
  src/core-impl/collections/mtp/jobs/MtpTransferJob.h PRE-CREATION 
  src/core-impl/collections/mtp/jobs/MtpTransferJob.cpp PRE-CREATION 
  src/core-impl/collections/mtp/jobs/ParseMtpTracksJob.h PRE-CREATION 
  src/core-impl/collections/mtp/jobs/ParseMtpTracksJob.cpp PRE-CREATION 
  src/core-impl/collections/mtp/jobs/SetMtpDeviceNameJob.h PRE-CREATION 
  src/core-impl/collections/mtp/jobs/SetMtpDeviceNameJob.cpp PRE-CREATION 
  src/core-impl/collections/mtp/jobs/UpdateMtpTrackJob.h PRE-CREATION 
  src/core-impl/collections/mtp/jobs/UpdateMtpTrackJob.cpp PRE-CREATION 
  src/core-impl/collections/mtp/jobs/UploadMtpTrackJob.h PRE-CREATION 
  src/core-impl/collections/mtp/jobs/UploadMtpTrackJob.cpp PRE-CREATION 
  src/core-impl/collections/mtp/meta/MtpAlbum.h PRE-CREATION 
  src/core-impl/collections/mtp/meta/MtpAlbum.cpp PRE-CREATION 
  src/core-impl/collections/mtp/meta/MtpEntity.h PRE-CREATION 
  src/core-impl/collections/mtp/meta/MtpTrack.h PRE-CREATION 
  src/core-impl/collections/mtp/meta/MtpTrack.cpp PRE-CREATION 
  src/core-impl/collections/mtp/support/MtpDeviceConfiguration.ui PRE-CREATION 
  src/core-impl/collections/mtp/support/MtpHelpers.h PRE-CREATION 
  src/core-impl/collections/mtp/support/MtpHelpers.cpp PRE-CREATION 
  src/core-impl/collections/mtp/support/MtpTranscodeCapability.h PRE-CREATION 
  src/core-impl/collections/mtp/support/MtpTranscodeCapability.cpp PRE-CREATION 
  src/core-impl/collections/mtp/support/MtpTransferJanitor.h PRE-CREATION 
  src/core-impl/collections/mtp/support/MtpTransferJanitor.cpp PRE-CREATION 
  src/core-impl/collections/mtp/support/OneToOneMap.h PRE-CREATION 
  src/core-impl/collections/mtp/support/OneToOneMap.cpp PRE-CREATION 
  src/core-impl/collections/mtp/support/forward_declarations.h PRE-CREATION 

Diff: http://git.reviewboard.kde.org/r/113666/diff/


Testing
---

Works well with my Samsung Android 4.1 phone; testing with greater variety of 
devices needed.


Thanks,

Matěj Laitl

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

Re: Review Request 113272: GSoC 2013 - Advanced Importers - 1/4: Changes in StatSyncing framework

[Matěj Laitl]

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


I'm starring at Diff v2 and either:
 1. I'm a fool
 2. ReviewBoard fools me (actually rather probable)
 3. You uploaded a diff between old part 1 and new part 1, not the complete new 
part 1.

I'm sorry it took too long for me just to realize this.

- Matěj Laitl


On Oct. 21, 2013, 6:34 p.m., Konrad Zemek wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/113272/
 ---
 
 (Updated Oct. 21, 2013, 6:34 p.m.)
 
 
 Review request for Amarok.
 
 
 Repository: amarok
 
 
 Description
 ---
 
 These are changes in the StatSyncing framework that I made as a part of my 
 project.
 
 
 Diffs
 -
 
   src/statsyncing/Config.cpp dd82dbe 
   src/statsyncing/Controller.h 10c72a8 
   src/statsyncing/Controller.cpp bf4c5d8 
   src/statsyncing/Provider.h d9663f9 
   src/statsyncing/Provider.cpp 775fce3 
   src/statsyncing/ProviderFactory.h PRE-CREATION 
   src/statsyncing/ProviderFactory.cpp PRE-CREATION 
   src/statsyncing/Track.h 2f91704 
   src/statsyncing/Track.cpp 9655cc1 
 
 Diff: http://git.reviewboard.kde.org/r/113272/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Konrad Zemek
 


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

Re: phonon5 and the metadata

[Matěj Laitl]

On 2. 11. 2013 Harald Sitter wrote:
 Strawpoll:
 
 What do you prefer? What do you use right now?
 
 QStringList Player::metaData(Enum e);
 or
 QStringList Player::metaData(QString s); // as per [1]

In Amarok we only use (and prefer) metaData(Enum e); feel free to remove the 
QString one.

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

Re: Review Request 113057: I am trying to kill Bug 322016 - Apply button is always enabled in Playlist Layout Editor dialog

[Matěj Laitl]

 On Oct. 2, 2013, 7:34 p.m., Matěj Laitl wrote:
  Hi, thanks for your patch. Unfortunately, this is not the right approach. 
  Instead of tracking user changes, the logic under apply button should be:
  1. watch every state change in the applicable controls and pressing of the 
  apply button.
  2. when a change happens, compare the current visible configuration with 
  the last saved one and call setEnabled( settingsDiffer ); where 
  settingsDiffer is a bool with obvious meaning.
 
 Ricardo Varas wrote:
 Hello, thanks for your comment. And do you know how to obtain if the OK 
 or Cancel button form the token has been clicked?

Not without studying the code, you must try yourself. :)


- Matěj


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


On Oct. 2, 2013, 7:27 p.m., Ricardo Varas wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/113057/
 ---
 
 (Updated Oct. 2, 2013, 7:27 p.m.)
 
 
 Review request for Amarok.
 
 
 Bugs: 322016
 https://bugs.kde.org/show_bug.cgi?id=322016
 
 
 Repository: amarok
 
 
 Description
 ---
 
 I am trying to kill Bug 322016 - Apply button is always enabled in Playlist 
 Layout Editor dialog.
 In multiple parts I call the enable property for the Apply button and 
 enable/disable it according to specific actions. I tried to cover all 
 possible actions from a user --of course it begins in the disabled status as 
 requested and it's disabled after the user clicks it --.
 One thing I am not happy about is the fact that when the Token (the option 
 with the tool icon) is used to configure the playlist items such as album, 
 album artist, etc. then the Apply button is always enabled and I believe 
 under that case it should only become enabled if the Configuration for... 
 dialog's OK button is clicked, and remain disabled after the Cancel button is 
 clicked or the dialog is closed. I think LayoutEditDialog.cpp is called every 
 time the token is chosen but right not I am unable to understand how they are 
 connected LayoutEditDialog.cpp and PlaylistLayoutEditDialog.cpp.
 I'd appreciate if someone can help me understand this one and put together a 
 good patch for this bug.
 Thanks.
 
 
 Diffs
 -
 
   PlaylistLayoutEditDialog1.cpp 99aee2a 
 
 Diff: http://git.reviewboard.kde.org/r/113057/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Ricardo Varas
 


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

Re: Review Request 113356: GSoC 2013: Revamping Amarok's Scripting Interface

[Matěj Laitl]

 On Oct. 20, 2013, 3:36 p.m., Matěj Laitl wrote:
  Thanks for finally posting this patch, please give me a couple of days to 
  review it.

I guess this review could be discarded because you've resubmitted it in 
multiple parts?


- Matěj


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


On Oct. 20, 2013, 3:13 p.m., Anmol Ahuja wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/113356/
 ---
 
 (Updated Oct. 20, 2013, 3:13 p.m.)
 
 
 Review request for Amarok.
 
 
 Repository: amarok
 
 
 Description
 ---
 
 Changes made as part of my GSoC project.
 
 
 Diffs
 -
 
   CMakeLists.txt fa1ab0c 
   cmake/modules/FindQtScriptQtBindings.cmake bd24328 
   cmake/modules/bindingstest/QtScriptBindingsTest.cpp 424556f 
   shared/MetaTagLib.h 2de3be2 
   shared/MetaTagLib.cpp d42dd32 
   shared/MetaValues.h 4b003ed 
   shared/tag_helpers/APETagHelper.cpp ba39a10 
   shared/tag_helpers/ASFTagHelper.cpp 3439845 
   shared/tag_helpers/ID3v2TagHelper.h a9c9ab9 
   shared/tag_helpers/ID3v2TagHelper.cpp 03a3836 
   shared/tag_helpers/MP4TagHelper.cpp 23d8879 
   shared/tag_helpers/VorbisCommentTagHelper.cpp 8ff52c5 
   src/App.h 9ee071d 
   src/App.cpp eb3f483 
   src/CMakeLists.txt 70fb67b 
   src/EngineController.h 80ec17b 
   src/EngineController.cpp cf29cf8 
   src/ScriptManager.h 1a252ca 
   src/ScriptManager.cpp 04b8eef 
   src/ScriptUpdater.h 0e9b37d 
   src/ScriptUpdater.cpp f174248 
   src/amarokconfig.kcfg 84cfa5b 
   src/amarokurls/BookmarkGroup.h ea42c30 
   src/browsers/CollectionTreeItem.h 0a5d197 
   src/browsers/CollectionTreeItem.cpp 4c35523 
   src/browsers/CollectionTreeItemModelBase.h 00e99fb 
   src/browsers/CollectionTreeView.h 454a70d 
   src/browsers/CollectionTreeView.cpp 6fad164 
   src/browsers/collectionbrowser/CollectionWidget.h 7480015 
   src/browsers/collectionbrowser/CollectionWidget.cpp 95a16dc 
   src/configdialog/dialogs/ScriptSelector.h 9ce1168 
   src/configdialog/dialogs/ScriptSelector.cpp 117cedf 
   src/configdialog/dialogs/ScriptsConfig.h 6a47847 
   src/configdialog/dialogs/ScriptsConfig.cpp 714b63e 
   src/configdialog/dialogs/ScriptsConfig.ui b4b8d37 
   src/context/applets/lyrics/LyricsApplet.cpp 7db356d 
   src/context/applets/songkick/SongkickApplet.cpp ed93e56 
   src/context/engines/lyrics/LyricsEngine.cpp d3273b0 
   src/core-impl/collections/support/jobs/WriteTagsJob.h a92ab8d 
   src/core-impl/collections/support/jobs/WriteTagsJob.cpp 5834cd4 
   src/core/collections/Collection.h 94d24d7 
   src/core/collections/QueryMaker.h 92b6a65 
   src/core/meta/support/MetaConstants.cpp 40d7122 
   src/dialogs/DiagnosticDialog.cpp c9bfce6 
   src/dialogs/EqualizerDialog.h 6c9e19e 
   src/dialogs/EqualizerDialog.cpp acf0da0 
   src/dialogs/deviceconfiguredialog.cpp e54edad 
   src/dynamic/BiasFactory.cpp 2a887c2 
   src/dynamic/TrackSet.h dee8ee3 
   src/dynamic/TrackSet.cpp 456a10a 
   src/equalizer/EqualizerPresets.h be8d267 
   src/equalizer/EqualizerPresets.cpp b9aa13b 
   src/playback/EqualizerController.h PRE-CREATION 
   src/playback/EqualizerController.cpp PRE-CREATION 
   src/playlistmanager/PlaylistManager.h a835c35 
   src/scriptengine/AmarokCollectionScript.h 224bd54 
   src/scriptengine/AmarokCollectionScript.cpp 3fcda84 
   src/scriptengine/AmarokEngineScript.h aa2b112 
   src/scriptengine/AmarokEngineScript.cpp 5471838 
   src/scriptengine/AmarokInfoScript.h 754498e 
   src/scriptengine/AmarokInfoScript.cpp 4d06568 
   src/scriptengine/AmarokKNotifyScript.h 720f9f6 
   src/scriptengine/AmarokKNotifyScript.cpp b98efed 
   src/scriptengine/AmarokLyricsScript.h a47c609 
   src/scriptengine/AmarokLyricsScript.cpp b1972fa 
   src/scriptengine/AmarokNetworkScript.h 785a645 
   src/scriptengine/AmarokNetworkScript.cpp 0eaa440 
   src/scriptengine/AmarokOSDScript.h 992fcee 
   src/scriptengine/AmarokOSDScript.cpp 659cd68 
   src/scriptengine/AmarokPlaylistScript.h a9df74e 
   src/scriptengine/AmarokPlaylistScript.cpp 2dde634 
   src/scriptengine/AmarokScript.h 4ec6d8f 
   src/scriptengine/AmarokScript.cpp b21a2ec 
   src/scriptengine/AmarokScriptConfig.h a77418f 
   src/scriptengine/AmarokScriptConfig.cpp 457104a 
   src/scriptengine/AmarokScriptableServiceScript.h 6afacfa 
   src/scriptengine/AmarokScriptableServiceScript.cpp a312671 
   src/scriptengine/AmarokServicePluginManagerScript.h ed7f307 
   src/scriptengine/AmarokServicePluginManagerScript.cpp 86f841c 
   src/scriptengine/AmarokStatusbarScript.h c871213 
   src/scriptengine/AmarokStatusbarScript.cpp 6efbb38 
   src/scriptengine/AmarokWindowScript.h 5b18e18 
   src/scriptengine/AmarokWindowScript.cpp c9a65ee 
   src/scriptengine/MetaTypeExporter.h ec961e8

Re: Review Request 113356: GSoC 2013: Revamping Amarok's Scripting Interface

[Matěj Laitl]

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


Thanks for finally posting this patch, please give me a couple of days to 
review it.

- Matěj Laitl


On Oct. 20, 2013, 3:13 p.m., Anmol Ahuja wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/113356/
 ---
 
 (Updated Oct. 20, 2013, 3:13 p.m.)
 
 
 Review request for Amarok.
 
 
 Repository: amarok
 
 
 Description
 ---
 
 Changes made as part of my GSoC project.
 
 
 Diffs
 -
 
   CMakeLists.txt fa1ab0c 
   cmake/modules/FindQtScriptQtBindings.cmake bd24328 
   cmake/modules/bindingstest/QtScriptBindingsTest.cpp 424556f 
   shared/MetaTagLib.h 2de3be2 
   shared/MetaTagLib.cpp d42dd32 
   shared/MetaValues.h 4b003ed 
   shared/tag_helpers/APETagHelper.cpp ba39a10 
   shared/tag_helpers/ASFTagHelper.cpp 3439845 
   shared/tag_helpers/ID3v2TagHelper.h a9c9ab9 
   shared/tag_helpers/ID3v2TagHelper.cpp 03a3836 
   shared/tag_helpers/MP4TagHelper.cpp 23d8879 
   shared/tag_helpers/VorbisCommentTagHelper.cpp 8ff52c5 
   src/App.h 9ee071d 
   src/App.cpp eb3f483 
   src/CMakeLists.txt 70fb67b 
   src/EngineController.h 80ec17b 
   src/EngineController.cpp cf29cf8 
   src/ScriptManager.h 1a252ca 
   src/ScriptManager.cpp 04b8eef 
   src/ScriptUpdater.h 0e9b37d 
   src/ScriptUpdater.cpp f174248 
   src/amarokconfig.kcfg 84cfa5b 
   src/amarokurls/BookmarkGroup.h ea42c30 
   src/browsers/CollectionTreeItem.h 0a5d197 
   src/browsers/CollectionTreeItem.cpp 4c35523 
   src/browsers/CollectionTreeItemModelBase.h 00e99fb 
   src/browsers/CollectionTreeView.h 454a70d 
   src/browsers/CollectionTreeView.cpp 6fad164 
   src/browsers/collectionbrowser/CollectionWidget.h 7480015 
   src/browsers/collectionbrowser/CollectionWidget.cpp 95a16dc 
   src/configdialog/dialogs/ScriptSelector.h 9ce1168 
   src/configdialog/dialogs/ScriptSelector.cpp 117cedf 
   src/configdialog/dialogs/ScriptsConfig.h 6a47847 
   src/configdialog/dialogs/ScriptsConfig.cpp 714b63e 
   src/configdialog/dialogs/ScriptsConfig.ui b4b8d37 
   src/context/applets/lyrics/LyricsApplet.cpp 7db356d 
   src/context/applets/songkick/SongkickApplet.cpp ed93e56 
   src/context/engines/lyrics/LyricsEngine.cpp d3273b0 
   src/core-impl/collections/support/jobs/WriteTagsJob.h a92ab8d 
   src/core-impl/collections/support/jobs/WriteTagsJob.cpp 5834cd4 
   src/core/collections/Collection.h 94d24d7 
   src/core/collections/QueryMaker.h 92b6a65 
   src/core/meta/support/MetaConstants.cpp 40d7122 
   src/dialogs/DiagnosticDialog.cpp c9bfce6 
   src/dialogs/EqualizerDialog.h 6c9e19e 
   src/dialogs/EqualizerDialog.cpp acf0da0 
   src/dialogs/deviceconfiguredialog.cpp e54edad 
   src/dynamic/BiasFactory.cpp 2a887c2 
   src/dynamic/TrackSet.h dee8ee3 
   src/dynamic/TrackSet.cpp 456a10a 
   src/equalizer/EqualizerPresets.h be8d267 
   src/equalizer/EqualizerPresets.cpp b9aa13b 
   src/playback/EqualizerController.h PRE-CREATION 
   src/playback/EqualizerController.cpp PRE-CREATION 
   src/playlistmanager/PlaylistManager.h a835c35 
   src/scriptengine/AmarokCollectionScript.h 224bd54 
   src/scriptengine/AmarokCollectionScript.cpp 3fcda84 
   src/scriptengine/AmarokEngineScript.h aa2b112 
   src/scriptengine/AmarokEngineScript.cpp 5471838 
   src/scriptengine/AmarokInfoScript.h 754498e 
   src/scriptengine/AmarokInfoScript.cpp 4d06568 
   src/scriptengine/AmarokKNotifyScript.h 720f9f6 
   src/scriptengine/AmarokKNotifyScript.cpp b98efed 
   src/scriptengine/AmarokLyricsScript.h a47c609 
   src/scriptengine/AmarokLyricsScript.cpp b1972fa 
   src/scriptengine/AmarokNetworkScript.h 785a645 
   src/scriptengine/AmarokNetworkScript.cpp 0eaa440 
   src/scriptengine/AmarokOSDScript.h 992fcee 
   src/scriptengine/AmarokOSDScript.cpp 659cd68 
   src/scriptengine/AmarokPlaylistScript.h a9df74e 
   src/scriptengine/AmarokPlaylistScript.cpp 2dde634 
   src/scriptengine/AmarokScript.h 4ec6d8f 
   src/scriptengine/AmarokScript.cpp b21a2ec 
   src/scriptengine/AmarokScriptConfig.h a77418f 
   src/scriptengine/AmarokScriptConfig.cpp 457104a 
   src/scriptengine/AmarokScriptableServiceScript.h 6afacfa 
   src/scriptengine/AmarokScriptableServiceScript.cpp a312671 
   src/scriptengine/AmarokServicePluginManagerScript.h ed7f307 
   src/scriptengine/AmarokServicePluginManagerScript.cpp 86f841c 
   src/scriptengine/AmarokStatusbarScript.h c871213 
   src/scriptengine/AmarokStatusbarScript.cpp 6efbb38 
   src/scriptengine/AmarokWindowScript.h 5b18e18 
   src/scriptengine/AmarokWindowScript.cpp c9a65ee 
   src/scriptengine/MetaTypeExporter.h ec961e8 
   src/scriptengine/MetaTypeExporter.cpp d832181 
   src/scriptengine/ScriptImporter.h 7ae9155 
   src/scriptengine/ScriptImporter.cpp

Re: Review Request 113346: BugFix: 317902 - Issue a warning when transcoding is not possible

[Matěj Laitl]

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


Hi, thanks for the patch. It certainly goes in the right direction, but it 
could be simplified in order not to touch the API that much. Also please attach 
here a screenshot of the dialog with the next version of this patch.


src/core-impl/collections/support/CollectionLocationDelegateImpl.h
http://git.reviewboard.kde.org/r/113346/#comment30633

TranscodingAssistantDialog knows what encoders are available (see its 
populateFormatList() method), no need to pass this as an argument. Also, please 
mind Amarok coding style (space between transcode( and const)



src/core-impl/collections/support/CollectionLocationDelegateImpl.cpp
http://git.reviewboard.kde.org/r/113346/#comment30634

ditto TranscodingAssistantDialog knows.



src/core/collections/CollectionLocation.cpp
http://git.reviewboard.kde.org/r/113346/#comment30635

ditto TranscodingAssistantDialog knows.



src/core/collections/CollectionLocation.cpp
http://git.reviewboard.kde.org/r/113346/#comment30636

ditto TranscodingAssistantDialog knows.



src/core/collections/CollectionLocationDelegate.h
http://git.reviewboard.kde.org/r/113346/#comment30637

ditto TranscodingAssistantDialog knows.



src/transcoding/TranscodingAssistantDialog.h
http://git.reviewboard.kde.org/r/113346/#comment30640

ditto TranscodingAssistantDialog knows.



src/transcoding/TranscodingAssistantDialog.ui
http://git.reviewboard.kde.org/r/113346/#comment30638

No need to specify default properties, please use the reset button in Qt 
Designer to make it default (non-bold in Qt Designer UI)



src/transcoding/TranscodingAssistantDialog.ui
http://git.reviewboard.kde.org/r/113346/#comment30639

The text could be even more helpful, i.e. tell user what she needs to 
install (ffmpeg or libav with ffmpeg wrapper).


- Matěj Laitl


On Oct. 19, 2013, 11:12 a.m., Jai  Luthra wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/113346/
 ---
 
 (Updated Oct. 19, 2013, 11:12 a.m.)
 
 
 Review request for Amarok.
 
 
 Bugs: 317902
 https://bugs.kde.org/show_bug.cgi?id=317902
 
 
 Repository: amarok
 
 
 Description
 ---
 
 When ffmpeg is not available, the transcoding dialog will not be skipped; 
 rather it will add a note for the user to install an encoder.
 
 
 Diffs
 -
 
   src/core-impl/collections/support/CollectionLocationDelegateImpl.h 9d90580 
   src/core-impl/collections/support/CollectionLocationDelegateImpl.cpp 
 14fd251 
   src/core/collections/CollectionLocation.cpp 2385570 
   src/core/collections/CollectionLocationDelegate.h 3c80d07 
   src/transcoding/TranscodingAssistantDialog.h 0acb701 
   src/transcoding/TranscodingAssistantDialog.cpp 17fc250 
   src/transcoding/TranscodingAssistantDialog.ui 0ad83b2 
 
 Diff: http://git.reviewboard.kde.org/r/113346/diff/
 
 
 Testing
 ---
 
 * Compiles
 * Works as expected
 
 
 Thanks,
 
 Jai  Luthra
 


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

Re: Review Request 113346: BugFix: 317902 - Issue a warning when transcoding is not possible

[Matěj Laitl]

 On Oct. 19, 2013, 7:49 p.m., Matěj Laitl wrote:
  src/core-impl/collections/support/CollectionLocationDelegateImpl.h, lines 
  41-42
  http://git.reviewboard.kde.org/r/113346/diff/3/?file=203475#file203475line41
 
  TranscodingAssistantDialog knows what encoders are available (see its 
  populateFormatList() method), no need to pass this as an argument. Also, 
  please mind Amarok coding style (space between transcode( and const)
 
 Jai  Luthra wrote:
 Inside populateFormatList(), if available.isEmpty(), I think I should 
 call (and define) another method 
 TranscodingAssistantDialog::encoderNotFound() rather than changing the UI 
 from the if block itself. Right?

That's up to you, the code to show the warning is simple, no problem with 
embedding it into populateFormatList().

Another idea: add You may check i%1/i in order to skip this dialog for 
future transfers - where %1 is text of the checkbox button to do this. (please 
do it this way to ensure that the text is exactly the same even in translations)


- Matěj


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


On Oct. 19, 2013, 11:12 a.m., Jai  Luthra wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/113346/
 ---
 
 (Updated Oct. 19, 2013, 11:12 a.m.)
 
 
 Review request for Amarok.
 
 
 Bugs: 317902
 https://bugs.kde.org/show_bug.cgi?id=317902
 
 
 Repository: amarok
 
 
 Description
 ---
 
 When ffmpeg is not available, the transcoding dialog will not be skipped; 
 rather it will add a note for the user to install an encoder.
 
 
 Diffs
 -
 
   src/core-impl/collections/support/CollectionLocationDelegateImpl.h 9d90580 
   src/core-impl/collections/support/CollectionLocationDelegateImpl.cpp 
 14fd251 
   src/core/collections/CollectionLocation.cpp 2385570 
   src/core/collections/CollectionLocationDelegate.h 3c80d07 
   src/transcoding/TranscodingAssistantDialog.h 0acb701 
   src/transcoding/TranscodingAssistantDialog.cpp 17fc250 
   src/transcoding/TranscodingAssistantDialog.ui 0ad83b2 
 
 Diff: http://git.reviewboard.kde.org/r/113346/diff/
 
 
 Testing
 ---
 
 * Compiles
 * Works as expected
 
 
 Thanks,
 
 Jai  Luthra
 


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

Re: Review Request 113346: BugFix: 317902 - Issue a warning when transcoding is not possible

[Matěj Laitl]

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



File Attachment: Transcoding dialog with the note. - snapshot1.png
http://git.reviewboard.kde.org//r/113346/#fcomment119
I'd say the note could look better under (rather than above) the Transcode 
group box.


src/transcoding/TranscodingAssistantDialog.cpp
http://git.reviewboard.kde.org/r/113346/#comment30649

This can be simplified to:

ui.groupBox-setEnabled( !available.isEmpty() );
ui.encoderNotFoundLabel-setVisible( available.isEmpty );

Notice that we prefer the positive names rather than the negative: 
visible/hidden, enabled/disabled. It is easier for brain.



src/transcoding/TranscodingAssistantDialog.ui
http://git.reviewboard.kde.org/r/113346/#comment30648

I don't think you need to play with the label's enabled property, just 
show/hide it.



src/transcoding/TranscodingAssistantDialog.ui
http://git.reviewboard.kde.org/r/113346/#comment30650

Proposed wording:

bNote:/b No encoder is available. If you want to transcode tracks 
please install iffmpeg/i or ilibav/i package (with iffmpeg/i 
wrapper) with appropriate encoders. Otherwise you may check iRemember this 
choice for the next time/i option in order to skip this dialog for future 
transfers.

Also please remove the notr=true property, it must be left to default 
(false) to make this string translatable.


- Matěj Laitl


On Oct. 19, 2013, 9:48 p.m., Jai  Luthra wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/113346/
 ---
 
 (Updated Oct. 19, 2013, 9:48 p.m.)
 
 
 Review request for Amarok.
 
 
 Bugs: 317902
 https://bugs.kde.org/show_bug.cgi?id=317902
 
 
 Repository: amarok
 
 
 Description
 ---
 
 When ffmpeg is not available, the transcoding dialog will not be skipped; 
 rather it will add a note for the user to install an encoder.
 
 
 Diffs
 -
 
   src/core/collections/CollectionLocation.cpp 2385570 
   src/transcoding/TranscodingAssistantDialog.cpp 17fc250 
   src/transcoding/TranscodingAssistantDialog.ui 0ad83b2 
 
 Diff: http://git.reviewboard.kde.org/r/113346/diff/
 
 
 Testing
 ---
 
 * Compiles
 * Works as expected
 
 
 File Attachments
 
 
 Transcoding dialog with the note.
   
 http://git.reviewboard.kde.org/media/uploaded/files/2013/10/19/424771d9-a0ae-4eaf-824b-4b6bd2b840ee__snapshot1.png
 
 
 Thanks,
 
 Jai  Luthra
 


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

Re: Review Request 113272: GSoC 2013 - Advanced Importers - 1/4: Changes in StatSyncing framework

[Matěj Laitl]

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


Hey, thanks for posting this. I'll try to review it ASAP. Also thanks for 
splitting it into meaningful pieces, this way it is more manageable.

- Matěj Laitl


On Oct. 16, 2013, 7:11 p.m., Konrad Zemek wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/113272/
 ---
 
 (Updated Oct. 16, 2013, 7:11 p.m.)
 
 
 Review request for Amarok.
 
 
 Repository: amarok
 
 
 Description
 ---
 
 These are changes in the StatSyncing framework that I made as a part of my 
 project.
 
 
 Diffs
 -
 
   src/statsyncing/Config.h 140e647ecbcd6edd11cbe637a3e7b482bd24ec70 
   src/statsyncing/Config.cpp dd82dbe7729bf1b8cc2f5153c8af39aade0a9b81 
   src/statsyncing/Controller.h 10c72a809d1cb7065363a978df6aa2561621de2a 
   src/statsyncing/Controller.cpp bf4c5d8324bce796cf9b0057d21721b20af700b5 
   src/statsyncing/Provider.h d9663f93073de3035eb609967adea491f8192ab9 
   src/statsyncing/Provider.cpp 775fce388248b8e7aca1af442714d26549d014ce 
   src/statsyncing/Track.h 2f917045371d8caab3ec48d37050dfa5cec15f18 
   src/statsyncing/Track.cpp 9655cc16078724f5aa4f1dde532c1b65f2e574c1 
   src/statsyncing/TrackTuple.h 157d6044c1c7c79f471b9059f6a812887fa730ac 
   src/statsyncing/TrackTuple.cpp 944279414ab82ff811b33e1de0cd0f73f623d23b 
   src/statsyncing/jobs/SynchronizeTracksJob.cpp 
 b789aa32314b345270c07b34823e82b2a74ed6dd 
 
 Diff: http://git.reviewboard.kde.org/r/113272/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Konrad Zemek
 


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

Re: GSoC 2013 is over, please merge your code

[Matěj Laitl]

On 11. 10. 2013 Myriam Schweingruber wrote:
 Hi all,
 
 congratulations again to all our students for passing GSoC, you did great
 work!
 
 But now it is time to bring in your code from you scratch repo into
 master. Of course going through reviewboard is not a bad idea, but
 please, do this ASAP so we have time to review the code and polish.

This has been on my mind since the GSoC has finished. The reason why I haven't 
placed the review request yet is that I wanted to implement one last feature: 
ability to specify filename scheme on the devices and associated duplicate 
handling. I think these would be pretty nice to have before having the code 
widely tested in master.

Or perhaps it is not that important and can be done when the code it already 
in master?

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

Re: Review Request 113057: I am trying to kill Bug 322016 - Apply button is always enabled in Playlist Layout Editor dialog

[Matěj Laitl]

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


Hi, thanks for your patch. Unfortunately, this is not the right approach. 
Instead of tracking user changes, the logic under apply button should be:
1. watch every state change in the applicable controls and pressing of the 
apply button.
2. when a change happens, compare the current visible configuration with the 
last saved one and call setEnabled( settingsDiffer ); where settingsDiffer is a 
bool with obvious meaning.

- Matěj Laitl


On Oct. 2, 2013, 7:27 p.m., Ricardo Varas wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/113057/
 ---
 
 (Updated Oct. 2, 2013, 7:27 p.m.)
 
 
 Review request for Amarok.
 
 
 Bugs: 322016
 https://bugs.kde.org/show_bug.cgi?id=322016
 
 
 Repository: amarok
 
 
 Description
 ---
 
 I am trying to kill Bug 322016 - Apply button is always enabled in Playlist 
 Layout Editor dialog.
 In multiple parts I call the enable property for the Apply button and 
 enable/disable it according to specific actions. I tried to cover all 
 possible actions from a user --of course it begins in the disabled status as 
 requested and it's disabled after the user clicks it --.
 One thing I am not happy about is the fact that when the Token (the option 
 with the tool icon) is used to configure the playlist items such as album, 
 album artist, etc. then the Apply button is always enabled and I believe 
 under that case it should only become enabled if the Configuration for... 
 dialog's OK button is clicked, and remain disabled after the Cancel button is 
 clicked or the dialog is closed. I think LayoutEditDialog.cpp is called every 
 time the token is chosen but right not I am unable to understand how they are 
 connected LayoutEditDialog.cpp and PlaylistLayoutEditDialog.cpp.
 I'd appreciate if someone can help me understand this one and put together a 
 good patch for this bug.
 Thanks.
 
 
 Diffs
 -
 
   PlaylistLayoutEditDialog1.cpp 99aee2a 
 
 Diff: http://git.reviewboard.kde.org/r/113057/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Ricardo Varas
 


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

Re: Changes in GMock

[Matěj Laitl]

On 1. 10. 2013 Konrad Zemek wrote:
 The way I see it, possible solutions are:
 
 * go the way of Mir: find sources installed by Kubuntu's package in
   FindGmock.cmake (in Mir it's FindGtest.cmake)
   downsides: depends on how distro packages gmock; e.g. Arch has no
   gmock package in official repos. Also, gmock version is out of our
   control.

We may affect how distros package gmock. As gmock upstream clearly recommends 
installing the sources and then let each project build gmock from the 
installed sources, I think we should push distributions to do it that way.

So I'm +1 for requiring distros to install gmock sources and ideally providing 
a standardized way of finding them (for example pkgconfig). Initially, we can 
provide our own FindGmock.cmake with some heuristics ourselves.

This works well for the Eigen library (that is installed as sources), it ships 
/usr/share/pkgconfig/eigen3.pc:
Name: Eigen3
Description: A C++ template library for linear algebra: vectors, matrices, and 
related algorithms
Requires:
Version: 3.0.6
Libs:
Cflags: -I/usr/include/eigen3

There is no reason the same shouldn't work for gmock.

 * use CMake's ExternalProject_add. This is the method that I use in
   my projects, additionally hidden behind FindGmock.cmake. Here we
   control gmock version and at the same time not store its sources
   in our repository.
   downsides: sources have to be downloaded during make step.
   Include directories are also not there before running cmake.

-1. Downloading stuff during build is a no-go for me, plus it is the least 
secure, you loose all the checksuming etc.

 * pull gmock's sources into our repository and add it through
   add_directory(). This is by far the easiest option, and we still control
   the version, and we have include files in place.
   downsides: sources are in our repository.

+0 for this. Bundling is bad, but gmock is intended to be bundled/distributed 
as sources.

 As I mentioned, ExternalProject_add() abstracted behind
 FindGmock.cmake is the option that I personally use and prefer.
 It can also be used in conjunction with in-repo tar file to dodge
 downloading.

This is essentially bundling with some sugar so that it doesn't look horrible, 
-1 from me.

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

Re: MTP gsoc nexus 4 testing

[Matěj Laitl]

[adding amarok-devel to CC as this contains some general discussion]

On 25. 9. 2013 Bart Cerneels wrote:
 [Bart was testing my GSoC code, providing lot of useful feedback]

 Starting Amarok *after* plugging in the MTP device and closing
 nautilus does work. I would put that in a warning dialog. Make sure it
 can be disabled from appearing again with a checkbox.

I've added a passive notification, please check that it appears in your case.

 Matěj Laitl wrote:
  Bart wrote:
   I was a bit confused because I didn't see the N4 yet in collection
   browser while it was shown to be scanning message area. Can the
   MtpCollection be registered before scanning is started?
  
  It behaved like that in the past, but it had 2 downsides:
  a) it showed empty collection for significant time, because we get the
   tracks at once at the end, we only get the progress reporting before
   (but work-aroundable, but it would be a hack)
  
  b) MetaProxy::Tracks listen to collectionAdded() and then query the
   collection right away. If it is not (yet) ready, the resolving fails for
   the tracks, which means that playlist won't be preserved.
  
  My proposed solution is to introduce Collection::Status { Updating, Ready
  } and make other parts act accordingly, including visual display.
 
 Collection::Status is a good stop-gap solution and can be useful for
 other states as well: Error, Network Problem, etc. It could be useful
 regardless of Proxy handling.

I wouldn't even consider it stop-gap. The Collection Browser could show 
Updating... instead of track count and perhaps even some animation.

 Changing the way MetaProxy::Track listens to collection changes
 (trackAdded, etc) would solve it completely, but it think that would
 be a huge change.

I've thought about it, but rejected it as too demanding performance-wise. If 
every unresolved proxy track would watch for every collection's updated() 
signal (and then perform a search in the updated collection), the update() 
could easily get very costly. (imagine 100 of unresolved tracks and iPod 
collection that emits updated() that emits updated() every second during 
loading to keep UI updated).

Note that neither Collection nor TrackProvider currently has any sort of 
trackAdded() and adding it would place a way too heavy burden on existing and 
new collections (remember that only public interface to see collection's 
tracks is its QueryMaker, the collection doesn't even need to know complete 
track count before querying them).

  Seems to have a problem with FLAC files. They're shown under Unknown
  Artist/Unknown Album; some with titles as mtp://Internal
  Storage//xxx.flac, others do have titles. The metadata is readable
  by taglig, these are perfectly fine in Local Collection. Guess this
  depends on the device's MTP reading the metadata?
  
  Yes. But we work-around it by reading the tags from the file (unless
  disabled) as soon as it is cached locally. You may try to play them or
  transfer to another collection. I may add optional support for
  opportunistic caching, OTOH it temporarily needs quite a bit of space.
 
 Google's Play Music does read those FLAC files correctly. A bit
 strange that the MTP handler doesn't, but nothing surprising in such a
 large complicated system.

Yeah, this is the sad truth, on my Samsung S III Mini Google Play Music can 
for sure read track numbers and the default player can also read year etc., 
but neither track numbers, album artists or years are presented on MTP side!

 Do we know what metadata protocols/versions it does support?

Yes, for example Nexus 7 says it can read only name from flac files, while it 
claims it can read Name, Display Name (?), Artist, Album Name, Album Artist, 
Track, Original Release Date, Duration, Genre, Composer from MP3 files.

OTOH, my S III Mini *claims* it can read (also) Track, Album Artist etc. from 
MP3s but it doesn't actually do that, so we cannot trust this information. :-(

 Could you rewrite tags when copying to MTP?

Yes, unless you disable Read  Write file metadata in the config dialog.

  I just managed to lock up Amarok, at least the GUI thread.
  Unplugged the N4 while track from it were playing. The upcoming tracks
  were greyed out in the playlist, as expected (but only after
  mouse-over - missing QAIM datachanged signal). Then I double clicked
  on a greyed-out track anyway and it locked up.
  
  Culprit: probably EngineController expecting data that will not be
  downloaded. Here are the last log lines: (...)
  
  (...) you've actually
  spotted a bug, that went it during refactoring, now it creates and empty
  file in this case, which is silly.

It is fixed now and should no longer hang even with phonon-gstreamer.

Regards,
Matěj
___
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel

Re: Review Request 112802: Extend dbus unterface for reading and writing the rating of the current song

[Matěj Laitl]

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


Thanks for the patch. I acknowledge a need for a way to edit track rating (or, 
even better would be a generic way to edit any metadata), but I think that a 
different approach should be used, similarly to what Alex Merry said. Please 
see below.


src/dbus/mpris2/MediaPlayer2AmarokExtensions.cpp
http://git.reviewboard.kde.org/r/112802/#comment29779

This is not the code style we use in Amarok, please see HACKING folder. 
[this is just for information and future reference]



src/dbus/mpris2/MediaPlayer2AmarokExtensions.cpp
http://git.reviewboard.kde.org/r/112802/#comment29780

I believe this is triggered automatically by EngineController signals and 
Osd listening.

For sure we should not notify OSD every time we change the rating, that 
would be braindead.



src/dbus/mpris2/org.kde.amarok.Mpris2Extensions.Player.xml
http://git.reviewboard.kde.org/r/112802/#comment29781

Hmm, I must say I don't like having a rating property for a player 
object, because it doesn't logically belong there. It logically belongs to a 
track.

For reading purposes, the only reasonable place is 
MediaPlayer2.TrackList.GetTracksMetadata, and we *already* fill in 
xesam:userRating, so please let's don't add unnecessary redundancy (yet another 
undocumented ad-hoc method).

For writing let's wait for the outcome of the discussion on the MPRIS list.


- Matěj Laitl


On Sept. 18, 2013, 9:34 p.m., Alex Busenius wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/112802/
 ---
 
 (Updated Sept. 18, 2013, 9:34 p.m.)
 
 
 Review request for Amarok and Alex Merry.
 
 
 Description
 ---
 
 The D-Bus interface is currently missing convenient method for reading and 
 setting user rating (the stars). The proposed patch adds an Mpris2 extension 
 to /org/mpris/MediaPlayer2 for that: 
 
 property readwrite int org.kde.amarok.Mpris2Extensions.Player.Rating
 
 
 Diffs
 -
 
   src/dbus/mpris2/MediaPlayer2AmarokExtensions.h 964c7b8 
   src/dbus/mpris2/MediaPlayer2AmarokExtensions.cpp ba8b54e 
   src/dbus/mpris2/org.kde.amarok.Mpris2Extensions.Player.xml 3397df4 
 
 Diff: http://git.reviewboard.kde.org/r/112802/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Alex Busenius
 


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

Re: Review Request 112815: Properly fix read compilation tag in APE (musepack...) files

[Matěj Laitl]

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



shared/tag_helpers/APETagHelper.cpp
http://git.reviewboard.kde.org/r/112815/#comment29769

Shouldn't that be toBool()
isCompilation is AFAIK a boolean value.


- Matěj Laitl


On Sept. 19, 2013, 3:12 p.m., Bruno Léon wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/112815/
 ---
 
 (Updated Sept. 19, 2013, 3:12 p.m.)
 
 
 Review request for Amarok.
 
 
 Description
 ---
 
 Properly fix read compilation tag in APE (musepack...) files
 
 Previous patch 5e5140b was not complete. The information was read but not 
 inserted in the data structure and thus not inserted in Aamrok DB.
 
 
 Diffs
 -
 
   shared/tag_helpers/APETagHelper.cpp ba39a10 
 
 Diff: http://git.reviewboard.kde.org/r/112815/diff/
 
 
 Testing
 ---
 
 Files that were not placed in Various Artists before are now correctly 
 recognised as compilation.
 
 
 Thanks,
 
 Bruno Léon
 


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

Re: Implementing CUE sheet support in Amarok

[Matěj Laitl]

On 15. 9. 2013 Abhinandan Ramprasath wrote:
 Hi,
 
 After spending a lot of time staring at the code, I would like to propose
 the following changes to implement CUE sheet support in Amarok.
 
 Most of the necessary CUE decoding and track splitting functions are already
 available in CueFileSupport.h . It's got functions to scan for and validate
 CUE sheets too. I'd like to use these functions in the collectionscanner,
 scan for .cue files once the XML is read. To be even more precise, just
 before/after the CollectionScanner::Track for each track is created. So
 each child track would be treated as a separate track right from the
 beginning.
 
 I was thinking of having a few additional data members in
 CollectionScanner::Track and Meta::SqlTrack to represent child tracks and
 to store their start times and end times.

Yeah, that makes sense.

 This would then be stored in the database. To do that, I'd like to do that
 by adding a few columns to the track or the urls table such as, a child or
 not boolean, start time and end time.

Yep, this is the natural approach, however, this might lead to some problems. 
There are assumptions all over that (deviceid, rpath) in the urls table is 
unique for all urls. This wouldn't hold anymore.

One possibility would be to change the assumption everywhere (tricky, perhaps 
doable).

Second to somehow mix the start/end times into the rpath (is there a character 
not allowed in UNIX file paths?) and carefully treat places that use it.

Third to recreate the child tracks late - after picking them up from the 
database (extremely tricky IMO).

Another open questions:
what should happen when you try to copy/move one child track out of Local 
Collection? What would happen if you edit the metadata? (I guess the change to 
title would go to the cuesheet, everything else to the underlying file).

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

Re: Review Request 112706: Allow to use Wikipedia over SSL

[Matěj Laitl]

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


Looks good to me (with a few style comments). Please don't forget to add BUG, 
FIXED-IN tags and ChangeLog entry when comitting.

The only reason why we should not hard-code https is that it is blocked by in 
some circumstances (most prominently by China and Iran goverments).


src/context/applets/wikipedia/WikipediaApplet_p.h
http://git.reviewboard.kde.org/r/112706/#comment29458

I believe we don't tend use the m_* prefix for members of Private d-pointer 
classes. Or perhaps we're not consistent, but please remove it so that it is 
consistent with surrounding code.

The real question is why the d-pointer pattern is applied here at all (and 
other places in Amarok) - we're not a library a a few positives (apart from 
not-needed BIC) are IMO greatly outweighted by the added complexity.



src/context/engines/wikipedia/WikipediaEngine.cpp
http://git.reviewboard.kde.org/r/112706/#comment29459

minor: code style: spaces



src/context/engines/wikipedia/WikipediaEngine.cpp
http://git.reviewboard.kde.org/r/112706/#comment29461

Ditto m_ prefix.



src/context/engines/wikipedia/WikipediaEngine.cpp
http://git.reviewboard.kde.org/r/112706/#comment29460

I believe this debugging is not needed.


- Matěj Laitl


On Sept. 12, 2013, 9:54 p.m., Frank Meerkoetter wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/112706/
 ---
 
 (Updated Sept. 12, 2013, 9:54 p.m.)
 
 
 Review request for Amarok.
 
 
 Description
 ---
 
 Allow to use wikipedia over SSL. Make encrypted connections the default. Add 
 an option to fall back to plain HTTP.
 
 
 This addresses bug 322249.
 https://bugs.kde.org/show_bug.cgi?id=322249
 
 
 Diffs
 -
 
   src/context/applets/wikipedia/WikipediaApplet.cpp 507db96 
   src/context/applets/wikipedia/WikipediaApplet_p.h c52a0bf 
   src/context/applets/wikipedia/wikipediaGeneralSettings.ui a615dee 
   src/context/engines/wikipedia/WikipediaEngine.cpp f22e443 
 
 Diff: http://git.reviewboard.kde.org/r/112706/diff/
 
 
 Testing
 ---
 
 Switched several times between HTTP and HTTPS. Switched between the normal 
 and the mobile version.
 Checked the traffic with wireshark. Verified that this setting is actually 
 persisted.
 
 
 Thanks,
 
 Frank Meerkoetter
 


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

Re: Review Request 112706: Allow to use Wikipedia over SSL

[Matěj Laitl]

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

Ship it!


Ship it! (and no, I think HTTPS should be the default, let's heat those NSA 
supercomputers a bit.


src/context/applets/wikipedia/WikipediaApplet.cpp
http://git.reviewboard.kde.org/r/112706/#comment29472

nitpick: spaces 'round args


- Matěj Laitl


On Sept. 12, 2013, 10:44 p.m., Frank Meerkoetter wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/112706/
 ---
 
 (Updated Sept. 12, 2013, 10:44 p.m.)
 
 
 Review request for Amarok.
 
 
 Description
 ---
 
 Allow to use wikipedia over SSL. Make encrypted connections the default. Add 
 an option to fall back to plain HTTP.
 
 
 This addresses bug 322249.
 https://bugs.kde.org/show_bug.cgi?id=322249
 
 
 Diffs
 -
 
   src/context/applets/wikipedia/WikipediaApplet.cpp 507db96 
   src/context/applets/wikipedia/WikipediaApplet_p.h c52a0bf 
   src/context/applets/wikipedia/wikipediaGeneralSettings.ui a615dee 
   src/context/engines/wikipedia/WikipediaEngine.cpp f22e443 
 
 Diff: http://git.reviewboard.kde.org/r/112706/diff/
 
 
 Testing
 ---
 
 Switched several times between HTTP and HTTPS. Switched between the normal 
 and the mobile version.
 Checked the traffic with wireshark. Verified that this setting is actually 
 persisted.
 
 
 Thanks,
 
 Frank Meerkoetter
 


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

Re: Propose: Change behaviour that 'Amarok guesses Album from Filepath if no album-tag is set

[Matěj Laitl]

On 6. 9. 2013 Mathias Dietrich wrote:
 unfortunately, nobody replied to my reminder
 That's why I needed to create a patch and to opened review request:
 
 https://git.reviewboard.kde.org/r/112551/

I have starred it which means that I'll get to review it sooner or later, 
unfortunately probably not within 2 weeks.

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

Re: Review Request 112023: Disable move/copy actions for non-writeable UMS collections.

[Matěj Laitl]

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


Hi, sorry for the delay replying, I had a king of work emergency. Please see 
some comments below.


src/core/collections/Collection.h
http://git.reviewboard.kde.org/r/112023/#comment28829

I'd love if this could be named WritableState, as WriteState triggers 
something very different in my mind. Code is written once and read many times, 
it is therefore hopefully worth the effort.



src/core/collections/Collection.h
http://git.reviewboard.kde.org/r/112023/#comment28828

I must say I don't like a method named isSomething() returning non-bool. We 
tend to adhere to Qt naming style and in Qt, isSomething() really means that 
Something is a bool.

I suggest this:
virtual WritableState writableState() const;

I also suggest a convenience non-virtual method:
bool isWritable() const;

As most users don't care whether non-writability if permanent or temporary, 
they could continue using isWritable() without changes, which would shrink this 
patch. It should be however checked that no subclass implements it. (or, can we 
use C++11 final keyword already?)

^^^ the same applies to CollectionLocation too.



src/core/collections/CollectionLocation.h
http://git.reviewboard.kde.org/r/112023/#comment28830

These fwd-declarations are no longer needed.


- Matěj Laitl


On Aug. 25, 2013, 11:09 p.m., Frank Meerkoetter wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/112023/
 ---
 
 (Updated Aug. 25, 2013, 11:09 p.m.)
 
 
 Review request for Amarok.
 
 
 Description
 ---
 
 The fix in https://git.reviewboard.kde.org/r/111991/ fixes isWriteable() for 
 UMS collections. This has the effect that if an UMS-collection is actually 
 non-writeable the copy/move operations will be missing from the 
 TreeView/Filebrowser context menus. There is no clue to the user what is 
 going on.
 
 This patch is introducing a new method to the Collection interface so we can 
 differentiate between collections that are non-writeable by principle and 
 collections that just happen to be non-writeable right now (for what ever 
 reason).
 
 I use this changed collection interface to disable move/copy actions (as 
 opposed to hide) for non-writeable UMS-colllections. This should give the 
 user at least a hint what is going on.
 
 Unfortunately i haven't found a way to display a tooltip (Currently 
 readonly). The action is offering an setTooltip() method but now effect was 
 observed.
 
 
 Diffs
 -
 
   src/browsers/CollectionTreeItemModel.cpp 1ac4463 
   src/browsers/CollectionTreeView.cpp 6fad164 
   src/browsers/filebrowser/FileView.cpp ad200eb 
   src/core-impl/collections/db/sql/SqlCollectionLocation.h da1b2e5 
   src/core-impl/collections/db/sql/SqlCollectionLocation.cpp 3b55f66 
   src/core-impl/collections/ipodcollection/IpodCollection.h 14fdd40 
   src/core-impl/collections/ipodcollection/IpodCollection.cpp 36738d7 
   src/core-impl/collections/ipodcollection/IpodCollectionLocation.h cc27e19 
   src/core-impl/collections/ipodcollection/IpodCollectionLocation.cpp f8105f9 
   src/core-impl/collections/ipodcollection/IpodMeta.cpp 6beca0a 
   src/core-impl/collections/ipodcollection/IpodPlaylist.cpp 61c60ba 
   src/core-impl/collections/ipodcollection/IpodPlaylistProvider.cpp dcef4c2 
   
 src/core-impl/collections/mediadevicecollection/MediaDeviceCollectionLocation.h
  e40529f 
   
 src/core-impl/collections/mediadevicecollection/MediaDeviceCollectionLocation.cpp
  603ceff 
   src/core-impl/collections/mediadevicecollection/MediaDeviceMeta.cpp 4a419d5 
   src/core-impl/collections/nepomukcollection/NepomukCollection.h c5d7a13 
   src/core-impl/collections/nepomukcollection/NepomukCollection.cpp 77aef8e 
   src/core-impl/collections/playdarcollection/PlaydarCollection.h 1a7ec89 
   src/core-impl/collections/playdarcollection/PlaydarCollection.cpp 5c2349d 
   src/core-impl/collections/support/FileCollectionLocation.h 9bd303e 
   src/core-impl/collections/support/FileCollectionLocation.cpp 4b9b4b0 
   src/core-impl/collections/support/TrashCollectionLocation.h 239a977 
   src/core-impl/collections/support/TrashCollectionLocation.cpp 61c2e49 
   src/core-impl/collections/umscollection/UmsCollection.cpp a73fd34 
   src/core-impl/collections/umscollection/UmsCollectionLocation.h 6cfe4df 
   src/core-impl/collections/umscollection/UmsCollectionLocation.cpp cef90d7 
   src/core/collections/Collection.h 94d24d7 
   src/core/collections/Collection.cpp 01a2e8c 
   src/core/collections/CollectionLocation.h 7a1ed1e 
   src/core/collections/CollectionLocation.cpp 2385570 
   src/services

Re: Review Request 111824: Adds multithreaded transcoding to IpodCollection ie fixes BUG 317093

[Matěj Laitl]

 On Aug. 25, 2013, 10:47 a.m., Mark Kretschmann wrote:
  Does anyone here have an actual iDevice for testing this?

I'll handle the patch incl. testing, but I'm unfortunately rather 
time-constrained now, so in the worst case it'll have to wait till the second 
half of September. Sorry about that, Vedant.


- Matěj


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


On July 31, 2013, 8:22 p.m., Vedant Agarwala wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/111824/
 ---
 
 (Updated July 31, 2013, 8:22 p.m.)
 
 
 Review request for Amarok.
 
 
 Description
 ---
 
 I created a new runnable class that performs transcoding of a single track 
 (IpodCopySingleTrack). I updated the existing code in IpodCopyTracksJob, 
 renamed it to IpodCopyTracks (since it does not extend ThreadWeaver::Job 
 anymore) and ported some of it to IpodCopySingleTrack.
 
 
 This addresses bug 317093.
 https://bugs.kde.org/show_bug.cgi?id=317093
 
 
 Diffs
 -
 
   src/core-impl/collections/ipodcollection/CMakeLists.txt cb4ef06 
   src/core-impl/collections/ipodcollection/IpodCollection.h 02e55f8 
   src/core-impl/collections/ipodcollection/IpodCollectionLocation.h cc27e19 
   src/core-impl/collections/ipodcollection/IpodCollectionLocation.cpp f8105f9 
   src/core-impl/collections/ipodcollection/jobs/IpodCopySingleTrack.h 
 PRE-CREATION 
   src/core-impl/collections/ipodcollection/jobs/IpodCopySingleTrack.cpp 
 PRE-CREATION 
   src/core-impl/collections/ipodcollection/jobs/IpodCopyTracks.h PRE-CREATION 
   src/core-impl/collections/ipodcollection/jobs/IpodCopyTracks.cpp 
 PRE-CREATION 
   src/core-impl/collections/ipodcollection/jobs/IpodCopyTracksJob.h 0258bf2 
   src/core-impl/collections/ipodcollection/jobs/IpodCopyTracksJob.cpp a7fd97e 
 
 Diff: http://git.reviewboard.kde.org/r/111824/diff/
 
 
 Testing
 ---
 
 Builds and installs but I could not test actual transcoding since I do not 
 have an iPod.
 
 
 Thanks,
 
 Vedant Agarwala
 


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

Re: Review Request 109369: Bug #254404: Copy files to USB storage devices in display order by sorting tracks in CollectionLocation.cpp

[Matěj Laitl]

 On Aug. 25, 2013, 10:49 a.m., Mark Kretschmann wrote:
  What's the current status of this patch?

Well, the question is whether we want such big complication of code for little 
gain. I fear that not, at not least in the current for of the patch. I'd be 
also against adding another duplicate of level sort to our code. Do I 
remember correctly that Konrad offered to make the level sort from playlist 
more generic (i.e. using Meta::val*) and reusable? That could help in shrinking 
this patch.


- Matěj


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


On March 12, 2013, 7:27 p.m., Anmol Ahuja wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/109369/
 ---
 
 (Updated March 12, 2013, 7:27 p.m.)
 
 
 Review request for Amarok.
 
 
 Description
 ---
 
 1. Made a levelSort() function to sort tracks according to multiple 
 parameters ( Should i just be sorting tracks belonging to the same album? )
 2. Modified copyUrlToCollection() to use QList instead of QMap
 3. FileView's files are being copied in the order of selection, thanks to 
 using QLists ( is that acceptable? )
 
 Updates:
 Implemented changes as suggested by strohel
 1. Moved levelSort function to the prepareCopy callers (CollectionTreeView)
 2. Restored CollectionLocation.cpp to the old version, with QLists instead of 
 QMaps
 
 Still not complete though, working on it.
 
 
 Diffs
 -
 
   src/browsers/CollectionTreeView.h 3b2ca80 
   src/browsers/CollectionTreeView.cpp fd9fe66 
   src/browsers/collectionbrowser/CollectionWidget.h c281f41 
   src/core-impl/collections/audiocd/AudioCdCollectionLocation.cpp be13551 
   src/core-impl/collections/db/sql/SqlCollectionLocation.h 0bcf244 
   src/core-impl/collections/db/sql/SqlCollectionLocation.cpp 93efe97 
   src/core-impl/collections/ipodcollection/IpodCollectionLocation.h cc27e19 
   src/core-impl/collections/ipodcollection/IpodCollectionLocation.cpp f8105f9 
   src/core-impl/collections/ipodcollection/jobs/IpodCopyTracksJob.h 3c2d9f2 
   src/core-impl/collections/ipodcollection/jobs/IpodCopyTracksJob.cpp 8a40c6c 
   
 src/core-impl/collections/mediadevicecollection/MediaDeviceCollectionLocation.h
  e40529f 
   
 src/core-impl/collections/mediadevicecollection/MediaDeviceCollectionLocation.cpp
  f60aff6 
   
 src/core-impl/collections/mediadevicecollection/handler/MediaDeviceHandler.h 
 821f1b0 
   
 src/core-impl/collections/mediadevicecollection/handler/MediaDeviceHandler.cpp
  c1b76f5 
   src/core-impl/collections/mtpcollection/handler/MtpHandler.cpp a8d9f52 
   src/core-impl/collections/support/PlaylistCollectionLocation.h 10a365f 
   src/core-impl/collections/support/PlaylistCollectionLocation.cpp c885046 
   src/core-impl/collections/support/TrashCollectionLocation.h 239a977 
   src/core-impl/collections/support/TrashCollectionLocation.cpp 61c2e49 
   src/core-impl/collections/umscollection/UmsCollection.cpp 6bebd98 
   src/core-impl/collections/umscollection/UmsCollectionLocation.h 45ba596 
   src/core-impl/collections/umscollection/UmsCollectionLocation.cpp e0ba0ac 
   src/core/collections/CollectionLocation.h d37ccfb 
   src/core/collections/CollectionLocation.cpp aecc068 
   src/services/ServiceCollectionLocation.cpp d1cb0d8 
   src/services/mp3tunes/Mp3tunesServiceCollectionLocation.h 2b06cb4 
   src/services/mp3tunes/Mp3tunesServiceCollectionLocation.cpp aa61072 
 
 Diff: http://git.reviewboard.kde.org/r/109369/diff/
 
 
 Testing
 ---
 
 Seems to be copying tracks in the correct order now
 
 
 Thanks,
 
 Anmol Ahuja
 


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

Re: Review Request 112266: Fix reading Album Artist / Compilation / Disc Number in APE tags

[Matěj Laitl]

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


Thanks for the patch. Please answer some remarks below.


shared/tag_helpers/APETagHelper.cpp
http://git.reviewboard.kde.org/r/112266/#comment28492

Hmm, is there a specification somewhere that says what the proper 
identifiers are? I fear of backwards compatibility, perhaps there are files out 
there that use the title-cased identifiers?



shared/tag_helpers/APETagHelper.cpp
http://git.reviewboard.kde.org/r/112266/#comment28493

Does the spec say what format does disc number has? Perhaps we should be 
able to read both formats?


- Matěj Laitl


On Aug. 25, 2013, 1:17 p.m., Bruno Léon wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/112266/
 ---
 
 (Updated Aug. 25, 2013, 1:17 p.m.)
 
 
 Review request for Amarok.
 
 
 Description
 ---
 
 Fix reading of Album Artist and Compilation tag in APE tags.
 Add support for reading Disc Number in APE tags.
 
 
 Diffs
 -
 
   shared/tag_helpers/APETagHelper.cpp c628694 
 
 Diff: http://git.reviewboard.kde.org/r/112266/diff/
 
 
 Testing
 ---
 
 Tested with Musepack files (that do use APE tags)
 
 
 Thanks,
 
 Bruno Léon
 


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

Re: Review Request 112266: Fix reading Album Artist / Compilation / Disc Number in APE tags

[Matěj Laitl]

 On Aug. 25, 2013, 2:06 p.m., Matěj Laitl wrote:
  shared/tag_helpers/APETagHelper.cpp, lines 33-37
  http://git.reviewboard.kde.org/r/112266/diff/1/?file=184486#file184486line33
 
  Hmm, is there a specification somewhere that says what the proper 
  identifiers are? I fear of backwards compatibility, perhaps there are files 
  out there that use the title-cased identifiers?
 
 Bruno Léon wrote:
 I did not find specifications for this.
 Actually when using Album Artist instead of ALBUM ARTIST, the tag is 
 not read at all.
 Taglib output it as ALBUM ARTIST which made me make this correction 
 (and checked it actually works).
 
 Same this for compilation. This makes Amarok compliant with Taglib output 
 format.

I don't think that TagLib itself is origin of the string, I think it just reads 
the identifier that is in the actual tag (which can vary file by file). Or 
perhaps you can prove me wrong by pointing out relevant part of TagLib source 
code?


- Matěj


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


On Aug. 25, 2013, 1:17 p.m., Bruno Léon wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/112266/
 ---
 
 (Updated Aug. 25, 2013, 1:17 p.m.)
 
 
 Review request for Amarok.
 
 
 Description
 ---
 
 Fix reading of Album Artist and Compilation tag in APE tags.
 Add support for reading Disc Number in APE tags.
 
 
 Diffs
 -
 
   shared/tag_helpers/APETagHelper.cpp c628694 
 
 Diff: http://git.reviewboard.kde.org/r/112266/diff/
 
 
 Testing
 ---
 
 Tested with Musepack files (that do use APE tags)
 
 
 Thanks,
 
 Bruno Léon
 


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

Re: Review Request 112255: UMS: Fix set album cover

[Matěj Laitl]

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

Ship it!


Looks good to me, please merge. Dunno how that could have been left broken for 
such a long time.

- Matěj Laitl


On Aug. 25, 2013, 12:11 a.m., Frank Meerkoetter wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/112255/
 ---
 
 (Updated Aug. 25, 2013, 12:11 a.m.)
 
 
 Review request for Amarok.
 
 
 Description
 ---
 
 When Setting-Config-Metadata-Write covers to file is enabled, it should be 
 possible to write covers for albums located on an UMS collection. The current 
 status is that amarok does the write but never updates itself to reflect the 
 new cover. There is code to achive this but is has been broken at some point.
 
 /home/frank/kde/src/amarok/src/core-impl/meta/file/File_p.h:358 tries to 
 create a signal/slot connection (to re-read the tags after the write is done) 
 to an invalid slot. This is fixed by my patch.
 
 
 Diffs
 -
 
   src/core-impl/meta/file/File.cpp f2abea4 
   src/core-impl/meta/file/File_p.h 40f631c 
 
 Diff: http://git.reviewboard.kde.org/r/112255/diff/
 
 
 Testing
 ---
 
 Writing album covers for files located on an USB stick.
 The album is updated now (collection browser, playlist). The current track 
 applet isn't updating but i guess this is a different problem.
 
 
 Thanks,
 
 Frank Meerkoetter
 


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

Re: BACKPORT keyword

[Matěj Laitl]

On 18. 8. 2013 Mark Kretschmann wrote:
 Heya,
 
 I just had a smart idea, which happens rarely enough: If we make
 commits where we already know that they should be backported later,
 put the keyword BACKPORT in the commit log. Later when we assemble the
 stable branch we can then simply grep over the whole log.

Yeah, this is a good idea, I in fact had a very similar one.

Still, when making possible 2.8.1, I'll go through all master commits and 
judge them as we *will* forget to add that tag. It is nonetheless still useful 
as the author thinks this should be backported.

Also please note that due to the fact we don't have stable/trunk translation 
branches (to keep things easy for translators), any BACKPORT commits should 
respect string freeze (we can change that, but it would be another 
discussion).

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

Re: Review Request 112023: Disable move/copy actions for non-writeable UMS collections.

[Matěj Laitl]

 On Aug. 12, 2013, 6:57 p.m., Matěj Laitl wrote:
  src/core/collections/Collection.h, lines 125-135
  http://git.reviewboard.kde.org/r/112023/diff/1/?file=178093#file178093line125
 
  Hmm, I don't like the combination of 2 bools to signify 3 possible 
  values (because you introduce one invalid combination).
  
  What about making it:
  Enum WritableType {
  Writable,
  TemporarilyNotWritable,
  NotWritable // permanently
  }
  WritableType writableType() const;
 
 Frank Meerkoetter wrote:
 Sure, i'll update the patch
 
 Frank Meerkoetter wrote:
 I have a question about your proposal.
 
 Your intention is to replace isWriteable() and isWriteableType() with 
 writeableType() which returns an enum?

Yes, exactly. It might be a good idea to change it also in CollectionLocation 
so that Collection::writableType() remains just a convenience shortcut.


- Matěj


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


On Aug. 11, 2013, 10:08 p.m., Frank Meerkoetter wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/112023/
 ---
 
 (Updated Aug. 11, 2013, 10:08 p.m.)
 
 
 Review request for Amarok.
 
 
 Description
 ---
 
 The fix in https://git.reviewboard.kde.org/r/111991/ fixes isWriteable() for 
 UMS collections. This has the effect that if an UMS-collection is actually 
 non-writeable the copy/move operations will be missing from the 
 TreeView/Filebrowser context menus. There is no clue to the user what is 
 going on.
 
 This patch is introducing a new method to the Collection interface so we can 
 differentiate between collections that are non-writeable by principle and 
 collections that just happen to be non-writeable right now (for what ever 
 reason).
 
 I use this changed collection interface to disable move/copy actions (as 
 opposed to hide) for non-writeable UMS-colllections. This should give the 
 user at least a hint what is going on.
 
 Unfortunately i haven't found a way to display a tooltip (Currently 
 readonly). The action is offering an setTooltip() method but now effect was 
 observed.
 
 
 Diffs
 -
 
   src/browsers/CollectionTreeView.cpp 7e93c9a 
   src/browsers/filebrowser/FileView.cpp ad200eb 
   src/core-impl/collections/umscollection/UmsCollection.h 749ff81 
   src/core-impl/collections/umscollection/UmsCollection.cpp 028966e 
   src/core/collections/Collection.h 94d24d7 
   src/core/collections/Collection.cpp 01a2e8c 
 
 Diff: http://git.reviewboard.kde.org/r/112023/diff/
 
 
 Testing
 ---
 
 I have tested with an USB-stick that is writeable and another one that is 
 non-writeable. I have also observed that for the FileBrowser the Local 
 collection is still shown as active.
  
 
 
 Thanks,
 
 Frank Meerkoetter
 


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

Re: Review Request 110187: Don't communicate with mysql by env vars and autogenerated files

[Matěj Laitl]

 On Aug. 16, 2013, 9:19 a.m., Mark Kretschmann wrote:
  What's the status of this patch?

Please merge, after testing this locally and resolving the two very minor 
issues that I and Kondard have found.


- Matěj


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


On July 19, 2013, 10:35 a.m., Patrick von Reth wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/110187/
 ---
 
 (Updated July 19, 2013, 10:35 a.m.)
 
 
 Review request for Amarok.
 
 
 Description
 ---
 
 Don't communicate with mysql by env vars and autogenerated files
 
 Instead of generating the my.cnf every time amarok starts and to pass
 the location of this file by setting an environment variable directly
 pass the settings as arguments to mysql. As this is probably a better
 approach and the only one working on windows.
 
 This fixes an issue where amarok is writing the database to C:\Program Files 
 (x86)\Amarok\data\amarok.
 This issue prevents Amarok from running correctly, because regarding to the 
 rights of the useraccount the directory can be not writeable.
 
 The commands used are taken from the output of mysqld --verbose --help. 
 
 
 Diffs
 -
 
   src/core-impl/collections/db/sql/mysqlecollection/MySqlEmbeddedStorage.cpp 
 0233498fdeb18ab51e709e9a78384fc37c47cb2a 
 
 Diff: http://git.reviewboard.kde.org/r/110187/diff/
 
 
 Testing
 ---
 
 Only on windows
 
 
 Thanks,
 
 Patrick von Reth
 


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

Re: ToolTips and Automatic Formatting

[Matěj Laitl]

On 16. 8. 2013 Mark Kretschmann wrote:
 Heya,
 
 as we noticed recently, many of Amarok's ToolTips look very ugly and
 have readability issues because they are not formatted. Therefore a
 longer ToolTip is being rendered as one giant long line. The Qt API
 offers automatic formatting with line-breaks for ToolTips, but there
 is an ugly API quirk: The formatting can only be switched on by adding
 a html tag to the string. In theory a wrapper could be created for
 QWidget::setToolTip() and friends, but this wouldn't play nice with Qt
 Designer.
 
 I think for consistency's sake it would be ideal to simply switch on
 automatic formatting for *all* of our ToolTips, and not bother with
 manual line-breaks. This is also because with translations it's
 impossible to know the final length and look of the strings. The
 downside is, we would have to change nearly all ToolTip strings in
 Amarok (this could be done with some clever sed use).
 
 What do you think is the best way to go forward?

Feel free to do it from me, unless better solution is found. The strings in 
toolTips may be duplicated in other contexts (What's this etc.), bonus point 
is if you manage to change also these to keep them as one translation string.

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

Re: Review Request 110920: Fight warnings.

[Matěj Laitl]

 On July 2, 2013, 12:53 p.m., Matěj Laitl wrote:
  If it compiles with the const, it cannot hurt, ship it!
 
 Albert Astals Cid wrote:
 Janitorial dude question: Has this been commited and you forgot to mark 
 it as submitted or it does still need submitting?


I forgot to commit it, but it will be commited in my next reviewboard session 
as I have it starred. :)


- Matěj


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


On June 9, 2013, 9:53 p.m., Wolf-Michael Bolle wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/110920/
 ---
 
 (Updated June 9, 2013, 9:53 p.m.)
 
 
 Review request for Amarok.
 
 
 Description
 ---
 
 Fight warnings.
 
 Nobody will write that string after it has beed set.
 
 
 Diffs
 -
 
   src/services/mp3tunes/libmp3tunes/harmony.h 
 e0941e348db09ba69c39426077b6798e758d6354 
 
 Diff: http://git.reviewboard.kde.org/r/110920/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Wolf-Michael Bolle
 


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

Re: Sync Amarok wiki documentation to git before 2.8?

[Matěj Laitl]

On 13. 8. 2013 Yuri Chornoivan wrote:
 написане Tue, 13 Aug 2013 19:08:48 +0300, Matěj Laitl ma...@laitl.cz:
  Hi Yuri,
  I'm really close to tagging Amarok 2.8 final. The last thing in my
  checklist is
  Amarok Handbook: do you think that syncing the in-progress wiki docs to
  git
  before release is a good idea? (Myriam already said it is okay with her)
  
  The thing it that I think that shipping in-progress 2.8 (english)
  documentation is better than shipping outdated 2.7 one, if we are able
  not to
  disrupt handbook translations. Note that I can specify a SVN revision to
  checkout (doc) translations from when creating the tarball.
  
  On a related note: do you know what is the status of wiki vs. SVN
  handbook
  translations? E.g. I see both
  http://userbase.kde.org/Amarok/Manual/pt-br and
  http://websvn.kde.org/trunk/l10n-kde4/pt_BR/docs/extragear-multimedia/amar
  ok/index.docbook?view=markup 
 
 Just a quick explanation from my side:
 
 1. Updating docs in git repo is a relatively simple operation (~10 mins
 because of the traffic for images and conversion of images operations).
 You can do it by yourself if you do not want me here.

Yes I know, thanks for documenting the process earlier. Still I feel more 
comfortable with you doing it as it seems that you've done some wiki tweaks 
before that wouldn't occur to me.

 2. There are 7 complete translations in SVN (and 1-2 translations of 2.5
 that are not very outdated):
 
 http://l10n.kde.org/stats/doc/trunk-kde4/po/amarok.po/
 
 There are about 1 or 2 mostly complete translations without localized
 screenshots in wiki (Danish and Brazilian Portuguese).
 
 3. André Marcelo Alvarenga (Brazilian Portuguese) is trying to sync his
 translations. I'm (Ukrainian) not trying to do so. That's our choice.
 
 4. I'm not happy with the current spare workflow with no time for
 translation at all. However, I'm almost sure that if you give just one day
 for SVN translators you will have at least 3 ready to use and updated
 translations of the handbook in packages. That's because offline tools are
 drastically more effective than online now.

I'm not happy with it either and thought that wiki handbooks could make it 
better, but I see that not (yet). The best would of course be to have the 
English handbook ready by the time beta is released (or shortly afterwards), 
but being realistic I don't think this is going to happen with current 
available documentation (wo)manpower (Myriam already has lots of things on her 
shoulders).

 So, what should I do?
 
 1. Send you a link to PDF for review.
 2. Silently update handbook.
 3. Something else.

Thanks for already doing the 2, that was the best option.

One more question: AFAICS, the translators generate the localized 
index.docbook manually themselves from the translated docmessages, right? 
Should I ping them to dump their work-in-progress there before creating Amarok 
2.8 tarballs (ETA: tonight in European time zone)?

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

Sync Amarok wiki documentation to git before 2.8?

[Matěj Laitl]

Hi Yuri,
I'm really close to tagging Amarok 2.8 final. The last thing in my checklist is 
Amarok Handbook: do you think that syncing the in-progress wiki docs to git 
before release is a good idea? (Myriam already said it is okay with her)

The thing it that I think that shipping in-progress 2.8 (english) 
documentation is better than shipping outdated 2.7 one, if we are able not to 
disrupt handbook translations. Note that I can specify a SVN revision to 
checkout (doc) translations from when creating the tarball.

On a related note: do you know what is the status of wiki vs. SVN handbook 
translations? E.g. I see both http://userbase.kde.org/Amarok/Manual/pt-br and 
http://websvn.kde.org/trunk/l10n-kde4/pt_BR/docs/extragear-multimedia/amarok/index.docbook?view=markup
 - one is apparently directly 
translated from English Amarok wiki handbook and the other takes a funny trip:
en Amarok wiki handbook - you - index.docbook in Amarok git - scripty - 
docmessages pot - translator - pt_BT docmessages - translator (scripted?) -
 pt_BR index.docbook

Thanks for all the work,
Matěj
___
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel

Re: Review Request 111626: Fix Amarok crash with Audio CD inserted

[Matěj Laitl]

 On July 22, 2013, 2:57 p.m., Lukáš Tinkl wrote:
  The problem is that udisks2 no longer associates drives with a block 
  device, nothing we can circumvent.
 
 Christoph Feck wrote:
 Does commit b92df276 (Do not clean the cache in UDisks2 backend) work 
 around this udisks2 issue, in other words, is this patch no longer necessary?

Yes, commit b92df276 fixes the root of this issue. Still, KFilePlacesItem 
should not crash should this happen to the $next solid backend.


- Matěj


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


On July 21, 2013, 2:28 p.m., Christoph Feck wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/111626/
 ---
 
 (Updated July 21, 2013, 2:28 p.m.)
 
 
 Review request for Amarok, kdelibs and Solid.
 
 
 Description
 ---
 
 https://bugs.kde.org/show_bug.cgi?id=314544#c40 says:
 
 Looking at the backtrace, KFilePlacesItem tries to detect Audio CDs as 
 follows:
 
 Solid::Device d = device();
 if (d.isValid()) {
 if (m_access) {
 return QUrl(KUrl(m_access-filePath()));
 } else if (m_disc  (m_disc-availableContent()  
 Solid::OpticalDisc::Audio)!=0) {
 QString device = d.asSolid::Block()-device();
 return QUrl(QString(audiocd:/?device=%1).arg(device));
 }
 }
 
 The crash happens, because d.asSolid::Block returns 0, in other words, 
 Audio CDs are no longer treated as block devices.
 
 Failure of said cast can be easily detected, but the question is rather, if 
 it's not a Solid bug, how the code needs to be modified to correctly get the 
 audiocd:/ URL for the block device.
 
 While I did not yet figure out, how I can get the block device URL for a disc 
 in case block is 0, I propose this temporary fix. It should fix the crash, 
 but could fail to correctly invoke the audiocd:/ kio with multiple disc 
 drives.
 
 
 This addresses bug 314544.
 http://bugs.kde.org/show_bug.cgi?id=314544
 
 
 Diffs
 -
 
   kfile/kfileplacesitem.cpp 5a12486 
 
 Diff: http://git.reviewboard.kde.org/r/111626/diff/
 
 
 Testing
 ---
 
 I could not test, my system has no disc drives.
 
 
 Thanks,
 
 Christoph Feck
 


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

Re: Review Request 112040: Do not create duplicate UMS collections for encrypted volumes

[Matěj Laitl]

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


Looks good, altough I'd suggest a tweak, see below.


src/core-impl/collections/umscollection/UmsCollection.cpp
http://git.reviewboard.kde.org/r/112040/#comment27782

This makes sense, altough looking at other UsageType enum values we want 
really the FileSystem one and no other. So please convert the logic from not 
encypted to only FileSystem. Code-wise, it is best to create a switch()/case 
with all the variants and no default: label to get notified on new enum values 
by the compiler in future.


- Matěj Laitl


On Aug. 12, 2013, 8:44 p.m., Frank Meerkoetter wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/112040/
 ---
 
 (Updated Aug. 12, 2013, 8:44 p.m.)
 
 
 Review request for Amarok.
 
 
 Description
 ---
 
 When I plug an encrypted USB-stick amarok is creating two UMS collections for 
 it. Both have the same mount point.
 
 $ solid-hardware list details
 [...]
 udi = '/org/freedesktop/UDisks/devices/dm_2d5'
   parent = '/org/freedesktop/UDisks/devices/sdd1'  (string)
   vendor = ''  (string)
   product = ''  (string)
   description = '29.9 GiB Removable Media'  (string)
   Block.major = 252  (0xfc)  (int)
   Block.minor = 5  (0x5)  (int)
   Block.device = '/dev/dm-5'  (string)
   StorageAccess.accessible = true  (bool)
   StorageAccess.filePath = '/media/62a745fa-6350-4ee5-ba37-0462dfa3530f'  
 (string)
   StorageAccess.ignored = false  (bool)
   StorageVolume.ignored = false  (bool)
   StorageVolume.usage = 'FileSystem'  (0x2)  (enum)
   StorageVolume.fsType = 'ext4'  (string)
   StorageVolume.label = ''  (string)
   StorageVolume.uuid = '62a745fa-6350-4ee5-ba37-0462dfa3530f'  (string)
   StorageVolume.size = 32125222912  (0x77ad0)  (qulonglong)
 [...]
 udi = '/org/freedesktop/UDisks/devices/sdd1'
   parent = '/org/freedesktop/UDisks/devices/sdd'  (string)
   vendor = 'JetFlash'  (string)
   product = 'Transcend 32GB'  (string)
   description = '29.9 GiB Encrypted Container'  (string)
   Block.major = 8  (0x8)  (int)
   Block.minor = 49  (0x31)  (int)
   Block.device = '/dev/sdd1'  (string)
   StorageAccess.accessible = true  (bool)
   StorageAccess.filePath = '/media/62a745fa-6350-4ee5-ba37-0462dfa3530f'  
 (string)
   StorageAccess.ignored = false  (bool)
   StorageVolume.ignored = false  (bool)
   StorageVolume.usage = 'Encrypted'  (0x5)  (enum)
   StorageVolume.fsType = 'crypto_LUKS'  (string)
   StorageVolume.label = ''  (string)
   StorageVolume.uuid = '1a38165b-2eee-41d0-acd1-6d34032f47fd'  (string)
   StorageVolume.size = 32127320064  (0x77af0)  (qulonglong)
 
 This patch is filtering out the storage volume where the usage field is set 
 to Encrypted (as opposed to Filesystem).
 
 
 Diffs
 -
 
   src/core-impl/collections/umscollection/UmsCollection.cpp 028966e 
 
 Diff: http://git.reviewboard.kde.org/r/112040/diff/
 
 
 Testing
 ---
 
 I have tested plugin an USB-stick containing an dmcrypt/luks encrypted 
 ext4fs. I also tested with an USB-stick that was not encrypted.
 
 
 Thanks,
 
 Frank Meerkoetter
 


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

Re: Review Request 112040: Do not create duplicate UMS collections for encrypted volumes

[Matěj Laitl]

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

Ship it!


Looks good, but please bear with those of us that are kinda stuck to code style 
consistency. I'd apply after releasing 2.8 (this is a little bug, but still 
there's a very small chance of regressions)


src/core-impl/collections/umscollection/UmsCollection.h
http://git.reviewboard.kde.org/r/112040/#comment27784

style: in header files, we don't put the return type on the separate line.



src/core-impl/collections/umscollection/UmsCollection.cpp
http://git.reviewboard.kde.org/r/112040/#comment27785

style: spaces around arg (in declaration it is right)



src/core-impl/collections/umscollection/UmsCollection.cpp
http://git.reviewboard.kde.org/r/112040/#comment27786

nitpicky simplification suggestion: if you use the interface afterwards, 
there's no point in calling the ::is() method, just call the ::as() method 
an then check the pointer.

Other extremely low-importance suggestion: we somehow prefer early exit 
than nesting, so something like
if( !sv )
retrun false;

would save you one nesting, which we like.



src/core-impl/collections/umscollection/UmsCollection.cpp
http://git.reviewboard.kde.org/r/112040/#comment27787

code style nitpick: spaces around args


- Matěj Laitl


On Aug. 12, 2013, 9:20 p.m., Frank Meerkoetter wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/112040/
 ---
 
 (Updated Aug. 12, 2013, 9:20 p.m.)
 
 
 Review request for Amarok.
 
 
 Description
 ---
 
 When I plug an encrypted USB-stick amarok is creating two UMS collections for 
 it. Both have the same mount point.
 
 $ solid-hardware list details
 [...]
 udi = '/org/freedesktop/UDisks/devices/dm_2d5'
   parent = '/org/freedesktop/UDisks/devices/sdd1'  (string)
   vendor = ''  (string)
   product = ''  (string)
   description = '29.9 GiB Removable Media'  (string)
   Block.major = 252  (0xfc)  (int)
   Block.minor = 5  (0x5)  (int)
   Block.device = '/dev/dm-5'  (string)
   StorageAccess.accessible = true  (bool)
   StorageAccess.filePath = '/media/62a745fa-6350-4ee5-ba37-0462dfa3530f'  
 (string)
   StorageAccess.ignored = false  (bool)
   StorageVolume.ignored = false  (bool)
   StorageVolume.usage = 'FileSystem'  (0x2)  (enum)
   StorageVolume.fsType = 'ext4'  (string)
   StorageVolume.label = ''  (string)
   StorageVolume.uuid = '62a745fa-6350-4ee5-ba37-0462dfa3530f'  (string)
   StorageVolume.size = 32125222912  (0x77ad0)  (qulonglong)
 [...]
 udi = '/org/freedesktop/UDisks/devices/sdd1'
   parent = '/org/freedesktop/UDisks/devices/sdd'  (string)
   vendor = 'JetFlash'  (string)
   product = 'Transcend 32GB'  (string)
   description = '29.9 GiB Encrypted Container'  (string)
   Block.major = 8  (0x8)  (int)
   Block.minor = 49  (0x31)  (int)
   Block.device = '/dev/sdd1'  (string)
   StorageAccess.accessible = true  (bool)
   StorageAccess.filePath = '/media/62a745fa-6350-4ee5-ba37-0462dfa3530f'  
 (string)
   StorageAccess.ignored = false  (bool)
   StorageVolume.ignored = false  (bool)
   StorageVolume.usage = 'Encrypted'  (0x5)  (enum)
   StorageVolume.fsType = 'crypto_LUKS'  (string)
   StorageVolume.label = ''  (string)
   StorageVolume.uuid = '1a38165b-2eee-41d0-acd1-6d34032f47fd'  (string)
   StorageVolume.size = 32127320064  (0x77af0)  (qulonglong)
 
 This patch is filtering out the storage volume where the usage field is set 
 to Encrypted (as opposed to Filesystem).
 
 
 Diffs
 -
 
   src/core-impl/collections/umscollection/UmsCollection.h 749ff81 
   src/core-impl/collections/umscollection/UmsCollection.cpp 028966e 
 
 Diff: http://git.reviewboard.kde.org/r/112040/diff/
 
 
 Testing
 ---
 
 I have tested plugin an USB-stick containing an dmcrypt/luks encrypted 
 ext4fs. I also tested with an USB-stick that was not encrypted.
 
 
 Thanks,
 
 Frank Meerkoetter
 


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

Re: Review Request 112040: Do not create duplicate UMS collections for encrypted volumes

[Matěj Laitl]

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

Ship it!


Yay, ship after 2.8 is tagged!

- Matěj Laitl


On Aug. 12, 2013, 9:46 p.m., Frank Meerkoetter wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/112040/
 ---
 
 (Updated Aug. 12, 2013, 9:46 p.m.)
 
 
 Review request for Amarok.
 
 
 Description
 ---
 
 When I plug an encrypted USB-stick amarok is creating two UMS collections for 
 it. Both have the same mount point.
 
 $ solid-hardware list details
 [...]
 udi = '/org/freedesktop/UDisks/devices/dm_2d5'
   parent = '/org/freedesktop/UDisks/devices/sdd1'  (string)
   vendor = ''  (string)
   product = ''  (string)
   description = '29.9 GiB Removable Media'  (string)
   Block.major = 252  (0xfc)  (int)
   Block.minor = 5  (0x5)  (int)
   Block.device = '/dev/dm-5'  (string)
   StorageAccess.accessible = true  (bool)
   StorageAccess.filePath = '/media/62a745fa-6350-4ee5-ba37-0462dfa3530f'  
 (string)
   StorageAccess.ignored = false  (bool)
   StorageVolume.ignored = false  (bool)
   StorageVolume.usage = 'FileSystem'  (0x2)  (enum)
   StorageVolume.fsType = 'ext4'  (string)
   StorageVolume.label = ''  (string)
   StorageVolume.uuid = '62a745fa-6350-4ee5-ba37-0462dfa3530f'  (string)
   StorageVolume.size = 32125222912  (0x77ad0)  (qulonglong)
 [...]
 udi = '/org/freedesktop/UDisks/devices/sdd1'
   parent = '/org/freedesktop/UDisks/devices/sdd'  (string)
   vendor = 'JetFlash'  (string)
   product = 'Transcend 32GB'  (string)
   description = '29.9 GiB Encrypted Container'  (string)
   Block.major = 8  (0x8)  (int)
   Block.minor = 49  (0x31)  (int)
   Block.device = '/dev/sdd1'  (string)
   StorageAccess.accessible = true  (bool)
   StorageAccess.filePath = '/media/62a745fa-6350-4ee5-ba37-0462dfa3530f'  
 (string)
   StorageAccess.ignored = false  (bool)
   StorageVolume.ignored = false  (bool)
   StorageVolume.usage = 'Encrypted'  (0x5)  (enum)
   StorageVolume.fsType = 'crypto_LUKS'  (string)
   StorageVolume.label = ''  (string)
   StorageVolume.uuid = '1a38165b-2eee-41d0-acd1-6d34032f47fd'  (string)
   StorageVolume.size = 32127320064  (0x77af0)  (qulonglong)
 
 This patch is filtering out the storage volume where the usage field is set 
 to Encrypted (as opposed to Filesystem).
 
 
 Diffs
 -
 
   src/core-impl/collections/umscollection/UmsCollection.cpp 028966e 
   src/core-impl/collections/umscollection/UmsCollection.h 749ff81 
 
 Diff: http://git.reviewboard.kde.org/r/112040/diff/
 
 
 Testing
 ---
 
 I have tested plugin an USB-stick containing an dmcrypt/luks encrypted 
 ext4fs. I also tested with an USB-stick that was not encrypted.
 
 
 Thanks,
 
 Frank Meerkoetter
 


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

Re: Triggering Copy to Collection from within a Service

[Matěj Laitl]

On 9. 8. 2013 Mark Kretschmann wrote:
 Heya,
 
 the Jamendo service has a Download button that currently triggers
 the download of a torrent file, and then starts a bittorrent
 application (if present) to download an album. However, this function
 uses a deprecated API from Jamendo, and what's worse, it barely ever
 works because there are no seeders.
 
 Jamendo however also allows direct download of tracks. As I was
 starting to implement this, I found out that the Copy to Collection
 feature does exactly that, and it also supports Amarok's progress
 logger system, and transcoding. So I want to use this same feature for
 the Download function as well, but of course I'd rather do it without
 duplicating the code.
 
 Can I trigger Copy to Collection from within the service in a clean way?

I don't think we have something generic enough. See CollectionTreeView.cpp 
around 257 and FileView.cpp around 101. Perhaps these are candidates for some 
deduplication and some shared helper methods?

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

Re: Propose: Change behaviour that 'Amarok guesses Album from Filepath if no album-tag is set

[Matěj Laitl]

On 30. 7. 2013 Mathias Dietrich wrote:
 So what would be the ideal solution for you, in case someone wants to
 step up and to fix this ?
 
 a) In additional checkbox in options which enables / disables this feature
 b) Change the behaviour to only start guessing when artist, trackname
 and album-tag is missing
 
 I would prefer the latter option.

Me too, adding yet another micro-option is a road to hell. :)

Markey, Edward, Myriam, Sam, what do you think?

 Maybe we can find a compromise where both kind of users are not affected.

Yeah, b) sound sensible to me.

 Also I would propose to create a feature request in kde's bugtracker that
 developers who want to step up can implement this request.

Do.

 Depending on the scope, I will also try to implement it but at first we
 need to find a compromise on that both can agree and which is not
 rejected at the reviewboard.

Yeah, this is a good approach. I don't think this will be hard to implement.

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

Re: Soft/hard dependency requirements post 2.8 - input, please

[Matěj Laitl]

On 29. 7. 2013 Myriam Schweingruber wrote:
 with the GSoC projects we discovered a few dependency requirements
 that will be necessary.
 
 libcdio 0.90

This is usually fine, the distros package the libraries *for us* 
(applications), not for themselves, so if we declare the dependency in advance 
and communicate it well, it is okay.

However, I've heard that libcdio 0.90 has changed the API wrt 0.83. (0.83 
being common is distros today) Sam/Tatjana, please answer the following 
decision tree:

* Is 0.90 API backward compatible with 0.83 one? (meaning that apps written
  against 0.83 compile  work fine with 0.90 without porting)
a) Yes - everything is fine, distros can compile all apps against 0.90 in
   their release after Amarok 2.9 (14.04 etc.)
b) No -
   * Can cdio 0.83 and 0.90 coexist on the same system without significant
 effort?
   ba) Yes - distros can just ship both
   bb) No -
   * Are other apps depending on cdio 0.83 (i.e. ffmpeg, mplayer, ...)
 being ported to 0.90? Are any patches floating around?
   bba) Yes - we need to coordinate with them to release versions
depending on 0.90 at roughly the same time.
   bbb) No - we might have a problem. Are the 0.83/0.90 changes
significant?

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

Re: Soft/hard dependency requirements post 2.8 - input, please

[Matěj Laitl]

On 29. 7. 2013 0inkane wrote:
 Hi,
 
 as apackager (Chakra) who's subscribed to this mailing list I guess I
 can help answering your question:
 libcdio is unfortunately not ABI compatible (I  guess you're interested
 in ABI, not API)

Actually not, I assumed ABI incompatibility. What matters here is API 
compatibility as we target to $next distro releases and we can tell them they 
need to rebase on newer library versions (in advance).

 see http://upstream-tracker.org/versions/libcdio.html

Hmm, this is nice, I didn't know about it. Although I assume that the 
algorithm to compute API/ABI compatibility is rather dumb, right?

Anyways, thanks for the input, distro packagers are the most competent to 
answer such questions.

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

Re: Review Request 111635: PlaybackConfig: add option whether playback should start on track add

[Matěj Laitl]

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

(Updated July 27, 2013, 12:31 p.m.)


Status
--

This change has been marked as submitted.


Review request for Amarok.


Description
---

PlaybackConfig: add option whether playback should start on track add

CHANGES:
 * Added an option whether adding tracks to playlist should start playing.

Also changed the entry in the 2.8 Beta ChangeLog, which is a bit strange,
but I believe the ChangeLog is most useful for differences between major
releases and it would be really confusing if the entry wasn't changed.

BUG: 322428
FIXED-IN: 2.8


Diffs
-

  ChangeLog 87e4708a6330e7a612db59052a50eef053294b06 
  src/amarokconfig.kcfg c9d202ad1212055f5877769c7add897547bb3f38 
  src/configdialog/dialogs/PlaybackConfig.ui 
2bbcba7e2b8f7fdafe98929741e9d08a069460d0 
  src/playlist/PlaylistController.h 185b4226c171c6a1fcd5b681721e39deae2e832b 
  src/playlist/PlaylistController.cpp 1d770fdff54ddc7f6bd6f5d813c377108d6f674d 

Diff: http://git.reviewboard.kde.org/r/111635/diff/


Testing
---


Thanks,

Matěj Laitl

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

Re: Review Request 111635: PlaybackConfig: add option whether playback should start on track add

[Matěj Laitl]

 On July 24, 2013, 6:21 p.m., Mark Kretschmann wrote:
  src/configdialog/dialogs/PlaybackConfig.ui, line 36
  http://git.reviewboard.kde.org/r/111635/diff/2/?file=173174#file173174line36
 
  The tooltip is too long, I think. In Amarok it is shown as one 
  extremely long line. As QToolTip supports Rich Text, it might be better to 
  break it up into paragraphs.

Good catch, thanks, I'll update the patch.


- Matěj


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


On July 24, 2013, 4:41 p.m., Matěj Laitl wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/111635/
 ---
 
 (Updated July 24, 2013, 4:41 p.m.)
 
 
 Review request for Amarok.
 
 
 Description
 ---
 
 PlaybackConfig: add option whether playback should start on track add
 
 CHANGES:
  * Added an option whether adding tracks to playlist should start playing.
 
 Also changed the entry in the 2.8 Beta ChangeLog, which is a bit strange,
 but I believe the ChangeLog is most useful for differences between major
 releases and it would be really confusing if the entry wasn't changed.
 
 BUG: 322428
 FIXED-IN: 2.8
 
 
 Diffs
 -
 
   ChangeLog 87e4708a6330e7a612db59052a50eef053294b06 
   src/amarokconfig.kcfg c9d202ad1212055f5877769c7add897547bb3f38 
   src/configdialog/dialogs/PlaybackConfig.ui 
 2bbcba7e2b8f7fdafe98929741e9d08a069460d0 
   src/playlist/PlaylistController.h 185b4226c171c6a1fcd5b681721e39deae2e832b 
   src/playlist/PlaylistController.cpp 
 1d770fdff54ddc7f6bd6f5d813c377108d6f674d 
 
 Diff: http://git.reviewboard.kde.org/r/111635/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Matěj Laitl
 


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

Re: Review Request 110187: Don't communicate with mysql by env vars and autogenerated files

[Matěj Laitl]

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


This looks good to me, let's test it after 2.8 is released. Just one very minor 
thing down below.


src/core-impl/collections/db/sql/mysqlecollection/MySqlEmbeddedStorage.cpp
http://git.reviewboard.kde.org/r/110187/#comment26926

I propose moving this just before 
--default-storage-engine=MyISAM as it was.


- Matěj Laitl


On July 19, 2013, 10:35 a.m., Patrick von Reth wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/110187/
 ---
 
 (Updated July 19, 2013, 10:35 a.m.)
 
 
 Review request for Amarok.
 
 
 Description
 ---
 
 Don't communicate with mysql by env vars and autogenerated files
 
 Instead of generating the my.cnf every time amarok starts and to pass
 the location of this file by setting an environment variable directly
 pass the settings as arguments to mysql. As this is probably a better
 approach and the only one working on windows.
 
 This fixes an issue where amarok is writing the database to C:\Program Files 
 (x86)\Amarok\data\amarok.
 This issue prevents Amarok from running correctly, because regarding to the 
 rights of the useraccount the directory can be not writeable.
 
 The commands used are taken from the output of mysqld --verbose --help. 
 
 
 Diffs
 -
 
   src/core-impl/collections/db/sql/mysqlecollection/MySqlEmbeddedStorage.cpp 
 0233498fdeb18ab51e709e9a78384fc37c47cb2a 
 
 Diff: http://git.reviewboard.kde.org/r/110187/diff/
 
 
 Testing
 ---
 
 Only on windows
 
 
 Thanks,
 
 Patrick von Reth
 


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

String freeze exception for Amarok (Extragear)

[Matěj Laitl]

Hi translators,
I'd like to request a string freeze exception (2 new strings) for Amarok. We 
changed mouse behaviour in 2.8 Beta, but it turned out the change was too 
drastic for some users, so we decided to introduce an option.

The patch is at https://git.reviewboard.kde.org/r/111635/diff/#index_header

Amarok 2.8 won't be released sooner than in a week (we're not part of KDE SC), 
so there should be some time to translate these.

Thanks,
Matěj
___
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel

Re: Review Request 111635: PlaybackConfig: add option whether playback should start on track add

[Matěj Laitl]

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

(Updated July 24, 2013, 4:41 p.m.)


Review request for Amarok.


Changes
---

Rebase on top of current master to fix build.


Description
---

PlaybackConfig: add option whether playback should start on track add

CHANGES:
 * Added an option whether adding tracks to playlist should start playing.

Also changed the entry in the 2.8 Beta ChangeLog, which is a bit strange,
but I believe the ChangeLog is most useful for differences between major
releases and it would be really confusing if the entry wasn't changed.

BUG: 322428
FIXED-IN: 2.8


Diffs (updated)
-

  ChangeLog 87e4708a6330e7a612db59052a50eef053294b06 
  src/amarokconfig.kcfg c9d202ad1212055f5877769c7add897547bb3f38 
  src/configdialog/dialogs/PlaybackConfig.ui 
2bbcba7e2b8f7fdafe98929741e9d08a069460d0 
  src/playlist/PlaylistController.h 185b4226c171c6a1fcd5b681721e39deae2e832b 
  src/playlist/PlaylistController.cpp 1d770fdff54ddc7f6bd6f5d813c377108d6f674d 

Diff: http://git.reviewboard.kde.org/r/111635/diff/


Testing
---


Thanks,

Matěj Laitl

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

Re: Review Request 111655: Reimplement RecentlyPlayedListWidget

[Matěj Laitl]

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

Ship it!


Although I don't really know the old code and haven't tested this, I like this 
change! I propose merging it to 2.8 (unless a problem is found when testing 
this), there is a bug closed, the behaviour should be better than the old one 
and it doesn't break string freeze. Before merging please have a look at the 
minor issues below.


src/context/widgets/RecentlyPlayedListWidget.h
http://git.reviewboard.kde.org/r/111655/#comment26858

I suggest adding explicit keyword as this could be called with one argument 
and misused as auto cast.



src/context/widgets/RecentlyPlayedListWidget.cpp
http://git.reviewboard.kde.org/r/111655/#comment26868

Much better to use ::insertOptioned( const KUrl url, ...) overload here 
because it calls CollectionManager::instance()-trackForUrl() asynchronously 
and handles other corner-cases.



src/context/widgets/RecentlyPlayedListWidget.cpp
http://git.reviewboard.kde.org/r/111655/#comment26860

In an extremely unlikely case, this is not safe. track-artist() may become 
null between the first and second call (remember Meta::Track is thread-safe).

We recommend to always use an extra temporary variable for all 
KSharedPtr-managed objects.



src/context/widgets/RecentlyPlayedListWidget.cpp
http://git.reviewboard.kde.org/r/111655/#comment26861

This doesn't work for right-to-left languages and languages that don't use 
- as a separator etc, it needs to be made translatable. On the other hand, you 
don't want to introduce an extra i18n string if this should be merged into 2.8.

The solution is to reuse (both context and text must be exactly the same)
ScrobblerAdapter.cpp:267:QString trackName = i18nc( %1 is artist, %2 
is title, %1 - %2, ...



src/context/widgets/RecentlyPlayedListWidget.cpp
http://git.reviewboard.kde.org/r/111655/#comment26866

It is much better to use the uidUrl() here. For example IpodCollection uses 
iPod serial name there, which is more unique than just file path. 
Additionally, playableUrl() might not be valid until prepareToPlay() is called 
and for example future MTP collection will return a temporary file name here.

With uidUrl(), you have much better chance of 
Playlist::Controller::insertOptioned() will find the original track (from the 
right collection).


- Matěj Laitl


On July 23, 2013, 12:36 p.m., Konrad Zemek wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/111655/
 ---
 
 (Updated July 23, 2013, 12:36 p.m.)
 
 
 Review request for Amarok.
 
 
 Description
 ---
 
 Reimplement RecentlyPlayedListWidget
 
 Rewrote RecentlyPlayedListWidget from the basics. There was a major
 inconsistency in this widget, where tracks were added to it if they were
 recently played at all, but the time shown was the Last Played
 statistics which is only updated after whole song is played. This
 caused gravely incorrect data to be displayed (see bug 302485).
 
 There were two different methods of solving the inconsistency: focusing
 on the Last Played time, and adding songs to the widget only if the
 Last Played changed; and focusing on recent plays completely
 disregarding Last Played. I opted for the latter as I felt it fits the
 widget's nature better.
 
 Because we can't rely on already available data, the widget needs to do
 its own housekeeping. It saves its data on shutdown, to be restored
 on next startup. One other major benefit of this approach is that
 widget's data remains correct even if collections change, while
 previously tracks from removed collections would disappear.
 
 Finally, I added the feature to add tracks to playlist directly from the
 widget, provided that the track exists. Adding to playlist works
 consistently with other parts of Amarok, with standard double-click and
 middle-click behavior.
 
 BUG: 302485
 FEATURE: 296090
 FEATURE: 279263
 REVIEW: 111655
 
 
 Diffs
 -
 
   src/context/widgets/RecentlyPlayedListWidget.h 
 caa78039b891511ca36ada8f61cd4f776d1f3c6d 
   src/context/widgets/RecentlyPlayedListWidget.cpp 
 6ea501affcf6e61d237002e15f7ed4e26989b91b 
 
 Diff: http://git.reviewboard.kde.org/r/111655/diff/
 
 
 Testing
 ---
 
 Manual.
 
 
 Thanks,
 
 Konrad Zemek
 


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

Re: Propose: Change behaviour that 'Amarok guesses Album from Filepath if no album-tag is set

[Matěj Laitl]

On 23. 7. 2013 Alan Ezust wrote:
 I really hate it when tags are written to my files by media players if all I
 am doing is importing and playing the songs.

No, you don't understand, Amarok has never written guessed tags to any files 
(unless you requested that). Amarok merely displays the guessed album.

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

Re: Review Request 111655: Reimplement RecentlyPlayedListWidget

[Matěj Laitl]

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

Ship it!


Looks nice to me. I unfortunately don't have time to actually test it. Perhaps 
markey can? Then please push.


src/context/widgets/RecentlyPlayedListWidget.cpp
http://git.reviewboard.kde.org/r/111655/#comment26889

Hmm, const-ref - by-value change doesn't seem really needed, but this is 
sooo unimportant, feel free to leave it as it it.



src/context/widgets/RecentlyPlayedListWidget.cpp
http://git.reviewboard.kde.org/r/111655/#comment26888

Yeah, this is sometimes needed, neither I like it but it seems like a fair 
price to pay for the fwd-declaration goodies.



src/playlist/PlaylistController.cpp
http://git.reviewboard.kde.org/r/111655/#comment26887

Nice catch! Please commit this separately right away.


- Matěj Laitl


On July 23, 2013, 9:21 p.m., Konrad Zemek wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/111655/
 ---
 
 (Updated July 23, 2013, 9:21 p.m.)
 
 
 Review request for Amarok.
 
 
 Description
 ---
 
 Reimplement RecentlyPlayedListWidget
 
 Also fix an issue where certain overloads of
 Playlist::Controller::insertOptioned couldn't automatically start
 playback when given StartPlay option.
 
 Rewrote RecentlyPlayedListWidget from the basics. There was a major
 inconsistency in this widget, where tracks were added to it if they were
 recently played at all, but the time shown was the Last Played
 statistics which is only updated after whole song is played. This
 caused gravely incorrect data to be displayed (see bug 302485).
 
 There were two different methods of solving the inconsistency: focusing
 on the Last Played time, and adding songs to the widget only if the
 Last Played changed; and focusing on recent plays completely
 disregarding Last Played. I opted for the latter as I felt it fits the
 widget's nature better.
 
 Because we can't rely on already available data, the widget needs to do
 its own housekeeping. It saves its data on shutdown, to be restored
 on next startup. One other major benefit of this approach is that
 widget's data remains correct even if collections change, while
 previously tracks from removed collections would disappear.
 
 Finally, I added the feature to add tracks to playlist directly from the
 widget, provided that the track exists. Adding to playlist works
 consistently with other parts of Amarok, with standard double-click and
 middle-click behavior.
 
 BUG: 302485
 FEATURE: 296090
 FEATURE: 279263
 REVIEW: 111655
 
 
 Diffs
 -
 
   src/context/widgets/RecentlyPlayedListWidget.h 
 caa78039b891511ca36ada8f61cd4f776d1f3c6d 
   src/context/widgets/RecentlyPlayedListWidget.cpp 
 6ea501affcf6e61d237002e15f7ed4e26989b91b 
   src/playlist/PlaylistController.cpp 
 60c99e5a10459b84b9839f07aad0d1e2bfa5d259 
 
 Diff: http://git.reviewboard.kde.org/r/111655/diff/
 
 
 Testing
 ---
 
 Manual.
 
 
 Thanks,
 
 Konrad Zemek
 


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

Re: Propose: Change behaviour that 'Amarok guesses Album from Filepath if no album-tag is set

[Matěj Laitl]

On 22. 7. 2013 Mathias Dietrich wrote:
 although it did not work in Amarok 2.7.1 and before, we now have the
 following behavior in Amarok-git, which is according to Matěj no bug and
 wanted:
 
 1. Collect multiple songs in one folder
 2. Ensure that the songs have no album-tag
 3. Start Amarok and import the collection
 
 Result:
 
 Although I specifically removed the album-tag from my files,

There is an important question: if you removed the album tag from the files and 
the heuristics, under which album do you want Amarok to show the tracks in 
Collection Browser? Because if we simply removed the heuristics and did 
nothing else, it would AFAICS show under Unknown Album. Is that really what 
you want?

 As a power user, managing a large collection of songs, using Amarok. I
 propose to change this behavior before it gets released.

Power users are just one of our target groups, please note that we also want 
to create a good player for people that don't know much about technical 
details of tags etc.

 To my mind this behavior makes no sense at all. I know other users that are
 the same opinion.

And I know other users of different opinion. For an application with half a 
million of installs, you can find a user with *any* opinion.

 My reasons are the following:
 
 1. If a user removes the album-tag using a tagging software, he wants to
 ensure that the track gets no album
 
 - e.g.: I have a large collection of single songs from different albums,
 like always the best 3 songs of the album. Although I don't want hundreds
 of albums in my collection just because I have 1-3 songs of an album. So I
 remove the album-tag. If I would wanted the all the albums in my
 collection, I would simply keep the album-tag.
 
 - Problem is, Amarok now starts guessing my albums according to my folder
 structure and I have to delete all guessed albums again within Amarok :(
 
 2. The heuristic to guess the album by file path is no good solution in
 two ways.
 
 Firstly, if I use a music player which is based and works with Tags, I just
 want to it to be based on tags. I just want to add my collection and don't
 necessary want to mess around with the file structure of my tracks.
 
 Secondly, the heuristic was broken in the past (=2.7.1) and nobody wanted
 to have an album guessed by file path. Correct me if you find a feature
 request for this.

nobody wanted -- did you ask every Amarok user? I guess the developer 
implementing the feature wasn't insane and actually wanted it.

 At third, the heuristic in the moment is a bad solution: take the folder
 containing all the files. If I throw a lot of files (1000) within a
 folder, like my music. All this files get the album my music. It's no
 good solution at all.

Not really, if at least one song of that directory would have the album set, 
it wouldn't assign album to the other ones.

 That is why I propose multiple solutions to improve situation:
 
 1. Remove this behavior at all, if it was no feature request. If someone
 removes the album-tag he does not want it. It's very unlikely that someone
 listens to music that, unintentionally, contains no tags.

Quite the contrary, it is very likely! Many our users don't really know about 
tags and we still want Amarok to be at least somehow usable for them. Many 
access music with no write permissions so that they cannot really tag the 
tracks even if they'd like to.

 2. Only guess something of no tags are available at all. This way it would
 be safe to assume that someone imported music without tags, because he does
 not know that tags exist, which is highly unlikely.

I'm -0.25 for removing the feature completely, because I still think it serves 
well for users that don't even know that their tracks actually don't have 
album set. 2. might not be a bad option. It also could be technically possible 
to differentiate between empty album tag and no album tag, but that may seem 
convoluted to the users.

I don't really care much about this. Other devs, if you want to change the 
behaviour somehow, go for it, because I don't think I'm going to touch it 
(enough behavioural changes from me).

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

Re: Review Request 111512: MPRIS2: avoid updating Metadata when between tracks

[Matěj Laitl]

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

Ship it!


Yeah, ship it!

- Matěj Laitl


On July 15, 2013, 3:18 p.m., Alex Merry wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/111512/
 ---
 
 (Updated July 15, 2013, 3:18 p.m.)
 
 
 Review request for Amarok.
 
 
 Description
 ---
 
 MPRIS2: avoid updating Metadata when between tracks
 
 When changing tracks, we would emit PropertiesChanged for Metadata
 twice, once with mpris:trackid set to /org/kde/amarok/PendingTrack and
 once with the actual new trackid (because the first time the playlist
 code had not yet updated the active track).  If the track was changed
 manually (not just progressing to the next one) we would often also emit
 a PropertiesChanged with an empty Metadata before repopulating it.
 
 This solves the first issue by making the signal connection for
 trackChanged from EngineController queued, meaning that by the time the
 MPRIS2 code gets the signal, the playlist has updated the activeTrack
 and we can easily figure out the correct trackid.
 
 It solves the second issue by ignoring the trackLengthChanged signal
 when it has a meaningless value (0), which seems to happen at some
 point during track changes that are not predictable.
 
 BUG: 321602
 
 
 Diffs
 -
 
   ChangeLog 5fe5d2e64c771d722f3f90bf6c98d5013e56553c 
   src/dbus/mpris2/MediaPlayer2Player.cpp 
 a633756bf558a89ba2a3db2307c0ebbc373a759b 
 
 Diff: http://git.reviewboard.kde.org/r/111512/diff/
 
 
 Testing
 ---
 
 Tested using a tool that listens to the PropertiesChanged signal of the 
 MPRIS2 interface and lists when the mpris:trackid changes.
 
 Without the patch, I get output like
 Track change: /org/kde/amarok/Track/5739423209746661216 - 
 /org/kde/amarok/PendingTrack
 Track change: /org/kde/amarok/PendingTrack - 
 /org/kde/amarok/Track/8264712350997591513
 when the track progresses because the previous track finished, and
 Track change: /org/kde/amarok/Track/5739423209746661216 - 
 Track change:  - /org/kde/amarok/Track/8264712350997591513
 when I manually change the track (eg: by clicking next or by 
 double-clicking another track).
 
 With the patch, I only get things like
 Track change: /org/kde/amarok/Track/5739423209746661216 - 
 /org/kde/amarok/Track/8264712350997591513
 
 
 Thanks,
 
 Alex Merry
 


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

Amarok 2.8 Beta Tarballs Available

[Matěj Laitl]

Dear Amarok packagers,
here you can find the tarball for the Amarok 2.8 Beta release:
http://download.kde.org/unstable/amarok/2.7.90/src/amarok-2.7.90.tar.bz2.mirrorlist

We plan to release the final about 3 weeks from now but please make
this available to users via a specialized beta channel if possible. We hope to 
get as much testers for this beta as possible.

Notable packaging changes since Amarok 2.7(.1):
 * Qt dependency was raised from 4.8.0 to 4.8.2
 * If compiled against pre-release of TagLib 1.9, Opus support is enabled
 * libmygpo-qt dependency raised from 1.0.6 to 1.0.7
 * TagLib (at least 1.7) is now mandatory, not optional

As usual, latest version of one of the maintained phonon backends (phonon-
vlc-0.6.2 or phonon-gstreamer-4.6.3) is strongly recommended for playback to 
function properly.

Checksums

MD5Sum: 71b7d5949578bbc2365d0861126b38c0  amarok-2.7.90.tar.bz2
SHA1Sum: 98975dcb757d6d82dba93f808278e4ae1e863a78  amarok-2.7.90.tar.bz2

Documentation (9)
###
de es et nl pt pt_BR ru sv uk

Translations (34)
##
ca ca@valencia cs da de el es et eu fi fr ga gl hu it ja lt lv nb nl pl pt 
pt_BR ru sl sr sr@ijekavian sr@ijekavianlatin sr@latin sv tr uk zh_CN zh_TW

Thanks for packaging,
Matěj Laitl on behalf of The Amarok Team

signature.asc
Description: This is a digitally signed message part.
___
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel

Amarok 2.8 Beta Release

[Matěj Laitl]

Hi everybody,
I'm happy to let you know that Amarok 2.8 Beta has just been released to the 
public. This release boasts many improvements all over place, great amount of 
bug fixes and behaviour corrections. It also should work with KDE 4.11 just 
fine.

Please see http://amarok.kde.org/en/releases/2.8/beta/1 for more details and 
download options. We'd like to get a lot of testers to be able to release 
*cough* (nearly) bug-free Amarok 2.8 final.

Translators,
Amarok is now in string freeze until 2.8 is out, we'd be happy to ship as many 
translations as possible. Following languages that were included in Amarok 
2.7(.1) need their teams to translate more strings to be included in 2.8:
Bosnian, British English, Punjabi.
We congratulate the Turkish team to be newly included. The Romanians could 
also see their language included if they translate some more strings.

Fellow developers,
please mind the string freeze and definitely switch to bugfixing mode. We've 
managed to get number of open bugs below 150, which may well be all-time-low 
for the Amarok 2.x series. The goal is to release Amarok 2.8 with under 100 
open bugs. :-)

Regards and thanks for all the work,
Matěj

signature.asc
Description: This is a digitally signed message part.
___
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel

Re: Review Request 110920: Fight warnings.

[Matěj Laitl]

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

Ship it!


If it compiles with the const, it cannot hurt, ship it!

- Matěj Laitl


On June 9, 2013, 9:53 p.m., Wolf-Michael Bolle wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/110920/
 ---
 
 (Updated June 9, 2013, 9:53 p.m.)
 
 
 Review request for Amarok.
 
 
 Description
 ---
 
 Fight warnings.
 
 Nobody will write that string after it has beed set.
 
 
 Diffs
 -
 
   src/services/mp3tunes/libmp3tunes/harmony.h 
 e0941e348db09ba69c39426077b6798e758d6354 
 
 Diff: http://git.reviewboard.kde.org/r/110920/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Wolf-Michael Bolle
 


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

Re: Review Request 111275: putting Artist - Title of the current track to the clipboard using a shortcut

[Matěj Laitl]

 On June 28, 2013, 6:59 p.m., Mark Kretschmann wrote:
  Nice patch! I recall we used to have this feature in an earlier Amarok 
  version, but it got lost at some point :)
  
  Do you have push rights for kde git, or should we push it for you?
 
 fleissig fleissig wrote:
 I don't have rights, you should push it.
 
 Mark Kretschmann wrote:
 Do you want fleissig fleissig as your name in the ChangeLog, or do you 
 prefer another name?

I'd even go further, we do really want to have contributions under the real 
names, be it for proper attribution or copyright matters.


- Matěj


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


On June 28, 2013, 10:57 p.m., fleissig fleissig wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/111275/
 ---
 
 (Updated June 28, 2013, 10:57 p.m.)
 
 
 Review request for Amarok.
 
 
 Description
 ---
 
 putting Artist - Title of the current track to the clipboard using a shortcut
 
 
 This addresses bug 228872.
 https://bugs.kde.org/show_bug.cgi?id=228872
 
 
 Diffs
 -
 
   src/MainWindow.h b2ee08014c110d1979176fe9d4abfad5076c1179 
   src/MainWindow.cpp af47db7226244cc81f1a4f5d3c980d758b93 
 
 Diff: http://git.reviewboard.kde.org/r/111275/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 fleissig fleissig
 


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

Re: Review Request 111275: putting Artist - Title of the current track to the clipboard using a shortcut

[Matěj Laitl]

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


Fine patch, I have just 2 minor suggestions.


src/MainWindow.cpp
http://git.reviewboard.kde.org/r/111275/#comment25807

I suggest safety against artist() being null. Note that:
if( ...-artist() )
...-artist()-...

is unsafe, you need a temporary.



src/MainWindow.cpp
http://git.reviewboard.kde.org/r/111275/#comment25804

nitpick: one extra whitespace


- Matěj Laitl


On June 27, 2013, 9:48 p.m., fleissig fleissig wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/111275/
 ---
 
 (Updated June 27, 2013, 9:48 p.m.)
 
 
 Review request for Amarok.
 
 
 Description
 ---
 
 putting Artist - Title of the current track to the clipboard using a shortcut
 
 
 This addresses bug 228872.
 https://bugs.kde.org/show_bug.cgi?id=228872
 
 
 Diffs
 -
 
   src/MainWindow.cpp af47db7226244cc81f1a4f5d3c980d758b93 
   src/MainWindow.h b2ee08014c110d1979176fe9d4abfad5076c1179 
 
 Diff: http://git.reviewboard.kde.org/r/111275/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 fleissig fleissig
 


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

Re: Review Request 111275: putting Artist - Title of the current track to the clipboard using a shortcut

[Matěj Laitl]

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



src/MainWindow.cpp
http://git.reviewboard.kde.org/r/111275/#comment25827

Oh this is convoluted. What about

QString text;
Meta::ArtistPtr artist = currentTrack-artist();
if( artist )
text = artist-prettyName() +  - ;
text += currentTrack-prettyName()

clipboard-setText( text );


- Matěj Laitl


On June 28, 2013, 9:18 p.m., fleissig fleissig wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/111275/
 ---
 
 (Updated June 28, 2013, 9:18 p.m.)
 
 
 Review request for Amarok.
 
 
 Description
 ---
 
 putting Artist - Title of the current track to the clipboard using a shortcut
 
 
 This addresses bug 228872.
 https://bugs.kde.org/show_bug.cgi?id=228872
 
 
 Diffs
 -
 
   src/MainWindow.h b2ee08014c110d1979176fe9d4abfad5076c1179 
   src/MainWindow.cpp af47db7226244cc81f1a4f5d3c980d758b93 
 
 Diff: http://git.reviewboard.kde.org/r/111275/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 fleissig fleissig
 


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

Re: [amarok] /: Make playlist-related actions consistent throughout Amarok code (behaviour change)

[Matěj Laitl]

On 27. 6. 2013 Myriam Schweingruber wrote:
 On Wed, Jun 26, 2013 at 9:34 PM, Matěj Laitl ma...@laitl.cz wrote:
  Options for the activate (double or single click, per configuration,
  if we so choose) action; feel free to suggest more:
  
  a) append track to playlist and start playing perhaps a different
  (depends on other factors) track in the playlist, if not already
  playing. The old behaviour.
 
 That was not the old behavior, the old behavior was double-click
 appends to playlist. There never was an immediate start of a track
 playing on double-click.

In fact, there was. Steps to reproduce:
 1. git checkout de62cfb920d05f  # the one before my commit
 2. build / run, ensure it is not playing on startup
 3. double-click on any artist in your collection; playback starts

But this doesn't matter at all for the current situation, I just think this 
may be the root of the misunderstanding here.

 So -1 on that, as it was never working like that.

-1 from me too, this (IMO unfortunate) behaviour was in fact the trigger for 
me to have a look at it. I thought you wanted to revert to exactly this 
behaviour (by saying keep the old behaviour), and I thought Markey insisted 
on starting playback on double-click.

  b) just append to playlist.
 
 +1

This is fine with me and I didn't know this is fine with you. That's why I'm 
suggesting there has been a terrible misunderstanding.

  c) insert after currently playing, play the track(s) immediately. The
  new
  and controversial behaviour.
 
 -2, it really is annoying on more than one point, not only does it add
 the track, but it adds it after the current track and starts playing,
 and it even adds queue markers, very disturbing. This makes an easy
 append to playlist action impossible, I have been swearing a hundred
 times abut this behavior since the change.

Understandable that you don't want to change your habits. I'm still +1 on this 
but I'm fine to be outweighed on this matter.

 Leave the double-click as it was, at least it doesn't disturb the way
 hundreds of users are used to.

^^^ This was the source of my prolonged confusion.

Regards,
Matěj
___
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel

[amarok] /: Collection Browser: replace Artist by Album Artist by default

[Matěj Laitl]

Git commit 78415dfb1d44c59a0f53748071ce57b794abd83a by Matěj Laitl.
Committed on 27/06/2013 at 18:13.
Pushed by laitl into branch 'master'.

Collection Browser: replace Artist by Album Artist by default

CHANGES:
 * Collection Browser: Artist level was renamed to Track Artist and replaced by 
Album
   Artist by default. Various Artists item is no longer shown under Track 
Artist level.

Following was done:
 a) shop showing Various Artists under Artist level and therefore
stop showing some tracks twice. Show Various Artists only under the
Album Artist level where it logically belongs
 b) tweak the default presets to use Album Artist where they previously
used Arist, except for Album / Artist where it doesn't make sense
 c) adapt user configs treat artist as album artist for users migrating
from older Amarok releases
 d) rename Artist level to Track Artist make the distinction more clear
 e) use current Artist icon even for Album Artist level, Sentynel and
Mamarok finds the current one way too fugly

^^^ I think that the points above combined would mean nearly no
disruption in user experience and would make the interface more logical
and correct.

GUI: A set of changes to Collection Browser levels, Artist level was
renamed to Track Artist, its behaviour was changed and it was replaced
by Album Artist by default.
CCMAIL: amarok-devel@kde.org
CCMAIL: amarok-pr...@kde.org

M  +2-0ChangeLog
M  +1-1src/browsers/BrowserDefines.h
M  +49   -30   src/browsers/CollectionTreeItemModelBase.cpp
M  +6-1src/browsers/CollectionTreeItemModelBase.h
M  +78   -88   src/browsers/collectionbrowser/CollectionWidget.cpp
M  +4-4src/browsers/collectionbrowser/CollectionWidget.h

http://commits.kde.org/amarok/78415dfb1d44c59a0f53748071ce57b794abd83a

diff --git a/ChangeLog b/ChangeLog
index a0a39d0..84eb9e2 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -33,6 +33,8 @@ VERSION 2.8-Beta 1
* Added Ctrl+H shortcut to randomize playlist, patch by Harsh Gupta. (BR 
208061)
 
   CHANGES:
+   * Collection Browser: Artist level was renamed to Track Artist and replaced 
by Album
+ Artist by default. Various Artists item is no longer shown under Track 
Artist level.
* Removed the splash screen.
* Playlist-related actions were harmonized, double-clicking or pressing 
enter will
  append tracks to playlist, middle-clicking or using any play media 
action will
diff --git a/src/browsers/BrowserDefines.h b/src/browsers/BrowserDefines.h
index d3f01f2..8e0f258 100644
--- a/src/browsers/BrowserDefines.h
+++ b/src/browsers/BrowserDefines.h
@@ -30,7 +30,7 @@ namespace CategoryId
 enum CatMenuId {
 None = 0,
 Album = 1,
-Artist = 2,
+Artist = 8, // used to be 2, transitioned to 8 to allow for transition 
pre Amarok 2.8
 AlbumArtist = 3,
 Composer = 4,
 Genre = 5,
diff --git a/src/browsers/CollectionTreeItemModelBase.cpp 
b/src/browsers/CollectionTreeItemModelBase.cpp
index b811c34..4ec642b 100644
--- a/src/browsers/CollectionTreeItemModelBase.cpp
+++ b/src/browsers/CollectionTreeItemModelBase.cpp
@@ -54,8 +54,12 @@ inline uint qHash( const Meta::DataPtr data )
 return qHash( data.data() );
 }
 
+/**
+ * This set determines which collection browser levels should have shown 
Various Artists
+ * item under them. AlbumArtist is certain, (Track)Artist is questionable.
+ */
 static const QSetCategoryId::CatMenuId variousArtistCategories =
-QSetCategoryId::CatMenuId()  CategoryId::AlbumArtist  
CategoryId::Artist;
+QSetCategoryId::CatMenuId()  CategoryId::AlbumArtist;
 
 CollectionTreeItemModelBase::CollectionTreeItemModelBase( )
 : QAbstractItemModel()
@@ -485,31 +489,6 @@ CollectionTreeItemModelBase::ensureChildrenLoaded( 
CollectionTreeItem *item )
 }
 }
 
-QIcon
-CollectionTreeItemModelBase::iconForLevel(int level) const
-{
-switch( m_levelType.value( level ) )
-{
-case CategoryId::Album :
-return KIcon( media-optical-amarok );
-case CategoryId::Artist :
-return KIcon( view-media-artist-amarok );
-case CategoryId::AlbumArtist :
-return KIcon( amarok_artist );
-case CategoryId::Composer :
-return KIcon( filename-composer-amarok );
-case CategoryId::Genre :
-return KIcon( favorite-genres-amarok );
-case CategoryId::Year :
-return KIcon( clock );
-case CategoryId::Label :
-return KIcon( label-amarok );
-case CategoryId::None :
-default:
-return KIcon( image-missing );
-}
-}
-
 void CollectionTreeItemModelBase::listForLevel(int level, 
Collections::QueryMaker * qm, CollectionTreeItem * parent)
 {
 if( qm  parent )
@@ -967,15 +946,47 @@ CollectionTreeItemModelBase::updateHeaderText()
 m_headerText.chop( 3 );
 }
 
+QIcon
+CollectionTreeItemModelBase::iconForCategory( CategoryId::CatMenuId category )
+{
+switch( category

Re: Static code analysis available

[Matěj Laitl]

On 27. 6. 2013 Mark Kretschmann wrote:
 Hi all,
 
 we have now available static code analysis from two different tools.
 Both of the tool chains are not yet automated, i.e. not integrated
 with the CI and updated automatically, but they currently depend on
 manual uploading of Amarok source tree snapshots.
 
 The first tool is clang-analyzer, which proved to be surprisingly
 effective. Note the very nice visualizations of the chain of logic
 leading to each defect. You can access the results of a preliminary
 scan here:
 
 http://dev.hades.name/scanview/amarok/
 
 Regarding quality of the analysis, there is one glaring issue with
 Q_ASSERT, which leads to a great number of false positives. To get rid
 of these it seems we would have to use a patched version of the macro,
 as detailed here:
 http://clang-analyzer.llvm.org/annotations.html#custom_assertions
 
 Then we also have access to Coverity, which is a popular commercial
 tool offering free scans for open source projects. To get access,
 create an account on the following site, and then simply click Add
 Project, and then Amarok:
 
 http://scan.coverity.com/

Good work, Markey and Edward, both reports seem to be quite useful and I've 
seen they discover real (potential) problems.

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

[amarok] src/playlist: PlaylistController: revert double-clicking to append start playing if not already

[Matěj Laitl]

Git commit a036b39f164ebe8e5060f4ea48bf8854ba6aa87e by Matěj Laitl.
Committed on 26/06/2013 at 20:18.
Pushed by laitl into branch 'master'.

PlaylistController: revert double-clicking to append  start playing if not 
already

I dislike this behaviour strongly, but it is fair to level the
playing field for the ongoing discussion. This should get the behaviour
to the state before the refactorings, without reverting the (much needed
IMO) refactorings themselves.

CCMAIL: amarok-devel@kde.org

M  +2-2src/playlist/PlaylistController.h

http://commits.kde.org/amarok/a036b39f164ebe8e5060f4ea48bf8854ba6aa87e

diff --git a/src/playlist/PlaylistController.h 
b/src/playlist/PlaylistController.h
index 25d020f..5f96d0e 100644
--- a/src/playlist/PlaylistController.h
+++ b/src/playlist/PlaylistController.h
@@ -55,11 +55,11 @@ enum AddOption
 // bahaviour of similarly-looking UI elements the same. These enums are 
the preferred
 // ones on calling sites. Feel free to add a new one if you find another 
UI element
 // that appears on multiple places. Prefix these with On*.
-OnDoubleClickOnSelectedItems = DirectPlay,
+OnDoubleClickOnSelectedItems = StartPlay,
 OnMiddleClickOnSelectedItems = 0, // append
 OnReturnPressedOnSelectedItems = OnDoubleClickOnSelectedItems, // these 
should be kept same
 
-OnPlayMediaAction = OnDoubleClickOnSelectedItems,
+OnPlayMediaAction = DirectPlay,
 OnAppendToPlaylistAction = 0, // no-brainer, just for consistency, applied 
to popup-dropper too
 OnReplacePlaylistAction = Replace, // ditto
 OnQueueToPlaylistAction = Queue,
___
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel

[amarok] src/browsers: Collection Browser: revert to showing Various Artists even under Artist level [RFC]

[Matěj Laitl]

Git commit c336d3777393a177c340f487d14508a6788e519a by Matěj Laitl.
Committed on 26/06/2013 at 19:54.
Pushed by laitl into branch 'master'.

Collection Browser: revert to showing Various Artists even under Artist level 
[RFC]

I don't like this, because it is illogical and shows some of your tracks twice:
http://picpaste.com/amarok-artist-level-shows-track-twice.png

...but the previous change alone was too disruptive for our users.

I suggest the following:
 a) show showing Various Artists under Artist level and therefore
stop showing some tracks twice. Show Various Artists only under the
Album Artist level where it logically belongs
 b) tweak the default presets to use Album Artist where they previously
used Arist, except for Album / Artist where it doesn't make sense
 c) (optional) adapt user configs treat artist as album artist for
users migrating from older Amarok releases
 d) (optional) rename Artist level to Track Artist make the
distinction more clear
 e) (optional) find better icon for Album Artist items/use Artist
icon, Sentynel finds the current one way too fugly

^^^ I think that the points above combined would mean nearly no
disruption in user experience and would make the interface more logical
and correct.

What do you think? Do you have some objections?

CCMAIL: amarok-devel@kde.org

M  +15   -19   src/browsers/CollectionTreeItemModelBase.cpp

http://commits.kde.org/amarok/c336d3777393a177c340f487d14508a6788e519a

diff --git a/src/browsers/CollectionTreeItemModelBase.cpp 
b/src/browsers/CollectionTreeItemModelBase.cpp
index ad78c96..b811c34 100644
--- a/src/browsers/CollectionTreeItemModelBase.cpp
+++ b/src/browsers/CollectionTreeItemModelBase.cpp
@@ -54,6 +54,8 @@ inline uint qHash( const Meta::DataPtr data )
 return qHash( data.data() );
 }
 
+static const QSetCategoryId::CatMenuId variousArtistCategories =
+QSetCategoryId::CatMenuId()  CategoryId::AlbumArtist  
CategoryId::Artist;
 
 CollectionTreeItemModelBase::CollectionTreeItemModelBase( )
 : QAbstractItemModel()
@@ -540,25 +542,20 @@ void CollectionTreeItemModelBase::listForLevel(int level, 
Collections::QueryMake
 
 qm-setQueryType( mapCategoryToQueryType( m_levelType.value( level 
) ) );
 
-switch( m_levelType.value( level ) )
+CategoryId::CatMenuId category = m_levelType.value( level );
+if( category == CategoryId::Album )
 {
-case CategoryId::Album:
-// restrict query to normal albums if the previous level
-// was the AlbumArtist category. In that case we handle 
compilations below
-if( levelCategory( level - 1 ) == CategoryId::AlbumArtist )
-qm-setAlbumQueryMode( 
Collections::QueryMaker::OnlyNormalAlbums );
-break;
-case CategoryId::AlbumArtist:
-// we used to handleCompilations() only if nextLevel is 
Album, but I cannot
-// tell any reason why we should have done this --- strohel
-handleCompilations( nextLevel, parent );
-break;
-case CategoryId::Label:
-handleTracksWithoutLabels( nextLevel, parent );
-break;
-default : //TODO handle error condition. return tracks?
-break;
+// restrict query to normal albums if the previous level
+// was the AlbumArtist category. In that case we handle 
compilations below
+if( levelCategory( level - 1 ) == CategoryId::AlbumArtist )
+qm-setAlbumQueryMode( 
Collections::QueryMaker::OnlyNormalAlbums );
 }
+else if( variousArtistCategories.contains( category ) )
+// we used to handleCompilations() only if nextLevel is Album, 
but I cannot
+// tell any reason why we should have done this --- strohel
+handleCompilations( nextLevel, parent );
+else if( category == CategoryId::Label )
+handleTracksWithoutLabels( nextLevel, parent );
 }
 
 for( CollectionTreeItem *tmp = parent; tmp; tmp = tmp-parent() )
@@ -641,7 +638,6 @@ CollectionTreeItemModelBase::addQueryMaker( 
CollectionTreeItem* item,
 m_runningQueries.insert( item, qm );
 }
 
-
 void
 CollectionTreeItemModelBase::queryDone()
 {
@@ -918,7 +914,7 @@ CollectionTreeItemModelBase::populateChildren( const 
DataList dataList, Collect
 if( child-isDataItem() )
 toBeRemoved = dataToBeRemoved.contains( child-data() );
 else if( child-isVariousArtistItem() )
-toBeRemoved = childCategory != CategoryId::AlbumArtist;
+toBeRemoved = !variousArtistCategories.contains( childCategory );
 else if( child-isNoLabelItem() )
 toBeRemoved = childCategory != CategoryId::Label;
 else

Re: Fwd: [amarok] [Bug 321081] Lyrics not fetching for any tracks

[Matěj Laitl]

On 18. 6. 2013 Myriam Schweingruber wrote:
 Subject: [amarok] [Bug 321081] Lyrics not fetching for any tracks
 
 https://bugs.kde.org/show_bug.cgi?id=321081
 
 FYI. It is indeed NOT in the list of required dependencies in the
 README file, we only list the QtScriptgenerator

This is fine and intentional. We list Qt as a dependency, QtScript is a part of 
qt. This is the same with qt-core, qt-gui, qt-opengl etc. - it is the distros 
that split the packages, so it is their problem. They also can split (and 
name) them arbitrarily, so we even *cannot* list such packages.

Matěj

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