-
Notifications
You must be signed in to change notification settings - Fork 1
V0.1.0 #2
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?
Conversation
Reviewer's Guide by SourceryThis pull request focuses on improving the user interface for preset management by making the grid layout responsive and adding a search filter for presets. Significant changes include the addition of a new function to filter presets, updates to the UI to use this filtered data, and various code cleanups and TODO removals. File-Level Changes
Tips
|
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.
Hey @ceebeel - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 6 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟡 Documentation: 2 issues found
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| .spacing([4., 4.]) | ||
| .show(ui, |ui| { | ||
| let mut preset_count = 0; | ||
| let max_preset_per_row = (ui.available_width() / 68.) as usize; // Preset width = 64. + 4. (spacing) |
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.
suggestion: Magic number for preset width
Consider defining a constant or a configuration parameter for the preset width (68) to improve readability and maintainability.
| let max_preset_per_row = (ui.available_width() / 68.) as usize; // Preset width = 64. + 4. (spacing) | |
| const PRESET_WIDTH: f32 = 68.0; | |
| let max_preset_per_row = (ui.available_width() / PRESET_WIDTH) as usize; |
| if self.presets.lists[*preset_id] | ||
| .name | ||
| .contains(&self.preset_search_query) |
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.
issue (performance): Removed search query filtering
The removal of the search query filtering might lead to performance issues if the preset list is large. Ensure that the filtering is handled elsewhere effectively.
| pub img: Option<PathBuf>, | ||
| } | ||
|
|
||
| #[derive(Debug, Clone)] |
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.
question: Added Debug and Clone traits
Adding Debug and Clone traits is useful for logging and copying. Ensure that these traits are necessary for the Node enum to avoid unnecessary trait implementations.
| } | ||
| } | ||
|
|
||
| pub fn filter_presets_tree(query: &str, node: &BTreeMap<String, Node>) -> BTreeMap<String, Node> { |
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.
suggestion (performance): New filter_presets_tree function
The new filter_presets_tree function is a good addition for filtering. Ensure that this function is optimized for large trees to avoid performance bottlenecks.
| pub fn filter_presets_tree(query: &str, node: &BTreeMap<String, Node>) -> BTreeMap<String, Node> { | |
| pub fn filter_presets_tree<'a>(query: &str, node: &'a BTreeMap<String, Node>) -> BTreeMap<String, Node> { | |
| node.iter() | |
| .filter(|(name, _)| name.contains(query)) | |
| .map(|(name, node)| (name.clone(), node.clone())) | |
| .collect() | |
| } |
| self.filtered_presets_tree = | ||
| filter_presets_tree(&self.preset_search_query, &self.presets.tree); |
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.
suggestion (performance): Filtering presets on search query change
Filtering presets on search query change is a good approach. Ensure that the UI updates efficiently to reflect the filtered results without noticeable lag.
| self.filtered_presets_tree = | |
| filter_presets_tree(&self.preset_search_query, &self.presets.tree); | |
| self.filtered_presets_tree = { | |
| let query = self.preset_search_query.clone(); | |
| let tree = self.presets.tree.clone(); | |
| filter_presets_tree(&query, &tree) | |
| }; |
| - [X] Filter preset / Hide Empty Categories | ||
| - [X] Grid Layout: Responsive | ||
| - [X] Show Flat Preset: Align text to the left | ||
| - [ ] Change preset time interval on nBar (4 beat) |
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.
question (documentation): Clarify 'Set Window default size (persistence !?)'
The exclamation and question marks might be confusing. Consider rephrasing to 'Set Window default size (persistent?)' or clarify the intended meaning.
Summary by Sourcery
This pull request introduces a responsive grid layout for displaying presets and adds a filtering mechanism to search and display presets based on a query string. It also includes enhancements to the preset display logic and updates the README to reflect these changes.