fix: persist group order on reopenning AppFlowy (#4080)

* fix: persist group order on reopenning AppFlowy

Currently if you change groups order, on restarting AppFlowy they
will go back to what they were before.
This was because merge_groups function was not respecting the group
orders which were saved before as group settings.

* refactor: enhance the merge_group function

* refactor: use extend instead of for loop
This commit is contained in:
Mohammad Zolfaghari 2023-12-20 08:01:00 +03:30 committed by GitHub
parent d68c847d59
commit f5a9f2bf4d
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23

View File

@ -82,7 +82,7 @@ pub struct GroupContext<C> {
reader: Arc<dyn GroupSettingReader>, reader: Arc<dyn GroupSettingReader>,
/// A writer that implement the [GroupSettingWriter] trait is used to save the /// A writer that implement the [GroupSettingWriter] trait is used to save the
/// configuration to disk /// configuration to disk
/// ///
writer: Arc<dyn GroupSettingWriter>, writer: Arc<dyn GroupSettingWriter>,
} }
@ -287,7 +287,7 @@ where
old_groups.clear(); old_groups.clear();
} }
// The `all_group_revs` is the combination of the new groups and old groups // The `all_group` is the combination of the new groups and old groups
let MergeGroupResult { let MergeGroupResult {
mut all_groups, mut all_groups,
new_groups, new_groups,
@ -474,25 +474,30 @@ fn merge_groups(
.into_iter() .into_iter()
.map(|group| (group.id.clone(), group)) .map(|group| (group.id.clone(), group))
.collect(); .collect();
let mut no_status_group_inserted = false;
// The group is ordered in old groups. Add them before adding the new groups // The group is ordered in old groups. Add them before adding the new groups
for old in old_groups { for old in old_groups {
if let Some(new) = new_group_map.shift_remove(&old.id) { if let Some(new) = new_group_map.shift_remove(&old.id) {
merge_result.all_groups.push(new.clone()); merge_result.all_groups.push(new.clone());
} else if matches!(&no_status_group, Some(group) if group.id == old.id) {
merge_result
.all_groups
.push(no_status_group.clone().unwrap());
no_status_group_inserted = true;
} else { } else {
merge_result.deleted_groups.push(old); merge_result.deleted_groups.push(old);
} }
} }
// Find out the new groups
merge_result merge_result
.all_groups .all_groups
.extend(new_group_map.values().cloned()); .extend(new_group_map.values().cloned());
merge_result.new_groups.extend(new_group_map.into_values()); merge_result.new_groups.extend(new_group_map.into_values());
// The `No status` group index is initialized to 0 // The `No status` group index is initialized to 0
if let Some(no_status_group) = no_status_group { if !no_status_group_inserted && no_status_group.is_some() {
merge_result.all_groups.insert(0, no_status_group); merge_result.all_groups.insert(0, no_status_group.unwrap());
} }
merge_result merge_result
} }
@ -520,3 +525,74 @@ impl MergeGroupResult {
} }
} }
} }
#[cfg(test)]
mod tests {
use crate::services::group::Group;
use super::{merge_groups, MergeGroupResult};
#[test]
fn merge_groups_test() {
struct GroupMergeTest<'a> {
no_status_group: &'a str,
old_groups: Vec<&'a str>,
new_groups: Vec<&'a str>,
exp_all_groups: Vec<&'a str>,
exp_new_groups: Vec<&'a str>,
exp_deleted_groups: Vec<&'a str>,
}
let new_group = |name: &str| Group::new(name.to_string(), name.to_string());
let groups_from_strings =
|strings: Vec<&str>| strings.iter().map(|s| new_group(s)).collect::<Vec<Group>>();
let group_stringify = |groups: Vec<Group>| {
groups
.iter()
.map(|group| group.name.clone())
.collect::<Vec<String>>()
.join(",")
};
let tests = vec![
GroupMergeTest {
no_status_group: "No Status",
old_groups: vec!["Doing", "Done", "To Do", "No Status"],
new_groups: vec!["To Do", "Doing", "Done"],
exp_all_groups: vec!["Doing", "Done", "To Do", "No Status"],
exp_new_groups: vec![],
exp_deleted_groups: vec![],
},
GroupMergeTest {
no_status_group: "No Status",
old_groups: vec!["To Do", "Doing", "Done", "No Status", "Archive"],
new_groups: vec!["backlog", "To Do", "Doing", "Done"],
exp_all_groups: vec!["To Do", "Doing", "Done", "No Status", "backlog"],
exp_new_groups: vec!["backlog"],
exp_deleted_groups: vec!["Archive"],
},
];
for test in tests {
let MergeGroupResult {
all_groups,
new_groups,
deleted_groups,
} = merge_groups(
Some(new_group(test.no_status_group)),
groups_from_strings(test.old_groups),
groups_from_strings(test.new_groups),
);
let exp_all_groups = groups_from_strings(test.exp_all_groups);
let exp_new_groups = groups_from_strings(test.exp_new_groups);
let exp_deleted_groups = groups_from_strings(test.exp_deleted_groups);
assert_eq!(group_stringify(all_groups), group_stringify(exp_all_groups));
assert_eq!(group_stringify(new_groups), group_stringify(exp_new_groups));
assert_eq!(
group_stringify(deleted_groups),
group_stringify(exp_deleted_groups)
);
}
}
}