Skip to content

fix for #191#192

Closed
RandyMcMillan wants to merge 1 commit intobitcoin-core:masterfrom
RandyMcMillan:191
Closed

fix for #191#192
RandyMcMillan wants to merge 1 commit intobitcoin-core:masterfrom
RandyMcMillan:191

Conversation

@RandyMcMillan
Copy link
Copy Markdown
Contributor

@hebasto - here is a barebones fix for #191
We can tweak the presentation in a follow up PR.

@hebasto
Copy link
Copy Markdown
Member

hebasto commented Jan 20, 2021

@RandyMcMillan

Thanks for fixing!

  1. Did you find out the reason of head flickering?
  2. Could using of GUIUtil::TableViewLastColumnResizingFixer be a better option as it is used in other places of the GUI?
  3. Do you mind dropping @ from the PR description?

Copy link
Copy Markdown
Contributor

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

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

This PR does not fix #191

It instead creates a new column in the Peer Table model. A fix to this should not hack on a new column so that the User Agent column does not glitch. Perhaps a refactor that creates a PeerTableModelView which makes use of TableViewLastColumnResizingFixer is more appropriate.

PR: Creates a new column to the right of User Agent
Screen Shot 2021-01-20 at 3 51 23 PM

@RandyMcMillan
Copy link
Copy Markdown
Contributor Author

@jarolrod - It sounds like you have the motivation to implement a proper solution.
I will be excited to see how you refactor this into a proper MVC implementation.
I will be ecstatic to see it actually get merged!
Happy coding!

@jarolrod
Copy link
Copy Markdown
Contributor

@RandyMcMillan was just relaying what I found while reviewing the PR. Wasn't trying to discourage you in anyway or come off as rude.

@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.

3 participants