Fix #367 #374

rebroad · 2021-06-28T09:17:42Z

Attempting to fix issue #367

maflcko · 2021-06-28T09:23:08Z

Please explain the change and remove the unscoped GitHub attribute. The unscoped link will mess with all monotree repos, not just this one.

rebroad · 2021-06-28T10:54:49Z

@MarcoFalke what is an "unscoped GitHub attribute" please?

This change just reverts two lines from ecbd911 which seems to fix the issue it introduced. I've not fully tested whether it causes the issues that #164 fixed to be reintroduced - ideally some tests ought to have been created with #164 that would be failing now if it re-broke things.

maflcko · 2021-06-28T11:00:47Z

#367 is unscoped. At the very least it should say bitcoin-core/gui#374.

rebroad · 2021-06-28T11:07:20Z

#367 is unscoped. At the very least it should say bitcoin-core/gui#374.

A pull/issue always defaults to the same project that it's in - it's only necessary to add the project if it's a different project to that which the pull/issue is in.

rebroad · 2021-06-28T11:28:28Z

How do I change this to a draft pull request? I think there may be a chance of Segmentation fault in this code.

maflcko · 2021-06-28T11:37:51Z

A pull/issue always defaults to the same project that it's in - it's only necessary to add the project if it's a different project to that which the pull/issue is in.

This is a monotree project. Meaning that different project will have the same tree and share the (merge) commits. So without a scope it won't be trivially possible to recover the metadata.

fanquake · 2021-06-28T11:38:34Z

A pull/issue always defaults to the same project that it's in - it's only necessary to add the project if it's a different project to that which the pull/issue is in.

Rather than arguing the point, how about just updating the link. As mentioned, having an unscoped link will mess with our merge process / resultant commit messages.

How do I change this to a draft pull request?

Click the button that says "Convert to draft"

You do realise you could read the GitHub documentation, or use any search engine to also find the answer to that question? Rather than posting another comment here?

I'd suggest you spend some time reading this projects developer docs and contributing guide. Opening a PR with no title, no explanation of the change, a useless commit message, and no commit body is not the standard that is expected here. I'm also almost certain you are aware of that, given you've been around the project for many years now.

hebasto

Tested 0d41edb on Linux Mint 20.1 (Qt 5.12.8) -- works as intended.

The change is correct (UPDATE: see #374 (comment)), and Qt docs clearly states:

For more complex changes to the model's structure, perhaps involving internal reorganization, sorting of data or any other structural change, it is necessary to perform the following sequence:

Emit the layoutAboutToBeChanged() signal

Update internal data which represents the structure of the model.

Update persistent indexes using changePersistentIndexList()

Emit the layoutChanged() signal.

UPDATE 2: see #374 (comment)

Also this signal seems redundant now:

--- a/src/qt/peertablemodel.h
+++ b/src/qt/peertablemodel.h
@@ -73,9 +73,6 @@ public:
 public Q_SLOTS:
     void refresh();
 
-Q_SIGNALS:
-    void changed();
-
 private:
     //! Internal peer data structure.
     QList<CNodeCombinedStats> m_peers_data{};

Hope that aforementioned comments will be addressed as well.

hebasto · 2021-06-28T13:00:05Z

OTOH, this change re-introduces #191 bug.

rebroad · 2021-06-28T13:09:47Z

Rather than arguing the point, how about just updating the link. As mentioned, having an unscoped link will mess with our merge process / resultant commit messages.

I'm trying to understand the issue. I can't fix something when I'm not aware of the problem.

Click the button that says "Convert to draft"

Thank you.

You do realise you could read the GitHub documentation, or use any search engine to also find the answer to that question? Rather than posting another comment here?

If it was that simple, I'd have done it. I always go for the simplest solution possible (the KISS rule).

I'd suggest you spend some time reading this projects developer docs and contributing guide. Opening a PR with no title, no explanation of the change, a useless commit message, and no commit body is not the standard that is expected here. I'm also almost certain you are aware of that, given you've been around the project for many years now.

I thought I'd read those docs, and was not aware that I had deviated from any documented practices. I believed the explanation provided was sufficient. I would appreciate it if you could point me towards the relevant documentation that you're thinking of.

rebroad · 2021-06-28T13:11:00Z

OTOH, this change re-introduces #191 bug.

Out of the two bugs #191 or #367 - I'd rather have former than the latter.

At the moment, this small patch causes bitcoin-qt to segmentation fault, so I'm thinking a complete revert of #164 might be the better solution for now until an improved #164 can be produced.

maflcko · 2021-06-28T13:20:02Z

I'm trying to understand the issue. I can't fix something when I'm not aware of the problem.

There are two issues with the commit message:

The hashtag is missing the repo namespace, so when the commit gets pushed to another repo it can't be resolved properly.
The commit message is lacking any sort of explanation.

You can read our contributing guide to learn on how to create patches: https://github.com/bitcoin-core/gui/blob/master/CONTRIBUTING.md#committing-patches . It also links to a guide on how to write commit messages.

hebasto · 2021-06-28T13:42:28Z

I still don't understand why this does not work:

By default, the model dynamically re-sorts and re-filters data whenever the original model changes.

And we have dynamicSortFilter == true for sure.

rebroad · 2021-06-28T13:46:17Z

The hashtag is missing the repo namespace, so when the commit gets pushed to another repo it can't be resolved properly.

Ah, I understand now - you're implying that it will also get pushed to https://github.com/bitcoin/bitcoin, yes?

The commit message is lacking any sort of explanation.

"Fixes #367" is an explanation. so I would disagree with your statement, but if it gets pushed to another repository then I can understand how it could point people to the wrong information. I'm not really understanding why we have two repositories with duplicate code though. Is there an explanation for this somewhere?

You can read our contributing guide to learn on how to create patches: https://github.com/bitcoin-core/gui/blob/master/CONTRIBUTING.md#committing-patches . It also links to a guide on how to write commit messages.

Thanks. I shall read it now.

hebasto · 2021-06-28T14:42:57Z

My previous comment about using of layoutAboutToBeChanged and layoutChanged signals is not correct because it should be applied to cases when no proxy models are used.

I believe that correct fix of #367 is suggested in #375.

hebasto · 2021-06-28T14:45:45Z

Does 71ab42d actually revert ecbd911?

jarolrod

NACK

This is not the way to fix the mentioned issue. This also reintroduces two bugs that we had fixed (#160 and #191)

jarolrod · 2021-06-28T18:01:12Z

src/qt/peertablemodel.cpp

@@ -154,30 +154,7 @@ void PeerTableModel::refresh()
 new_peers_data.append(stats);
 }

- // Handle peer addition or removal as suggested in Qt Docs. See:


removing this reintroduces #160 as described here: #160 (comment)

986bf78 qt: Emit dataChanged signal to dynamically re-sort Peers table (Hennadii Stepanov) Pull request description: [By default](https://doc.qt.io/qt-5/qsortfilterproxymodel.html#details), the `PeerTableSortProxy` > dynamically re-sorts ... data whenever the original model changes. That is not the case on master (8cdf917) as in ecbd911 (#164) no signals are emitted to notify about model changes. This PR uses a dedicated [`dataChanged`](https://doc.qt.io/qt-5/qabstractitemmodel.html#dataChanged) signal. Fixes #367. An alternative to #374. ACKs for top commit: jarolrod: ACK 986bf78 Tree-SHA512: dcb92c2f9a2c632880429e9528007db426d2ad938c64dfa1f1538c03e4b62620df52ad7daf33b582976c67b472ff76bc0dae707049f4bbbd4941232cee9ce3d4

hebasto · 2021-07-05T21:06:36Z

Closing due to the #375 is merged.

rebroad force-pushed the Fix367 branch from f6a9c02 to 0d41edb Compare June 28, 2021 11:34

hebasto reviewed Jun 28, 2021

View reviewed changes

rebroad marked this pull request as draft June 28, 2021 12:58

hebasto mentioned this pull request Jun 28, 2021

Emit dataChanged signal to dynamically re-sort Peers table #375

Merged

Fix bitcoin-core#367

71ab42d

rebroad force-pushed the Fix367 branch from 0d41edb to 71ab42d Compare June 28, 2021 14:42

hebasto added Bug Something isn't working UI All about "look and feel" labels Jun 28, 2021

jarolrod reviewed Jun 28, 2021

View reviewed changes

hebasto closed this Jul 5, 2021

bitcoin-core locked as resolved and limited conversation to collaborators Aug 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #367 #374

Fix #367 #374

rebroad commented Jun 28, 2021 •

edited

Loading

maflcko commented Jun 28, 2021

rebroad commented Jun 28, 2021 •

edited

Loading

maflcko commented Jun 28, 2021

rebroad commented Jun 28, 2021

rebroad commented Jun 28, 2021

maflcko commented Jun 28, 2021

fanquake commented Jun 28, 2021

hebasto left a comment •

edited

Loading

hebasto commented Jun 28, 2021

rebroad commented Jun 28, 2021

rebroad commented Jun 28, 2021 •

edited

Loading

maflcko commented Jun 28, 2021

hebasto commented Jun 28, 2021

rebroad commented Jun 28, 2021

hebasto commented Jun 28, 2021

hebasto commented Jun 28, 2021

jarolrod left a comment

jarolrod Jun 28, 2021

hebasto commented Jul 5, 2021

Fix #367 #374

Fix #367 #374

Conversation

rebroad commented Jun 28, 2021 • edited Loading

maflcko commented Jun 28, 2021

rebroad commented Jun 28, 2021 • edited Loading

maflcko commented Jun 28, 2021

rebroad commented Jun 28, 2021

rebroad commented Jun 28, 2021

maflcko commented Jun 28, 2021

fanquake commented Jun 28, 2021

hebasto left a comment • edited Loading

Choose a reason for hiding this comment

hebasto commented Jun 28, 2021

rebroad commented Jun 28, 2021

rebroad commented Jun 28, 2021 • edited Loading

maflcko commented Jun 28, 2021

hebasto commented Jun 28, 2021

rebroad commented Jun 28, 2021

hebasto commented Jun 28, 2021

hebasto commented Jun 28, 2021

jarolrod left a comment

Choose a reason for hiding this comment

jarolrod Jun 28, 2021

Choose a reason for hiding this comment

hebasto commented Jul 5, 2021

rebroad commented Jun 28, 2021 •

edited

Loading

rebroad commented Jun 28, 2021 •

edited

Loading

hebasto left a comment •

edited

Loading

rebroad commented Jun 28, 2021 •

edited

Loading