Keep sub-sorting order #15074
Keep sub-sorting order #15074
Conversation
src/gui/transferlistsortmodel.cpp
Outdated
| return compare(left.sibling(left.row(), m_subSortColumn), right.sibling(right.row(), m_subSortColumn)) < 0; | ||
| if (result == 0) { | ||
| const int subResult = compare(left.sibling(left.row(), m_subSortColumn), right.sibling(right.row(), m_subSortColumn)); | ||
| return m_subSortColumnOrder == m_lastSortColumnOrder ? subResult < 0 : subResult > 0; |
There was a problem hiding this comment.
Why do you compare m_subSortColumnOrder with m_lastSortColumnOrder? It's not clear. At least you should add a code comment.
There was a problem hiding this comment.
Hm... Actually this is wrong. Looks like I've outsmarted myself. Fixed, comment added
There was a problem hiding this comment.
@glassez
Well, I was wrong last time but right first time. Qt reverses lessThan() as a whole so sorting descending means that sub-
sorting has to be reversed. And if sub-sorting is descending the sub-result must be inversed too. That means if both ordered descending the sub-result is double-inversed (which is the same as no inversion). Obviously, if both ordered Ascending this also means that no inversion should happen. And to check that exactly one is descending is the same as checking that both are not equal.
Fixed, comment added.
d-s-x
force-pushed
the
keep-subsort-order
branch
from
fb8e5be
to
2446c8a
Compare
src/gui/transferlistsortmodel.h
Outdated
| mutable int m_lastSortColumn = -1; | ||
| mutable int m_lastSortColumnOrder = 0; |
There was a problem hiding this comment.
It looks wasteful to use 4 bytes to store very few items. I would suggest using signed char (to differentiate with the char type which is usually for characters).
This applies to CachedSettingValue<> too.
There was a problem hiding this comment.
I think it's too much... This is not a memory-critical place.
There was a problem hiding this comment.
I can use single int8_t to encode both column index and sort order at the same time so every user will waste 7 bytes less at the expense of some runtime overhead and inability to express more than 126 columns. Sort order will be encoded using sign bit and index itself will be stored with +1 offset, 0 will mean "unset" which also allows to save on code size by replacing if (m_lastSortColumn != -1) with much shorter if (m_lastSortColumn). If we agree to never have more than 62 columns we may even spare 1 bit for future use.
There was a problem hiding this comment.
I think it's too much...
It is only a smaller type isn't? Or we can use the alias qint8.
This is not a thing that I very insist so I still gave my approval.
I can use single int8_t to encode both column index and sort order at the same time so every user will waste 7 bytes less at the expense of some runtime overhead and inability to express more than 126 columns.
That is a way to encode it but it is overly complex for our simple usage here. For now let's stick to simple code.
d-s-x
force-pushed
the
keep-subsort-order
branch
from
2446c8a
to
b8e07a5
Compare
d-s-x
force-pushed
the
keep-subsort-order
branch
from
b8e07a5
to
656fbbb
Compare
d-s-x
force-pushed
the
keep-subsort-order
branch
from
656fbbb
to
3d04641
Compare
|
@qbittorrent/frequent-contributors |
src/gui/transferlistsortmodel.h
Outdated
| mutable int m_lastSortColumn = -1; | ||
| mutable int m_lastSortOrder = 0; |
There was a problem hiding this comment.
TransferListSortModel::sort is not const, it should work without marking these variables mutable.
d-s-x
force-pushed
the
keep-subsort-order
branch
from
3d04641
to
de1e64b
Compare
|
@d-s-x |
Fixes #15073
Sub-sorting now works like in most grids where sub-sorting is a thing (except there is still no indication in UI).
Sub-sorting column tracked from
sort()method that is called exactly once per view sort unlikelessThan()so it is a bit cleaner now too.This complements earlier work done in #14423