Fix sort arrow in peer table #8959

Currently the arrow points in the wrong direction, i.e. we have an ascending arrow when the output is descending, and vice versa.

Isn't the triangle active button showing what will be shown once clicked?

It should show the current sort order, so Concept ACK.

Yup, comparing other sortable tables shows so.

Hmm, running current master on testnet, Peers is sorted:

1 2 3 ...

The triangle shown is:

 .....
  . .
   .

Clicking on the triangle, the triangle flips. BUT the sorting order does not change. It changes on the subsequent click though.

I think this should be fixed first...

@paveljanik I am not able to reproduce the behaviour you describe on ubuntu. What OS are you using?

Also, it's not necessary to click on the triangle specifically - anywhere in the title of the column should be sufficient.

OS X here, Qt 5.6.0 official binary installer. Only the first click on the "NodeId" column header does not change the sorting order, but flips triangle.

@paveljanik Might be a bug with the OS X Qt implementation - do you see this triangle flipping behaviour (albeit with reversed sorting) without apply this PR?

Tested ACK dd790b0 Triangles are indeed the wrong way around without this patch, and it seems fixed with it. I also noticed the behavior mentioned by @paveljanik and that too seems to be fixed by this patch. (Ubuntu 16.04)

Reverted ACK: different behavior on different platforms, I'm no longer sure.

Confirm @paveljanik test on OSX. Without the patch, the arrow is pointing to the wrong direction before I click on the table header. Once I have clicked one of the table headers, the arrow points to the right direction.

With this PR, it always points to the wrong direction. With this PR: NodeId

6
4
1

Arrow shows

    *
   **
  ***
 *****
*******

(although not curved, excuse my bad ASCIIart skills, :)

Hmm, this is basic UI knowledge that I probably miss. Where should the triangle point to when the list is sorted as

1
2
3

In my understanding, it should be shown as

  *
 ***
*****

Right?

Yes, from how I understand it, the arrow up means "ascending sort", so "1 2 3" or "A B C a b c".

Yes. But with this PR, it shows the opposite direction (on OSX, Qt5.6).

(With patch) Initial: After clicking once: After clicking twice:

hmm... (With patch [commit dd790b09633519c5f0daa6a8e00c78a68b038dac] on OSX, QT5.6) Initial

<img width="116" alt="bildschirmfoto 2016-10-20 um 10 40 25" src="https://cloud.githubusercontent.com/assets/178464/19552805/bd6de352-96b1-11e6-871e-3c7709b95fa4.png">

Click 1

<img width="124" alt="bildschirmfoto 2016-10-20 um 10 40 30" src="https://cloud.githubusercontent.com/assets/178464/19552804/bd6ccec2-96b1-11e6-8a3a-049cd8745be2.png">

Click 2

<img width="121" alt="bildschirmfoto 2016-10-20 um 10 40 33" src="https://cloud.githubusercontent.com/assets/178464/19552806/bd6f827a-96b1-11e6-8dc8-1cb06946933c.png">

I confirm @jonasschnelli 's result here. OS X, Qt 5.6.0.

But with this PR, there is no "first click" resort issue here.

The transaction table sort-behavior is correct at my end

Seems to be an upstream issue. The sort arrow points to the opposite direction on OSX compared against Ubuntu. Maybe search/file an issue upstream?

We got sidetracked by the arrow issue.

The way to check correctness of the change that this patch makes would be to dump the sorted list for order == Qt::DescendingOrder, and for order == Qt::DescendingOrder. If that is correct (either before or after the patch) that's what we should stick with. No matter how the OS handles the arrow :) (The place to compensate for a wrong arrow is not here. This is just a sorting function)

Added some debugging code:

        if (sortColumn >= 0) {
            // sort cacheNodeStats (use stable sort to prevent rows jumping around unnecessarily)
            qStableSort(cachedNodeStats.begin(), cachedNodeStats.end(), NodeLessThan(sortColumn, sortOrder));
            if (sortOrder == Qt::DescendingOrder) {
                printf("==== order=Qt::DescendingOrder\n");
            } else if (sortOrder == Qt::AscendingOrder) {
                printf("==== order=Qt::AscendingOrder\n");
            }
            for (const auto &rec: cachedNodeStats) {
                printf("%i\n", rec.nodeStats.nodeid);
            }
            printf("====\n");
        }

Without this patch:

==== order=Qt::AscendingOrder
1
2
3
4
6
7
8
10
====
==== order=Qt::DescendingOrder
10
8
7
6
4
3
2
1
====

With this patch:

==== order=Qt::AscendingOrder
8
7
6
5
4
3
2
1
====
==== order=Qt::DescendingOrder
1
2
3
4
5
6
7
8
====

So: have to NACK this. The sort order matches the specification without this change. If the arrow is off (I'm not sure about that anymore, this is sure confusing), that will have to be solved another way.

@laanwj Thanks for the testing. How odd that Ubuntu Qt is doing it wrong (assuming it is)... I wonder how this appears on MS Windows. Anyway, I shall close it for now, and attempt to fix in the correct place! Thanks again.

Apparently it's a feature/bug of Gnome itself: https://github.com/qbittorrent/qBittorrent/issues/1300