< bitcoin-git>
[gui] jonatack closed pull request #162: Add network to peers window and peer details (master...display-peer-networks) https://github.com/bitcoin-core/gui/pull/162
< bitcoin-git>
[gui] jonatack closed pull request #163: Peer details: replace Direction with Connection Type (master...display-peer-conn-types) https://github.com/bitcoin-core/gui/pull/163
< michaelfolkson>
prayank: I've reviewed. I'm a touch confused though on what the comment is saying. You can explain here rather than on the PR if you'd like
< michaelfolkson>
I looked at it. It appears reviewers were confused with the motivation in that issue too
< michaelfolkson>
Just a pointer, if you are going to ask people to review, writing things like "I hope you did this...." isn't the best way to succeed in getting that review
< prayank>
michaelfolkson: I've replied to your questions in the PR
< michaelfolkson>
Ok thanks. Probably better to start discussing a PoC here than on a PR for documentation if you have one
< jonatack>
too much nonsense and complication with transversal changes between the gui and the main repo. i tried to follow the contributing guide, but apparently it has to be more complicated, with preliminary pulls in one gating changes in the other. nah.
< bitcoin-git>
[bitcoin] jonatack closed pull request #20778: net, p2p, gui: replace direction with connection type in gui peer details window (master...display-peer-conn-types) https://github.com/bitcoin/bitcoin/pull/20778
< jonatack>
I understand the reasons for splitting the repos, but unreasonably complicating things for contributions to the gui repo to the point where opening multiple pulls and reading between tea leaves in the CONTRIBUTING guide and still being wrong surely wasn't one of them.
< jonatack>
For simple changes like those pulls, too much bandwidth is taken by this and it's a waste of bandwidth.
< michaelfolkson>
I think the separation between GUI and non GUI makes sense. Though I don't know what happened in this particular example. Why can't the PR in the non-GUI repo state as a motivation a future change to the GUI. And then once that is merged, open a PR in the GUI for the GUI specific changes
< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #20786: net: [refactor] Prefer integral types in CNodeStats (master...2012-net) https://github.com/bitcoin/bitcoin/pull/20786
< michaelfolkson>
Sounds more of a "I am used to things how used to be done" rather than any new problem. Reviewers in the non-GUI repo will need to get used to reviewing changes that are lining up a future PR in the GUI repo
< michaelfolkson>
But maybe I'm missing something
< michaelfolkson>
I get it is still confusing and there are teething problems which is frustrating
< jonatack>
michaelfolkson: per the contributing guide, changes to the gui that are also transversal are to be opened in the main repo. the pulls were originally gui code only, to avoid this issue. reviewers asked for transversal changes with the main repo. i followed the guide and opened second pulls in the main repo. nope, new rule: you need to open one part of the them in the main repo, hope
< jonatack>
someone will understand why it's a good idea and review it (attracting review is hard enough), then open a second pull for the gui-only changes in the gui repo. for simple changes like these, that seems unreasonably complicating things and a waste of bandwidth.
< jonatack>
maybe if a maintainer does it, it can work, but for simple contributors it's a large increase in difficulty in making gui changes that also touch the main repo.
< michaelfolkson>
For a traversal change the code obviously needs to be written for both the GUI and the main repo at some stage. So it isn't more code, it is just a question of process for how you get review.
< michaelfolkson>
Go to GUI for a design/concept ACK on a visual. Go back to the non-GUI repo for an ACK on the non-GUI code. Then back to the GUI repo for review of the GUI code.
< jonatack>
I think the contributing guide needs to be updated if this is the complicated new process to follow.
< MarcoFalke>
I can see the point that opening two pulls is a slightly heavier process and may delay things a bit, but it shouldn't cost more review. 6 commits are still to be reviewed. It shouldn't matter if two are in main and 4 in gui or all in one.
< michaelfolkson>
Right, that's what it seems like to me too. The bottleneck is always the actual review and getting the ACKs. Slighty heavier as you say but the biggest problem is getting the actual review
< jonatack>
MarcoFalke: I think leaving the pulls in the gui repo was fine in this case (simple, non-dangerous changes).
< MarcoFalke>
Yeah, maybe relaxing the rules a bit to allow simple refactors also in the gui repo might be a good solution
< jonatack>
ISTM that getting review twice is at least twice as hard, especially for gui-motivated changes in the main repo.
< MarcoFalke>
I was hoping that the cross-repo pulls aren't too numerous for it to matter much
< jonatack>
This seems to be one of the first times iirc
< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #20787: Use C++17 std::array deduction for OUTPUT_TYPES, ALL_FEE_ESTIMATE_HORIZONS (master...2012-WswitchFeeEstimateHorizon) https://github.com/bitcoin/bitcoin/pull/20787
< wumpus>
jonatack: fwiw i think that's a great addition and wouldn't have minded reviewing it
< wumpus>
I don't want to get involved in gui design decisions I just can't do that, just too subjective and arbitrary, but adding useful information to the debug window is something that shouldn't be too difficult?
< luke-jr>
hmm, on that note I wonder if "tailing" the debug log would be a good feature for the GUI
< wumpus>
i think a live debug log view would be awesome
< wumpus>
though implementing it in a way that is efficient (doesn't end up logging everything to memory) is non-trivial
< wumpus>
qt's build in stuff is really bad at handling large amounts of data in general, this is why the current 'open log' button forks out to an external editor
< luke-jr>
well, if it only shows the last N lines, it's probably not hard
< luke-jr>
adding a scrollbar would ofc :P
< wumpus>
right, 'subscribe to log messages and keep the last 10' or so would be easy enough and maybe useful enough
< wumpus>
or, last 1000, it's not like messages take that much memory :)
< phantomcircuit>
wumpus, i've seen text boxes that compress the text that isn't being actively displayed but i cant remember anything about them except the concept
< wumpus>
phantomcircuit: it's definitely possible to do things like that, just qt doesn't do it by default, if you want anything smart you have to implement it yourself
< jonatack>
wumpus: yes, and both pulls were reviewed, finished, collaborated together with hebasto, had acks... but then it unraveled with the process, I followed the instructions in contributing.md but it seems to needs updating, and the new process is arduous for contributors without the social capital to garner review on two repos for one change.
< jonatack>
(the new new process, not the new process :D)
< jonatack>
it's hard enough to garner review in one repo alone
< jonatack>
simple changes like that aren't worth it imho. i've resolved to concentrate on more interesting things that are worth doing that for.
< luke-jr>
phantomcircuit: it would make more sense if we're goibng to that complexity, to just load it off disk as needed
< wumpus>
jonatack: I hadn't noticed it, if you'd told me I'd have merged it
< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #20789: fuzz: Rework strong and weak net enum fuzzing (master...2012-fuzzRefactor) https://github.com/bitcoin/bitcoin/pull/20789
< wumpus>
jonatack: for changes that both involve invasive changes to the GUI and to the core code, it's probably better to do it in phases, but this isn't one of them
< bitcoin-git>
[bitcoin] jonatack opened pull request #20790: policy: create CFeeRate::FromSatB and FromBtcKb named ctors (master...CFeeRate-named-constructors) https://github.com/bitcoin/bitcoin/pull/20790