feat(canvas): add canvas centering for all or single image, add re-al…#109
Open
Jaaco wants to merge 1 commit into
Open
feat(canvas): add canvas centering for all or single image, add re-al…#109Jaaco wants to merge 1 commit into
Jaaco wants to merge 1 commit into
Conversation
…ign for all images
📝 WalkthroughWalkthroughCanvas alignment utilities are added to app state (reAlignAllImages, centerCanvasOnImage, centerCanvasOnAllImages) and integrated into UI components (panel list item, main frame list, zoom controls). Image utilities are refactored with formatting changes. State mutations now validate bounds and clear selections appropriately. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant UI as UI Components
participant AppState as AppState
participant Canvas as Canvas Controller
User->>UI: Double-tap image in panel
UI->>AppState: centerCanvasOnImage(imageId)
AppState->>Canvas: Compute center & update position
User->>UI: Click align button
UI->>AppState: reAlignAllImages()
AppState->>AppState: Compute center of all images
AppState->>Canvas: Reposition images, recenter canvas
User->>UI: Click center button in zoom
UI->>AppState: centerCanvasOnAllImages()
AppState->>Canvas: Center on average of all images
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
apps/flites/lib/widgets/tool_controls/zoom_controls.dart (1)
13-13: Remove stale commented layout code.Line 13 (
// width: 64,) looks like dead code and makes intent less clear.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/flites/lib/widgets/tool_controls/zoom_controls.dart` at line 13, Remove the stale commented layout code "// width: 64," from the ZoomControls widget file to keep the layout intent clear; locate the commented line inside apps/flites/lib/widgets/tool_controls/zoom_controls.dart (near the ZoomControls widget/build method) and delete that single-line comment so only active layout properties remain.apps/flites/lib/core/app_state.dart (2)
138-153: Comment/behavior mismatch in centering docs.The comment says “currently selected images,” but the method computes the center of all images in the current row. Please align wording with actual behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/flites/lib/core/app_state.dart` around lines 138 - 153, The doc comment for centerCanvasOnAllImages/_centerOfAllImages is misleading: it says "currently selected images" but the implementation uses _getCurrentRow().images (i.e., all images in the current row). Update the comments to accurately describe behavior (e.g., "Centers the canvas on the average of all images in the current row" and "The non-scaled center point of all images in the current row"), or if you intended selected images instead, change _centerOfAllImages to iterate over the selected set; reference the methods centerCanvasOnAllImages and _centerOfAllImages and the call to _getCurrentRow() to locate the change.
108-123: BatchreAlignAllImagesupdates to avoid repeated full-row rewrites.Current implementation updates state once per image via
saveImageChanges, which causes avoidable O(n²) work and multiple reactive updates.♻️ Proposed refactor
void reAlignAllImages() { - final center = _centerOfAllImages(); + final currentRow = _getCurrentRow(); + final center = _centerOfAllImages(); if (center == null) { return; } - for (final image in _getCurrentRow().images) { - final offsetFromCenter = image.center - center; - final newPosition = image.positionOnCanvas - offsetFromCenter; - - saveImageChanges(image.copyWith(positionOnCanvas: newPosition)); - } + final updatedImages = currentRow.images.map((image) { + final offsetFromCenter = image.center - center; + final newPosition = image.positionOnCanvas - offsetFromCenter; + return image.copyWith(positionOnCanvas: newPosition); + }).toList(); + + _updateCurrentRow(currentRow.copyWith(images: updatedImages)); centerCanvasOnAllImages(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/flites/lib/core/app_state.dart` around lines 108 - 123, reAlignAllImages is calling saveImageChanges per image causing repeated full-row/state writes and O(n²) reactive work; instead compute the new positions using _centerOfAllImages and _getCurrentRow(), build a list/map of updated Image models (e.g., updatedImages) and perform a single batched state update (create or use a batch method such as saveImagesChangesBatch or updateCurrentRowImages) to replace the row's images in one write, then call centerCanvasOnAllImages() once; update or add a batch-saving helper if none exists and reference reAlignAllImages, saveImageChanges, _getCurrentRow, _centerOfAllImages, and centerCanvasOnAllImages when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/flites/lib/widgets/project_file_list/widgets/main_frame_list.dart`:
- Around line 77-80: The IconBtn rendering the re-align control uses the wrong
tooltip text; replace the tooltip value currently set to
context.l10n.toggleVisibility with a dedicated localized string (e.g.,
context.l10n.realignAllImages or similar) for the reAlignAllImages action so the
tooltip matches the control; if that localization key does not exist, add it to
the localization files and use the new key in the IconBtn next to the existing
reference to appState.reAlignAllImages to ensure correct text is shown.
In `@apps/flites/lib/widgets/tool_controls/zoom_controls.dart`:
- Around line 49-51: The IconButton using onPressed:
appState.centerCanvasOnAllImages currently sets tooltip: context.l10n.zoomOut
which is incorrect; replace that tooltip with a dedicated localization key (e.g.
context.l10n.centerCanvas or context.l10n.centerCanvasOnAllImages) and add the
corresponding entry to the app's localization files (arb/intl) and translations;
ensure the new localized string is used for tooltip (and semanticsLabel if
applicable) so the centerCanvasOnAllImages action has the correct
hover/accessibility text.
---
Nitpick comments:
In `@apps/flites/lib/core/app_state.dart`:
- Around line 138-153: The doc comment for
centerCanvasOnAllImages/_centerOfAllImages is misleading: it says "currently
selected images" but the implementation uses _getCurrentRow().images (i.e., all
images in the current row). Update the comments to accurately describe behavior
(e.g., "Centers the canvas on the average of all images in the current row" and
"The non-scaled center point of all images in the current row"), or if you
intended selected images instead, change _centerOfAllImages to iterate over the
selected set; reference the methods centerCanvasOnAllImages and
_centerOfAllImages and the call to _getCurrentRow() to locate the change.
- Around line 108-123: reAlignAllImages is calling saveImageChanges per image
causing repeated full-row/state writes and O(n²) reactive work; instead compute
the new positions using _centerOfAllImages and _getCurrentRow(), build a
list/map of updated Image models (e.g., updatedImages) and perform a single
batched state update (create or use a batch method such as
saveImagesChangesBatch or updateCurrentRowImages) to replace the row's images in
one write, then call centerCanvasOnAllImages() once; update or add a
batch-saving helper if none exists and reference reAlignAllImages,
saveImageChanges, _getCurrentRow, _centerOfAllImages, and
centerCanvasOnAllImages when making the change.
In `@apps/flites/lib/widgets/tool_controls/zoom_controls.dart`:
- Line 13: Remove the stale commented layout code "// width: 64," from the
ZoomControls widget file to keep the layout intent clear; locate the commented
line inside apps/flites/lib/widgets/tool_controls/zoom_controls.dart (near the
ZoomControls widget/build method) and delete that single-line comment so only
active layout properties remain.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/flites/lib/core/app_state.dartapps/flites/lib/ui/panel/controls/panel_list_item.dartapps/flites/lib/utils/flites_image_factory.dartapps/flites/lib/utils/png_utils.dartapps/flites/lib/widgets/project_file_list/widgets/main_frame_list.dartapps/flites/lib/widgets/tool_controls/zoom_controls.dart
Comment on lines
+77
to
+80
| IconBtn( | ||
| icon: Icons.center_focus_strong, | ||
| tooltip: context.l10n.toggleVisibility, | ||
| onPressed: appState.reAlignAllImages, |
There was a problem hiding this comment.
Re-align button tooltip is misleading.
The new reAlignAllImages control uses toggleVisibility text, which does not describe the action. Please use a dedicated localized tooltip for re-alignment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/flites/lib/widgets/project_file_list/widgets/main_frame_list.dart`
around lines 77 - 80, The IconBtn rendering the re-align control uses the wrong
tooltip text; replace the tooltip value currently set to
context.l10n.toggleVisibility with a dedicated localized string (e.g.,
context.l10n.realignAllImages or similar) for the reAlignAllImages action so the
tooltip matches the control; if that localization key does not exist, add it to
the localization files and use the new key in the IconBtn next to the existing
reference to appState.reAlignAllImages to ensure correct text is shown.
Comment on lines
+49
to
+51
| IconButton( | ||
| tooltip: context.l10n.zoomOut, | ||
| onPressed: appState.centerCanvasOnAllImages, |
There was a problem hiding this comment.
Center action has the wrong tooltip text.
Line 50 uses zoomOut for a centering action, so hover text/accessibility hint is misleading. Please wire a dedicated localized tooltip for “center canvas”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/flites/lib/widgets/tool_controls/zoom_controls.dart` around lines 49 -
51, The IconButton using onPressed: appState.centerCanvasOnAllImages currently
sets tooltip: context.l10n.zoomOut which is incorrect; replace that tooltip with
a dedicated localization key (e.g. context.l10n.centerCanvas or
context.l10n.centerCanvasOnAllImages) and add the corresponding entry to the
app's localization files (arb/intl) and translations; ensure the new localized
string is used for tooltip (and semanticsLabel if applicable) so the
centerCanvasOnAllImages action has the correct hover/accessibility text.
Collaborator
|
Generally LGTM. But the issues coderabbit flagged are valid about the tooltip and |
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
2 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.
Description
Type of Change
Summary by CodeRabbit
New Features
Bug Fixes
Refactor