Skip to content
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

Add Go to line feature for the blame view #2262

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open
Changes from 1 commit
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
Prev Previous commit
Next Next commit
Load blame from the last blame file instead of context
Remove context and references to it

Addres #2262 (comment)

Address #2262 (comment)
andrea-berling authored and extrawurst committed Sep 17, 2024
commit 20ed1d213dc7d6f1769ad833344c10006d14f135
17 changes: 7 additions & 10 deletions src/app.rs
Original file line number Diff line number Diff line change
@@ -21,7 +21,7 @@ use crate::{
TagListPopup,
},
queue::{
Action, AppTabs, Context, InternalEvent, NeedsUpdate, Queue,
Action, AppTabs, InternalEvent, NeedsUpdate, Queue,
StackablePopupOpen,
},
setup_popups,
@@ -675,8 +675,8 @@ impl App {
StackablePopupOpen::CompareCommits(param) => {
self.compare_commits_popup.open(param)?;
}
StackablePopupOpen::GotoLine(context) => {
self.goto_line_popup.open(Some(context));
StackablePopupOpen::GotoLine => {
self.goto_line_popup.open();
}
}

@@ -880,21 +880,18 @@ impl App {
InternalEvent::CommitSearch(options) => {
self.revlog.search(options);
}
InternalEvent::GotoLine(line, context) => {
InternalEvent::GotoLine(line) => {
if let Some(popup) = self.popup_stack.pop() {
if let StackablePopupOpen::BlameFile(params) =
popup
{
let blame =
self.blame_file_popup.blame.clone();
self.popup_stack.push(
StackablePopupOpen::BlameFile(
BlameFileOpen {
selection: Some(line),
blame: context.and_then(|ctx| {
let Context::Blame(
blame_process,
) = ctx;
blame_process
}),
blame,
..params
},
),
11 changes: 5 additions & 6 deletions src/popups/blame_file.rs
Original file line number Diff line number Diff line change
@@ -7,7 +7,7 @@ use crate::{
},
keys::{key_match, SharedKeyConfig},
popups::{FileRevOpen, InspectCommitOpen},
queue::{Context, InternalEvent, Queue, StackablePopupOpen},
queue::{InternalEvent, Queue, StackablePopupOpen},
string_utils::tabs_to_spaces,
strings,
ui::{self, style::SharedTheme, AsyncSyntaxJob, SyntaxText},
@@ -96,7 +96,7 @@ pub struct BlameFilePopup {
table_state: std::cell::Cell<TableState>,
key_config: SharedKeyConfig,
current_height: std::cell::Cell<usize>,
blame: Option<BlameProcess>,
pub blame: Option<BlameProcess>,
app_sender: Sender<AsyncAppNotification>,
git_sender: Sender<AsyncGitNotification>,
repo: RepoPathRef,
@@ -327,9 +327,7 @@ impl Component for BlameFilePopup {
self.hide_stacked(true);
self.visible = true;
self.queue.push(InternalEvent::OpenPopup(
StackablePopupOpen::GotoLine(Context::Blame(
self.blame.clone(),
)),
StackablePopupOpen::GotoLine,
Copy link
Collaborator

Choose a reason for hiding this comment

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

here we can provide the context: Blame, MaxLine

));
}

@@ -751,7 +749,8 @@ impl BlameFilePopup {
{
let mut table_state = self.table_state.take();
let max_line_number = self.get_max_line_number();
table_state.select(Some(selection.min(max_line_number)));
table_state
.select(Some(selection.clamp(0, max_line_number)));
self.table_state.set(table_state);
}
}
10 changes: 3 additions & 7 deletions src/popups/goto_line.rs
Original file line number Diff line number Diff line change
@@ -5,7 +5,7 @@ use crate::{
DrawableComponent, EventState,
},
keys::{key_match, SharedKeyConfig},
queue::{Context, InternalEvent, Queue},
queue::{InternalEvent, Queue},
ui::{self, style::SharedTheme},
};

@@ -25,7 +25,6 @@ pub struct GotoLinePopup {
key_config: SharedKeyConfig,
queue: Queue,
theme: SharedTheme,
context: Option<Context>,
}

impl GotoLinePopup {
@@ -36,13 +35,11 @@ impl GotoLinePopup {
key_config: env.key_config.clone(),
queue: env.queue.clone(),
theme: env.theme.clone(),
context: None,
}
}

pub fn open(&mut self, context: Option<Context>) {
pub fn open(&mut self) {
self.visible = true;
self.context = context;
}
}

@@ -82,8 +79,7 @@ impl Component for GotoLinePopup {
self.visible = false;
if !self.line.is_empty() {
self.queue.push(InternalEvent::GotoLine(
self.line.parse::<usize>().expect("This shouldn't happen since the input is constrained to ascii digits only"),
self.context.clone()
self.line.parse::<usize>()?,
));
}
self.queue.push(InternalEvent::PopupStackPop);
2 changes: 1 addition & 1 deletion src/popups/mod.rs
Original file line number Diff line number Diff line change
@@ -25,7 +25,7 @@ mod submodules;
mod tag_commit;
mod taglist;

pub use blame_file::{BlameFileOpen, BlameFilePopup, BlameProcess};
pub use blame_file::{BlameFileOpen, BlameFilePopup};
pub use branchlist::BranchListPopup;
pub use commit::CommitPopup;
pub use compare_commits::CompareCommitsPopup;
15 changes: 4 additions & 11 deletions src/queue.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
use crate::{
components::FuzzyFinderTarget,
popups::{
AppOption, BlameFileOpen, BlameProcess, FileRevOpen,
FileTreeOpen, InspectCommitOpen,
AppOption, BlameFileOpen, FileRevOpen, FileTreeOpen,
InspectCommitOpen,
},
tabs::StashingOptions,
};
@@ -56,13 +56,6 @@ pub enum Action {
UndoCommit,
}

#[derive(Debug, Clone)]
pub enum Context {
Blame(Option<BlameProcess>),
//FileView,
//PossibleRange(u32, u32),
}

#[derive(Debug)]
pub enum StackablePopupOpen {
///
@@ -76,7 +69,7 @@ pub enum StackablePopupOpen {
///
CompareCommits(InspectCommitOpen),
///
GotoLine(Context),
GotoLine,
Copy link
Collaborator

Choose a reason for hiding this comment

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

i think its a bit of a missed opportunity to not pass it some context: enum Context {Blame, FileView} and PossibleRange(min,max) to be able to reuse this in various places

Copy link
Author

Choose a reason for hiding this comment

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

In my last commit I added the Context enum and used it to preserve the BlameProcess status across line jumps, and I added the two variants you mentioned in line comments. That's because cargo wouldn't let me compile if I didn't use those variants, which I didn't. Do you have a specific example in mind where those pieces of information could be used? If so, I could use them to implement those use cases as to make them more interesting, otherwise I can leave the comments around as placeholder for future functionality 🙂

}

pub enum AppTabs {
@@ -156,7 +149,7 @@ pub enum InternalEvent {
///
CommitSearch(LogFilterSearchOptions),
///
GotoLine(usize, Option<Context>),
GotoLine(usize),
}

/// single threaded simple queue for components to communicate with each other