Fix #367

rebroad commented at 9:17 am on June 28, 2021: contributor

Attempting to fix issue #367

MarcoFalke commented at 9:23 am on June 28, 2021: contributor

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

rebroad commented at 10:54 am on June 28, 2021: contributor

@MarcoFalke what is an “unscoped GitHub attribute” please?

This change just reverts two lines from https://github.com/bitcoin-core/gui/commit/ecbd91153875c8cdd5b92b840afc116f65e457fb 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.

MarcoFalke commented at 11:00 am on June 28, 2021: contributor

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

rebroad commented at 11:07 am on June 28, 2021: contributor

#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 commented at 11:28 am on June 28, 2021: contributor

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

rebroad force-pushed on Jun 28, 2021

MarcoFalke commented at 11:37 am on June 28, 2021: contributor

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 commented at 11:38 am on June 28, 2021: member

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”

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 commented at 12:53 pm on June 28, 2021: member

Tested 0d41edb8b3ce6923821985a633b49c3be002a05b 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:

 0--- a/src/qt/peertablemodel.h
 1+++ b/src/qt/peertablemodel.h
 2@@ -73,9 +73,6 @@ public:
 3 public Q_SLOTS:
 4     void refresh();
 5 
 6-Q_SIGNALS:
 7-    void changed();
 8-
 9 private:
10     //! Internal peer data structure.
11     QList<CNodeCombinedStats> m_peers_data{};

Hope that aforementioned comments will be addressed as well.

rebroad marked this as a draft on Jun 28, 2021

hebasto commented at 1:00 pm on June 28, 2021: member

OTOH, this change re-introduces #191 bug.

rebroad commented at 1:09 pm on June 28, 2021: contributor

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 commented at 1:11 pm on June 28, 2021: contributor

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.

MarcoFalke commented at 1:20 pm on June 28, 2021: contributor

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 commented at 1:42 pm on June 28, 2021: member

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 commented at 1:46 pm on June 28, 2021: contributor

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.

Fix #367 71ab42d00e

rebroad force-pushed on Jun 28, 2021

hebasto commented at 2:42 pm on June 28, 2021: member

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 commented at 2:45 pm on June 28, 2021: member

Does 71ab42d00e12c3bbdf3ca588da6408ec209a10d8 actually revert ecbd91153875c8cdd5b92b840afc116f65e457fb?

hebasto added the label Bug on Jun 28, 2021

hebasto added the label UI on Jun 28, 2021

in src/qt/peertablemodel.cpp:157 in 71ab42d00e

153@@ -154,30 +154,7 @@ void PeerTableModel::refresh()
154         new_peers_data.append(stats);
155     }
156 
157-    // Handle peer addition or removal as suggested in Qt Docs. See:

jarolrod commented at 6:01 pm on June 28, 2021:

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

jarolrod commented at 6:04 pm on June 28, 2021: member

NACK

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

hebasto referenced this in commit a62fc35a15 on Jul 5, 2021

hebasto commented at 9:06 pm on July 5, 2021: member

Closing due to the #375 is merged.

hebasto closed this on Jul 5, 2021

bitcoin-core locked this on Aug 16, 2022

Fix #367 #374