Use the stored CSubNet entry when unbanning #717
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
This would simplify bitcoin/bitcoin#27071. |
There was a problem hiding this comment.
Concept ACK
Would be better to not access the ban model internals from outside, it breaks encapsulation.
Could do furszy@81b667e
The previous code visualized the `CSubNet` object as string, then parsed that string back to `CSubNet`. This is sub-optimal given that the original `CSubNet` object can be used directly instead. This avoids calling `LookupSubNet()` from the GUI. Co-authored-by: furszy <matiasfurszyfer@protonmail.com>
|
@furszy, done, thanks! |
| @@ -37,7 +37,7 @@ class BannedNodeLessThan | |||
| }; | |||
|
|
|||
| /** | |||
| Qt model providing information about connected peers, similar to the | |||
| Qt model providing information about banned peers, similar to the | |||
| "getpeerinfo" RPC call. Used by the rpc console UI. | |||
There was a problem hiding this comment.
If this comment is fixed anyway (it was copied from peertablemodel.h), could also replace "getpeerinfo" with "listbanned".
| } | ||
| BanTableModel* ban_table_model{clientModel->getBanTableModel()}; | ||
| bool unbanned{false}; | ||
| for (const auto& node_index : nodes) { |
There was a problem hiding this comment.
Not a major issue, but this looks like it's designed to support unbanning multiple nodes with one click, but the GUI doesn't allow me to select multiple entries when I try (unlike in the peertable where banning multiple nodes with one click works).
There was a problem hiding this comment.
Yes, I noticed the same - looks like we don't need a loop here since it is always just one being selected. But it was like this and I left it as is (with a loop) to minimize the amount of unnecessary changes that can have unexpected adverse effects. I do not know why the GUI wouldn't let me select more than one row. Maybe a change elsewhere in the code would allow that and then this code would be broken without a loop.
There was a problem hiding this comment.
to enable it, just need to set the table selection mode to multi selection. e.g. peerTable->setSelectionMode(QAbstractItemView::MultiSelection);.
I didn't mention it because the PR is fine, it is not changing behavior.
There was a problem hiding this comment.
Agree, no need to change it here, I just mentioned it because I found it a bit strange and maybe someone would be interested to change it in a follow-up.
4be57a5 gui: fix comments for BanTableModel and BanTablePriv::refreshBanlist() (Vasil Dimov) a981af4 gui: use the stored CSubNet entry when unbanning (Vasil Dimov) Pull request description: The previous code visualized the `CSubNet` object as string, then parsed that string back to `CSubNet`. This is sub-optimal given that the original `CSubNet` object can be used directly instead. This avoids calling `LookupSubNet()` from the GUI. ACKs for top commit: furszy: utACK 4be57a5 mzumsande: Tested ACK 4be57a5 Tree-SHA512: b783c18c9d676aa9486cff2d27039dd5c5ef3f1cc67e5056a2be68e35930926f368f26dacdf4f3d394a1f73e3e28f42dc8a6936cd1765c6e6e60695c7b4d78af
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
The previous code visualized the
CSubNetobject as string, then parsed that string back toCSubNet. This is sub-optimal given that the originalCSubNetobject can be used directly instead.This avoids calling
LookupSubNet()from the GUI.