fixed: reorder layer bug #256
Merged
Conversation
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
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR patches an off-by-one issue in the layer reordering flow by inserting index adjustment logic into the onReorder callback, preventing null check errors when dragging items downward. Sequence diagram for updated layer reordering logicsequenceDiagram
actor User
participant "ReorderLayerSheet"
participant "onReorder callback"
User->>"ReorderLayerSheet": Drag layer from oldIndex to newIndex
"ReorderLayerSheet"->>"onReorder callback": Check if oldIndex == 0 or newIndex == 0
alt oldIndex < newIndex
"ReorderLayerSheet"->>"onReorder callback": newIndex -= 1
end
"ReorderLayerSheet"->>"onReorder callback": Call onReorder(oldIndex, newIndex)
File-Level Changes
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 there - I've reviewed your changes - here's some feedback:
- Consider adding explicit bounds checking for newIndex (e.g., clamping it to the list length) to avoid potential out-of-range errors when dragging items to the very end.
- The early return for index 0 is a bit implicit—extract the magic number into a named constant or add a comment explaining why reordering at index 0 is disallowed to improve readability.
- To make the adjustment logic clearer, factor the
if (oldIndex < newIndex) newIndex -= 1into a small helper or annotate it to explain that it compensates for the removed item's shift.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider adding explicit bounds checking for newIndex (e.g., clamping it to the list length) to avoid potential out-of-range errors when dragging items to the very end.
- The early return for index 0 is a bit implicit—extract the magic number into a named constant or add a comment explaining why reordering at index 0 is disallowed to improve readability.
- To make the adjustment logic clearer, factor the `if (oldIndex < newIndex) newIndex -= 1` into a small helper or annotate it to explain that it compensates for the removed item's shift.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
|
@mariobehling can you review and merge this pr? |
Contributor
Author
|
@hpdang can review and merge it please? |
Vishveshwara
approved these changes
Nov 2, 2025
There was a problem hiding this comment.
Pull Request Overview
This PR fixes index calculation in the layer reordering functionality by adding the standard Flutter ReorderableListView index adjustment logic. When dragging items downward in a ReorderableListView, the newIndex needs to be decremented by 1 to account for the item being removed from its original position before insertion.
Key changes:
- Added index adjustment logic when
oldIndex < newIndexin theonReordercallback
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
145
to
+151
| if (oldIndex == 0 || newIndex == 0) { | ||
| return; | ||
| } | ||
|
|
||
| if (oldIndex < newIndex) { | ||
| newIndex -= 1; | ||
| } |
There was a problem hiding this comment.
The check for newIndex == 0 should occur after the index adjustment on line 150. Currently, if an item is dragged downward where the adjusted newIndex would be 0, it won't be caught by the guard clause. Move the index adjustment before the guard clause, or add an additional check after the adjustment: if (oldIndex < newIndex) { newIndex -= 1; if (newIndex == 0) return; }
Contributor
Build StatusBuild successful. APKs to test: https://github.com/fossasia/magic-epaper-app/actions/runs/18337650160/artifacts/4476033112. ScreenshotsAndroid Screenshots
iPhone Screenshots
iPad Screenshots
|
-1_display_selection.png?raw=true)
-2_sidebar.png?raw=true)
-3_ndef_screen.png?raw=true)
-4_filter_selection.png?raw=true)
-5_barcode_screen.png?raw=true)
-6_Barcode_added.png?raw=true)
-7_Templates_screen.png?raw=true)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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: #255 - Reorder Layer functionality fails with null check error when dragging items downward
Added index adjustment logic in the onReorder callback
Screen Record:
Screen.Recording.2025-10-08.at.1.03.59.PM.mov
Summary by Sourcery
Bug Fixes: