refactor: No need to implement index for non-hierarchical models #174

pull hebasto wants to merge 1 commits into bitcoin-core:master from hebasto:210102-index changing 10 files +8 −63

hebasto commented at 8:50 PM on January 2, 2021: member
From Qt docs:
- https://doc.qt.io/qt-5/model-view-programming.html#a-read-only-example-model:
If our model was hierarchical, we would also have to implement the index() and parent() functions.
- https://doc.qt.io/qt-5/qabstracttablemodel.html#subclassing:
Default implementations of the index() and parent() functions are provided by QAbstractTableModel.
hebasto marked this as a draft on Jan 2, 2021
hebasto closed this on Jan 2, 2021
hebasto reopened this on Jan 2, 2021
hebasto marked this as ready for review on Jan 2, 2021
jarolrod commented at 2:27 AM on January 10, 2021: member

ACK 199830ff920825f5306555e9f6fc08be8534e9bb

Tested on macOS Big Sur 11.1 with Qt 5.15.2. Tested that there were no weird side-effects under Transactions and the Peers Window. This is a welcome change as it adheres to the Qt docs and removes unnecessary code.
DrahtBot added the label Needs rebase on Jan 11, 2021
qt, refactor: No need to implement index for non-hierarchical models d64b338bc7
hebasto force-pushed on Jan 11, 2021
hebasto commented at 10:27 AM on January 11, 2021: member

Rebased 199830ff920825f5306555e9f6fc08be8534e9bb -> d64b338bc7ac7f47e4e9921a29d1534f53e9de86 (pr174.01 -> pr174.02) due to the conflict with #161.
DrahtBot removed the label Needs rebase on Jan 11, 2021

in src/qt/addresstablemodel.cpp:199 in d64b338bc7

 195 | @@ -196,7 +196,7 @@ QVariant AddressTableModel::data(const QModelIndex &index, int role) const
 196 |      if(!index.isValid())
 197 |          return QVariant();
 198 |  
 199 | -    AddressTableEntry *rec = static_cast<AddressTableEntry*>(index.internalPointer());
 200 | +    const AddressTableEntry* rec{priv->index(index.row())};

promag commented at 12:04 PM on January 11, 2021:

Need to check if rec != nullptr, index.isValid() doesn't check for out of bounds.

Alternatively you could inline priv->index and use reference instead const AddressTableEntry&. You still need to check for out of bounds.

promag commented at 12:23 PM on January 11, 2021: contributor

I think custom QModelIndex with internal pointers are nice when it saves duplicate long lookups on the internal model. Typically, views makes lots of data(index, role) calls and this approach increases lookups to the underlaying model. Maybe that's not a concern since views only queries data for visible items and we are using containers with O(1) lookups. Still, for addresses and transactions models it would be nice to make sure we don't make user experience worse.
hebasto commented at 1:36 PM on January 11, 2021: member

@promag

Still, for addresses and transactions models it would be nice to make sure we don't make user experience worse.

Right. Blind applying such changes to all models is not warranted.

Closing for now.
hebasto closed this on Jan 11, 2021
bitcoin-core locked this on Aug 16, 2022

Contributors

hebasto

jarolrod

promag