Inverted sorting is broken

[JakeSFR]

When you open ROX window and keep right-clicking the "Change sort criteria" icon, it rotates only through these 4:

1. Sort by type, descending
2. Sort by name, descending
3. Sort by type, ascending
4. Sort by name, ascending
5. GOTO 1

whereas it should be:

1. Sort by group, descending
2. Sort by owner, descending
3. Sort by permissions, descending
4. Sort by size, descending
5. Sort by date, descending
6. Sort by type, descending
7. Sort by name, descending
8. Sort by group, asscending
9. Sort by owner, asscending
10. Sort by permissions, asscending
11. Sort by size, asscending
12. Sort by date, asscending
13. Sort by type, asscending
14. Sort by name, asscending
15. GOTO 1

Looks like it used to work, but got broken in this commit from 2003, which was apparently an attempt to simplify it: https://github.com/jun7/rox-filer/commit/f6d7ea77cc5a2569cb1a9ecfa9524943e7520333#diff-e6aed3e68ecc5151ca322f12adc2413d1efab0999ed3794c31c76f693ce58e43

I've backported the previous code to the current codebase and it seems to work fine now:

diff -ur rox-filer_old/ROX-Filer/src/toolbar.c rox-filer_new/ROX-Filer/src/toolbar.c
--- rox-filer_old/ROX-Filer/src/toolbar.c   2021-09-19 22:30:35.000000000 +0200
+++ rox-filer_new/ROX-Filer/src/toolbar.c   2021-09-22 20:59:03.000000000 +0200
@@ -522,7 +522,7 @@
                    FilerWindow *filer_window)
 {
    gint eb = get_release();
-   int i, current, next, next_wrapped;
+   int i, current, next;
    gboolean adjust;
    GtkSortType dir;
    gchar *tip;
@@ -531,6 +531,9 @@
        SORT_NAME, SORT_TYPE, SORT_DATEC, SORT_SIZE,
        SORT_PERM, SORT_OWNER, SORT_GROUP,
    };
+
+   static const int nsorts = G_N_ELEMENTS(sorts);
+
    static const char *sort_names[] = {
        N_("Sort by name"), N_("Sort by type"), N_("Sort by date"), N_("Sort by size"),
        N_("Sort by permissions"), N_("Sort by owner"), N_("Sort by group"),
@@ -540,7 +543,7 @@

    current = -1;
    dir = filer_window->sort_order;
-   for (i=0; i < G_N_ELEMENTS(sort_names); i++)
+   for (i=0; i < nsorts; i++)
    {
        if (filer_window->sort_type == sorts[i])
        {
@@ -549,21 +552,29 @@
        }
    }

-   if (current == -1)
+   if (current == -1 || eb == 2) {
        next = 0;
-   else if (adjust)
+       dir = GTK_SORT_ASCENDING;
+   }
+   else if (adjust) {
        next = current - 1;
-   else
+       if (next < 0) {
+           next = nsorts - 1;
+           dir = (dir == GTK_SORT_ASCENDING)
+               ? GTK_SORT_DESCENDING : GTK_SORT_ASCENDING;
+       }
+   }
+   else {
        next = current + 1;
+       if (next >= nsorts) {
+           next = 0;
+           dir = (dir == GTK_SORT_ASCENDING)
+               ? GTK_SORT_DESCENDING : GTK_SORT_ASCENDING;
+       }
+   }

-   next_wrapped = next % G_N_ELEMENTS(sorts);
-
-   if (next_wrapped != next)
-       dir = (dir == GTK_SORT_ASCENDING)
-           ? GTK_SORT_DESCENDING : GTK_SORT_ASCENDING;
-
-   display_set_sort_type(filer_window, sorts[next_wrapped], dir);
-   tip = g_strconcat(_(sort_names[next_wrapped]), ", ",
+   display_set_sort_type(filer_window, sorts[next], dir);
+   tip = g_strconcat(_(sort_names[next]), ", ",
            dir == GTK_SORT_ASCENDING
                ? _("ascending") : _("descending"),
            NULL);

I also added a middle-click (which is currently an alias for right-click) action to instantly return to the default view, which is very useful when one is in the middle of some strange sort criteria. That was actually my original intention, until I noticed that sorting doesn't work the way it should.

If it looks ok to you, I'll make a PR.

[jun7]

Hmm not reproduced. The tooltip text is fine.

for (i=0; i < G_N_ELEMENTS(sort_names); i++) -> for (i=0; i < G_N_ELEMENTS(sorts); i++) works?

if (current == -1 || eb == 2) {

very smart!

[jun7]

I think SORT_DATEC I changed broked it.

[JakeSFR]

Hmm not reproduced. The tooltip text is fine.

for (i=0; i < G_N_ELEMENTS(sort_names); i++) -> for (i=0; i < G_N_ELEMENTS(sorts); i++) works?

No, makes no difference, it's the same number of elements, anyway.

Ok, maybe you left-clicked a few times, before right-clicking and your starting position was not 'by name, ascending'? In such a case you need to keep right-clicking until the "loop of 4" kicks in.

Adding this at the end of toolbar_sort_clicked():

printf("> %s: %d, %s\n",
    eb == 1 ? "Left-click" : "Right-Click",
    next_wrapped,
    dir == GTK_SORT_ASCENDING ? _("ascending") : _("descending"));

opening a ROX window, left-clicking a few times and then right-clicking, should give you this:

Left-click: 1, ascending Left-click: 2, ascending Left-click: 3, ascending Left-click: 4, ascending Left-click: 5, ascending Left-click: 6, ascending Right-Click: 5, ascending Right-Click: 4, ascending Right-Click: 3, ascending Right-Click: 2, ascending Right-Click: 1, ascending Right-Click: 0, ascending Right-Click: 1, descending Right-Click: 0, descending Right-Click: 1, ascending Right-Click: 0, ascending Right-Click: 1, descending Right-Click: 0, descending Right-Click: 1, ascending

and so on...

if (current == -1 || eb == 2) {

very smart!

Thanks!

I think SORT_DATEC I changed broked it.

I don't think so. The problem seems to be:

next_wrapped = next % G_N_ELEMENTS(sorts);

It will wrap from 7 to 0 when you left-clicking (next = current + 1), but won't wrap from -1 to 6, when you right-clicking (next = current - 1).

[jun7]

Ah sorry I get it. I will pull when PR is made.

BTW can I invite you as a collaborator? these process are not bad but if you have minor fixes, being a collaborator is good I think.

[JakeSFR]

Ah sorry I get it. I will pull when PR is made.

No worries, done: https://github.com/jun7/rox-filer/pull/226

BTW can I invite you as a collaborator? these process are not bad but if you have minor fixes, being a collaborator is good I think.

Sure, why not. For small time commits like typos it would definitely be convenient. Thank you! :)

[jun7]

Thank you for reporting!