Bug 243344 - [PATCH] Wrong grouping algorithm.
Summary: [PATCH] Wrong grouping algorithm.
Status: RESOLVED FIXED
Alias: None
Product: amarok
Classification: Unclassified
Component: Playlist (show other bugs)
Version: 2.4-GIT
Platform: Archlinux Packages Linux
: NOR normal with 20 votes (vote)
Target Milestone: ---
Assignee: Amarok Developers
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-07-01 14:00 UTC by Alexander
Modified: 2011-11-05 14:28 UTC (History)
6 users (show)

See Also:
Latest Commit:
Version Fixed In: 2.5.0


Attachments
Example. (25.84 KB, image/png)
2010-07-01 17:41 UTC, Alexander
Details
An another example. (158.55 KB, image/png)
2010-07-01 17:48 UTC, Alexander
Details
Screenshot 1 (grouping by artist) (85.43 KB, image/png)
2010-07-03 00:03 UTC, Andrew Meakovski
Details
Screenshot 2 (grouping by genre) (79.01 KB, image/png)
2010-07-03 00:04 UTC, Andrew Meakovski
Details
triple-screenshot to describe grouping issue (983.60 KB, image/png)
2010-07-07 16:49 UTC, Andrew Meakovski
Details
Fourth screenshot. (212.51 KB, image/png)
2010-07-09 14:44 UTC, Alexander
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander 2010-07-01 14:00:48 UTC
Version:           2.3.1-GIT (using KDE 4.4.4) 
OS:                Linux

I just can't understand, how Amarok was able to grouped a .mp3 track and a shoutcast stream under the same heading? You compare only album names? If this track has unknown album and stream has the same, you think it should be under the same heading? And what about the artist? It does not matter?

Reproducible: Always
Comment 2 Myriam Schweingruber 2010-07-01 17:06:14 UTC
Alexander, please do not give external links to images, attach it to the report instead.

So far I don't see any grouping, you should maybe have a look at your playlist layout in use. The stream just doesn't have a header.
Comment 3 Alexander 2010-07-01 17:41:26 UTC
Created attachment 48515 [details]
Example.

> Alexander, please do not give external links to images, attach it to the report instead.
Ok, I am sorry, already done.

> The stream just doesn't have a header.
Look carefully, the first track in the playlist doesn't has a header, but second and third has, and it is the one for two.
Comment 4 Alexander 2010-07-01 17:48:31 UTC
Created attachment 48516 [details]
An another example.

Look, I've just copied the two first tracks from playlist onto my desktop and added they again, but with one difference — I've changed the names of their albums on "Nonexistent album".
Comment 5 Alexander 2010-07-01 18:50:55 UTC
Amarok is great, the one of greatest, but it is full of errors and lacks and I am curious, why you do not want to notice them. I am sorry if say sad, but sometimes it seem, that you yourself do not using Amarok, only develops it :)
Comment 6 Myriam Schweingruber 2010-07-02 13:19:12 UTC
Well, your assumption is completely wrong, since Amarok 2.3.1-git runs all day long on this computer, and so it does for all the developers who can do so in their day job.
And we are well aware that there are errors, but there are only 24 hours in a day and our developers have only two hands. If you know better, you are very welcome to give a hand instead of criticizing a small team trying to do their best in the little time they have. I don't know if you are aware that Amarok developers have real life jobs and can't afford to live of water and Amarok alone.
Comment 7 Andrew Meakovski 2010-07-03 00:03:31 UTC
Created attachment 48549 [details]
Screenshot 1 (grouping by artist)

grouping is «by artist» and it works as it should. (genre labels are displayed on the right)
Comment 8 Andrew Meakovski 2010-07-03 00:04:31 UTC
Created attachment 48550 [details]
Screenshot 2 (grouping by genre)

grouping is «by genre», filelist is the same as on Screenshot1, but groups are builded totally wrong (and weird).
Comment 9 Andrew Meakovski 2010-07-03 00:06:28 UTC
Confirming this (or similar) bug. Showed up after recent update (2.3.1).

Grouping files in playlist by genre is now completely broken.
Added two attachments with same filelist, but different grouping. «by artist» works correct, but «by genre» is acting wrong.
Comment 10 Nikolaj Hald Nielsen 2010-07-07 10:54:50 UTC
Andrew, I am finding it hard to see the issue from your screenshots since they are not showing the actual grouping settings

Could you please make a new screenshot of the issue where a) it also shows the grouping settings (the breadcrumb bar above the playlist) and b) if possible, uses a single line playlist layout showing track-album-artist-genre
Comment 11 Andrew Meakovski 2010-07-07 16:49:41 UTC
Created attachment 48659 [details]
triple-screenshot to describe grouping issue

Nikolaj:

Wow, didn't see those breadcrumbs. But actually that's not grouping, that's sorting. and it works.

I've made three screenshots with proposed layout and different grouping settings. (also screenshoted right behaviour in 2.3.0) and merged them into single image with comments.
Comment 12 Nikolaj Hald Nielsen 2010-07-09 12:33:28 UTC
commit 4102c91119c42a9ed5eb976769a313de29802515
Author: Nikolaj Hald Nielsen <nhn@kde.org>
Date:   Fri Jul 9 12:29:52 2010 +0200

    Fix inconsistency between list of groupable fields and logig causing "genre" and other groupings not to work.
    
    This code is still very fragile though and we should find a better solution
    
    BUG: 243344

diff --git a/src/playlist/proxymodels/GroupingProxy.cpp b/src/playlist/proxymodels/GroupingProxy.cpp
index 1eef9ca..9bfb930 100644
--- a/src/playlist/proxymodels/GroupingProxy.cpp
+++ b/src/playlist/proxymodels/GroupingProxy.cpp
@@ -288,8 +288,11 @@ Playlist::GroupingProxy::shouldBeGrouped( Meta::TrackPtr track1, Meta::TrackPtr
     // If the grouping category is empty or invalid, 'm_groupingCategoryIndex' will be -1.
     // That will cause us to choose "no grouping".
 
+    DEBUG_BLOCK
+            debug() << m_groupingCategoryIndex;
     switch( m_groupingCategoryIndex )
     {
+
         case 0: //Album
             if( track1 && track1->album() && track2 && track2->album() )
                 return ( *track1->album().data() ) == ( *track2->album().data() ) && ( track1->discNumber() == track2->discNumber() );
@@ -299,13 +302,19 @@ Playlist::GroupingProxy::shouldBeGrouped( Meta::TrackPtr track1, Meta::TrackPtr
         case 2: //Composer
             if( track1 && track1->composer() && track2 && track2->composer() )
                 return ( *track1->composer().data() ) == ( *track2->composer().data() );
-        case 3: //Genre
+        case 3: //Directory
+            return false;  //FIXME
+        case 4: //Genre
             if( track1 && track1->genre() && track2 && track2->genre() )
+            {
+                debug() << "gruping by genre. Comparing " << track1->genre()->prettyName() << " with " << track2->genre()->prettyName();
+                debug() << track1->genre().data() << " == " << track2->genre().data() << " : " << ( *track1->genre().data() == *track2->genre().data());
                 return ( *track1->genre().data() ) == ( *track2->genre().data() );
-        case 4: //Rating
+            }
+        case 5: //Rating
             if( track1 && track1->rating() && track2 && track2->rating() )
                 return ( track1->rating() ) == ( track2->rating() );
-        case 5: //Source
+        case 6: //Source
             if( track1 && track2 )
             {
                 QString source1, source2;
@@ -337,7 +346,7 @@ Playlist::GroupingProxy::shouldBeGrouped( Meta::TrackPtr track1, Meta::TrackPtr
             }
             else
                 return false;
-        case 6: //Year
+        case 7: //Year
             if( track1 && track1->year() && track2 && track2->year() )
                 return ( *track1->year().data() ) == ( *track2->year().data() );
         default:
Comment 13 Alexander 2010-07-09 14:44:26 UTC
Created attachment 48715 [details]
Fourth screenshot.

Just now I've rebuilt Amarok from git and would like to inform that this bug isn't fixed. You can see it on the screenshot. I took first two track, copied them to the desktop, set for the same album names (nonexistence album) and re-add them to the playlist. As result I get two tracks from different artists under the general heading.
Comment 14 Alexander 2010-07-09 22:40:28 UTC
(In reply to comment #6)
Most all of the open source community exists on the same principle, is nothing new there. But Amarok is one application I know, which contains so many bugs, too many, man. This even not bugs, this is some stupid flaws and blunders, because all done in hurriedly and without proper testing. And then it becomes very difficult to catch bugs. All of this characterizes Amarok as a very unstable product.

Always, people will perceive calmer a delay of three-four months, instead of many new bugs in the fresh (but quick) release.

For all that, Amarok 2+ is still head and shoulders above the previous version. See, do not bury it.

Personally, I wish you luck and success.
Comment 15 Myriam Schweingruber 2010-07-10 00:26:22 UTC
Reopened based on Comment #13. Alexander, instead of making unnecessary denigrating comments, you are welcome to give a hand, our developers have only 2 hands and the day only has 24 hours...
Comment 16 Myriam Schweingruber 2010-07-10 00:34:15 UTC
Reopening correctly.
Comment 17 Alexander 2010-07-10 08:11:07 UTC
> you are welcome to give a hand
I do not know C++ to help the project. Let me know if I can help somehow else. At least, I can post bug reports. It also requires some time consuming for me, because english isn't my native language, you maybe already noticed it :).
Comment 18 Myriam Schweingruber 2011-05-07 11:31:02 UTC
Could you please upgrade to a newer Amarok version and test again? Current is Amarok 2.4.0, Amarok 2.4.1 is to be released tomorrow.

Please report back.
Comment 19 Alexander 2011-05-08 10:20:59 UTC
I've built amarok-git right now. What new I've seen — many warnings about virtual functions and non-virtual destructors. How I know it is fraught with memory leaks. Very sad that you guys do not know about this.

The second innovation is that drag and drop from my collection into the playlist does not work. Not a problem! I still can open the RMB menu and click on Add to Playlist. But one moment please, why it starts to play when I clicked it? There is add to playlist option but it should be named as add to playlist and play.

Anyway, you can see that two different artists were combined under the single header and title on this header belongs to the first artist. So, this is totally wrong. So, no any changes.

http://www.youtube.com/watch?v=l-QZh_5FUb8
Comment 20 Alexander 2011-05-08 11:23:47 UTC
Also, while I perform today's tests Amarok has crashed about 8-10 times with different error messages.
Comment 21 Myriam Schweingruber 2011-05-10 02:40:03 UTC
Thank you for the feedback.
Comment 22 Romain 2011-11-04 13:25:52 UTC
This bug is silly and has been a pain for months (years ?) ! I'm not a dev but I guess something is simply broken in 

Playlist::GroupingProxy::shouldBeGrouped( Meta::TrackPtr track1, Meta::TrackPtr track2 )

Album grouping is done only by comparing album names and (I guess) even if they are empty. And album grouping is the first-level grouping of the default layout. So, if I understand correctly, for two successive tracks:
* two different artists but empty album name  => grouping ! (BUG)
* two different artists with same album name (think "The Very Best Of") => grouping ! (BUG)

Anyway, here is my patch for it, feel free to use it, discard it, whatever:

-------------------------------------------
   switch( m_groupingCategoryIndex )
    {

        case 0: //Album
            if( track1 && track1->album() && track2 && track2->album() )
            {
                // Don't group empty albums
                if( track1->album()->prettyName().isEmpty() && track2->album()->prettyName().isEmpty() )
                {
                    return false;
                }
                // Don't group different artists
                if( track1 && track1->artist() && track2 && track2->artist() )
                {
                    // WTF !! What kind of joker implements == and not != ? 
                    if(!((*track1->artist().data() ) == (*track2->artist().data()) ))
                    {
                        return false;
                    }
                }
                // Non-empty albums, same artists: group !
                return ( *track1->album().data() ) == ( *track2->album().data() ) && ( track1->discNumber() == track2->discNumber() );
            }
  
-------------------------------------------

I don't know if this is the proper method but I now have what I want.

Regards
Comment 23 Myriam Schweingruber 2011-11-05 12:11:34 UTC
Thank you for the patch. We didn't ignore this report, we simply don't have the manpower to take care for all bugs immediately, and there are more severe ones to care for.
Comment 24 Sven Krohlas 2011-11-05 13:03:29 UTC
Git commit 8dcf7eaad8b23874d67441c2fb8bae3dde215646 by Sven Krohlas.
Committed on 05/11/2011 at 13:59.
Pushed by krohlas into branch 'master'.

Playlist: Don't group albums without name.

I don't agree with the proposed patch in the bug, as in my understanding
this would break grouping for compilations.
Correct me if I'm wrong. ;-)

BUG:243344
FIXED-IN:2.5.0

M  +1    -0    ChangeLog
M  +7    -1    src/playlist/proxymodels/GroupingProxy.cpp

http://commits.kde.org/amarok/8dcf7eaad8b23874d67441c2fb8bae3dde215646
Comment 25 Sven Krohlas 2011-11-05 13:04:34 UTC
Another thing: please create patches with diff.

Anyway, thanks for your work, with such good preparations fixing bugs is a piece of cake. ^^
Comment 26 Romain 2011-11-05 14:28:41 UTC
@Myriam: perhaps you should implement fewer (half-done IMHO) features and have more time to concentrate on existing bugs ? It's a plague with Amarok in particular and KDE in general. I find it hard to recommend either (although I've been a great fan for many years).

@Sven: you don't handle my second bug case, but maybe it's not something that's possible to do at the same time as handling compilations.
Regarding diffs: sorry, copy-paste was just about as far as I was prepared to go to help fix this bug. Maybe next time I'll do it, if I feel that us users aren't ignored.