fix: Refactor badge selection from index-based to name-based to fix state persistence issue#1601
Open
samruddhi-Rahegaonkar wants to merge 2 commits intofossasia:developmentfrom
Open
Conversation
…persistence issue
Contributor
Reviewer's GuideRefactors badge selection from index-based to name-based, adds persistence for selected badges by name, and updates UI components to rely on the new selection model to avoid mismatches when badge order changes. Sequence diagram for deleting selected badges via SettingsScreensequenceDiagram
actor User
participant SettingsScreenState
participant BadgeScanProvider
participant SharedPreferences
User ->> SettingsScreenState: tap delete selected badges
SettingsScreenState ->> BadgeScanProvider: removeSelectedDevices()
alt no badges selected
BadgeScanProvider-->>SettingsScreenState: return (no-op)
else badges selected
BadgeScanProvider->>BadgeScanProvider: _badgeNames.removeWhere(name in _selectedBadgeNames)
BadgeScanProvider->>BadgeScanProvider: _selectedBadgeNames.clear()
BadgeScanProvider->>SharedPreferences: _saveToPrefs()
SharedPreferences-->>BadgeScanProvider: persist badge_scan_mode, badge_names, selected_badge_names
BadgeScanProvider-->>SettingsScreenState: notifyListeners()
end
SettingsScreenState-->>User: UI updates with remaining badges
Sequence diagram for loading persisted badge selection by namesequenceDiagram
participant BadgeScanProvider
participant SharedPreferences
participant UIWidget
BadgeScanProvider->>BadgeScanProvider: constructor()
BadgeScanProvider->>BadgeScanProvider: _loadFromPrefs()
BadgeScanProvider->>SharedPreferences: getInstance()
SharedPreferences-->>BadgeScanProvider: SharedPreferences
BadgeScanProvider->>SharedPreferences: getInt(badge_scan_mode)
SharedPreferences-->>BadgeScanProvider: modeIndex
BadgeScanProvider->>BadgeScanProvider: set _mode from BadgeScanMode.values[modeIndex]
BadgeScanProvider->>SharedPreferences: getStringList(badge_names)
SharedPreferences-->>BadgeScanProvider: storedNames
BadgeScanProvider->>BadgeScanProvider: set _badgeNames from storedNames
BadgeScanProvider->>SharedPreferences: getStringList(selected_badge_names)
SharedPreferences-->>BadgeScanProvider: storedSelected
BadgeScanProvider->>BadgeScanProvider: _selectedBadgeNames = storedSelected filtered by _badgeNames
BadgeScanProvider->>BadgeScanProvider: _isLoaded = true
BadgeScanProvider-->>UIWidget: notifyListeners()
UIWidget->>BadgeScanProvider: isSelected(index)
BadgeScanProvider-->>UIWidget: _selectedBadgeNames.contains(_badgeNames[index])
Class diagram for updated BadgeScanProvider and related widgetsclassDiagram
class BadgeScanProvider {
-BadgeScanMode _mode
-List~String~ _badgeNames
-Set~String~ _selectedBadgeNames
-bool _isLoaded
+BadgeScanProvider()
+BadgeScanMode get mode()
+List~String~ get badgeNames()
+Set~String~ get selectedBadgeNames()
+bool get isLoaded()
-Future~void~ _loadFromPrefs()
-Future~void~ _saveToPrefs()
+void setMode(BadgeScanMode mode)
+void setBadgeNames(List~String~ names)
+void addBadgeName(String name)
+void removeBadgeNameAt(int index)
+void updateBadgeName(int index, String newName)
+void toggleSelection(int index)
+bool isSelected(int index)
+void clearSelection()
+void selectAll()
+void removeSelectedDevices()
+List~String~ getSelectedBadgeNames()
}
class BadgeScanSettingsWidgetState {
-List~TextEditingController~ _controllers
+void onDeleteSelectedPressed()
}
class SettingsScreenState {
+void onDeleteSelectedPressed()
}
class SharedPreferences {
+static Future~SharedPreferences~ getInstance()
+int getInt(String key)
+List~String~ getStringList(String key)
+Future~bool~ setInt(String key, int value)
+Future~bool~ setStringList(String key, List~String~ value)
}
BadgeScanProvider ..> SharedPreferences : uses
BadgeScanSettingsWidgetState --> BadgeScanProvider : reads badgeNames
BadgeScanSettingsWidgetState --> BadgeScanProvider : reads selectedBadgeNames
SettingsScreenState --> BadgeScanProvider : calls removeSelectedDevices
SettingsScreenState --> BadgeScanProvider : reads selectedBadgeNames
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Using
Set<String> _selectedBadgeNamesand converting back to indices withindexOfwill behave unexpectedly when there are duplicate badge names (only the first index is found and selection cannot distinguish between duplicates); consider either enforcing unique badge names or using a stable identifier (e.g., UUID) instead of the display name. - In
updateBadgeName, a name that trims to an empty string still replaces the old name and is propagated into selections; if empty names should be disallowed like insetBadgeNames, add an early return or reuse the same validation/filtering logic there for consistency.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Using `Set<String> _selectedBadgeNames` and converting back to indices with `indexOf` will behave unexpectedly when there are duplicate badge names (only the first index is found and selection cannot distinguish between duplicates); consider either enforcing unique badge names or using a stable identifier (e.g., UUID) instead of the display name.
- In `updateBadgeName`, a name that trims to an empty string still replaces the old name and is propagated into selections; if empty names should be disallowed like in `setBadgeNames`, add an early return or reuse the same validation/filtering logic there for consistency.
## Individual Comments
### Comment 1
<location path="lib/providers/BadgeScanProvider.dart" line_range="9-18" />
<code_context>
+ Set<String> _selectedBadgeNames = {};
</code_context>
<issue_to_address>
**issue:** Using badge names as selection keys can break with duplicate names.
`setBadgeNames` and `addBadgeName` don’t enforce uniqueness, so duplicate names will make `toggleSelection`, `removeSelectedDevices`, and the UI’s `indexOf` mapping ambiguous or only affect the first match. Either normalize `_badgeNames` to unique values (e.g., via a `Set`/deduping) or constrain the API so badge names are guaranteed unique.
</issue_to_address>
### Comment 2
<location path="lib/providers/BadgeScanProvider.dart" line_range="106-115" />
<code_context>
void updateBadgeName(int index, String newName) {
if (index < 0 || index >= _badgeNames.length) return;
- _badgeNames[index] = newName.trim();
+
+ final oldName = _badgeNames[index];
+ final trimmed = newName.trim();
+
+ _badgeNames[index] = trimmed;
+
+ // Update selection if name changed
+ if (_selectedBadgeNames.contains(oldName)) {
+ _selectedBadgeNames.remove(oldName);
+ _selectedBadgeNames.add(trimmed);
+ }
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Updating a badge name can introduce empty or duplicate names, affecting selection integrity.
In `updateBadgeName`, `newName` is trimmed but not validated like in `setBadgeNames`/`addBadgeName`. This permits setting a badge to `""` and to names that already exist. Since selection is keyed by name, this can introduce invalid data and merge distinct selections due to `Set<String>` de-duplication. Consider early-returning on `trimmed.isEmpty` and, if appropriate, applying the same duplicate-handling rules used elsewhere.
Suggested implementation:
```
void updateBadgeName(int index, String newName) {
if (index < 0 || index >= _badgeNames.length) return;
final oldName = _badgeNames[index];
final trimmed = newName.trim();
// Disallow empty names to keep badge and selection state valid
if (trimmed.isEmpty) return;
// Disallow duplicate names to avoid merging distinct selections via Set<String>
if (_badgeNames.contains(trimmed) && trimmed != oldName) return;
_badgeNames[index] = trimmed;
// Update selection if name changed
if (_selectedBadgeNames.contains(oldName)) {
_selectedBadgeNames.remove(oldName);
_selectedBadgeNames.add(trimmed);
}
```
I assumed that duplicates should be disallowed and handled via an early return.
If `setBadgeNames`/`addBadgeName` use a different policy (e.g., auto-renaming duplicates or merging), you should align the duplicate check here with that exact behavior to keep the logic consistent across all badge-name mutations.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
|
Thanks for the contribution! A process note. We have automatic Copilot PR reviews enabled on this repository. These reviews are only triggered if the contributor has GitHub Copilot enabled and an active license on their own account. Please enable Copilot in your GitHub settings if you have access. In many regions, free licenses are available through educational institutions or developer programs. Enabling Copilot helps us speed up the auto review process and reduces manual review overhead for the core team. |
mariobehling
requested changes
Apr 13, 2026
Member
mariobehling
left a comment
mariobehling
left a comment
There was a problem hiding this comment.
Please address failing tests.
There was a problem hiding this comment.
Pull request overview
Refactors the badge scan settings selection model from index-based to name-based, aiming to fix the issue where selected badge checkboxes become unchecked after saving (state persistence across badge list changes).
Changes:
- Replaced index-based selection (
selectedIndices) with name-based selection (selectedBadgeNames) in UI and provider logic. - Added persistence for selected badge names via SharedPreferences.
- Updated removal logic to resolve controller indices dynamically from selected badge names.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lib/view/settings_screen.dart | Updates delete button visibility/count to use selectedBadgeNames instead of selectedIndices. |
| lib/view/badgeScanSettingsWidget.dart | Updates delete logic to derive controller indices from selected badge names and updates delete count. |
| lib/providers/BadgeScanProvider.dart | Refactors selection state to Set<String>, persists it, and updates selection/removal logic accordingly. |
Comments suppressed due to low confidence (1)
lib/view/badgeScanSettingsWidget.dart:133
- The controller-removal logic converts selected badge names to indices using
badgeNames.indexOf(name). This breaks if badge names are not unique (it always returns the first match), and can desync_controllersfromprovider.badgeNames(disposing/removing the wrong controller while the provider removes different rows). Consider enforcing unique badge names, or mapping names to all indices (or using stable IDs) so controller disposal targets the exact items being removed.
final badgeNames = provider.badgeNames;
// Convert selected badge names to indices
final selectedIndices = provider
.selectedBadgeNames
.map((name) => badgeNames.indexOf(name))
.where((index) => index != -1)
.toList()
..sort((a, b) =>
b.compareTo(a)); // descending order
for (final index in selectedIndices) {
if (index < _controllers.length) {
_controllers[index].dispose();
_controllers.removeAt(index);
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
75
to
+80
| void setBadgeNames(List<String> names) { | ||
| _badgeNames = names.where((name) => name.trim().isNotEmpty).toList(); | ||
| _selectedIndices.clear(); // Clear selections when badge names change | ||
|
|
||
| // Remove selections that no longer exist | ||
| _selectedBadgeNames.removeWhere((name) => !_badgeNames.contains(name)); | ||
|
|
There was a problem hiding this comment.
selectedBadgeNames is stored as a Set<String>, but _badgeNames is still allowed to contain duplicates (no de-duplication in setBadgeNames). With duplicate names, selecting one row will mark all rows with that name as selected (isSelected checks by name), and removeSelectedDevices() will remove all duplicates of a selected name. Consider enforcing unique badge names (reject/merge duplicates on set/add/update) or changing selection to use stable IDs so duplicates can be handled predictably.
Comment on lines
+114
to
+119
| // Update selection if name changed | ||
| if (_selectedBadgeNames.contains(oldName)) { | ||
| _selectedBadgeNames.remove(oldName); | ||
| _selectedBadgeNames.add(trimmed); | ||
| } | ||
|
|
There was a problem hiding this comment.
updateBadgeName updates _selectedBadgeNames to the trimmed value even when it becomes an empty string during editing (e.g., user clears a selected field before typing a new name). Because _saveToPrefs() is called on every change, this can persist an empty badge name and/or empty selection to SharedPreferences. Consider treating an empty trimmed as a special case (don’t add it to _selectedBadgeNames, and/or defer persistence until an explicit save).
Suggested change
| // Update selection if name changed | |
| if (_selectedBadgeNames.contains(oldName)) { | |
| _selectedBadgeNames.remove(oldName); | |
| _selectedBadgeNames.add(trimmed); | |
| } | |
| // Update selection if name changed, but never keep an empty badge name | |
| // in the selected set during transient editing states. | |
| if (_selectedBadgeNames.contains(oldName)) { | |
| _selectedBadgeNames.remove(oldName); | |
| if (trimmed.isNotEmpty) { | |
| _selectedBadgeNames.add(trimmed); | |
| } | |
| } | |
| // Avoid persisting transient empty values while the user is editing. | |
| if (trimmed.isEmpty) { | |
| notifyListeners(); | |
| return; | |
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1597
Changes
Screenshots / Recordings
Checklist:
constants.dartwithout hard coding any value.Summary by Sourcery
Refactor badge selection to be name-based instead of index-based to ensure stable selection and persistence when badge lists change.
Bug Fixes:
Enhancements: