search

zellij-org / zellij

A terminal workspace with batteries included
https://zellij.dev
MIT License
22.06k stars 674 forks source link

When a pane closes on its own and is in full screen, the FULLSCREEN message remains in the tip line #748

Closed imsnif closed 3 years ago

imsnif commented 3 years ago

To reproduce:

  1. Open a new pane (alt + n)
  2. Change it to full screen
  3. type "exit" to get the shell to close itself
  4. the "FULLSCREEN" message still appears at the bottom of the screen
tlinford commented 3 years ago

@imsnif not sure why the tests failed in the pr, everything seems fine on my machine.

imsnif commented 3 years ago

@tlinford - the tests also passed a few times for me locally. I re-ran them in the CI now, but this is a little concerning... could be that we introduced an extra render here and there's a race condition?

tlinford commented 3 years ago

All that was added here was two calls to update_tabs, as far as I can see that should only cause some PluginInstruction::Update to be sent to the plugins?

imsnif commented 3 years ago

I need to go afk now so can't check, but it's worth checking if those calls add an additional Render down the line. If so it might be a race.

tlinford commented 3 years ago

You are right, there are some additional calls to render down the line, I think because when PluginInstruction::Update is handled and plugins are updated, ScreenInstruction::Render is sent from zellij-server/src/wasm_vm.rs.

Don't really get what happened in the failed test run though.

imsnif commented 3 years ago

Ideally I'd like to avoid adding more renders. We might need to rethink some of the update_tabs flow if it causes more renders than needed. Ideally I'd like things to be:

  1. Update state of all actors involved as a result of an action (eg. terminals, plugins)
  2. Render once this is done

But we don't live in an ideal world :)

Do you think we can achieve this without adding extra renders somehow?

tlinford commented 3 years ago

In this case, as a workaround, I think we can skip the screen.render() after screen.update_tabs(), since we know that update_tabs() will update plugins, and after that a render is always requested by wasm_vm.

imsnif commented 3 years ago

Sounds good. Will have to play with it a little to make sure there isn't some edge case the tests don't cover that is harmed by this.

tlinford commented 3 years ago

cool, I pushed the changes and the tests ran fine, and I'm seeing the expected behavior locally.

imsnif commented 3 years ago

Looks great, thanks!