Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Jan 2, 2021

From Qt docs:

If our model was hierarchical, we would also have to implement the index() and parent() functions.

Default implementations of the index() and parent() functions are provided by QAbstractTableModel.

@hebasto hebasto marked this pull request as draft January 2, 2021 20:56
@hebasto hebasto closed this Jan 2, 2021
@hebasto hebasto reopened this Jan 2, 2021
@hebasto hebasto marked this pull request as ready for review January 2, 2021 21:04
@jarolrod
Copy link
Member

ACK 199830f

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.

@hebasto
Copy link
Member Author

hebasto commented Jan 11, 2021

Rebased 199830f -> d64b338 (pr174.01 -> pr174.02) due to the conflict with #161.

Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -196,7 +196,7 @@ QVariant AddressTableModel::data(const QModelIndex &index, int role) const
if(!index.isValid())
return QVariant();

AddressTableEntry *rec = static_cast<AddressTableEntry*>(index.internalPointer());
const AddressTableEntry* rec{priv->index(index.row())};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@hebasto
Copy link
Member Author

hebasto commented Jan 11, 2021

@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 hebasto closed this Jan 11, 2021
@bitcoin-core bitcoin-core locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants