游戏功能更新1 - 收藏夹功能 || Function Update cp1 - Favorites#662
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a new “Favorites / 收藏夹” feature to the Phira client, including folder-based favorites management and UI integration in both the song detail scene and the local library page.
Changes:
- Introduces a new Favorites page with default + custom folders, plus custom cover support.
- Adds a star toggle button on the song scene (short press toggles default favorites; long press opens folder selection).
- Extends local library filtering to show all charts or only charts from a selected favorites folder, with supporting data persistence and localization updates.
Reviewed changes
Copilot reviewed 34 out of 36 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
phira/src/scene/song.rs |
Adds favorites star button + popup folder selector in song scene. |
phira/src/page/library.rs |
Adds favorites filter button and filtering logic for local charts. |
phira/src/page/favorites.rs |
New Favorites management page (folders, covers, rename/delete). |
phira/src/page.rs |
Exposes the new favorites module and re-exports FavoritesPage. |
phira/src/icons.rs |
Adds starred icon texture loading. |
phira/src/data.rs |
Adds persisted Favorites structure and related helper APIs. |
| `phira/locales/*/(song | library).ftl` |
assets/starred.png |
Adds starred icon asset. |
.gitignore |
Adds ignores for several local/dev directories. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn enter(&mut self, _s: &mut SharedState) -> Result<()> { | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Page::enter already has a default no-op implementation; this override just returns Ok(()) and adds noise. Please remove this method unless you plan to add enter-time logic here.
| let fav_folder = self.current_fav_folder.clone(); | ||
| let mut charts = Vec::new(); | ||
| charts.push(ChartDisplayItem::new(None, None)); | ||
| if fav_folder.is_none() { | ||
| charts.push(ChartDisplayItem::new(None, None)); | ||
| } | ||
| let fav_paths: Option<Vec<String>> = fav_folder.as_ref().map(|folder| { | ||
| get_data().favorites.get_paths(folder) | ||
| }); | ||
| charts.append( | ||
| &mut s | ||
| .charts_local | ||
| .iter() | ||
| .filter(|it| it.info.name.contains(&search)) | ||
| .filter(|it| { | ||
| let name_match = it.info.name.contains(&search); | ||
| let fav_match = match &fav_paths { | ||
| Some(paths) => it.local_path.as_ref().map_or(false, |p| paths.contains(p)), | ||
| None => true, | ||
| }; | ||
| name_match && fav_match | ||
| }) |
There was a problem hiding this comment.
Local favorites filtering does repeated linear Vec::contains checks (and clones the folder path list) for every chart, which can become O(charts × favorites) and noticeably slow for large libraries. Consider turning the folder paths into a HashSet once (or adding an API that returns a borrowed slice / iterator) and then do O(1) membership checks while filtering.
| let all_names = get_data().favorites.all_folder_names(); | ||
| let local_path = self.local_path.as_ref().unwrap(); | ||
| let options: Vec<String> = all_names | ||
| .iter() | ||
| .map(|name| { | ||
| let display = if name == DEFAULT_FAVORITES_KEY { | ||
| tl!("fav-default-folder").to_string() | ||
| } else { | ||
| name.clone() | ||
| }; | ||
| let in_folder = get_data().favorites.get_paths(name).contains(&local_path.to_string()); | ||
| if in_folder { | ||
| format!("\u{2713} {display}") | ||
| } else { | ||
| format!(" {display}") | ||
| } | ||
| }) |
There was a problem hiding this comment.
Building the favorites popup options repeatedly clones favorites path lists and allocates new Strings (local_path.to_string()) just for contains checks. This is avoidable overhead in a hot UI path; prefer checking membership without cloning (e.g., iterate/borrow the stored Vec<String> and compare against &String / &str).
| pub fn is_in_default(&self, local_path: &str) -> bool { | ||
| self.folders | ||
| .get(DEFAULT_FAVORITES_KEY) | ||
| .map_or(false, |v| v.contains(&local_path.to_string())) | ||
| } | ||
|
|
||
| pub fn is_favorited(&self, local_path: &str) -> bool { | ||
| self.folders.values().any(|v| v.contains(&local_path.to_string())) | ||
| } | ||
|
|
||
| pub fn add_to(&mut self, folder: &str, local_path: &str) { | ||
| let list = self.folders.entry(folder.to_string()).or_default(); | ||
| if !list.contains(&local_path.to_string()) { | ||
| list.push(local_path.to_string()); | ||
| } |
There was a problem hiding this comment.
Favorites membership helpers allocate Strings on every call (e.g., v.contains(&local_path.to_string()) / list.contains(&local_path.to_string())). This creates unnecessary allocations; prefer v.iter().any(|p| p == local_path) (or similar) to compare against &str without allocating.
| if let Some(ref name) = folder.folder_name { | ||
| let count = get_data().favorites.get_paths(name).len(); | ||
| ui.text(format!("{}", count)) | ||
| .pos(r.right() - 0.02, r.y + 0.02) | ||
| .anchor(1., 0.) | ||
| .size(0.35) | ||
| .color(semi_white(0.6)) | ||
| .draw(); | ||
| } |
There was a problem hiding this comment.
Rendering the per-folder chart count calls get_paths(name).len(), which currently clones the entire Vec<String> just to get its length. Consider adding a non-cloning accessor (e.g., folder_len) or returning a borrowed slice so this doesn’t allocate every frame.
| .buttons(vec![ | ||
| tl!("favorites-all").to_string(), | ||
| tl!("favorites-delete").to_string(), | ||
| ]) |
There was a problem hiding this comment.
In the delete confirmation dialog, the first button label uses tl!("favorites-all") ("All") as the cancel option. This is confusing UX and doesn’t match the dialog intent; please use a proper cancel/back label (existing generic cancel key if available) and keep favorites-all for the “show all” filter only.
| data::DEFAULT_FAVORITES_KEY, | ||
| get_data, get_data_mut, | ||
| icons::Icons, | ||
| page::favorites::FAV_PAGE_RESULT, | ||
| popup::Popup, | ||
| rate::RateDialog, | ||
| save_data, | ||
| scene::{check_read_tos_and_policy, ChartOrder, JUST_LOADED_TOS, ORDERS}, |
There was a problem hiding this comment.
get_data_mut and save_data are imported but not used in this file. Please remove unused imports to avoid warnings and keep the import list accurate.
|
Closed in favor of https://github.com/TeamFlos/phira/tree/favorites |
Pr内容包含
收藏夹功能
其它内容
代码已经过验证
联系方式(注意需要的话备注来意因为我不认识会拒绝)
QQ: 212898630
邮箱:212898630@qq.com || 212898630fan@gmail.com