screen: Don't crash when first tab doesn't exist #1648
Conversation
|
Curiously, when attaching now the status bar believes we are in tab mode when we are in fact not. Investigating... Also I think this deserves a test to prevent regressions, but where/how would I test that? Is this a candidate for an e2e test? |
Is this something introduced by this fix? |
Hard to say, previously it would just crash and die when there wasn't a first tab. I guess you could say so. |
|
Cool, happy you're looking into this. About tests, maybe try here: https://github.com/zellij-org/zellij/blob/main/zellij-server/src/unit/screen_tests.rs |
|
@imsnif It seems the Do you have an idea how that can be? Are the client IDs shared in a session? Edit: It get's even weirder: Within a second, the client is attached to the session twice, first with input mode |
3759da8
to
74bb3f3
Compare
|
Been fiddling with it but I cannot make out the reason for this behavior. Maybe it's because something in the code sees that the tab index is different from what it expects, and then just thinks "Oh we switched to a new tab (as in: We made a new tab), so we are in tab mode now"? Other than that I'm out of ideas for now. Or maybe zellij starts out with the client being in "Tab" mode, sends that to the plugins, immediately updates that to be "Normal" mode and for some reason that doesn't propagate to the plugins anymore? Just guessing... FYI: I'll be out of town until Tuesday, so if someone else wants to pick this up in the meantime that's fine by me. |
while trying to attach a new client. Instead, check whether the first tab does exist and if not, take the first tab index from the tabs present in the session. If no tabs exist, panic with a better error message.
74bb3f3
to
37600a4
Compare
|
On Wed, 2022-08-10 at 09:41 -0700, Thomas Linford wrote:
@tlinford commented on this pull request.
> self.active_tab_indices.insert(client_id, tab_index);
self.connected_clients.borrow_mut().insert(client_id);
self.tab_history.insert(client_id, tab_history);
self.tabs
.get_mut(&tab_index)
- .unwrap()
+ .unwrap_or_else(|| panic!("Failed to attach client to tab with
index {tab_index}"))
couldn't this just be an expect?
Originally it was, but that makes clippy unhappy, because `expect()` takes a
`&str`, so you end up doing `expect(format!(...))`. Apparently that's bad. :)
… |
|
I wouldn't call it easier, because IMO it makes the code less readable to the
extent that there are two places where the `tab_index` is handled separately.
Also this gets rid of the `mut`, since I'm assigning the whole thing in one go.
The idea of making things `mut` and then assigning them inside an `if` branch
just looks/feels too much like C to me, and I don't like that.
But you've been a project member for longer, so if you prefer this I'll accept
it, of course.
…On Wed, 2022-08-10 at 09:40 -0700, Thomas Linford wrote:
@tlinford commented on this pull request.
> @@ -702,21 +702,29 @@ impl Screen {
}
pub fn add_client(&mut self, client_id: ClientId) {
- let mut tab_index = 0;
```suggestion
let mut tab_index = *self.tabs.keys().next().expect("no tabs found");
```
I think this is maybe easier than changing the logic of the ifs below?
|
ha, I see.
I can get on board with that reasoning! The one thing I still don't get though is the special casing for tab id 0: EDIT:
Not at all, I had just started looking at this before I saw your changes and figured I would give my 2 cents :) |
while trying to attach a new client. Instead, check whether the first
tab does exist and if not, take the first tab index from the tabs
present in the session. If no tabs exist, panic with a better error
message.
Fixes #1645