Skip to content

Commit

Permalink
[WIP] feat: add reply w/ Reviewed-by to subset of patches
Browse files Browse the repository at this point in the history
Draft commit to implement the possibility to only reply to a subset of
patches from a patchset with the `Reviewed-by` tag. Previously, it was
only possible to reply to every patch in the series.

Now, hitting the lowercase `r` key results in the current patch being
(un)staged to be replied with the tag, whilst hitting the uppercase `R`
key results in all patches being (un)staged. Beside the button "[x]
reviewed-by" in the "Actions" tab there is a display of the number of
the patches that are staged in gray, e.g., if the patches 1, 2, and
7 are staged, the button will look like

```
[x] reviewed-by (1,2,7)
```

This commit maybe will need to broken in two or more commit, but will
probably need some refactoring. The user experience must be reviewed and
I can't say for sure if I am not complicating the implementation
unnecessarily, as I had to introduce considerable changes (maybe a bad
smell of the project quality...).

Closes: #78

Signed-off-by: David Tadokoro <[email protected]>
  • Loading branch information
davidbtadokoro committed Oct 31, 2024
1 parent 20d9c0e commit 176a60b
Show file tree
Hide file tree
Showing 6 changed files with 95 additions and 23 deletions.
10 changes: 7 additions & 3 deletions src/app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -135,16 +135,20 @@ impl App {

match log_on_error!(lore_session::split_patchset(&patchset_path)) {
Ok(patches) => {
let has_cover_letter = representative_patch.number_in_series() == 0;
let patches_to_reply = vec![false; patches.len()];
self.patchset_details_and_actions_state = Some(PatchsetDetailsAndActionsState {
representative_patch,
patches,
has_cover_letter,
patches_to_reply,
preview_index: 0,
preview_scroll_offset: 0,
preview_pan: 0,
preview_fullscreen: false,
patchset_actions: HashMap::from([
(PatchsetAction::Bookmark, is_patchset_bookmarked),
(PatchsetAction::ReplyWithReviewedBy, false),
(PatchsetAction::ReplyAllWithReviewedBy, false),
]),
last_screen: current_screen,
lore_api_client: self.lore_api_client.clone(),
Expand Down Expand Up @@ -184,7 +188,7 @@ impl App {
self.config.bookmarked_patchsets_path(),
)?;

if *actions.get(&PatchsetAction::ReplyWithReviewedBy).unwrap() {
if details_and_actions.patches_to_reply.contains(&true) {
let successful_indexes = details_and_actions
.reply_patchset_with_reviewed_by("all", self.config.git_send_email_options())?;

Expand All @@ -203,7 +207,7 @@ impl App {
self.patchset_details_and_actions_state
.as_mut()
.unwrap()
.toggle_action(PatchsetAction::ReplyWithReviewedBy);
.reset_reply_with_reviewed_by_action();
}

Ok(())
Expand Down
42 changes: 33 additions & 9 deletions src/app/screens/details_actions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@ use std::{collections::HashMap, path::Path, process::Command};
pub struct PatchsetDetailsAndActionsState {
pub representative_patch: Patch,
pub patches: Vec<String>,
/// Indicates if patchset has a cover letter
pub has_cover_letter: bool,
/// Which patches to reply
pub patches_to_reply: Vec<bool>,
pub preview_index: usize,
pub preview_scroll_offset: usize,
/// Horizontal offset
Expand All @@ -22,7 +26,7 @@ const LAST_LINE_PADDING: usize = 10;
#[derive(Hash, Eq, PartialEq)]
pub enum PatchsetAction {
Bookmark,
ReplyWithReviewedBy,
ReplyAllWithReviewedBy,
}

impl PatchsetDetailsAndActionsState {
Expand Down Expand Up @@ -96,8 +100,24 @@ impl PatchsetDetailsAndActionsState {
self.toggle_action(PatchsetAction::Bookmark);
}

pub fn toggle_reply_with_reviewed_by_action(&mut self) {
self.toggle_action(PatchsetAction::ReplyWithReviewedBy);
pub fn toggle_reply_with_reviewed_by_action(&mut self, all: bool) {
if all {
for entry in &self.patches_to_reply {
// If there is at least one patch not to be replied, set all to be
if !*entry {
self.patches_to_reply = vec![true; self.patches_to_reply.len()];
return;
}
}
// If all patches are set to be replied, set none to be
self.patches_to_reply = vec![false; self.patches_to_reply.len()];
} else if let Some(entry) = self.patches_to_reply.get_mut(self.preview_index) {
*entry = !*entry;
}
}

pub fn reset_reply_with_reviewed_by_action(&mut self) {
self.patches_to_reply = vec![false; self.patches_to_reply.len()];
}

pub fn toggle_action(&mut self, patchset_action: PatchsetAction) {
Expand All @@ -107,10 +127,7 @@ impl PatchsetDetailsAndActionsState {
}

pub fn actions_require_user_io(&self) -> bool {
*self
.patchset_actions
.get(&PatchsetAction::ReplyWithReviewedBy)
.unwrap()
self.patches_to_reply.contains(&true)
}

pub fn reply_patchset_with_reviewed_by(
Expand All @@ -134,6 +151,7 @@ impl PatchsetDetailsAndActionsState {
tmp_dir,
target_list,
&self.patches,
&self.patches_to_reply,
&format!("{git_user_name} <{git_user_email}>"),
git_send_email_options,
) {
Expand All @@ -143,11 +161,17 @@ impl PatchsetDetailsAndActionsState {
}
};

for (index, mut command) in git_reply_commands.into_iter().enumerate() {
let reply_indexes: Vec<usize> = self
.patches_to_reply
.iter()
.enumerate()
.filter_map(|(i, &val)| if val { Some(i) } else { None })
.collect();
for (i, mut command) in git_reply_commands.into_iter().enumerate() {
let mut child = command.spawn().unwrap();
let exit_status = child.wait().unwrap();
if exit_status.success() {
successful_indexes.push(index);
successful_indexes.push(reply_indexes[i]);
}
}

Expand Down
10 changes: 7 additions & 3 deletions src/handler/details_actions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,12 @@ pub fn handle_patchset_details<B: Backend>(
let patchset_details_and_actions = app.patchset_details_and_actions_state.as_mut().unwrap();

if key.modifiers.contains(KeyModifiers::SHIFT) {
if let KeyCode::Char('G') = key.code {
patchset_details_and_actions.go_to_last_line()
match key.code {
KeyCode::Char('G') => patchset_details_and_actions.go_to_last_line(),
KeyCode::Char('R') => {
patchset_details_and_actions.toggle_reply_with_reviewed_by_action(true);
}
_ => {}
}
return Ok(());
}
Expand Down Expand Up @@ -86,7 +90,7 @@ pub fn handle_patchset_details<B: Backend>(
patchset_details_and_actions.toggle_bookmark_action();
}
KeyCode::Char('r') => {
patchset_details_and_actions.toggle_reply_with_reviewed_by_action();
patchset_details_and_actions.toggle_reply_with_reviewed_by_action(false);
}
KeyCode::Enter => {
if patchset_details_and_actions.actions_require_user_io() {
Expand Down
7 changes: 6 additions & 1 deletion src/lore/lore_session.rs
Original file line number Diff line number Diff line change
Expand Up @@ -367,6 +367,7 @@ pub fn prepare_reply_patchset_with_reviewed_by<T>(
tmp_dir: &Path,
target_list: &str,
patches: &[String],
patches_to_reply: &[bool],
git_signature: &str,
git_send_email_options: &str,
) -> Result<Vec<Command>, LoreSessionError>
Expand All @@ -376,7 +377,11 @@ where
let mut git_reply_commands: Vec<Command> = Vec::new();
let re_message_id = Regex::new(r#"(?m)^Message-Id: <(.*?)>"#).unwrap();

for patch in patches.iter() {
for (i, patch) in patches.iter().enumerate() {
if !patches_to_reply[i] {
continue;
}

let message_id = re_message_id
.captures(patch)
.unwrap()
Expand Down
3 changes: 3 additions & 0 deletions src/lore/lore_session/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -534,11 +534,14 @@ fn should_prepare_reply_patchset_with_reviewed_by() {
.unwrap(),
];

let patches_to_reply = vec![true; patches.len()];

let git_reply_commands = prepare_reply_patchset_with_reviewed_by(
&lore_api_client,
tmp_dir,
target_list,
&patches,
&patches_to_reply,
"Bar Foo <[email protected]>",
"--dry-run --suppress-cc=all",
)
Expand Down
46 changes: 39 additions & 7 deletions src/ui/details_actions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,29 @@ fn render_details_and_actions(f: &mut Frame, app: &App, details_chunk: Rect, act

f.render_widget(patchset_details, details_chunk);

let mut patches_to_reply = String::new();
if patchset_details_and_actions
.patches_to_reply
.contains(&true)
{
patches_to_reply.push('(');
let number_offset = if patchset_details_and_actions.has_cover_letter {
0
} else {
1
};
let patches_to_reply_numbers: Vec<usize> = patchset_details_and_actions
.patches_to_reply
.iter()
.enumerate()
.filter_map(|(i, &val)| if val { Some(i + number_offset) } else { None })
.collect();
for number in patches_to_reply_numbers {
patches_to_reply.push_str(&format!("{number},"));
}
patches_to_reply = format!("{})", &patches_to_reply[..patches_to_reply.len() - 1]);
}

let patchset_actions = &patchset_details_and_actions.patchset_actions;
let patchset_actions = vec![
Line::from(vec![
Expand All @@ -83,8 +106,9 @@ fn render_details_and_actions(f: &mut Frame, app: &App, details_chunk: Rect, act
Span::styled("ookmark", Style::default().fg(Color::Cyan)),
]),
Line::from(vec![
if *patchset_actions
.get(&PatchsetAction::ReplyWithReviewedBy)
if *patchset_details_and_actions
.patches_to_reply
.get(patchset_details_and_actions.preview_index)
.unwrap()
{
Span::styled("[x] ", Style::default().fg(Color::Green))
Expand All @@ -98,7 +122,8 @@ fn render_details_and_actions(f: &mut Frame, app: &App, details_chunk: Rect, act
.add_modifier(Modifier::UNDERLINED)
.add_modifier(Modifier::BOLD),
),
Span::styled("eviewed-by", Style::default().fg(Color::Cyan)),
Span::styled("eviewed-by ", Style::default().fg(Color::Cyan)),
Span::styled(patches_to_reply, Style::default().fg(Color::DarkGray)),
]),
];
let patchset_actions = Paragraph::new(patchset_actions)
Expand All @@ -124,10 +149,17 @@ fn render_preview(f: &mut Frame, app: &App, chunk: Rect) {
.message_id()
.href;
let mut preview_title = String::from(" Preview ");
if let Some(successful_indexes) = app.reviewed_patchsets.get(representative_patch_message_id) {
if successful_indexes.contains(&preview_index) {
preview_title = " Preview [REVIEWED] ".to_string();
}
if matches!(
app.reviewed_patchsets.get(representative_patch_message_id),
Some(successful_indexes) if successful_indexes.contains(&preview_index)
) {
preview_title = " Preview [REVIEWED-BY] ".to_string();
} else if *patchset_details_and_actions
.patches_to_reply
.get(preview_index)
.unwrap()
{
preview_title = " Preview [REVIEWED-BY]* ".to_string();
};

let preview_offset = patchset_details_and_actions.preview_scroll_offset;
Expand Down

0 comments on commit 176a60b

Please sign in to comment.