Bug 331597

Summary: Tags are not sorted in UI [patch]
Product: [Applications] digikam Reporter: Michal Sylwester <msylwester>
Component: Tags-EngineAssignee: Digikam Developers <digikam-bugs-null>
Status: RESOLVED FIXED    
Severity: normal CC: anna, caulier.gilles, kaefert, mohammed.ahmed.anwer
Priority: NOR    
Version: 4.0.0   
Target Milestone: ---   
Platform: unspecified   
OS: Linux   
Latest Commit: Version Fixed In: 4.0.0
Attachments: Collection for reproduction
Reverses part of commit c853f51357b043126a23626264bd86b4455ab085
Updated patch

Description Michal Sylwester 2014-02-28 05:32:46 UTC
All tags lists I've checked (left hand panel, right hand on/off panel and filters) show tags in apparently random order. It seems however to be constant: all places show same order, it seems to be persistent between restarts.

Expected: Sorted alphabetically.
Comment 1 Michal Sylwester 2014-03-04 05:04:01 UTC
I tried to pinpoint this better, but just got more confused...
I have some problems with my collection that caused some tags to have garbled characters. I suspect this may be causing the sorting to fail altogether, but I couldn't reproduce this manually. Still if this is the case then it's not digikam bug.

What I know:
1. Can't reproduce this on a test collection, tags stay sorted whatever I try
2. In my "normal" collection tags are not sorted, but in different order on different PCs.
3. Clicking the header actually reverses the order (but does not sort)

If nobody else experiences this then I think it can be blamed on my borked tags. I will try to clean up and see if it helps.
Comment 2 Michal Sylwester 2014-03-06 01:29:47 UTC
Tried to narrow this down..

Point 3 above was wrong: the order changes when I reverse the sort order. It seems the tags are somewhat sorted, but not completely.

I managed to create a collection of just 1 picture (with a load of tags) and 3 empty subalbums. 
- When digikam is started with empty db and the collections root is added tags are not sorted correctly. 
- Removing any sub-album sorts them.
- After adding the album back tags stay sorted
- Restarting - stay sorted
- Stop, rm *.db, restart, re-add collection - unsorted again
Comment 3 Michal Sylwester 2014-03-06 01:36:40 UTC
Created attachment 85442 [details]
Collection for reproduction

I can reproduce this with:
cd /tmp
tar xvf test.tar.bz2
digikam --database-directory test2     # db doesn't matter, just to not mess with the normal collection
add the /tmp/test2 as new local collection
"Z" and "Zi" tags tend to get sorted before "People". Changing the sort order few times makes it easier to notice.
Comment 4 Anna Timm 2014-03-07 23:08:35 UTC
I observed the same behavior. Additional Info:
Clean install of Manjaro Linux with digikam 4.0.0beta3 from the AUR. Fresh SQLight database. But the images I scanned had old tags and metadata from Windows digikam 3.4 (and Picasa and Windows Photo Gallery...).
Comment 5 Thomas Käfer 2014-04-02 20:42:34 UTC
I am running digikam 4.0.0 beta4 and I see this random order of tags both in the tag lists and also in the calendar which shows me this order of years: 1980, 2009, 2006, 2010, 2011, 2007, 2008, ...
Comment 6 Michal Sylwester 2014-05-02 01:58:42 UTC
I think I found the root cause. Between beta2 and beta3 there was some code which seems to be preparation to add album sorting by date (which seems to have not been finished). It always assumed that it's sorting albums even though it was in a class which was also used for tag models. 
As it looked for an album based on it's ID it was possible (especially for small collections) that there will be no album/tag id clash (id not matching any album would cause it to default to default sort order), and it worked fine. With larger collection it would make a walkaround: take tag ids, find matching albums, and sort according to album names...

As it seems that the date-based sort is not used anywhere, I made a patch that reverses these changes, seems to work fine for me (also fixes the years, I missed it before)
Comment 7 Michal Sylwester 2014-05-02 02:00:40 UTC
Created attachment 86395 [details]
Reverses part of commit c853f51357b043126a23626264bd86b4455ab085

Proposed patch
Comment 8 Michal Sylwester 2014-05-02 06:17:31 UTC
Ups, I found the code that triggers sorting albums by date, which I broke with the patch above.  So a better solution would be needed to make both work...
Comment 9 caulier.gilles 2014-05-02 07:34:39 UTC
Mohamed,

Can you take  a look to the patch attached to this file which is related to your album sort improvements done few month ago ?

Also, please look in file #333385 reported recently where i suspect a side effect of your album sort improvements in Calendar view.

Thanks in advance

Gilles Caulier
Comment 10 Michal Sylwester 2014-05-02 07:42:26 UTC
Just a idea, I won't have time to check it over next few days. I think it would be better to leave the lessThan as it is, and override sortRoleData in AlbumModel to return either date or title (it seems to be there exactly in order to provide different key for sorting depending on needs). 
It seems to me to  better leave AlbumFilterModel with just generic code, and add the specific way of sorting albums into a more physical album specific class.
Comment 11 Mohamed 2014-05-03 17:46:06 UTC
Hi guys,

I was working on the sorting albums in tree-view on (small collections), so I didn't notice the problem happened to the Tag tree-view and Calender tree-view.

The problem in the calender tree-view was solved yesterday, You can check the current implementation on the master and test the Calender View.

I promise to solve the problem of Tags tree-view as soon as possible.

Best
Comment 12 Michal Sylwester 2014-05-07 04:23:23 UTC
Created attachment 86503 [details]
Updated patch

I tried the solution I mentioned, here is updated patch. I couldn't test the date sorting fully though as my album's dates are a mess...

Note, I've seen some checks of the result of static_cast for nullness, but I believe this is not necessary. I believe static_cast (unlike dynamic_cast) will always return valid pointer, and if wrong album type ends here, then there is something seriously wrong...
Comment 13 Mohamed 2014-05-07 17:24:15 UTC
Hi,

Mr Michal Sylwester, I just pushed the bug fix and it works fine for me, please test it against your big collection.

Best
Comment 14 caulier.gilles 2014-05-07 17:26:25 UTC
Git commit a4320cbf03ce1f2df89649739db6c641d259adcc by Mohamed Anwer.
Committed on 07/05/2014 at 17:08.
Pushed by mohamedanwer into branch 'master'.

Fixing Bug #331597

M  +33   -15   libs/models/albumfiltermodel.cpp

http://commits.kde.org/digikam/a4320cbf03ce1f2df89649739db6c641d259adcc

diff --git a/libs/models/albumfiltermodel.cpp b/libs/models/albumfiltermodel.cpp
index 66dbfbf..d095a90 100644
--- a/libs/models/albumfiltermodel.cpp
+++ b/libs/models/albumfiltermodel.cpp
@@ -336,34 +336,52 @@ bool AlbumFilterModel::filterAcceptsRow(int source_row, const QModelIndex& sourc

 bool AlbumFilterModel::lessThan(const QModelIndex& left, const QModelIndex& right) const
 {
-    PAlbum* leftAlbum = AlbumManager::instance()->findPAlbum(albumForIndex(left)->id());
-    PAlbum* rightAlbum = AlbumManager::instance()->findPAlbum(albumForIndex(right)->id());
+    Album* leftAlbum = albumForIndex(left);
+    Album* rightAlbum = albumForIndex(right);
+    QVariant valLeft  = left.data(sortRole());
+    QVariant valRight = right.data(sortRole());
+
     AlbumSettings::AlbumSortOrder sortRole = AlbumSettings::instance()->getAlbumSortOrder();

     if (leftAlbum && rightAlbum)
     {
-        switch (sortRole)
+        if(leftAlbum->type() == 0 && rightAlbum->type()== 0)//checking for PAlbums
+        {
+            switch (sortRole)
+            {
+                case AlbumSettings::ByFolder:
+                    switch (AlbumSettings::instance()->getStringComparisonType())
+                    {
+                        case AlbumSettings::Natural:
+                            return KStringHandler::naturalCompare(leftAlbum->title(), rightAlbum->title(), sortCaseSensitivity()) < 0;
+
+                        case AlbumSettings::Normal:
+                        default:
+                            return QString::compare(leftAlbum->title(), rightAlbum->title(), sortCaseSensitivity()) < 0;
+                    }
+                case AlbumSettings::ByDate:
+                    return compareByOrder(static_cast<PAlbum*>(leftAlbum)->date(),static_cast<PAlbum*>(rightAlbum)->date(),Qt::AscendingOrder) < 0;
+                default:
+                    return KStringHandler::naturalCompare(static_cast<PAlbum*>(leftAlbum)->category(),static_cast<PAlbum*>(rightAlbum)->category()) < 0;
+            }
+        }
+        else
         {
-            case AlbumSettings::ByFolder:
+            if((valLeft.type() == QVariant::String) && (valRight.type() == QVariant::String))
+            {
                 switch (AlbumSettings::instance()->getStringComparisonType())
                 {
                     case AlbumSettings::Natural:
-                        return KStringHandler::naturalCompare(leftAlbum->title(), rightAlbum->title(), sortCaseSensitivity()) < 0;
-
+                        return KStringHandler::naturalCompare(valLeft.toString(), valRight.toString(), sortCaseSensitivity()) < 0;
                     case AlbumSettings::Normal:
                     default:
-                        return QString::compare(leftAlbum->title(), rightAlbum->title(), sortCaseSensitivity()) < 0;
+                        return QString::compare(valLeft.toString(), valRight.toString(), sortCaseSensitivity()) < 0;
                 }
-            case AlbumSettings::ByDate:
-                return compareByOrder(leftAlbum->date(),rightAlbum->date(),Qt::AscendingOrder) < 0;
-            default:
-                return KStringHandler::naturalCompare(leftAlbum->category(),rightAlbum->category()) < 0;
+            }
         }
     }
-    else
-    {
-        return QSortFilterProxyModel::lessThan(left, right);
-    }
+
+    return QSortFilterProxyModel::lessThan(left, right);
 }

 void AlbumFilterModel::slotAlbumRenamed(Album* album)
Comment 15 caulier.gilles 2014-05-07 17:27:47 UTC
Mohamed,

Please "CCBUGS: bugid" as macro in your commit comment to CC bugzilla automatically.

Look here for details :

https://community.kde.org/Sysadmin/GitKdeOrgManual#Commit_hook_keywords

Gilles Caulier
Comment 16 Mohamed 2014-05-07 17:41:05 UTC
OK, I will do it in future commits.
Comment 17 Michal Sylwester 2014-05-07 22:49:19 UTC
Seems to work better, except sort by category seems to be broken. Not sure whether it worked before or not...

Still, few comments:
- I noticed however another problem: many of my albums have default date, which happens to be the same. These are sorted randomly, and whenever I expand/collapse an album branch the sort order changes... Would be nice to have something like "use date first, if not sufficient than use also name"
- The album metadata is only in db. As I sync all metadata to images (in order to sync the collection between programs/computers) album metadata is not synced... Perhaps something to think about in future (album metadata sidecar like? or is this already saved somewhere I missed?) 

- I'm not sure why you insist on putting the specific logic of this sorting into the generic lessThan instead of using the existing sortRole system. This forces you to use duplicate code (lessThan is now almost 2 copies of same code, for PAlbum and other), specialized code in generic class, magic numbers (==0), hide the sortOrder method by variable, and I suspect this makes a bunch of the old sortOrder code a dead code. But, well, it works, so perhaps I shouldn't complain too much :)
Comment 18 Mohamed 2014-05-08 01:15:54 UTC
I'm not insisting :) ,, this is a temporary solution, I'm using this code to prevent sorting roles like "by category" and "by date" to affect the sorting of "Tags tree view" which should only be sorted by name.

I duplicated the switch code to differentiate between the sorting of "Physical album (PAlbum)" and other album types "TAlbum, DAlbum,...".

And don't worry albumfiltermodel will get generic again soon.
Comment 19 Michal Sylwester 2014-05-08 01:22:59 UTC
Ok, thats fine with me :)

I was just afraid that its getting away from the model suggested by qt, making things confusing. Or at least it got quite confusing for me when I was trying to debug this problem, and the sort role was never requested for albums the way I expected it to happen according to qt docs. It suggested that the specialized code should be where the value for sort order is returned, so that's what I was looking for, and that's what I tried to do with my patch. Oh, I'm getting boring again...

Anyway, I'm glad this will make it into the 4.0 :)
Comment 20 Mohamed 2014-05-08 01:34:51 UTC
That discussion was useful and active, and not boring at all.
Thanks for reporting the issue to the bugzilla.
Comment 21 caulier.gilles 2014-05-08 05:27:28 UTC
Git commit fa2ae1d7254c3b08e1a94d840aac06623722752e by Mohamed Anwer.
Committed on 08/05/2014 at 03:16.
Pushed by mohamedanwer into branch 'master'.

Removing magic number and polish

M  +4    -3    libs/models/albumfiltermodel.cpp

http://commits.kde.org/digikam/fa2ae1d7254c3b08e1a94d840aac06623722752e

diff --git a/libs/models/albumfiltermodel.cpp b/libs/models/albumfiltermodel.cpp
index d095a90..42b43d7 100644
--- a/libs/models/albumfiltermodel.cpp
+++ b/libs/models/albumfiltermodel.cpp
@@ -342,15 +342,16 @@ bool AlbumFilterModel::lessThan(const QModelIndex& left, const QModelIndex& righ
     QVariant valRight = right.data(sortRole());

     AlbumSettings::AlbumSortOrder sortRole = AlbumSettings::instance()->getAlbumSortOrder();
+    AlbumSettings::StringComparisonType strComparisonType = AlbumSettings::instance()->getStringComparisonType();

     if (leftAlbum && rightAlbum)
     {
-        if(leftAlbum->type() == 0 && rightAlbum->type()== 0)//checking for PAlbums
+        if(leftAlbum->type() == Album::PHYSICAL && rightAlbum->type()== Album::PHYSICAL)
         {
             switch (sortRole)
             {
                 case AlbumSettings::ByFolder:
-                    switch (AlbumSettings::instance()->getStringComparisonType())
+                    switch (strComparisonType)
                     {
                         case AlbumSettings::Natural:
                             return KStringHandler::naturalCompare(leftAlbum->title(), rightAlbum->title(), sortCaseSensitivity()) < 0;
@@ -369,7 +370,7 @@ bool AlbumFilterModel::lessThan(const QModelIndex& left, const QModelIndex& righ
         {
             if((valLeft.type() == QVariant::String) && (valRight.type() == QVariant::String))
             {
-                switch (AlbumSettings::instance()->getStringComparisonType())
+                switch (strComparisonType)
                 {
                     case AlbumSettings::Natural:
                         return KStringHandler::naturalCompare(valLeft.toString(), valRight.toString(), sortCaseSensitivity()) < 0;
Comment 22 caulier.gilles 2014-05-08 09:00:50 UTC
Mohamed, Michal,

Can be conderate this entry fixed or not for 4.0.0 ?

Gilles Caulier
Comment 23 Michal Sylwester 2014-05-08 09:13:27 UTC
I'd say close this as fixed for 4.0.0 (tags are sorted, and this is what this bug is about), and open a separate bug for fixing the other issues (fix category sort and work out how to sort albums with same date). I guess it can be done together with the other planned work Mohamed mentioned.

Michal
Comment 24 caulier.gilles 2014-05-08 09:58:15 UTC
Thanks Michal,

I close this file. Don't forget to report others issues in new files

Thanks for your feedback

Gilles Caulier
Comment 25 Mohamed 2014-05-08 20:46:51 UTC
Mr Michal,
Would you please mention how to reproduce the problem of sorting "by category", because sorting by category works for me as expected.
Thanks in advance.
Comment 26 Michal Sylwester 2014-05-09 01:31:41 UTC
Hi Mohamed,

Sorry, my mistake, category sort works fine. I think my problem is that updating metadata does not automatically updates the treeview, I have to trigger it manually (like by clicking on the column header). Can you reproduce this?

Michal
Comment 27 Mohamed 2014-05-09 11:40:54 UTC
Yes, I know that.
It can be considered a wish.

Mohamed.