From 3159cf4ab6f7129bd6e885e047e722170f2baa7c Mon Sep 17 00:00:00 2001 From: abdo Date: Tue, 19 Jan 2021 02:29:09 +0300 Subject: [PATCH 1/2] Make tags match their parents case https://github.com/ankitects/anki/pull/900/#issuecomment-762018745 --- rslib/src/storage/tag/mod.rs | 46 ++++++++++++++++++++++++++++++++---- rslib/src/tags.rs | 18 ++++++++++---- 2 files changed, 54 insertions(+), 10 deletions(-) diff --git a/rslib/src/storage/tag/mod.rs b/rslib/src/storage/tag/mod.rs index e0f1b0bd4..1f00268ab 100644 --- a/rslib/src/storage/tag/mod.rs +++ b/rslib/src/storage/tag/mod.rs @@ -2,7 +2,8 @@ // License: GNU AGPL, version 3 or later; http://www.gnu.org/licenses/agpl.html use super::SqliteStorage; -use crate::{err::Result, tags::Tag, types::Usn}; +use crate::err::{AnkiError, Result}; +use crate::{tags::Tag, types::Usn}; use rusqlite::{params, Row, NO_PARAMS}; use std::collections::HashMap; @@ -15,6 +16,10 @@ fn row_to_tag(row: &Row) -> Result { }) } +fn immediate_parent_name(tag_name: &str) -> Option<&str> { + tag_name.rsplitn(2, "::").nth(1) +} + impl SqliteStorage { /// All tags in the collection, in alphabetical order. pub(crate) fn all_tags(&self) -> Result> { @@ -56,12 +61,43 @@ impl SqliteStorage { Ok(()) } - pub(crate) fn preferred_tag_case(&self, tag: &str) -> Result> { + /// If parent tag(s) exist, rewrite name to match their case. + fn match_parents(&self, tag: &str) -> Result { + let child_split: Vec<_> = tag.split("::").collect(); + let t = if let Some(parent_tag) = self.first_existing_parent(&tag)? { + let parent_count = parent_tag.matches("::").count() + 1; + format!( + "{}::{}", + parent_tag, + &child_split[parent_count..].join("::") + ) + } else { + tag.into() + }; + + Ok(t) + } + + fn first_existing_parent(&self, mut tag: &str) -> Result> { + while let Some(parent_name) = immediate_parent_name(tag) { + if let Some(parent_tag) = self.get_tag(parent_name)? { + return Ok(Some(parent_tag.name)); + } + tag = parent_name; + } + + Ok(None) + } + + // Get stored tag name or the same passed name if it doesn't exist, rewritten to match parents case. + // Returns a tuple of the preferred name and a boolean indicating if the tag exists. + pub(crate) fn preferred_tag_case(&self, tag: &str) -> Result<(Result, bool)> { self.db .prepare_cached("select tag from tags where tag = ?")? - .query_and_then(params![tag], |row| row.get(0))? - .next() - .transpose() + .query_row(params![tag], |row| { + Ok((self.match_parents(row.get_raw(0).as_str()?), true)) + }) + .or_else::(|_| Ok((self.match_parents(tag), false))) .map_err(Into::into) } diff --git a/rslib/src/tags.rs b/rslib/src/tags.rs index 0e70d77bb..ab5c74a16 100644 --- a/rslib/src/tags.rs +++ b/rslib/src/tags.rs @@ -225,13 +225,13 @@ impl Collection { if normalized_name.is_empty() { return Ok((t, false)); } - if let Some(preferred) = self.storage.preferred_tag_case(&normalized_name)? { - t.name = preferred; - Ok((t, false)) - } else { + let (preferred, exists) = self.storage.preferred_tag_case(&normalized_name)?; + t.name = preferred?; + if !exists { self.storage.register_tag(&t)?; - Ok((t, true)) } + + Ok((t, !exists)) } pub fn clear_unused_tags(&self) -> Result<()> { @@ -533,6 +533,14 @@ mod test { ) ); + // children should match the case of their parents + col.storage.clear_tags()?; + *(&mut note.tags[0]) = "FOO".into(); + *(&mut note.tags[1]) = "foo::BAR".into(); + *(&mut note.tags[2]) = "foo::bar::baz".into(); + col.update_note(&mut note)?; + assert_eq!(note.tags, vec!["FOO", "FOO::BAR", "FOO::BAR::baz"]); + Ok(()) } From 71f1d3b982151d308cc76417907dc915be425ba7 Mon Sep 17 00:00:00 2001 From: Damien Elmes Date: Tue, 19 Jan 2021 11:50:37 +1000 Subject: [PATCH 2/2] tweaks to the parent matching behaviour - move logic out of the storage layer - its job is only to read and write data from the DB - avoid the Result within a Result - return the preferred case as an option, so we can avoid a copy in the unchanged case - return a Cow when normalizing, so we can avoid copying in the unchanged case - add tags directly in clear_unused_tags(), so we avoid doing lookups for every tag in the tag list --- rslib/src/storage/tag/mod.rs | 46 +++----------------- rslib/src/tags.rs | 81 ++++++++++++++++++++++++++---------- 2 files changed, 65 insertions(+), 62 deletions(-) diff --git a/rslib/src/storage/tag/mod.rs b/rslib/src/storage/tag/mod.rs index 1f00268ab..e0f1b0bd4 100644 --- a/rslib/src/storage/tag/mod.rs +++ b/rslib/src/storage/tag/mod.rs @@ -2,8 +2,7 @@ // License: GNU AGPL, version 3 or later; http://www.gnu.org/licenses/agpl.html use super::SqliteStorage; -use crate::err::{AnkiError, Result}; -use crate::{tags::Tag, types::Usn}; +use crate::{err::Result, tags::Tag, types::Usn}; use rusqlite::{params, Row, NO_PARAMS}; use std::collections::HashMap; @@ -16,10 +15,6 @@ fn row_to_tag(row: &Row) -> Result { }) } -fn immediate_parent_name(tag_name: &str) -> Option<&str> { - tag_name.rsplitn(2, "::").nth(1) -} - impl SqliteStorage { /// All tags in the collection, in alphabetical order. pub(crate) fn all_tags(&self) -> Result> { @@ -61,43 +56,12 @@ impl SqliteStorage { Ok(()) } - /// If parent tag(s) exist, rewrite name to match their case. - fn match_parents(&self, tag: &str) -> Result { - let child_split: Vec<_> = tag.split("::").collect(); - let t = if let Some(parent_tag) = self.first_existing_parent(&tag)? { - let parent_count = parent_tag.matches("::").count() + 1; - format!( - "{}::{}", - parent_tag, - &child_split[parent_count..].join("::") - ) - } else { - tag.into() - }; - - Ok(t) - } - - fn first_existing_parent(&self, mut tag: &str) -> Result> { - while let Some(parent_name) = immediate_parent_name(tag) { - if let Some(parent_tag) = self.get_tag(parent_name)? { - return Ok(Some(parent_tag.name)); - } - tag = parent_name; - } - - Ok(None) - } - - // Get stored tag name or the same passed name if it doesn't exist, rewritten to match parents case. - // Returns a tuple of the preferred name and a boolean indicating if the tag exists. - pub(crate) fn preferred_tag_case(&self, tag: &str) -> Result<(Result, bool)> { + pub(crate) fn preferred_tag_case(&self, tag: &str) -> Result> { self.db .prepare_cached("select tag from tags where tag = ?")? - .query_row(params![tag], |row| { - Ok((self.match_parents(row.get_raw(0).as_str()?), true)) - }) - .or_else::(|_| Ok((self.match_parents(tag), false))) + .query_and_then(params![tag], |row| row.get(0))? + .next() + .transpose() .map_err(Into::into) } diff --git a/rslib/src/tags.rs b/rslib/src/tags.rs index ab5c74a16..6940fba8e 100644 --- a/rslib/src/tags.rs +++ b/rslib/src/tags.rs @@ -86,19 +86,29 @@ fn normalized_tag_name_component(comp: &str) -> Cow { } } -fn normalize_tag_name(name: &str) -> String { - let mut out = String::with_capacity(name.len()); - for comp in name.split("::") { - out.push_str(&normalized_tag_name_component(comp)); - out.push_str("::"); +fn normalize_tag_name(name: &str) -> Cow { + if name + .split("::") + .any(|comp| matches!(normalized_tag_name_component(comp), Cow::Owned(_))) + { + name.split("::") + .map(normalized_tag_name_component) + .collect::() + .into() + } else { + // no changes required + name.into() } - out.trim_end_matches("::").into() } -fn immediate_parent_name(tag_name: UniCase<&str>) -> Option> { +fn immediate_parent_name_unicase(tag_name: UniCase<&str>) -> Option> { tag_name.rsplitn(2, '\x1f').nth(1).map(UniCase::new) } +fn immediate_parent_name_str(tag_name: &str) -> Option<&str> { + tag_name.rsplitn(2, "::").nth(1) +} + /// For the given tag, check if immediate parent exists. If so, add /// tag and return. /// If the immediate parent is missing, check and add any missing parents. @@ -109,7 +119,7 @@ fn add_tag_and_missing_parents<'a, 'b>( missing: &'a mut Vec>, tag_name: UniCase<&'b str>, ) { - if let Some(parent) = immediate_parent_name(tag_name) { + if let Some(parent) = immediate_parent_name_unicase(tag_name) { if !all.contains(&parent) { missing.push(parent); add_tag_and_missing_parents(all, missing, parent); @@ -214,24 +224,52 @@ impl Collection { Ok((tags, added)) } - /// Register tag if it doesn't exist. - /// Returns a tuple of the tag with its name normalized and a boolean indicating if it was added. + /// Adjust tag casing to match any existing parents, and register it if it's not already + /// in the tags list. Returns a tuple of the tag with its name normalized, and a boolean + /// indicating if it was added. pub(crate) fn register_tag(&self, tag: Tag) -> Result<(Tag, bool)> { let normalized_name = normalize_tag_name(&tag.name); - let mut t = Tag { - name: normalized_name.clone(), - ..tag - }; if normalized_name.is_empty() { - return Ok((t, false)); + // this should not be possible + return Err(AnkiError::invalid_input("blank tag")); } - let (preferred, exists) = self.storage.preferred_tag_case(&normalized_name)?; - t.name = preferred?; - if !exists { - self.storage.register_tag(&t)?; + if let Some(out_tag) = self.storage.get_tag(&normalized_name)? { + // already registered + Ok((out_tag, false)) + } else { + let name = self + .adjusted_case_for_parents(&normalized_name)? + .unwrap_or_else(|| normalized_name.into()); + let out_tag = Tag { name, ..tag }; + self.storage.register_tag(&out_tag)?; + Ok((out_tag, true)) + } + } + + /// If parent tag(s) exist and differ in case, return a rewritten tag. + fn adjusted_case_for_parents(&self, tag: &str) -> Result> { + if let Some(parent_tag) = self.first_existing_parent_tag(&tag)? { + let child_split: Vec<_> = tag.split("::").collect(); + let parent_count = parent_tag.matches("::").count() + 1; + Ok(Some(format!( + "{}::{}", + parent_tag, + &child_split[parent_count..].join("::") + ))) + } else { + Ok(None) + } + } + + fn first_existing_parent_tag(&self, mut tag: &str) -> Result> { + while let Some(parent_name) = immediate_parent_name_str(tag) { + if let Some(parent_tag) = self.storage.preferred_tag_case(parent_name)? { + return Ok(Some(parent_tag)); + } + tag = parent_name; } - Ok((t, !exists)) + Ok(None) } pub fn clear_unused_tags(&self) -> Result<()> { @@ -239,7 +277,8 @@ impl Collection { self.storage.clear_tags()?; let usn = self.usn()?; for name in self.storage.all_tags_in_notes()? { - self.register_tag(Tag { + let name = normalize_tag_name(&name).into(); + self.storage.register_tag(&Tag { collapsed: collapsed.contains(&name), name, usn,