add syntax highlighting for blame view (#745) #1914

tdtrung17693 · 2023-10-03T17:02:25Z

This Pull Request fixes/closes #745.

It changes the following:

Add syntax highlighting for blame view

I followed the checklist:

I added unittests
I ran make check without errors
I tested the overall application
I added an appropriate item to the changelog

tdtrung17693 · 2023-10-13T06:37:01Z

Hi @extrawurst, could you take a look into this PR? If there is anything need to change please let me know. Thank you!

src/app.rs

extrawurst · 2023-10-16T15:21:45Z

src/components/blame_file.rs

+struct SyntaxFileBlame {
+	///
+	pub commit_id: CommitId,
+	///


why some empty comments and some without? consistency please

src/components/blame_file.rs

extrawurst · 2023-10-16T15:23:44Z

src/components/blame_file.rs

@@ -295,6 +315,10 @@ impl BlameFileComponent {
 			table_state: std::cell::Cell::new(TableState::default()),
 			key_config,
 			current_height: std::cell::Cell::new(0),
+			async_highlighting: AsyncSingleJob::new(


i would argue we should put it in an option and only keep it around while there is an async job ongoing and initialize it to None first

As I see in the codebase, this is currently the way that other components instantiate the async jobs. Also, keeping this way makes it easier to process the job's result, instead of multi-level pattern-matching.

in order to keep things maintainable i started using this pattern successfully in Revlog with LogSearch.
in our case here we can combine probably 4 member (async_blame, syntax_progress, file_blame and async_highlighting) variables into an enum depending on our state and it makes it easier to reason which ones are expected to be valid at what state.

Looks great. Let me try it.

What's the status here? Would be great to carry over the finish line!

Will put it in draft till reworked

I refactored it and combined all those 4 processes into the one below just as you suggested.

gitui/src/components/blame_file.rs

Lines 57 to 64 in 1faaf4a

enum BlameProcess {

GettingBlame(AsyncBlame),

SyntaxHighlighting {

unstyled_file_blame: SyntaxFileBlame,

job: AsyncSingleJob<AsyncSyntaxJob>,

},

Result(SyntaxFileBlame),

}

gitui/src/components/blame_file.rs

Lines 86 to 98 in 1faaf4a

pub struct BlameFileComponent {

title: String,

theme: SharedTheme,

queue: Queue,

visible: bool,

open_request: Option<BlameFileOpen>,

params: Option<BlameParams>,

table_state: std::cell::Cell<TableState>,

key_config: SharedKeyConfig,

current_height: std::cell::Cell<usize>,

blame: BlameProcess,

app_sender: Sender<AsyncAppNotification>,

}

extrawurst · 2023-10-16T15:26:12Z

src/components/blame_file.rs

+			match progress {
+				SyntaxHighlightProgress::Progress => {
+					self.syntax_progress =
+						self.async_highlighting.progress();


i have to double check but i think the progress retrieval is pretty lightweight, no need to cache it

extrawurst · 2023-10-16T15:26:34Z

src/components/blame_file.rs

+									file_blame.styled_text =
+										Some(syntax);
+								}
+								// *(self.file_blame.s = Either::Left(syntax);


please clean up not needed comments

extrawurst · 2023-10-16T15:27:41Z

src/components/blame_file.rs

 		self.file_blame
 			.as_ref()
 			.map_or_else(Vec::new, |file_blame| {
-				file_blame
+				return file_blame


please keep it consistent with the non-return call style, last statement without ; is the return value

extrawurst · 2023-10-16T15:29:25Z

src/components/blame_file.rs

+			text.clone(),
+			params.file_path.clone(),
+		));
+		self.async_highlighting.spawn(AsyncSyntaxJob::new(


why spawn the same job twice?

extrawurst · 2023-10-18T06:53:52Z

src/components/blame_file.rs


 static NO_COMMIT_ID: &str = "0000000";
 static NO_AUTHOR: &str = "<no author>";
 static MIN_AUTHOR_WIDTH: usize = 3;
 static MAX_AUTHOR_WIDTH: usize = 20;

+struct SyntaxFileBlame {
+	pub commit_id: CommitId,


these elements (commit_id, path and lines) are FileBlame please reuse that component

extrawurst · 2023-10-18T07:04:16Z

src/components/blame_file.rs

@@ -295,6 +315,10 @@ impl BlameFileComponent {
 			table_state: std::cell::Cell::new(TableState::default()),
 			key_config,
 			current_height: std::cell::Cell::new(0),
+			async_highlighting: AsyncSingleJob::new(


in order to keep things maintainable i started using this pattern successfully in Revlog with LogSearch.
in our case here we can combine probably 4 member (async_blame, syntax_progress, file_blame and async_highlighting) variables into an enum depending on our state and it makes it easier to reason which ones are expected to be valid at what state.

extrawurst

Please update

extrawurst

looks much better. please update to current master and then we can merge

extrawurst · 2024-02-19T16:15:14Z

@tdtrung17693 thank you so much!

tdtrung17693 force-pushed the feat/feat_745 branch from 2a47110 to 243d7a5 Compare October 4, 2023 04:43

extrawurst requested changes Oct 16, 2023

View reviewed changes

tdtrung17693 force-pushed the feat/feat_745 branch from 243d7a5 to 045b204 Compare October 17, 2023 15:37

tdtrung17693 requested a review from extrawurst October 17, 2023 15:52

extrawurst requested changes Oct 18, 2023

View reviewed changes

tdtrung17693 force-pushed the feat/feat_745 branch from 045b204 to e5a711f Compare October 19, 2023 16:53

tdtrung17693 requested a review from extrawurst October 19, 2023 16:58

extrawurst added this to the v0.25 milestone Dec 20, 2023

extrawurst marked this pull request as draft December 25, 2023 20:47

extrawurst requested changes Dec 25, 2023

View reviewed changes

tdtrung17693 force-pushed the feat/feat_745 branch from e5a711f to 1faaf4a Compare December 31, 2023 17:43

tdtrung17693 marked this pull request as ready for review January 1, 2024 04:36

tdtrung17693 requested a review from extrawurst January 1, 2024 05:50

extrawurst requested changes Feb 12, 2024

View reviewed changes

tdtrung17693 force-pushed the feat/feat_745 branch 2 times, most recently from a7999ef to 2bf6c45 Compare February 13, 2024 05:42

tdtrung17693 requested a review from extrawurst February 13, 2024 05:43

add syntax highlighting for blame view (extrawurst#745)

5c331ff

tdtrung17693 force-pushed the feat/feat_745 branch from 2bf6c45 to 5c331ff Compare February 19, 2024 12:59

extrawurst removed this from the v0.25 milestone Feb 19, 2024

extrawurst merged commit af9da95 into extrawurst:master Feb 19, 2024
17 of 18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add syntax highlighting for blame view (#745) #1914

add syntax highlighting for blame view (#745) #1914

tdtrung17693 commented Oct 3, 2023 •

edited

Loading

tdtrung17693 commented Oct 13, 2023

extrawurst Oct 16, 2023

extrawurst Oct 16, 2023

tdtrung17693 Oct 17, 2023

extrawurst Oct 18, 2023

tdtrung17693 Oct 18, 2023

extrawurst Dec 20, 2023

extrawurst Dec 25, 2023

tdtrung17693 Jan 1, 2024 •

edited

Loading

extrawurst Oct 16, 2023

extrawurst Oct 16, 2023

extrawurst Oct 16, 2023

extrawurst Oct 16, 2023

extrawurst Oct 18, 2023

extrawurst Oct 18, 2023

extrawurst left a comment

extrawurst left a comment

extrawurst commented Feb 19, 2024

	enum BlameProcess {
	GettingBlame(AsyncBlame),
	SyntaxHighlighting {
	unstyled_file_blame: SyntaxFileBlame,
	job: AsyncSingleJob<AsyncSyntaxJob>,
	},
	Result(SyntaxFileBlame),
	}

	pub struct BlameFileComponent {
	title: String,
	theme: SharedTheme,
	queue: Queue,
	visible: bool,
	open_request: Option<BlameFileOpen>,
	params: Option<BlameParams>,
	table_state: std::cell::Cell<TableState>,
	key_config: SharedKeyConfig,
	current_height: std::cell::Cell<usize>,
	blame: BlameProcess,
	app_sender: Sender<AsyncAppNotification>,
	}

add syntax highlighting for blame view (#745) #1914

add syntax highlighting for blame view (#745) #1914

Conversation

tdtrung17693 commented Oct 3, 2023 • edited Loading

tdtrung17693 commented Oct 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdtrung17693 Jan 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

extrawurst left a comment

Choose a reason for hiding this comment

extrawurst left a comment

Choose a reason for hiding this comment

extrawurst commented Feb 19, 2024

tdtrung17693 commented Oct 3, 2023 •

edited

Loading

tdtrung17693 Jan 1, 2024 •

edited

Loading