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.

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

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.

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

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

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)

Created attachment 86395 [details]
Reverses part of commit c853f51357b043126a23626264bd86b4455ab085

Proposed patch

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

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

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.

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

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

Hi,

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

Best

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)

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

OK, I will do it in future commits.

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

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.

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

That discussion was useful and active, and not boring at all.
Thanks for reporting the issue to the bugzilla.

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;

Mohamed, Michal,

Can be conderate this entry fixed or not for 4.0.0 ?

Gilles Caulier

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

Thanks Michal,

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

Thanks for your feedback

Gilles Caulier

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.

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

Yes, I know that.
It can be considered a wish.

Mohamed.