feat: added a custom slot provider option for user to select the slot no and rearrange the order #1494
Conversation
…d rearrange the order
|
🧙 Sourcery has finished reviewing your pull request! 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}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The SaveBadgeCard build method is very large and nested—consider extracting sections like the action buttons row, status chips, and overlay badge into smaller private widgets or methods to improve readability and maintainability.
- In BadgeListViewState, the
_draggingItemand_hoveredIndexfields are never used in your widget tree—either remove them or implement their intended hover/drop‐target highlighting behavior. - You repeatedly call
file.jsonToData(badgeData.value)and then extract properties; consider parsing the JSON once into a local data object (or caching it) to avoid redundant deserialization in build.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The SaveBadgeCard build method is very large and nested—consider extracting sections like the action buttons row, status chips, and overlay badge into smaller private widgets or methods to improve readability and maintainability.
- In BadgeListViewState, the `_draggingItem` and `_hoveredIndex` fields are never used in your widget tree—either remove them or implement their intended hover/drop‐target highlighting behavior.
- You repeatedly call `file.jsonToData(badgeData.value)` and then extract properties; consider parsing the JSON once into a local data object (or caching it) to avoid redundant deserialization in build.
## Individual Comments
### Comment 1
<location> `lib/view/widgets/save_badge_card.dart:59-69` </location>
<code_context>
+ }
+ }
+
+ bool _safeGetInvertValue(Map<String, dynamic> data) {
+ try {
+ if (data.containsKey('messages') &&
+ data['messages'] is List &&
+ data['messages'].isNotEmpty &&
+ data['messages'][0] is Map<String, dynamic>) {
+ return data['messages'][0]['invert'] ?? false;
+ }
+ return false;
+ } catch (e) {
+ // If there's an error, default to false
</code_context>
<issue_to_address>
**suggestion:** Unify safe property access for badge data.
Consider updating _safeGetInvertValue to use file.jsonToData for consistency with the other methods, unless a different approach is required for a specific reason.
```suggestion
bool _safeGetInvertValue(Map<String, dynamic> data) {
try {
return file.jsonToData(data).messages[0].invert ?? false;
} catch (e) {
// If there's an error, default to false
```
</issue_to_address>
### Comment 2
<location> `lib/providers/badge_slot_provider..dart:87-89` </location>
<code_context>
notifyListeners();
}
+
+ bool canTransfer(String badgeKey) {
+ final slot = _badgeKeyToSlot[badgeKey];
+ return slot != null && slot <= 8;
+ }
+
</code_context>
<issue_to_address>
**suggestion:** Magic number for slot limit should be centralized.
Use maxSelectedBadges instead of the hardcoded value to ensure consistency and simplify future updates.
Suggested implementation:
```
static const int maxSelectedBadges = 8;
bool canTransfer(String badgeKey) {
final slot = _badgeKeyToSlot[badgeKey];
return slot != null && slot <= maxSelectedBadges;
}
```
If `maxSelectedBadges` is already defined elsewhere in your codebase, you should use that existing definition instead of introducing a new one. Adjust the code to reference the correct variable or constant as needed.
</issue_to_address>
### Comment 3
<location> `lib/view/widgets/saved_badge_listview.dart:254-22` </location>
<code_context>
+ _hoveredIndex = null;
+ });
+ },
+ onDraggableCanceled: (velocity, offset) {
+ setState(() {
+ _draggingItem = null;
+ _hoveredIndex = null;
+ });
+ },
+ feedback: _buildDragFeedback(badgeKey, slotProvider),
</code_context>
<issue_to_address>
**suggestion:** Consider providing user feedback on failed drag operations.
A visual or haptic indicator would help users understand when and why a drag fails.
Suggested implementation:
```
onDraggableCanceled: (velocity, offset) {
setState(() {
_draggingItem = null;
_hoveredIndex = null;
});
// Provide haptic feedback
HapticFeedback.vibrate();
// Show visual feedback
ScaffoldMessenger.of(context).showSnackBar(
SnackBar(
content: Text('Drag canceled: Item could not be dropped here.'),
duration: Duration(seconds: 2),
),
);
},
```
Make sure to import the following at the top of your file if not already present:
```dart
import 'package:flutter/services.dart';
import 'package:flutter/material.dart';
```
If your widget is not directly under a `Scaffold`, you may need to ensure that `ScaffoldMessenger.of(context)` works as expected, or pass a suitable context.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Hi, I reviewed the actual code changes and ran a consistency pass. I also used AI to speed up the analysis (highly recommended for you as well—paste key code blocks into ChatGPT/Copilot Chat to quickly spot edge cases and suggest diffs).
❗ Must fix before merge
- Two sources of truth for selection (duplicated getters & logic conflict)
InBadgeSlotProvider, both_selectedBadgesand_badgeKeyToSlotare used to represent selection state, and some getters are duplicated/contradict each other:
Set<String> get selectedBadges => _selectedBadges;
Set<String> get selectedBadges => _badgeKeyToSlot.keys.toSet();
bool isSelected(String badgeKey) => _selectedBadges.contains(badgeKey);
bool isSelected(String badgeKey) => _badgeKeyToSlot.containsKey(badgeKey);
bool get canSelectMore => _selectedBadges.length < maxSelectedBadges;
bool get canSelectMore =>
_badgeKeyToSlot.length < maxSelectedBadges && _availableSlots.isNotEmpty;This will compile-fail (duplicate getters) or, if resolved manually, still cause state divergence between _selectedBadges and _badgeKeyToSlot.
Change request: Choose a single source of truth. Given the slot feature, make _badgeKeyToSlot the canonical state and remove _selectedBadges entirely. Then:
selectedBadges→=> _badgeKeyToSlot.keys.toSet()isSelected→=> _badgeKeyToSlot.containsKey(badgeKey)canSelectMore→=> _badgeKeyToSlot.length < maxSelectedBadges && _availableSlots.isNotEmpty
(These conflicts appear directly in the diff view.) (GitHub)
toggleSelectioncontrol flow is inverted in places
Inside the currenttoggleSelection, after the “select” path adds to_selectedBadges, there’s a block that treats an already assigned badge as unselect (“free its slot”), which can’t be reached logically when you just added it. This is a logic bug caused by the dual state.
Change request: With_selectedBadgesremoved, simplify to:
void toggleSelection(String badgeKey) {
// Unselect
if (_badgeKeyToSlot.containsKey(badgeKey)) {
final freed = _badgeKeyToSlot.remove(badgeKey);
if (freed != null) _availableSlots.add(freed);
notifyListeners();
return;
}
// Select
if (_badgeKeyToSlot.length >= maxSelectedBadges || _availableSlots.isEmpty) {
return;
}
final smallest = _availableSlots.reduce((a, b) => a < b ? a : b);
_availableSlots.remove(smallest);
_badgeKeyToSlot[badgeKey] = smallest;
notifyListeners();
}This prevents contradictory transitions and keeps slots consistent. (See the current mixed logic in the provider diff.) (GitHub)
reorderSlotsshould not reset_availableSlotsblindly
Current implementation clears and repopulates_availableSlotsfor every reorder, then iterates allorderedKeysto reassign slots 1..N. This is OK as a full reindex, but you must ensure the pool stays correct for any remaining unassigned slots (e.g., if only 3 badges are selected,_availableSlotsshould be{4..8}afterwards). That is what your loop intends, but it relies on the global reset being correct.
Change request: Keep the full reindex but make it explicit and safe:
void reorderSlots(String fromBadgeKey, String toBadgeKey) {
if (!_badgeKeyToSlot.containsKey(fromBadgeKey) ||
!_badgeKeyToSlot.containsKey(toBadgeKey)) return;
final keys = getSelectionsOrderedBySlot();
keys.remove(fromBadgeKey);
final toIndex = keys.indexOf(toBadgeKey);
keys.insert(toIndex, fromBadgeKey);
_badgeKeyToSlot
..clear()
..addEntries(Iterable.generate(keys.length, (i) => MapEntry(keys[i], i + 1)));
_availableSlots
..clear()
..addAll({1,2,3,4,5,6,7,8}.difference(_badgeKeyToSlot.values.toSet()));
notifyListeners();
}Add a short comment: “Reindex assigned badges to slots 1..N in display order; free the remaining slots into the pool.” (Reordering code is in the provider diff.) (GitHub)
- Guard
getTransferableBadges()against overrun and undefined order
You switched the send path togetTransferableBadges()and it returnstake(8)fromgetSelectionsOrderedBySlot()—good. Ensure the ordering function always sorts by slot, even after any partial state changes. Today it does, but it depends entirely on_badgeKeyToSlot. This ties back to Must-fix #1: once_selectedBadgesis removed, ordering is stable.
Change request: KeepgetSelectionsOrderedBySlot()as the single ordering source and unit-test it for: no selection, 1 selection, N<8, N>8, and after two reorders. (SaveBadgeScreen change is visible in the diff.) (GitHub)
⚠️ Should fix (robustness / UX)
-
Slot bounds & type safety
Where you checkslot <= 8incanTransfer, also ensureslot >= 1. Consider makingmaxSelectedBadgesthe single upper bound to avoid magic numbers creeping elsewhere. -
Atomicity of state updates
Some methods mutate multiple structures (_badgeKeyToSlotand_availableSlots). If you add more logic later (e.g., persistence), wrap updates so that either everything succeeds or nothing does. For now a clear pattern (mutate → notify) is fine, but comment that these two fields must stay in sync. -
UI labels and a11y
Ifsave_badge_card.dartshows slot numbers, ensure they’re accessible (semantics label like “Slot 3 selected”) and localized where needed. (The PR mentions slot numbers on cards; please double-check these widgets.) (GitHub)
🧪 Minimal test plan (manual + unit)
-
Unit tests for provider logic
toggleSelectionadd/remove cycles; verify pool content and assigned slots.reorderSlotswith 2–5 items; after multiple reorders; verify slots become 1..N and pool is{N+1..8}.getTransferableBadgesreturns at most 8 keys in ascending slot order.
-
Manual flow
- Select badges A,B,C → confirm slots 1,2,3 and pool
{4..8}. - Reorder: drag C before A → slots become C=1, A=2, B=3; pool remains
{4..8}. - Deselect B → pool adds
3; select D → D gets3. - Select more than 8 → last selections are rejected gracefully (button disabled or no-op).
- Send flow in Save Badge: check that only the first 8 (by slot) are transferred and in the correct order. (Change visible in
SaveBadgeScreen.) (GitHub)
- Select badges A,B,C → confirm slots 1,2,3 and pool
✨ Nice to have
-
Persist
_badgeKeyToSlotso users don’t lose assignments if they navigate away. -
Add a small provider doc-comment explaining the invariants:
- keys of
_badgeKeyToSlotare the only selected badges, - values are unique in
1..8, _availableSlots={1..8} \ values(_badgeKeyToSlot).
- keys of
Build StatusBuild successful. APKs to test: https://github.com/fossasia/badgemagic-app/actions/runs/18816732001/artifacts/4374214672. ScreenshotsAndroid Screenshots
iPhone Screenshots
iPad Screenshots
|
||||||||||||||||||||||||
-1_home_screen.png?raw=true)
-2_text_badge.png?raw=true){kind=icon}
-3_emoji_badge.png?raw=true){kind=icon}
-4_inverted_emoji_badge.png?raw=true){kind=icon}
-5_saved_badges.png?raw=true)
-6_saved_badges_clicked.png?raw=true)
-7_draw_badge.png?raw=true){kind=icon}
There was a problem hiding this comment.
Pull Request Overview
This PR implements a custom slot provider feature that allows users to select badges into numbered slots (1-8) and reorder them via drag-and-drop functionality. The implementation replaces the simple selection mechanism with a slot-based system that maintains ordering and provides visual feedback.
Key changes:
- Transformed badge selection from a simple set to a slot-based system with numbered assignments
- Added drag-and-drop reordering functionality with visual feedback and auto-scrolling
- Enhanced UI with slot indicators, drag handles, and improved card interactions
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| lib/view/widgets/saved_badge_listview.dart | Converted from StatelessWidget to StatefulWidget, added drag-and-drop reordering with auto-scroll and visual feedback |
| lib/view/widgets/save_badge_card.dart | Added slot overlay indicators, drag handles, safe data access methods, and confirmation dialog for edit actions |
| lib/view/save_badge_screen.dart | Updated transfer logic to use slot-ordered badges instead of simple selection |
| lib/providers/badge_slot_provider..dart | Refactored from simple set-based selection to slot-mapping system with reordering capabilities |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
@nope3472 please make the changes that the slot numbers do not change when users select/deselect a slot. |
There was a problem hiding this comment.
@nope3472 Just change make the slot remains in same position after toogle on and off then we are good to go!
|
@nope3472 please resolve the conflict |
|
Mind sharing screenshots/video of this in action. I'm interested to see the UX |
🎥 Recording 1RECORDING1.webm🎥 Recording 2RECORDING2.webm |
|
@nope3472 this is still not correct when I tested it. Slot number and position should not change at all. Please do a demo today of what you got |
SLOTDECIDER.webm |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| extension FirstOrNullExtension<E> on Iterable<E> { | ||
| E? get firstOrNull => isEmpty ? null : first; | ||
| } |
There was a problem hiding this comment.
The FirstOrNullExtension is defined but never used in the codebase. Consider removing it to reduce code clutter, or use it if it was intended for future functionality.
| ), | ||
| onPressed: () { | ||
| logger.d("BadgeData: ${badgeData.value}"); | ||
| //We can Acrtually call a method to generate the data just by transffering the JSON data |
There was a problem hiding this comment.
Corrected spelling of 'Acrtually' to 'Actually' and 'transffering' to 'transferring'.
| //We can Acrtually call a method to generate the data just by transffering the JSON data | |
| //We can actually call a method to generate the data just by transferring the JSON data |
| // ignore: unused_field | ||
| String? _draggingItem; | ||
| // ignore: unused_field | ||
| int? _hoveredIndex; | ||
|
|
There was a problem hiding this comment.
The fields _draggingItem and _hoveredIndex are marked with // ignore: unused_field but are assigned values in the code (lines 195, 205, 211, 278, 283). If these fields serve a purpose for future features or debugging, consider adding a comment explaining their intent. Otherwise, remove them if they're truly unused.
| // ignore: unused_field | |
| String? _draggingItem; | |
| // ignore: unused_field | |
| int? _hoveredIndex; |
| child: Padding( | ||
| padding: EdgeInsets.only( | ||
| right: 8 | ||
| .w), // Adding some padding to separate text and buttons |
There was a problem hiding this comment.
[nitpick] Comment should end with a period for consistency with other comments in the file.
| .w), // Adding some padding to separate text and buttons | |
| .w), // Adding some padding to separate text and buttons. |
5 participants
Fixes #1459
Changes
Screenshots / Recordings
Checklist:
constants.dartwithout hard coding any value.Summary by Sourcery
Enable users to select badges into numbered slots, rearrange their order via drag-and-drop, and maintain slot metadata for subsequent transfers
New Features:
Enhancements: