Skip to content

fix: patch-hub crashes when loading patches after b4 fails #142

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: unstable
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 19 additions & 4 deletions src/app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use crate::{
ui::popup::{info_popup::InfoPopUp, PopUp},
};
use ansi_to_tui::IntoText;
use color_eyre::eyre::bail;
use color_eyre::eyre::{bail};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unecessary braces. They should be used when importing different parts in the same package.

Suggested change
use color_eyre::eyre::{bail};
use color_eyre::eyre::bail;

use config::Config;
use cover_renderer::render_cover;
use logging::Logger;
Expand All @@ -23,6 +23,8 @@ use screens::{
CurrentScreen,
};
use std::collections::{HashMap, HashSet};
use std::ffi::OsStr;
use std::path::Path;

use crate::utils;

Expand All @@ -32,6 +34,11 @@ pub mod logging;
pub mod patch_renderer;
pub mod screens;

pub enum PatchFound {
Found,
NotFound
}
Comment on lines +37 to +40
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this naming is a bit unidiomatic.
If you look at the usage, PatchFound::Found and PatchFound::NotFound is a bit "funny".

I would like to propose a change that goes a bit beyond just changing the name.
My suggestion is to name it B4Result, and the lore_session::download_patchset function should return it.
Doing this, the "file exists" check (see image) could also be moved inside the download_patchset function too, and the enum can carry the Path for us.
image

Suggested change
pub enum PatchFound {
Found,
NotFound
}
pub enum B4Result {
PatchFound(PathBuf),
PatchNotFound,
}


/// Type that represents the overall state of the application. It can be viewed
/// as the **Model** component of `patch-hub`.
pub struct App {
Expand Down Expand Up @@ -132,7 +139,7 @@ impl App {
/// Initializes field [App::details_actions], from currently selected
/// patchset in [App::bookmarked_patchsets] or [App::latest_patchsets],
/// depending on the value of [App::current_screen].
pub fn init_details_actions(&mut self) -> color_eyre::Result<()> {
pub fn init_details_actions(&mut self) -> color_eyre::Result<PatchFound> {
let representative_patch: Patch;
let mut is_patchset_bookmarked = true;
let mut reviewed_by = Vec::new();
Expand Down Expand Up @@ -164,7 +171,15 @@ impl App {
self.config.patchsets_cache_dir(),
&representative_patch,
)) {
Ok(result) => result,
Ok(result) => {
let path = Path::new(OsStr::new(&result));

if ! path.exists() {
return Ok(PatchFound::NotFound);
}

result
}
Err(io_error) => bail!("{io_error}"),
};

Expand Down Expand Up @@ -250,7 +265,7 @@ impl App {
lore_api_client: self.lore_api_client.clone(),
patchset_path,
});
Ok(())
Ok(PatchFound::Found)
}
Err(message) => bail!(message),
}
Expand Down
20 changes: 15 additions & 5 deletions src/handler/latest.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
use std::ops::ControlFlow;

use crate::{
app::{screens::CurrentScreen, App},
app::{screens::CurrentScreen, App, PatchFound},
loading_screen,
ui::popup::{help::HelpPopUpBuilder, PopUp},
ui::popup::{help::HelpPopUpBuilder, info_popup::InfoPopUp, PopUp},
};
use color_eyre::eyre::Ok;
use ratatui::{
crossterm::event::{KeyCode, KeyEvent},
prelude::Backend,
Expand Down Expand Up @@ -55,10 +56,19 @@ where
"Loading patchset" => {
let result = app.init_details_actions();
if result.is_ok() {
app.set_current_screen(CurrentScreen::PatchsetDetails);
match result.unwrap() {
PatchFound::Found => {
app.set_current_screen(CurrentScreen::PatchsetDetails);
}

PatchFound::NotFound => {
app.popup = Some(InfoPopUp::generate_info_popup("Error","The selected patchset couldn't be retrieved.\nPlease choose another patchset."));
app.set_current_screen(CurrentScreen::LatestPatchsets);
}
Comment on lines +65 to +67
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix formatting

Suggested change
app.popup = Some(InfoPopUp::generate_info_popup("Error","The selected patchset couldn't be retrieved.\nPlease choose another patchset."));
app.set_current_screen(CurrentScreen::LatestPatchsets);
}
app.popup = Some(InfoPopUp::generate_info_popup("Error","The selected patchset couldn't be retrieved.\nPlease choose another patchset."));
app.set_current_screen(CurrentScreen::LatestPatchsets);
}

}
}
result
}
Ok(())
}
Comment on lines +70 to +71
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix formatting

Suggested change
Ok(())
}
Ok(())
}

};
}
_ => {}
Expand Down
Loading