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.

Sign up for GitHub

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

Jump to bottom

Add PeerDetails page #388

Merged
merged 3 commits into from
Sep 27, 2024
Merged

Add PeerDetails page #388

merged 3 commits into from
Sep 27, 2024

Conversation

jarolrod
Copy link
Member

@jarolrod jarolrod commented Feb 28, 2024 •
edited
Loading

Alternative to #387 which expands to think about a reusable row component, allowing to load any element(s) to the value field, not hardcoding in a height of 21 for a row (but still ensuring it's a minimum of 21), proper icon usage for N/A cases, dummy net indicators in place

TODO:

  • Ban buttons
  • Hover hints
  • Disconnected state instead of flying back to the peers table
  • Smart text fixups
Screenshot 2024-02-28 at 3 53 43 AM

Link to github actions build artifacts.

Build Artifacts

@jarolrod jarolrod mentioned this pull request Feb 28, 2024
Closed
@jarolrod jarolrod force-pushed the peer-details-alternate branch from b7161c5 to 4909ba5 Compare February 28, 2024 10:04
johnny9
johnny9 reviewed Mar 1, 2024
View reviewed changes
Copy link
Contributor

@johnny9 johnny9 left a comment

Choose a reason for hiding this comment

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

Looks really good. Only thing that stood out to me was the arrow icon was slight too small but that difference makes it harder to see.

src/qml/pages/node/PeerDetails.qml Show resolved Hide resolved
@mouxdesign mouxdesign mentioned this pull request Mar 1, 2024
Open
MarnixCroes
MarnixCroes reviewed Mar 6, 2024
View reviewed changes
Copy link
Contributor

@MarnixCroes MarnixCroes left a comment

Choose a reason for hiding this comment

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

looks good.

onion address can be a bit cut off when not on full window
image

nit, the spacing of Ping wait looks a bit odd
image

src/qml/pages/node/PeerDetails.qml
bottomPadding: 5

KeyValueRow { key: KeyText { text: qsTr("Address"); } value: ValText { text: details.address; color: Theme.color.neutral9; }}
KeyValueRow { key: KeyText { text: qsTr("VIA"); } value: ValText { text: details.addressLocal; color: Theme.color.neutral9; }}
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe VIA could be displayed only when it has a value?
for example, for an onion peer there is no VIA

Copy link
Contributor

Choose a reason for hiding this comment

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

I do agree, unless there's reason for leaving it empty.

Copy link
Contributor

@pablomartin4btc pablomartin4btc Mar 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

As I saw in previous approach of the PeerDetails page #387, perhaps a "N/A" could be displayed or "-"(as it's shown in the description above)? (e.g.)

D33r-Gee
D33r-Gee reviewed Mar 8, 2024
View reviewed changes
Copy link
Contributor

@D33r-Gee D33r-Gee left a comment

Choose a reason for hiding this comment

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

On Ubuntu 22.04 and Android arm7 (Galaxy A13 5G)
tACK 4909ba5 c33abef 6480cfe

pablomartin4btc
pablomartin4btc reviewed Mar 13, 2024
View reviewed changes
Copy link
Contributor

@pablomartin4btc pablomartin4btc left a comment

Choose a reason for hiding this comment

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

tACK 4909ba5

Screenshot from 2024-03-13 12-53-22

@jarolrod jarolrod marked this pull request as draft March 20, 2024 08:01
@jarolrod
Copy link
Member Author

draft until I fix the line spacing when text wraps

@jarolrod jarolrod force-pushed the peer-details-alternate branch from 4909ba5 to 02d3121 Compare September 25, 2024 10:59
@jarolrod jarolrod marked this pull request as ready for review September 25, 2024 11:00
@jarolrod jarolrod changed the title Add PeerDetails page [Alternative] Add PeerDetails page Sep 25, 2024
@jarolrod
Copy link
Member Author

jarolrod commented Sep 25, 2024 •
edited
Loading

Updated from 4909ba5 to 02d3121

Addressed review comments on id naming and icon size, additionally rebased over main to address conflicts. TODO's to be left for follow-ups

MarnixCroes
MarnixCroes approved these changes Sep 26, 2024
View reviewed changes
Copy link
Contributor

@MarnixCroes MarnixCroes left a comment

Choose a reason for hiding this comment

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

apart from some (minor) things that can be improved indeed, this works as expected.

tACK 02d3121

D33r-Gee
D33r-Gee reviewed Sep 26, 2024
View reviewed changes
src/qml/pages/node/PeerDetails.qml
}
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: there's seems to be extra lines at the bottom. Is that on purpose?

D33r-Gee
D33r-Gee reviewed Sep 27, 2024
View reviewed changes
Copy link
Contributor

@D33r-Gee D33r-Gee left a comment

Choose a reason for hiding this comment

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

tACK 02d3121

Other than the nit in the PeersDetails.qml looks good to go, see screenshots below

Ubuntu 22.04

Screenshot 2024-09-27 095631

Android Screenshot

Android_Peers_Detail

pablomartin4btc
pablomartin4btc reviewed Sep 27, 2024
View reviewed changes
Copy link
Contributor

@pablomartin4btc pablomartin4btc left a comment

Choose a reason for hiding this comment

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

utACK 02d3121

Minor changes since my last review. Perhaps an issue could be created with the TODO list.

@hebasto hebasto merged commit 7b056a0 into bitcoin-core:main Sep 27, 2024
8 of 9 checks passed
@hebasto
Copy link
Member

hebasto commented Oct 12, 2024

CI reports a linker error on macOS 13:

ld: Undefined symbols:
  qInitResources_bitcoin_qml(), referenced from:
      QmlGuiMain(int, char**) in libbitcoinqt.a[60](qt_libbitcoinqt_a-bitcoin.o)
clang: error: linker command failed with exit code 1 (use -v to see invocation)

@hebasto
Copy link
Member

hebasto commented Oct 12, 2024

CI reports a linker error on macOS 13:

ld: Undefined symbols:
  qInitResources_bitcoin_qml(), referenced from:
      QmlGuiMain(int, char**) in libbitcoinqt.a[60](qt_libbitcoinqt_a-bitcoin.o)
clang: error: linker command failed with exit code 1 (use -v to see invocation)

Fixed in #425.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@D33r-Gee D33r-Gee D33r-Gee left review comments

@pablomartin4btc pablomartin4btc pablomartin4btc left review comments

@johnny9 johnny9 johnny9 left review comments

@MarnixCroes MarnixCroes MarnixCroes approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jarolrod @hebasto @johnny9 @MarnixCroes @pablomartin4btc @D33r-Gee