fix: patch-hub crashes when loading patches after b4 fails #142
Conversation
Seeking to fix kworkflow#69, this commit introduces a check to the function init_details_actions. We hoped to stop the crash described in kworkflow#69 by throwing an Err and catching it in the handle_latest_patchsets fucntion before showing a popup to the user. For some still unkown reason, the TUI freezes and 'melts' after the popup is shown. Co-developed-by: Bruno Stephan <[email protected]> Signed-off-by: Bruno Stephan <[email protected]> Co-developed-by: Andre de Lima <[email protected]> Signed-off-by: Andre de Lima <[email protected]> Signed-off-by: Arthur Pilone <[email protected]>
|
@davidbtadokoro and @lorenzoberts we'd really appreciate it if you could check out this bug our partial "fix" introduces. It seems to have something to do with the rendering or with the thread management. Try to reproduce the original bug from #69 with the patch Do you guys have any info or what exactly we should look after in order to fix this? |
This commit fixes the previous one. The terminal was unresponsive because the macro received an Err. We created a new enum to test whether the init_details_action function found a patch in the expected path. This enum is used to test whether the file exists in the handle_latest_patchsets function still using an Ok(). Fixes: kworkflow#69 Co-developed-by: Bruno Stephan <[email protected]> Signed-off-by: Bruno Stephan <[email protected]> Co-developed-by: Andre de Lima <[email protected]> Signed-off-by: Andre de Lima <[email protected]> Signed-off-by: Arthur Pilone <[email protected]>
|
Fixed. |
davidbtadokoro
force-pushed
the
unstable
branch
2 times, most recently
from
167d77b to
deba389
Compare
| @@ -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}; | |||
There was a problem hiding this comment.
Unecessary braces. They should be used when importing different parts in the same package.
| use color_eyre::eyre::{bail}; | |
| use color_eyre::eyre::bail; |
| Ok(()) | ||
| } |
There was a problem hiding this comment.
fix formatting
| Ok(()) | |
| } | |
| Ok(()) | |
| } |
| 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); | ||
| } |
There was a problem hiding this comment.
fix formatting
| 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); | |
| } |
| pub enum PatchFound { | ||
| Found, | ||
| NotFound | ||
| } |
There was a problem hiding this comment.
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.
| pub enum PatchFound { | |
| Found, | |
| NotFound | |
| } | |
| pub enum B4Result { | |
| PatchFound(PathBuf), | |
| PatchNotFound, | |
| } |
There was a problem hiding this comment.
Hi guys! I'm just dropping in to add a few comments to compliment @auyer's review (thank you so much, BTW, Rafa). Nevertheless, I can't thank you guys enough for tackling this annoying and ghostly bug! Also, feel free to ping me in case of any doubt!
Number of Commits
Because a PR is still unmerged code, we have the opportunity to refine it and make it the most atomic (along with all the other good git practices stuff).
In this sense, I feel like the first commit goes in one direction (tries to fix the bug using Result), while the second goes in another direction (the "correct" one that works). So, you should make this PR a single commit by dropping the first and leaving just the second (it may need some adjustments).
If you don't know exactly how to do this, I recommend taking a look at this excellent section of the Git book about how you can manipulate a branch and "rewrite" history. Beyond this PR and even patch-hub, having this know-how is invaluable!
After rewriting the history locally, you need to force-push to the same remote branch, and GitHub will automatically update the PR.
Formatting and Linting
We have a formatter and a linter implemented in the project. To fix the problems pointed out by @auyer, you can use
cargo fmt --all
and
cargo lint --fix
for formatting and linting, respectively (I recommend running in this order). With the --fix option, the linter will try to detect and also fix rule violations, but it can "fail" to automatically solve one or another problem.
Optionally, you can use the pre-commit hook we have implemented to run both formatter and linter before every and each commit. For now, just locally and explicitly running both will suffice, IMO.
Seeking to fix #69, this first commit introduces a check to the function init_details_actions. We hoped to stop the crash described in #69 by throwing an Err and catching it in the handle_latest_patchsets fucntion before showing a popup to the user. For some still unkown reason, the TUI freezes and 'melts' after the popup is shown.
Co-developed-by: Bruno Stephan [email protected]
Co-developed-by: Andre de Lima [email protected]