ronantakizawa/codereview-bench huggingface dataset:
Repository: zellij-org/zellij
File: default-plugins/tab-bar/src/main.rs (Rust)

Code before:
        for (idx, bar_part) in tab_line.iter().enumerate() {
            s = format!("{}{}", s, &bar_part.part);

            if self.should_render
                && self.mouse_click_pos > len_cnt
                && self.mouse_click_pos <= len_cnt + bar_part.len
                && idx > 1
            {
                // First two elements of tab_line are "Zellij" and session name, hence the idx > 1 condition.
                // Tabs are indexed starting from 1, therefore we need subtract 1 below.
                switch_tab_to(TryInto::<u32>::try_into(idx).unwrap() - 1);
            }
            len_cnt += bar_part.len;
        }
        match self.mode_info.palette.cyan {
            PaletteColor::Rgb((r, g, b)) => {
                println!("{}\u{1b}[48;2;{};{};{}m\u{1b}[0K", s, r, g, b);
            }
            PaletteColor::EightBit(color) => {
                println!("{}\u{1b}[48;5;{}m\u{1b}[0K", s, color);
            }
        }
        self.should_render = false;
    }
}


Review comment (bug):
As the width of the window shrinks, parts of the tab bar get either merged or left off. This logic can break down a little there, but so do some other (visual) things and these are bugs we've been living with. 

Overall then, this is fine as is, but if you want to also add scrolling with the mouse-wheel to change tabs, that might provide a nice, user-friendly way to change tabs with the mouse even when they don't all fit on the screen.

Code after:
            );
            all_tabs.push(tab);
        }
        let tab_line = tab_line(
            self.mode_info.session_name.as_deref(),
            all_tabs,
            active_tab_index,
            cols.saturating_sub(1),
            self.mode_info.palette,
            self.mode_info.capabilities,
        );
        let mut s = String::new();
        let mut len_cnt = 0;
        dbg!(&tab_line);
        for (idx, bar_part) in tab_line.iter().enumerate() {
            s = format!("{}{}", s, &bar_part.part);

            if self.should_render
                && self.mouse_click_pos > len_cnt
                && self.mouse_click_pos <= len_cnt + bar_part.len
                && idx > 2
            {
                // First three elements of tab_line are "Zellij", session name and empty thing, hence the idx > 2 condition.
                // Tabs are indexed starting from 1, therefore we need subtract 2 below.
                switch_tab_to(TryInto::<u32>::try_into(idx).unwrap() - 2);
            }
            len_cnt += bar_part.len;
        }
        match self.mode_info.palette.cyan {
            PaletteColor::Rgb((r, g, b)) => {
                println!("{}\u{1b}[48;2;{};{};{}m\u{1b}[0K", s, r, g, b);
            }
            PaletteColor::EightBit(color) => {
                println!("{}\u{1b}[48;5;{}m\u{1b}[0K", s, color);
            }
        }
        self.should_render = false;
    }
}
Repository: zellij-org/zellij
File: zellij-tile/src/data.rs (Rust)

Code before:
    Backspace,
    Left,
    Right,
    Up,
    Down,
    Home,
    End,
    PageUp,
    PageDown,
    BackTab,
    Delete,
    Insert,
    F(u8),
    Char(char),
    Alt(char),
    Ctrl(char),
    Null,
    Esc,
}

#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)]

pub enum Mouse {
    ScrollUp(usize),   // number of lines
    ScrollDown(usize), // number of lines
    ClearScroll,
    LeftClick(isize, usize),              // line and column
    MouseHold(isize, usize),              // line and column
    MouseRelease(Option<(isize, usize)>), // line and column
}

#[derive(Debug, Clone, PartialEq, EnumDiscriminants, ToString, Serialize, Deserialize)]
#[strum_discriminants(derive(EnumString, Hash, Serialize, Deserialize))]
#[strum_discriminants(name(EventType))]
#[non_exhaustive]
pub enum Event {
    ModeUpdate(ModeInfo),
    TabUpdate(Vec<TabInfo>),
    KeyPress(Key),
    Mouse(Mouse),
    Timer(f64),
    CopyToClipboard,
    InputReceived,
}

/// Describes the different input modes, which change the way that keystrokes will be interpreted.
#[derive(Debug, PartialEq, Eq, Hash, Copy, Clone, EnumIter, Serialize, Deserialize)]
pub enum InputMode {
    /// In `Normal` mode, input is always written to the terminal, except for the shortcuts leading
    /// to other modes
    #[serde(alias = "normal")]

Review comment (suggestion):
I'd vote for removing `ClearScroll` probably (if you can't send it with a mouse)

Code after:
pub enum Key {
    Backspace,
    Left,
    Right,
    Up,
    Down,
    Home,
    End,
    PageUp,
    PageDown,
    BackTab,
    Delete,
    Insert,
    F(u8),
    Char(char),
    Alt(char),
    Ctrl(char),
    Null,
    Esc,
}

#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)]

pub enum Mouse {
    ScrollUp(usize),                      // number of lines
    ScrollDown(usize),                    // number of lines
    LeftClick(isize, usize),              // line and column
    MouseHold(isize, usize),              // line and column
    MouseRelease(Option<(isize, usize)>), // line and column
}

#[derive(Debug, Clone, PartialEq, EnumDiscriminants, ToString, Serialize, Deserialize)]
#[strum_discriminants(derive(EnumString, Hash, Serialize, Deserialize))]
#[strum_discriminants(name(EventType))]
#[non_exhaustive]
pub enum Event {
    ModeUpdate(ModeInfo),
    TabUpdate(Vec<TabInfo>),
    KeyPress(Key),
    Mouse(Mouse),
    Timer(f64),
    CopyToClipboard,
    InputReceived,
    Visible(bool),
}

/// Describes the different input modes, which change the way that keystrokes will be interpreted.
#[derive(Debug, PartialEq, Eq, Hash, Copy, Clone, EnumIter, Serialize, Deserialize)]
pub enum InputMode {
    /// In `Normal` mode, input is always written to the terminal, except for the shortcuts leading
    /// to other modes
Repository: zellij-org/zellij
File: zellij-tile/src/shim.rs (Rust)

Code before:
pub fn set_selectable(selectable: bool) {
    unsafe { host_set_selectable(if selectable { 1 } else { 0 }) };
}

// Query Functions
pub fn get_plugin_ids() -> PluginIds {
    unsafe { host_get_plugin_ids() };
    object_from_stdin()
}

pub fn get_exec_permission() -> bool {
    unsafe { host_get_exec_permission() };
    object_from_stdin()
}

// Host Functions

pub fn open_file(path: &Path) {
    object_to_stdout(&path);
    unsafe { host_open_file() };
}

pub fn set_timeout(secs: f64) {
    unsafe { host_set_timeout(secs) };
}
pub fn exec_cmd(cmd: &[String]) -> Result<(), PermissionDenied> {
    if !get_exec_permission() {
        return Err(PermissionDenied);
    }
    object_to_stdout(&cmd);
    unsafe { host_exec_cmd() };
    Ok(())
}

// Internal Functions

#[doc(hidden)]
pub fn object_from_stdin<T: DeserializeOwned>() -> T {
    let mut json = String::new();
    io::stdin().read_line(&mut json).unwrap();
    serde_json::from_str(&json).unwrap()
}

#[doc(hidden)]
pub fn object_to_stdout(object: &impl Serialize) {
    println!("{}", serde_json::to_string(object).unwrap());
}

#[link(wasm_import_module = "zellij")]
extern "C" {
    fn host_subscribe();

Review comment (bug):
I don't think we need the `Result` here and I'd be fine with things failing silently (we don't currently have a way to see if the command was run successfully, even if we did have permissions).

I'd personally just return nothing here, but if you disagree, just let me know!

Code after:

// Query Functions
pub fn get_plugin_ids() -> PluginIds {
    unsafe { host_get_plugin_ids() };
    object_from_stdin()
}

// Host Functions

pub fn open_file(path: &Path) {
    object_to_stdout(&path);
    unsafe { host_open_file() };
}

pub fn set_timeout(secs: f64) {
    unsafe { host_set_timeout(secs) };
}
pub fn exec_cmd(cmd: &[&str]) {
    object_to_stdout(&cmd);
    unsafe { host_exec_cmd() };
}

// Internal Functions

#[doc(hidden)]
pub fn object_from_stdin<T: DeserializeOwned>() -> T {
    let mut json = String::new();
    io::stdin().read_line(&mut json).unwrap();
    serde_json::from_str(&json).unwrap()
}

#[doc(hidden)]
pub fn object_to_stdout(object: &impl Serialize) {
    println!("{}", serde_json::to_string(object).unwrap());
}

#[link(wasm_import_module = "zellij")]
extern "C" {
    fn host_subscribe();
    fn host_unsubscribe();
    fn host_set_selectable(selectable: i32);
    fn host_get_plugin_ids();
    fn host_open_file();
    fn host_set_timeout(secs: f64);
    fn host_exec_cmd();
}
Repository: zellij-org/zellij
File: zellij-tile/src/shim.rs (Rust)

Code before:
use serde::{de::DeserializeOwned, Serialize};
use std::{io, path::Path};

use crate::data::*;

#[derive(Debug)]
pub struct PermissionDenied;

// Subscription Handling

pub fn subscribe(event_types: &[EventType]) {
    object_to_stdout(&event_types);
    unsafe { host_subscribe() };
}

pub fn unsubscribe(event_types: &[EventType]) {
    object_to_stdout(&event_types);
    unsafe { host_unsubscribe() };
}

// Plugin Settings

pub fn set_selectable(selectable: bool) {
    unsafe { host_set_selectable(if selectable { 1 } else { 0 }) };
}

// Query Functions
pub fn get_plugin_ids() -> PluginIds {
    unsafe { host_get_plugin_ids() };
    object_from_stdin()
}


Review comment (suggestion):
Probably don't need this if we change `exec_cmd` to return nothing.

Code after:
use serde::{de::DeserializeOwned, Serialize};
use std::{io, path::Path};

use crate::data::*;

// Subscription Handling

pub fn subscribe(event_types: &[EventType]) {
    object_to_stdout(&event_types);
    unsafe { host_subscribe() };
}

pub fn unsubscribe(event_types: &[EventType]) {
    object_to_stdout(&event_types);
    unsafe { host_unsubscribe() };
}

// Plugin Settings

pub fn set_selectable(selectable: bool) {
    unsafe { host_set_selectable(if selectable { 1 } else { 0 }) };
}

// Query Functions
pub fn get_plugin_ids() -> PluginIds {
    unsafe { host_get_plugin_ids() };
    object_from_stdin()
}

// Host Functions

pub fn open_file(path: &Path) {
Repository: zellij-org/zellij
File: zellij-tile/src/shim.rs (Rust)

Code before:
    unsafe { host_set_selectable(if selectable { 1 } else { 0 }) };
}

// Query Functions
pub fn get_plugin_ids() -> PluginIds {
    unsafe { host_get_plugin_ids() };
    object_from_stdin()
}

pub fn get_exec_permission() -> bool {
    unsafe { host_get_exec_permission() };
    object_from_stdin()
}

// Host Functions

pub fn open_file(path: &Path) {
    object_to_stdout(&path);
    unsafe { host_open_file() };
}

pub fn set_timeout(secs: f64) {
    unsafe { host_set_timeout(secs) };
}
pub fn exec_cmd(cmd: &[String]) -> Result<(), PermissionDenied> {
    if !get_exec_permission() {
        return Err(PermissionDenied);
    }
    object_to_stdout(&cmd);
    unsafe { host_exec_cmd() };
    Ok(())
}

// Internal Functions

#[doc(hidden)]
pub fn object_from_stdin<T: DeserializeOwned>() -> T {
    let mut json = String::new();
    io::stdin().read_line(&mut json).unwrap();
    serde_json::from_str(&json).unwrap()
}

#[doc(hidden)]
pub fn object_to_stdout(object: &impl Serialize) {
    println!("{}", serde_json::to_string(object).unwrap());
}

#[link(wasm_import_module = "zellij")]
extern "C" {
    fn host_subscribe();
    fn host_unsubscribe();

Review comment (suggestion):
No need to have this check here once the check has been moved inside of `host_exec_cmd()` in `wasm_vm.rs`!

Code after:
// Query Functions
pub fn get_plugin_ids() -> PluginIds {
    unsafe { host_get_plugin_ids() };
    object_from_stdin()
}

// Host Functions

pub fn open_file(path: &Path) {
    object_to_stdout(&path);
    unsafe { host_open_file() };
}

pub fn set_timeout(secs: f64) {
    unsafe { host_set_timeout(secs) };
}
pub fn exec_cmd(cmd: &[&str]) {
    object_to_stdout(&cmd);
    unsafe { host_exec_cmd() };
}

// Internal Functions

#[doc(hidden)]
pub fn object_from_stdin<T: DeserializeOwned>() -> T {
    let mut json = String::new();
    io::stdin().read_line(&mut json).unwrap();
    serde_json::from_str(&json).unwrap()
}

#[doc(hidden)]
pub fn object_to_stdout(object: &impl Serialize) {
    println!("{}", serde_json::to_string(object).unwrap());
}

#[link(wasm_import_module = "zellij")]
extern "C" {
    fn host_subscribe();
    fn host_unsubscribe();
    fn host_set_selectable(selectable: i32);
    fn host_get_plugin_ids();
    fn host_open_file();
    fn host_set_timeout(secs: f64);
    fn host_exec_cmd();
}
Repository: zellij-org/zellij
File: zellij-tile/src/shim.rs (Rust)

Code before:
    unsafe { host_unsubscribe() };
}

// Plugin Settings

pub fn set_selectable(selectable: bool) {
    unsafe { host_set_selectable(if selectable { 1 } else { 0 }) };
}

// Query Functions
pub fn get_plugin_ids() -> PluginIds {
    unsafe { host_get_plugin_ids() };
    object_from_stdin()
}

// Host Functions

pub fn open_file(path: &Path) {
    object_to_stdout(&path);
    unsafe { host_open_file() };
}

pub fn set_timeout(secs: f64) {
    unsafe { host_set_timeout(secs) };
}
pub fn exec_cmd(cmd: &[String]) {
    object_to_stdout(&cmd);
    unsafe { host_exec_cmd() };
}

// Internal Functions

#[doc(hidden)]
pub fn object_from_stdin<T: DeserializeOwned>() -> T {
    let mut json = String::new();
    io::stdin().read_line(&mut json).unwrap();
    serde_json::from_str(&json).unwrap()
}

#[doc(hidden)]
pub fn object_to_stdout(object: &impl Serialize) {
    println!("{}", serde_json::to_string(object).unwrap());
}

#[link(wasm_import_module = "zellij")]
extern "C" {
    fn host_subscribe();
    fn host_unsubscribe();
    fn host_set_selectable(selectable: i32);
    fn host_get_plugin_ids();
    fn host_open_file();

Review comment (suggestion):
Additionally, I might change this to take either `&[&str]` or maybe `&[impl AsRef<str> + Serialize]` if you really want to pass strings in directly.

Both solutions allow very simple calls like: 
```rs
exec_cmd(&["playerctl", "pause"]);
```

And the more complex type allows this as well, which you can decide whether you'll need or not:
```rs
let cmds: Vec<String> = vec!["playerctl".into(), "play".into()];
exec_cmd(&cmds);
```

Code after:
    unsafe { host_unsubscribe() };
}

// Plugin Settings

pub fn set_selectable(selectable: bool) {
    unsafe { host_set_selectable(if selectable { 1 } else { 0 }) };
}

// Query Functions
pub fn get_plugin_ids() -> PluginIds {
    unsafe { host_get_plugin_ids() };
    object_from_stdin()
}

// Host Functions

pub fn open_file(path: &Path) {
    object_to_stdout(&path);
    unsafe { host_open_file() };
}

pub fn set_timeout(secs: f64) {
    unsafe { host_set_timeout(secs) };
}
pub fn exec_cmd(cmd: &[&str]) {
    object_to_stdout(&cmd);
    unsafe { host_exec_cmd() };
}

// Internal Functions

#[doc(hidden)]
pub fn object_from_stdin<T: DeserializeOwned>() -> T {
    let mut json = String::new();
    io::stdin().read_line(&mut json).unwrap();
    serde_json::from_str(&json).unwrap()
}

#[doc(hidden)]
pub fn object_to_stdout(object: &impl Serialize) {
    println!("{}", serde_json::to_string(object).unwrap());
}

#[link(wasm_import_module = "zellij")]
extern "C" {
    fn host_subscribe();
    fn host_unsubscribe();
    fn host_set_selectable(selectable: i32);
    fn host_get_plugin_ids();
    fn host_open_file();
Repository: zellij-org/zellij
File: zellij-server/src/ui/overlay/mod.rs (Rust)

Code before:
                .generate_overlay(size)
                .context("failed to generate overlay for overlay window")?;
            output.push_str(&vte_output);
        }
        Ok(output)
    }
}

impl Overlay {
    pub fn prompt_confirm(self) -> Option<Box<ServerInstruction>> {
        match self.overlay_type {
            OverlayType::Prompt(p) => p.confirm(),
        }
    }
    pub fn prompt_deny(self) -> Option<Box<ServerInstruction>> {
        match self.overlay_type {
            OverlayType::Prompt(p) => p.deny(),
        }
    }
}

impl Overlayable for Overlay {
    fn generate_overlay(&self, size: Size) -> Result<String> {
        self.overlay_type
            .generate_overlay(size)
            .context("failed to generate overlay for overlay")
    }
}

impl Overlay {
    pub fn new(overlay_type: OverlayType) -> Self {
        Self { overlay_type }
    }

    fn pad_cols(output: &mut String, cols: usize) {
        if let Some(padding) = cols.checked_sub(output.len()) {
            for _ in 0..padding {
                output.push(' ');
            }
        }
    }
}


Review comment (bug):
```suggestion
            .context("failed to generate VTE output from overlay")
```

Code after:
                .generate_overlay(size)
                .context("failed to generate VTE output from overlay window")?;
            output.push_str(&vte_output);
        }
        Ok(output)
    }
}

impl Overlay {
    pub fn prompt_confirm(self) -> Option<Box<ServerInstruction>> {
        match self.overlay_type {
            OverlayType::Prompt(p) => p.confirm(),
        }
    }
    pub fn prompt_deny(self) -> Option<Box<ServerInstruction>> {
        match self.overlay_type {
            OverlayType::Prompt(p) => p.deny(),
        }
    }
}

impl Overlayable for Overlay {
    fn generate_overlay(&self, size: Size) -> Result<String> {
        self.overlay_type
            .generate_overlay(size)
            .context("failed to generate VTE output from overlay")
    }
}

impl Overlay {
    pub fn new(overlay_type: OverlayType) -> Self {
        Self { overlay_type }
    }

    fn pad_cols(output: &mut String, cols: usize) {
        if let Some(padding) = cols.checked_sub(output.len()) {
            for _ in 0..padding {
                output.push(' ');
            }
        }
    }
}
Repository: zellij-org/zellij
File: zellij-server/src/ui/overlay/prompt.rs (Rust)

Code before:
            on_deny,
        }
    }
    pub fn confirm(self) -> Option<Box<ServerInstruction>> {
        self.on_confirm
    }
    pub fn deny(self) -> Option<Box<ServerInstruction>> {
        self.on_deny
    }
}

impl Overlayable for Prompt {
    fn generate_overlay(&self, size: Size) -> Result<String> {
        let mut output = String::new();
        let rows = size.rows;
        let mut vte_output = self.message.clone();
        Overlay::pad_cols(&mut vte_output, size.cols);
        for (x, h) in vte_output.chars().enumerate() {
            write!(
                &mut output,
                "\u{1b}[{};{}H\u{1b}[48;5;238m{}",
                rows,
                x + 1,
                h,
            )
            .context("failed to generate overlay for prompt")?;
        }
        Ok(output)
    }
}

pub fn _generate_quit_prompt(client_id: ClientId) -> Overlay {
    let prompt = Prompt::new(
        (" Do you want to quit zellij? [Y]es / [N]o").to_string(),
        Some(Box::new(ServerInstruction::ClientExit(client_id))),
        None,
    );
    Overlay {
        overlay_type: OverlayType::Prompt(prompt),
    }
}


Review comment (bug):
```suggestion
            .context("failed to generate VTE output for prompt")?;
```

Code after:
            on_deny,
        }
    }
    pub fn confirm(self) -> Option<Box<ServerInstruction>> {
        self.on_confirm
    }
    pub fn deny(self) -> Option<Box<ServerInstruction>> {
        self.on_deny
    }
}

impl Overlayable for Prompt {
    fn generate_overlay(&self, size: Size) -> Result<String> {
        let mut output = String::new();
        let rows = size.rows;
        let mut vte_output = self.message.clone();
        Overlay::pad_cols(&mut vte_output, size.cols);
        for (x, h) in vte_output.chars().enumerate() {
            write!(
                &mut output,
                "\u{1b}[{};{}H\u{1b}[48;5;238m{}",
                rows,
                x + 1,
                h,
            )
            .context("failed to generate VTE output from prompt")?;
        }
        Ok(output)
    }
}

pub fn _generate_quit_prompt(client_id: ClientId) -> Overlay {
    let prompt = Prompt::new(
        (" Do you want to quit zellij? [Y]es / [N]o").to_string(),
        Some(Box::new(ServerInstruction::ClientExit(client_id))),
        None,
    );
    Overlay {
        overlay_type: OverlayType::Prompt(prompt),
    }
}
Repository: zellij-org/zellij
File: zellij-client/src/cli_client.rs (Rust)

Code before:
use crate::os_input_output::ClientOsApi;
use zellij_utils::{
    input::actions::Action,
    ipc::{ClientToServerMsg, ServerToClientMsg},
};

pub fn start_cli_client(os_input: Box<dyn ClientOsApi>, session_name: &str, actions: Vec<Action>) {
    let zellij_ipc_pipe: PathBuf = {
        let mut sock_dir = zellij_utils::consts::ZELLIJ_SOCK_DIR.clone();
        fs::create_dir_all(&sock_dir).unwrap();
        zellij_utils::shared::set_permissions(&sock_dir, 0o700).unwrap();
        sock_dir.push(session_name);
        sock_dir
    };
    os_input.connect_to_server(&*zellij_ipc_pipe);
    for action in actions {
        let msg = ClientToServerMsg::Action(action, None);
        os_input.send_to_server(msg);
    }
    loop {
        match os_input.recv_from_server() {
            Some((ServerToClientMsg::UnblockInputThread, _)) => {
                os_input.send_to_server(ClientToServerMsg::ClientExited);
                process::exit(0);
            },
            Some((ServerToClientMsg::TabNameList(tab_names), _)) => {
                tab_names.iter().for_each(|name| println!("{name}"));
                process::exit(0);
            },
            _ => {},
        }
    }
}


Review comment (suggestion):
Hey @naosense - let's change `ServerToClientMsg::TabNameList` to `ServerToClientMsg::Log`. In `cli_client` it will do exactly what it does here, and in the normal client it will log to file (with `log::info`).

Code after:
use crate::os_input_output::ClientOsApi;
use zellij_utils::{
    input::actions::Action,
    ipc::{ClientToServerMsg, ServerToClientMsg},
};

pub fn start_cli_client(os_input: Box<dyn ClientOsApi>, session_name: &str, actions: Vec<Action>) {
    let zellij_ipc_pipe: PathBuf = {
        let mut sock_dir = zellij_utils::consts::ZELLIJ_SOCK_DIR.clone();
        fs::create_dir_all(&sock_dir).unwrap();
        zellij_utils::shared::set_permissions(&sock_dir, 0o700).unwrap();
        sock_dir.push(session_name);
        sock_dir
    };
    os_input.connect_to_server(&*zellij_ipc_pipe);
    for action in actions {
        let msg = ClientToServerMsg::Action(action, None);
        os_input.send_to_server(msg);
    }
    loop {
        match os_input.recv_from_server() {
            Some((ServerToClientMsg::UnblockInputThread, _)) => {
                os_input.send_to_server(ClientToServerMsg::ClientExited);
                process::exit(0);
            },
            Some((ServerToClientMsg::Log(log_lines), _)) => {
                log_lines.iter().for_each(|line| println!("{line}"));
                process::exit(0);
            },
            _ => {},
        }
    }
}