Feat cover image reposition(#2462) #8102
Open
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
…DesktopCoverAlignState
…mageStreamListener.
Reviewer's GuideThis PR adds a desktop/web cover image repositioning feature by introducing a draggable alignment component, integrating new alignment UI into existing cover widgets, extending the data model to store cover offset, and plumbing through the image rendering pipeline to support custom builders. Sequence diagram for saving cover image alignmentsequenceDiagram
actor User
participant DocumentCoverState
participant DesktopCoverAlignController
participant DocumentHeaderBlockKeys
participant App (Data Store)
User->>DocumentCoverState: Drag and reposition cover image
DocumentCoverState->>DesktopCoverAlignController: changeAlign(x, y)
User->>DocumentCoverState: Click 'Save'
DocumentCoverState->>DesktopCoverAlignController: getAlignAttribute()
DocumentCoverState->>App (Data Store): Save cover_offset attribute
App (Data Store)-->>DocumentCoverState: Confirm save
DocumentCoverState-->>User: Update cover image position
Class diagram for cover image alignment featureclassDiagram
class DocumentHeaderBlockKeys {
<<static>>
+coverType : String
+coverDetails : String
+icon : String
+coverOffset : String
}
class DesktopCoverAlignController {
-_initialAlignment : Alignment
-_adjustedAlign : Alignment
+alignment : Alignment
+reset()
+cancel()
+changeAlign(x: double, y: double)
+isModified : bool
+getAlignAttribute() : String
}
class DesktopCoverAlign {
+controller : DesktopCoverAlignController
+imageProvider : ImageProvider
+fit : BoxFit
+alignEnable : bool
}
class DocumentCoverState {
+isAlignOpen : bool
+coverAlignController : DesktopCoverAlignController?
+switchAlignMode()
+cancelCoverAlign()
+saveCoverAlign()
}
DocumentCoverState --> DesktopCoverAlignController
DesktopCoverAlign --> DesktopCoverAlignController
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
PirateBrook
changed the title
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @PirateBrook - I've reviewed your changes - here's some feedback:
- The new onChangeCover signature with a triple tuple is hard to follow—consider introducing a small Cover value class or data object to encapsulate type, details, and offset instead of passing three separate values.
- There’s a lot of repeated logic for wrapping images (
Image.asset,Image.file, network) with DesktopCoverAlign; extracting a single helper that accepts an ImageProvider and returns the aligned widget would reduce duplication. - The DesktopCoverAlign widget manually manages ImageStream and listeners, which is verbose and error-prone; consider leveraging Flutter’s built-in Image widget callbacks or a simpler approach to avoid potential memory leaks.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new onChangeCover signature with a triple tuple is hard to follow—consider introducing a small Cover value class or data object to encapsulate type, details, and offset instead of passing three separate values.
- There’s a lot of repeated logic for wrapping images (`Image.asset`, `Image.file`, network) with DesktopCoverAlign; extracting a single helper that accepts an ImageProvider and returns the aligned widget would reduce duplication.
- The DesktopCoverAlign widget manually manages ImageStream and listeners, which is verbose and error-prone; consider leveraging Flutter’s built-in Image widget callbacks or a simpler approach to avoid potential memory leaks.
## Individual Comments
### Comment 1
<location> `frontend/appflowy_flutter/lib/plugins/document/presentation/editor_plugins/header/desktop_cover_align.dart:32` </location>
<code_context>
+
+ Alignment get alignment => _adjustedAlign;
+
+ void reset() {
+ _adjustedAlign = Alignment.center;
+ notifyListeners();
+ }
</code_context>
<issue_to_address>
reset() always sets alignment to center, not to initial value.
reset() should set _adjustedAlign to _initialAlignment to properly restore the original alignment.
</issue_to_address>
### Comment 2
<location> `frontend/appflowy_flutter/lib/plugins/document/presentation/editor_plugins/header/desktop_cover_align.dart:42` </location>
<code_context>
+ notifyListeners();
+ }
+
+ void changeAlign(double x, double y) {
+ _adjustedAlign = Alignment(x, y);
+ }
+
</code_context>
<issue_to_address>
changeAlign does not notify listeners after updating alignment.
Please add notifyListeners() at the end of changeAlign to ensure UI updates when alignment changes.
</issue_to_address>
### Comment 3
<location> `frontend/appflowy_flutter/lib/plugins/document/presentation/editor_plugins/header/desktop_cover_align.dart:172` </location>
<code_context>
+ final frameRatio = _frameSize!.aspectRatio;
+ final isVertical = imageRatio < frameRatio;
+
+ final imageFrameHeight =
+ _frameSize!.width / _imageSize!.width * _imageSize!.height;
+ final imageFrameWidth =
+ _frameSize!.height / _imageSize!.height * _imageSize!.width;
</code_context>
<issue_to_address>
Potential division by zero in imageFrameHeight and imageFrameWidth calculations.
Add checks to ensure _imageSize!.width and _imageSize!.height are not zero before performing these calculations.
</issue_to_address>
### Comment 4
<location> `frontend/appflowy_flutter/lib/plugins/document/presentation/editor_plugins/header/desktop_cover_align.dart:181` </location>
<code_context>
+
+ if (isVertical) {
+ final targetY = y + offset.dy / exceedHeight * 2;
+ if (targetY >= -1 && targetY <= 1) {
+ y = targetY;
+ }
+ } else {
</code_context>
<issue_to_address>
No clamping for x/y values outside [-1, 1] range.
Clamping targetX and targetY to [-1, 1] would ensure the UI remains responsive even with large or fast drags.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
if (isVertical) {
final targetY = y + offset.dy / exceedHeight * 2;
if (targetY >= -1 && targetY <= 1) {
y = targetY;
}
} else {
final targetX = x + offset.dx / exceedWidth * 2;
if (targetX >= -1 && targetX <= 1) {
x = targetX;
}
}
=======
if (isVertical) {
final targetY = y + offset.dy / exceedHeight * 2;
y = targetY.clamp(-1.0, 1.0);
} else {
final targetX = x + offset.dx / exceedWidth * 2;
x = targetX.clamp(-1.0, 1.0);
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 5
<location> `frontend/appflowy_flutter/lib/plugins/document/presentation/editor_plugins/header/document_cover_widget.dart:330` </location>
<code_context>
if (cover != null) {
attributes[DocumentHeaderBlockKeys.coverType] = cover.$1.toString();
attributes[DocumentHeaderBlockKeys.coverDetails] = cover.$2;
+ attributes[DocumentHeaderBlockKeys.coverOffset] = cover.$3;
}
if (icon != null) {
</code_context>
<issue_to_address>
coverOffset is always set, even if null.
Only assign attributes[DocumentHeaderBlockKeys.coverOffset] if cover.$3 is not null to avoid introducing null values into the attribute map.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
if (cover != null) {
attributes[DocumentHeaderBlockKeys.coverType] = cover.$1.toString();
attributes[DocumentHeaderBlockKeys.coverDetails] = cover.$2;
attributes[DocumentHeaderBlockKeys.coverOffset] = cover.$3;
}
=======
if (cover != null) {
attributes[DocumentHeaderBlockKeys.coverType] = cover.$1.toString();
attributes[DocumentHeaderBlockKeys.coverDetails] = cover.$2;
if (cover.$3 != null) {
attributes[DocumentHeaderBlockKeys.coverOffset] = cover.$3;
}
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 6
<location> `frontend/appflowy_flutter/lib/plugins/document/presentation/editor_plugins/header/desktop_cover.dart:66` </location>
<code_context>
+
+ @override
+ void didUpdateWidget(covariant DesktopCover oldWidget) {
+ if (widget.coverDetails != oldWidget.coverDetails) {
+ coverAlignController.reset();
+ }
+ super.didUpdateWidget(oldWidget);
</code_context>
<issue_to_address>
reset() on coverAlignController may not restore previous alignment.
Currently, reset() always sets alignment to center, which may not match the new coverDetails. Please update the controller to initialize alignment based on the new coverDetails.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+32
to
+33
| void reset() { | ||
| _adjustedAlign = Alignment.center; |
There was a problem hiding this comment.
issue (bug_risk): reset() always sets alignment to center, not to initial value.
reset() should set _adjustedAlign to _initialAlignment to properly restore the original alignment.
Comment on lines
+42
to
+43
| void changeAlign(double x, double y) { | ||
| _adjustedAlign = Alignment(x, y); |
There was a problem hiding this comment.
issue (bug_risk): changeAlign does not notify listeners after updating alignment.
Please add notifyListeners() at the end of changeAlign to ensure UI updates when alignment changes.
Comment on lines
+172
to
+173
| final imageFrameHeight = | ||
| _frameSize!.width / _imageSize!.width * _imageSize!.height; |
There was a problem hiding this comment.
issue (bug_risk): Potential division by zero in imageFrameHeight and imageFrameWidth calculations.
Add checks to ensure _imageSize!.width and _imageSize!.height are not zero before performing these calculations.
Comment on lines
+179
to
+189
| if (isVertical) { | ||
| final targetY = y + offset.dy / exceedHeight * 2; | ||
| if (targetY >= -1 && targetY <= 1) { | ||
| y = targetY; | ||
| } | ||
| } else { | ||
| final targetX = x + offset.dx / exceedWidth * 2; | ||
| if (targetX >= -1 && targetX <= 1) { | ||
| x = targetX; | ||
| } | ||
| } |
There was a problem hiding this comment.
suggestion (bug_risk): No clamping for x/y values outside [-1, 1] range.
Clamping targetX and targetY to [-1, 1] would ensure the UI remains responsive even with large or fast drags.
Suggested change
| if (isVertical) { | |
| final targetY = y + offset.dy / exceedHeight * 2; | |
| if (targetY >= -1 && targetY <= 1) { | |
| y = targetY; | |
| } | |
| } else { | |
| final targetX = x + offset.dx / exceedWidth * 2; | |
| if (targetX >= -1 && targetX <= 1) { | |
| x = targetX; | |
| } | |
| } | |
| if (isVertical) { | |
| final targetY = y + offset.dy / exceedHeight * 2; | |
| y = targetY.clamp(-1.0, 1.0); | |
| } else { | |
| final targetX = x + offset.dx / exceedWidth * 2; | |
| x = targetX.clamp(-1.0, 1.0); | |
| } |
Comment on lines
330
to
334
| if (cover != null) { | ||
| attributes[DocumentHeaderBlockKeys.coverType] = cover.$1.toString(); | ||
| attributes[DocumentHeaderBlockKeys.coverDetails] = cover.$2; | ||
| attributes[DocumentHeaderBlockKeys.coverOffset] = cover.$3; | ||
| } |
There was a problem hiding this comment.
suggestion (bug_risk): coverOffset is always set, even if null.
Only assign attributes[DocumentHeaderBlockKeys.coverOffset] if cover.$3 is not null to avoid introducing null values into the attribute map.
Suggested change
| if (cover != null) { | |
| attributes[DocumentHeaderBlockKeys.coverType] = cover.$1.toString(); | |
| attributes[DocumentHeaderBlockKeys.coverDetails] = cover.$2; | |
| attributes[DocumentHeaderBlockKeys.coverOffset] = cover.$3; | |
| } | |
| if (cover != null) { | |
| attributes[DocumentHeaderBlockKeys.coverType] = cover.$1.toString(); | |
| attributes[DocumentHeaderBlockKeys.coverDetails] = cover.$2; | |
| if (cover.$3 != null) { | |
| attributes[DocumentHeaderBlockKeys.coverOffset] = cover.$3; | |
| } | |
| } |
Comment on lines
+66
to
+67
| if (widget.coverDetails != oldWidget.coverDetails) { | ||
| coverAlignController.reset(); |
There was a problem hiding this comment.
issue (bug_risk): reset() on coverAlignController may not restore previous alignment.
Currently, reset() always sets alignment to center, which may not match the new coverDetails. Please update the controller to initialize alignment based on the new coverDetails.
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.
Feature Preview
related feat #2462
Kapture.2025-07-02.at.18.35.13.webm
PR Checklist
Summary by Sourcery
Enable repositioning of document cover images on desktop and web by introducing an alignment mode, draggable reposition controls, and persistent alignment offsets.
New Features:
Enhancements: