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: change session print loop to block until all events are handled #2920

Merged
merged 3 commits into from
Oct 14, 2022

Conversation

jeronimoalbi
Copy link
Member

Fixes #2917

@jeronimoalbi jeronimoalbi added the type:error Something isn't working label Oct 14, 2022
@jeronimoalbi jeronimoalbi self-assigned this Oct 14, 2022
@lumtis
Copy link
Contributor

lumtis commented Oct 14, 2022

Thanks for having fixed this so quickly
The main issue with ProgressFinished is fixed

ignite n chain join 24 --amount 95000000stake
? Peer's address 200.46.122.3:26656
Source code fetched
Blockchain set up
Account added to the network by the coordinator!
Validator added to the network by the coordinator!

There are still some inconsistency in some output using event bus like:

ignite n validator show spn1daefnhnupn85e8vv0yc5epmnkcr5epkqncn2le
◢ Fetching validator description identity: "0xaaa"
details: foo foo fii fii
website: haha.com
security: haha

Event printed from ProgressStarted is not erased. I'm trying to see the difference with the logic of ignite n chain list where this issue doesn't occur but can't find any difference

@jeronimoalbi
Copy link
Member Author

jeronimoalbi commented Oct 14, 2022

I believe it has something to do with how the spinner works. I think that the text is only deleted when it's the spinner text setted by calling session.StartSpinner("TEXT"). Event messages are deleted only when using the events.ProgressStarted() option.

Actually I want to explore how to improve the session/events API. I didn't move far away from the initial PR, but I think there is room for improvements.

@tbruyelle
Copy link
Contributor

I checked and I think I found the cause of the problem: session is duplicated. The one used in the events loop is not the same as the one returned by clui.New(). Thus, the unerlying spinner is also different, when the event loop affects a new one on IndicationStart, session.Println() still has a nil spinner and so the pause has no effect.

A simple fix is to return a pointer in clui.New() and use pointer receiver method for all methods that affects the session.

@lumtis
Copy link
Contributor

lumtis commented Oct 14, 2022

I checked and I think I found the cause of the problem: session is duplicated. The one used in the events loop is not the same as the one returned by clui.New(). Thus, the unerlying spinner is also different, when the event loop affects a new one on IndicationStart, session.Println() still has a nil spinner and so the pause has no effect.

A simple fix is to return a pointer in clui.New() and use pointer receiver method for all methods that affects the session.

Thank you for looking at this. I will check that.

Can be handled in another PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:error Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Event Bus: events not printed for ProgressFinished
3 participants