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

fix(TUI): selected column and row of table are now preserved during update #3161

Merged
merged 1 commit into from Dec 10, 2019

Conversation

@JosephGoulden
Copy link
Contributor

JosephGoulden commented Dec 7, 2019

Currently if you move around a TUI table (e.g. peers) with the arrow keys your selection is reset every time the table is updated with a call to set_items(). This change is to preserve the selected row and column which should make is easier to monitor a selected peer for example.

… when set_items() is called
@JosephGoulden JosephGoulden changed the title fix(TUI): Selected column and selected row of table are now preserved… fix(TUI): Selected column and row of table are now preserved during update Dec 7, 2019
@JosephGoulden JosephGoulden changed the title fix(TUI): Selected column and row of table are now preserved during update fix(TUI): selected column and row of table are now preserved during update Dec 7, 2019
@antiochp

This comment has been minimized.

Copy link
Member

antiochp commented Dec 9, 2019

🔥 Let me test this out locally. 👍 on what we are fixing here.

@antiochp

This comment has been minimized.

Copy link
Member

antiochp commented Dec 9, 2019

We do need to make a decision around the trade-offs involved in merging this (and any other fixes) between now (beta.1) and 3.0.0 release.
I would lean toward merging this but we should have some consensus here around the decision making process itself.

The trade-off being fixing issues and improvements vs. the risk of introducing other (potentially subtle) issues inadvertently.

@lehnberg

This comment has been minimized.

Copy link
Collaborator

lehnberg commented Dec 9, 2019

As per the timeline in our process doc, code freeze is scheduled for Dec 12, defined as:

No changes are permitted on the branch to be released except critical bugs.

Today is the 9th. So technically, this could get merged in before Thursday and it would be fine. After Thursday it would not be fine.

Personally however, I lean towards not merging anything that's not in scope and not critical at this point, because I prefer a more solid quality, less capable build, over a more capable but less performant release. And I worry about our testing capabilities. I suspect it's quite easy for problematic bugs to sneak in without us noticing it, but I have no basis for this claim. We'd add more risk, at a point where I feel we have enough risk as is already.

That's just my 2 grins worth. But yes, might be something to talk about in tmrw's dev meeting, if you'd like to add a point to the agenda?

@hashmap

This comment has been minimized.

Copy link
Member

hashmap commented Dec 9, 2019

what about aiming for 3.1 in a month after 3.0?

@yeastplume

This comment has been minimized.

Copy link
Member

yeastplume commented Dec 9, 2019

There's one or two things I wouldn't mind squeezing in this week, (mostly very superficial), let's discuss at dev meeting tomorrow.

@JosephGoulden

This comment has been minimized.

Copy link
Contributor Author

JosephGoulden commented Dec 9, 2019

Sorry I might not be able to available to discuss tomorrow. This is relatively low risk in my opinion, over a month of testing seems like plenty of time. But I've not been around for previous releases so if consensus is it's too risky then that's fine. I'll have some time the next few weeks to help fix any issues (particularly with things I worked on).

@antiochp

This comment has been minimized.

Copy link
Member

antiochp commented Dec 9, 2019

So technically, this could get merged in before Thursday and it would be fine.

Personally however, I lean towards not merging anything that's not in scope and not critical

Yes. That's kind of what I mean - "code freeze" does not necessarily mean anything goes prior to that.
That said I agree with @JosephGoulden here, this is low risk and I think likely fine (pending some extra eyes on it and some local testing).

I just wanted to bring this up somewhere and this was as good a place as any.

@antiochp

This comment has been minimized.

Copy link
Member

antiochp commented Dec 10, 2019

Tested this locally with #3160 and it all looks good. I think its worth the minimal risk involved in merging this close to beta.2

@antiochp antiochp merged commit 5c7bc3d into mimblewimble:master Dec 10, 2019
10 checks passed
10 checks passed
mimblewimble.grin #20191207.3 succeeded
Details
mimblewimble.grin (linux api/util/store) linux api/util/store succeeded
Details
mimblewimble.grin (linux chain/core/keychain) linux chain/core/keychain succeeded
Details
mimblewimble.grin (linux pool/p2p/src) linux pool/p2p/src succeeded
Details
mimblewimble.grin (linux release) linux release succeeded
Details
mimblewimble.grin (linux servers) linux servers succeeded
Details
mimblewimble.grin (macos release) macos release succeeded
Details
mimblewimble.grin (macos test) macos test succeeded
Details
mimblewimble.grin (windows release) windows release succeeded
Details
mimblewimble.grin (windows test) windows test succeeded
Details
@antiochp antiochp added this to the 3.0.0 milestone Dec 11, 2019
garyyu added a commit to garyyu/grin that referenced this pull request Jan 14, 2020
… when set_items() is called (mimblewimble#3161)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.