screen: Don't crash when first tab doesn't exist

[har7an]

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

[har7an]

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?

[imsnif]

Curiously, when attaching now the status bar believes we are in tab mode when we are in fact not...

Is this something introduced by this fix?

[har7an]

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.

[imsnif]

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

[har7an]

@imsnif It seems the mode_info sent to the plugins has mode set to Tab. It happens in server/src/screen.rs around line 644, where the Screen queries itself for the ModeInfo of the client that is attached. I would expect that to turn up empty and fallback to the default, but it finds one with ModeInfo.mode set to Tab.

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 Normal, and then with input mode Tab.

[har7an]

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.

[har7an]

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. :)

…

[har7an]

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?

[tlinford]

Originally it was, but that makes clippy unhappy, because expect() takes a &str, so you end up doing expect(format!(...)). Apparently that's bad. :)

ha, I see.

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.

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: else if self.tabs.contains_key(&0)? wouldn't that be covered by else if let Some(tab_index) = self.tabs.keys().next()?

EDIT:

But you've been a project member for longer, so if you prefer this I'll accept
it, of course.

Not at all, I had just started looking at this before I saw your changes and figured I would give my 2 cents :)

[imsnif]

I fixed the exposed Tab mode bug. For some reason* we were sending the wrong default mode to tabs upon creation.
I think this is good to go. Thanks for the collab on this @har7an and @tlinford :)

*nice to meet you, I'm Some Reason