Repository: lapce/lapce
File: lapce-ui/src/terminal.rs (Rust)

Code before:
        // than one empty TerminalSplitData at the same time. Thus `SmallVec<[WidgetId; 1]>`
        // will allow how to avoid unnecessary allocation and at the same time will not take
        // up more space on the stack than necessary.
        let empty_tabs = data
            .terminal
            .tabs
            .iter()
            .filter(|(_, t)| t.terminals.is_empty())
            .map(|(tab_id, _)| *tab_id)
            .collect::<SmallVec<[WidgetId; 1]>>();
        if !empty_tabs.is_empty() {
            // We remove all empty entries, but at the same time it is necessary to synchronize
            // changes in `data.terminal.tabs` with `data.terminal.tabs.tabs_order`.
            // The `data.terminal.tabstabs_order` is a vector, so `removing` elements from it in
            // a loop is inefficient due to shifting all elements to the left after each removal.
            //
            // Therefore, we first store the first `WidgetId` to be removed in the `id_to_remove`
            // variable, and in the loop itself, instead of deleting, we will equate all the values
            // to be removed to `id_to_remove`. After the loop, we simply rebuild the vector again,
            // excluding all elements equal to `id_to_remove`.
            //
            // This will be equally effective with one removed element, and with more of them, as
            // it will allow us to allocate a vector with the desired capacity only once.
            //
            // SAFETY: We have already checked that empty_tabs is not empty
            let id_to_remove = unsafe { *empty_tabs.get_unchecked(0) };
            let removed_len = empty_tabs.len();

            let terminal = Arc::make_mut(&mut data.terminal);
            let tabs_order = Arc::make_mut(&mut terminal.tabs_order);
            for tab_id in empty_tabs {
                self.tabs.remove(&tab_id);
                terminal.tabs.remove(&tab_id);

                if let Some(id) = tabs_order.iter_mut().find(|id| *id == &tab_id) {
                    *id = id_to_remove;
                }
            }
            let mut new_order: Vec<WidgetId> =
                Vec::with_capacity(tabs_order.len() - removed_len);
            new_order.extend(tabs_order.iter().filter(|id| *id != &id_to_remove));
            *tabs_order = new_order;

            ctx.children_changed();

            if tabs_order.is_empty() {
                if data.panel.is_panel_visible(&PanelKind::Terminal) {
                    Arc::make_mut(&mut data.panel).hide_panel(&PanelKind::Terminal);
                }
                if let Some(active) = *data.main_split.active_tab {
                    ctx.submit_command(Command::new(

Review comment (suggestion):
If possible, I'd like you to replace the unsafe code and the outer if with `if let Some(id_to_remove) = empty_tabs.get(0)` that does the same without introducing unsafe.

Code after:
        // than one empty TerminalSplitData at the same time. Thus `SmallVec<[WidgetId; 1]>`
        // will allow how to avoid unnecessary allocation and at the same time will not take
        // up more space on the stack than necessary.
        //
        // Note: Here we are using a SmallVec instead of iterating directly with deletion, since
        // that would require `Arc::make_mut` to be called every time, even if no changes were made.
        let empty_tabs = data
            .terminal
            .tabs
            .iter()
            .filter(|(_, t)| t.terminals.is_empty())
            .map(|(tab_id, _)| *tab_id)
            .collect::<SmallVec<[WidgetId; 1]>>();

        // We remove all empty entries, but at the same time it is necessary to synchronize
        // changes in `data.terminal.tabs` with `data.terminal.tabs.tabs_order`.
        // The `data.terminal.tabstabs_order` is a vector, so `removing` elements from it in
        // a loop is inefficient due to shifting all elements to the left after each removal.
        //
        // Therefore, we first store the first `WidgetId` to be removed in the `id_to_remove`
        // variable, and in the loop itself, instead of deleting, we will equate all the values
        // to be removed to `id_to_remove`. After the loop, we simply rebuild the vector again,
        // excluding all elements equal to `id_to_remove`.
        //
        // This will be equally effective with one removed element, and with more of them, as
        // it will allow us to allocate a vector with the desired capacity only once.
        if let Some(&id_to_remove) = empty_tabs.get(0) {
            let removed_len = empty_tabs.len();

            let terminal = Arc::make_mut(&mut data.terminal);
            let tabs_order = Arc::make_mut(&mut terminal.tabs_order);
            for tab_id in empty_tabs {
                self.tabs.remove(&tab_id);
                terminal.tabs.remove(&tab_id);

                if let Some(id) = tabs_order.iter_mut().find(|id| *id == &tab_id) {
                    *id = id_to_remove;
                }
            }
            let mut new_order: Vec<WidgetId> =
                Vec::with_capacity(tabs_order.len() - removed_len);
            new_order.extend(tabs_order.iter().filter(|id| *id != &id_to_remove));
            *tabs_order = new_order;

            ctx.children_changed();

            if tabs_order.is_empty() {
                if data.panel.is_panel_visible(&PanelKind::Terminal) {
                    Arc::make_mut(&mut data.panel).hide_panel(&PanelKind::Terminal);
                }
                if let Some(active) = *data.main_split.active_tab {