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.

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

Refresh all avatars upon setting a peer's tag #6348

Merged
merged 4 commits into from
Sep 21, 2022

Conversation

Android-X13
Copy link
Contributor

Fixes #4660

Copy link
Contributor

@ripcurlx ripcurlx left a comment

Choose a reason for hiding this comment

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

NACK - I reviewed the code and tested it quickly on Regtest to confirm my assumption that you also need to do some changes in UnconfirmedBsqSwapsView, ClosedTradesView and PendingTradesView which use the PeerInfoIconTrading which extends PeerInfoIcon

@Android-X13
Copy link
Contributor Author

I rewrote this using a different approach to eliminate the interface and the multiple identical overrides (since it's now used by many classes). Of course I can revert to using the old approach if you prefer

@ripcurlx
Copy link
Contributor

How about having something like a PeerInfoIconMap that holds all PeerInfoIcons, listens to a save event of each PeerInfoIcon and calls refreshTag on all instances in the internal Map accordingly. Controlling behavior of other PeerInfoIcons from within a PeerInfoIcon doesn't feel right to me. WDYT?

@Android-X13
Copy link
Contributor Author

No problem, re-re-wrote this 😅
Maybe I misinterpreted you, but I suppose you didn't mean one universal avatar map holding all PeerInfoIcons from all views from different sections because I think that would introduce other problems.
I'm using one custom map in each view like before, which now listens for tag change events, close to what you requested.
The only thing I'm not sure about is where to place the new PeerInfoIconMap class in the vast directory structure of this project. I've put it in components along with the other PeerInfoIcon classes although it's not exactly a component...

@ripcurlx
Copy link
Contributor

No problem, re-re-wrote this 😅 Maybe I misinterpreted you, but I suppose you didn't mean one universal avatar map holding all PeerInfoIcons from all views from different sections because I think that would introduce other problems. I'm using one custom map in each view like before, which now listens for tag change events, close to what you requested. The only thing I'm not sure about is where to place the new PeerInfoIconMap class in the vast directory structure of this project. I've put it in components along with the other PeerInfoIcon classes although it's not exactly a component...

Yes, that is what I was thinking about 👍 . Location of the extended HashMap is fine IMO. Maybe we could have put all PeerInfoIcon components into one sub-package, but I think it is fine as it is right now. I'm doing one quick Regtest test and will merge it afterwards.

ripcurlx
ripcurlx previously approved these changes Sep 19, 2022
@ripcurlx ripcurlx added this to the v1.9.6 milestone Sep 19, 2022
@ripcurlx
Copy link
Contributor

@Android-X13 Your last commits haven't been signed somehow. Could you update them and force push it and I'll merge this PR. Thanks!

@ripcurlx
Copy link
Contributor

@Android-X13 I don't know why, but your first two commits are still not signed. You can see it if the 'Verified' label is not shown next to the commit.

Copy link
Contributor

@ripcurlx ripcurlx left a comment

Choose a reason for hiding this comment

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

ACK

@ripcurlx ripcurlx merged commit 5d06b51 into bisq-network:master Sep 21, 2022
@Android-X13 Android-X13 deleted the update-peer-tags branch November 22, 2022 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tagging peer only updates one row instead of all affected rows before restart
2 participants