-
Notifications
You must be signed in to change notification settings - Fork 9
Feature Request: Keyboard-first text selection & improved annotations #18 #21
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
base: main
Are you sure you want to change the base?
Changes from all commits
1059d34
a8a9971
061d657
6cc088a
a4ddf6c
6a8d9cf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| { | ||
| "permissions": { | ||
| "allow": [ | ||
| "Bash(cargo fmt:*)", | ||
| "Bash(cargo build:*)", | ||
| "Bash(rustup default:*)", | ||
| "Bash(cargo check:*)" | ||
| ] | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,3 +15,4 @@ temp_images/ | |
| tmp/ | ||
| logo.inspirations/ | ||
| flamegraph.svg | ||
| .claude/ | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -68,6 +68,7 @@ pub struct Comment { | |
| pub chapter_href: String, | ||
| pub target: CommentTarget, | ||
| pub content: String, | ||
| pub selected_text: Option<String>, // The text that was highlighted/selected | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This stores the annotated text, but we can always derive it from the word range and the book content at render time. I don't this is really needed. this can always be derived from original epub. |
||
| pub updated_at: DateTime<Utc>, | ||
| } | ||
|
|
||
|
|
@@ -77,6 +78,8 @@ struct CommentModernSerde { | |
| #[serde(flatten)] | ||
| pub target: CommentTarget, | ||
| pub content: String, | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| pub selected_text: Option<String>, | ||
| pub updated_at: DateTime<Utc>, | ||
| } | ||
|
|
||
|
|
@@ -88,6 +91,8 @@ struct CommentLegacySerde { | |
| #[serde(default)] | ||
| pub word_range: Option<(usize, usize)>, | ||
| pub content: String, | ||
| #[serde(default)] | ||
| pub selected_text: Option<String>, | ||
| pub updated_at: DateTime<Utc>, | ||
| } | ||
|
|
||
|
|
@@ -107,6 +112,7 @@ impl From<CommentLegacySerde> for Comment { | |
| word_range: legacy.word_range, | ||
| }, | ||
| content: legacy.content, | ||
| selected_text: legacy.selected_text, | ||
| updated_at: legacy.updated_at, | ||
| } | ||
| } | ||
|
|
@@ -118,6 +124,7 @@ impl From<CommentModernSerde> for Comment { | |
| chapter_href: modern.chapter_href, | ||
| target: modern.target, | ||
| content: modern.content, | ||
| selected_text: modern.selected_text, | ||
| updated_at: modern.updated_at, | ||
| } | ||
| } | ||
|
|
@@ -129,6 +136,7 @@ impl From<&Comment> for CommentModernSerde { | |
| chapter_href: comment.chapter_href.clone(), | ||
| target: comment.target.clone(), | ||
| content: comment.content.clone(), | ||
| selected_text: comment.selected_text.clone(), | ||
| updated_at: comment.updated_at, | ||
| } | ||
| } | ||
|
|
@@ -401,6 +409,7 @@ mod tests { | |
| word_range: None, | ||
| }, | ||
| content: content.to_string(), | ||
| selected_text: None, | ||
| updated_at: Utc::now(), | ||
| } | ||
| } | ||
|
|
@@ -418,6 +427,7 @@ mod tests { | |
| line_range, | ||
| }, | ||
| content: content.to_string(), | ||
| selected_text: None, | ||
| updated_at: Utc::now(), | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2107,15 +2107,16 @@ impl App { | |
| } | ||
| _ => "Search mode active".to_string(), | ||
| } | ||
| } else if self.text_reader.has_text_selection() { | ||
| } else if self.text_reader.has_text_selection() || self.text_reader.is_visual_mode_active() | ||
| { | ||
| "a: Add comment | c/Ctrl+C: Copy to clipboard | ESC: Clear selection".to_string() | ||
| } else { | ||
| let help_text = match self.focused_panel { | ||
| FocusedPanel::Main(MainPanel::NavigationList) => { | ||
| "j/k: Navigate | Enter: Select | h/l: Fold/Unfold | H/L: Fold/Unfold All | Tab: Switch | q: Quit" | ||
| } | ||
| FocusedPanel::Main(MainPanel::Content) => { | ||
| "j/k: Scroll | h/l: Chapter | Ctrl+d/u: Half-screen | Tab: Switch | Space+o: Open | q: Quit" | ||
| "j/k: Scroll | h/l: Chapter | Ctrl+d/u: Half-screen | v/V: Visual Mode | Tab: Switch | Space+o: Open | q: Quit" | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. lets not add this :) this will ruin a lot test for no good reason |
||
| } | ||
| FocusedPanel::Popup(PopupWindow::ReadingHistory) => { | ||
| "j/k/Scroll: Navigate | Enter/DblClick: Open | ESC: Close" | ||
|
|
@@ -3119,6 +3120,22 @@ impl App { | |
| let visual_mode = self.text_reader.get_visual_mode(); | ||
|
|
||
| match key.code { | ||
| KeyCode::Char('a') => { | ||
| let success = self.text_reader.convert_visual_to_text_selection() | ||
| && self.text_reader.start_comment_input(); | ||
|
|
||
| if success { | ||
| debug!("Started comment input mode from visual selection"); | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. consider removing useless logging. this doesn't help in debugging that much |
||
| } else { | ||
| debug!("Failed to start comment input from visual selection"); | ||
| self.notifications | ||
| .show_warning("Cannot annotate this selection"); | ||
| } | ||
|
|
||
| // Always exit visual mode when 'a' is pressed | ||
| self.text_reader.exit_visual_mode(); | ||
| return None; | ||
| } | ||
| KeyCode::Char('y') => { | ||
| if let Some(text) = self.text_reader.yank_visual_selection() { | ||
| let _ = self.text_reader.copy_to_clipboard(text); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -49,6 +49,9 @@ pub struct Settings { | |
| #[serde(default)] | ||
| pub margin: u16, | ||
|
|
||
| #[serde(default = "default_annotation_highlight_color")] | ||
| pub annotation_highlight_color: String, | ||
|
|
||
| #[serde(default, skip_serializing_if = "Vec::is_empty")] | ||
| pub custom_themes: Vec<YamlTheme>, | ||
| } | ||
|
|
@@ -61,12 +64,17 @@ fn default_theme() -> String { | |
| "Oceanic Next".to_string() | ||
| } | ||
|
|
||
| fn default_annotation_highlight_color() -> String { | ||
| "7FB4CA".to_string() // Cyan (base0C) from Kanagawa theme | ||
| } | ||
|
|
||
|
Comment on lines
+67
to
+70
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This introduces a separate color configuration that bypasses the existing Base16Palette theme system. Could we use a color from the palette instead? That way it would automatically work with all themes. the current approach looks really harsh in most of predefined themes. -- Also i think it would make it more visually neutral (and easier to conform to themes) to use underline for the text being annotated. |
||
| impl Default for Settings { | ||
| fn default() -> Self { | ||
| Self { | ||
| version: CURRENT_VERSION, | ||
| theme: default_theme(), | ||
| margin: 0, | ||
| annotation_highlight_color: default_annotation_highlight_color(), | ||
| custom_themes: Vec::new(), | ||
| } | ||
| } | ||
|
|
@@ -161,6 +169,15 @@ fn generate_settings_yaml(settings: &Settings) -> String { | |
| content.push_str(&format!("theme: \"{}\"\n", settings.theme)); | ||
| content.push_str(&format!("margin: {}\n", settings.margin)); | ||
| content.push('\n'); | ||
| content.push_str( | ||
| "# Annotation highlight color (hex color without #, e.g., \"7FB4CA\" for cyan)\n", | ||
| ); | ||
| content.push_str("# Set to \"none\" or \"disabled\" to disable highlighting\n"); | ||
| content.push_str(&format!( | ||
| "annotation_highlight_color: \"{}\"\n", | ||
| settings.annotation_highlight_color | ||
| )); | ||
| content.push('\n'); | ||
|
|
||
| content.push_str(CUSTOM_THEMES_TEMPLATE); | ||
|
|
||
|
|
@@ -257,3 +274,10 @@ pub fn get_custom_themes() -> Vec<YamlTheme> { | |
| .map(|s| s.custom_themes.clone()) | ||
| .unwrap_or_default() | ||
| } | ||
|
|
||
| pub fn get_annotation_highlight_color() -> String { | ||
| SETTINGS | ||
| .read() | ||
| .map(|s| s.annotation_highlight_color.clone()) | ||
| .unwrap_or_else(|_| default_annotation_highlight_color()) | ||
| } | ||

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't think this needs to be committed.