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

feat: add a cmd to request window size #988

Merged
merged 2 commits into from
Jul 16, 2024
Merged

feat: add a cmd to request window size #988

merged 2 commits into from
Jul 16, 2024

Conversation

aymanbagabas
Copy link
Member

commands.go Outdated Show resolved Hide resolved
Co-authored-by: Christian Rocha <christian@rocha.is>
@aymanbagabas aymanbagabas added this to the v0.27.0 milestone Jun 21, 2024
@aymanbagabas aymanbagabas merged commit 7d70838 into master Jul 16, 2024
18 checks passed
@aymanbagabas aymanbagabas deleted the win-size-cmd branch July 16, 2024 16:56
@atticus-sullivan
Copy link

Because the PR got merged only a few hours ago and this issue is 100% certain linked to this PR, I'm writing no new issue but comment on this PR.

Sorry for waiting until after the merge before testing this. In my test, when I send a tea.WindowSize() in the Update() function of a model, it causes the program to hang up. My analysis shows that the reason behind this most probably (well tbh, I'm pretty certain on this) is because
you call p.checkResize() in the eventLoop() which in turn calls p.Send() which in the end sends on the p.msgs channel. The problem is (by default) this channel is not buffered, thus the send blocks until another goroutine reads from it (but the one which would read from it is the one sending on that channel).

I'm not too familiar with the codebase but searching for other occurrences of p.Send(), it seems like this function is called in a new goroutine usually. In this case I'm not sure if the p.checkResize() should call go p.Send() (would also affect the init stuff which I'm not familiar with) or if we should call go p.checkResize() in the eventLoop().

I hope I didn't formulate this too complicated otherwise feel free to ask me what I actually mean. Any thoughts on this? Also, didn't someone else had this issue as well?

aymanbagabas added a commit that referenced this pull request Jul 16, 2024
We need to run checkResize in a goroutine, otherwise, it will block.

Related: #988 (comment)
Fixes: 7d70838 (feat: add a cmd to request window size (#988))
@aymanbagabas
Copy link
Member Author

@atticus-sullivan Thank you for noticing this one! I've opened #1059 to fix this one and add an example to request window size.

@atticus-sullivan
Copy link

Alright, thanks for the fast reply + fix. Also having an example for this is quite nice 👍

aymanbagabas added a commit that referenced this pull request Jul 18, 2024
We need to run checkResize in a goroutine, otherwise, it will block.

Related: #988 (comment)
Fixes: 7d70838 (feat: add a cmd to request window size (#988))
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.

Allow WindowSizeMsg to be sent on sub-model Init
4 participants