zmtzawqlp
commented
1 year ago could you provide a simple runnable demo to reproduce your issue?
Closed EgorK0rshun closed 1 year ago
zmtzawqlp
commented
1 year ago could you provide a simple runnable demo to reproduce your issue?
EgorK0rshun
commented
1 year ago @zmtzawqlp , unfortunately no. This is reproduced by real users. i only have stats. And in code of library
onDragUpdate have ambiguous implementation:
`void onDragUpdate(DragUpdateDetails details) {
// _drag might be null if the drag activity ended and called _disposeDrag.
assert(_hold == null || _drag == null);
//final Offset delta = details.delta;
if (!widget.canScrollPage(extendedImageGestureState?.gestureDetails)) {
return;
}
_drag?.update(details);
// return;
// if (extendedImageGestureState != null) {
// final GestureDetails? gestureDetails =
// extendedImageGestureState!.gestureDetails;
// if (gestureDetails != null) {
// final int currentPage = pageController.page!.round();
// // bool pageChanging = false;
// //
// // if (widget.scrollDirection == Axis.horizontal) {
// // if (delta.dx != 0.0) {
// // if (delta.dx < 0) {
// // pageChanging = pageController.page > currentPage;
// // } else {
// // pageChanging = pageController.page < currentPage;
// // }
// // }
// // } else {
// // if (delta.dy != 0.0) {
// // if (delta.dy < 0) {
// // pageChanging = pageController.page < currentPage;
// // } else {
// // pageChanging = pageController.page > currentPage;
// // }
// // }
// // }
// if ((gestureDetails.movePage(delta, widget.scrollDirection) ||
// (currentPage != pageController.page)) &&
// widget.canMovePage(gestureDetails)) {
// _drag?.update(details);
// } else {
// if (currentPage == pageController.page) {
// extendedImageGestureState!.gestureDetails = GestureDetails(
// offset: gestureDetails.offset! +
// delta *
// extendedImageGestureState!.imageGestureConfig!.speed,
// totalScale: gestureDetails.totalScale,
// gestureDetails: gestureDetails);
// }
// }
// } else {
// _drag?.update(details);
// }
// } else {
// _drag?.update(details);
// }
}but ok. Its work. And here used ? operator ( _drag?.update(details);).
In the method onDragEnd you use bang operator:
// _drag might be null if the drag activity ended and called _disposeDrag.
assert(_hold == null || _drag == null);
if (!widget.canScrollPage(extendedImageGestureState?.gestureDetails)) {
_drag!.end(DragEndDetails(primaryVelocity: 0.0));
return;
}
_drag!.end(details);
assert(_drag == null);
// return;
// DragEndDetails temp = details;
// if (extendedImageGestureState != null) {
// final GestureDetails? gestureDetails =
// extendedImageGestureState!.gestureDetails;
// final int currentPage = pageController.page!.round();
// final bool movePage = pageController.page != currentPage;
// if (!widget.canMovePage(gestureDetails)) {
// //stop
// temp = DragEndDetails(primaryVelocity: 0.0);
// }
// /// stop when zoom in, so that it will not move to next/previous page
// if (!movePage &&
// gestureDetails != null &&
// gestureDetails.totalScale! > 1.0 &&
// (gestureDetails.computeHorizontalBoundary ||
// gestureDetails.computeVerticalBoundary)) {
// //stop
// temp = DragEndDetails(primaryVelocity: 0.0);
// // get magnitude from gesture velocity
// final double magnitude = details.velocity.pixelsPerSecond.distance;
// // do a significant magnitude
// if (magnitude.greaterThanOrEqualTo(minMagnitude)) {
// Offset direction = details.velocity.pixelsPerSecond /
// magnitude *
// (extendedImageGestureState!.imageGestureConfig!.inertialSpeed);
// if (widget.scrollDirection == Axis.horizontal) {
// direction = Offset(direction.dx, 0.0);
// } else {
// direction = Offset(0.0, direction.dy);
// }
// _gestureAnimation.animationOffset(
// gestureDetails.offset, gestureDetails.offset! + direction);
// }
// }
// }
// _drag!.end(temp);
// assert(_drag == null);
}maybe error here _drag!.end(details);
zmtzawqlp
commented
1 year ago _drag should not be null at that moment , it's better to reproduce it so that i can bug it.
EgorK0rshun
commented
1 year ago no, drug = null, because u a set null when start onDragStart gesture:
void _disposeDrag() {
_drag = null;
}let me fork it, fix this string, look at the stats, and if it helps, I'll make a requisition for the extended_image
zmtzawqlp
commented
1 year ago no, drug = null, because u a set null when start
onDragStartgesture:void _disposeDrag() { _drag = null; }let me fork it, fix this string, look at the stats, and if it helps, I'll make a requisition for the extended_image
i mean that it should not be null at that moment(onDragEnd). _disposeDrag callback is called after onDragEnd
EgorK0rshun
commented
1 year ago @zmtzawqlp , shouldn't, but there is. analytics and stacktrace of the exception says that this place is null
zmtzawqlp
commented
1 year ago @zmtzawqlp , shouldn't, but there is. analytics and stacktrace of the exception says that this place is null
so we should better find out the case of this issue, not simplely use '?' instead of '!'.
EgorK0rshun
commented
1 year ago @zmtzawqlp , so it is, after all, for the reason that he wrote above: at the beginning of the gesture, _drag is dispositions. you can debug. according to the order of calls, _drag becomes null in the course of the gesture execution
EgorK0rshun
commented
1 year ago @zmtzawqlp , please review https://github.com/fluttercandies/extended_image/pull/620
zmtzawqlp
commented
1 year ago @zmtzawqlp , please review #620
Hi guy, thank you very much for reporting this issue. and I think we should find the real reason for this problem. I only received a report about this issue from you for now. I added an assert here because it should not be null. If it happens, we should find the real reason to solve it instead of avoiding it directly.
EgorK0rshun
commented
1 year ago @zmtzawqlp
I described the reason above. The changes in this request helped to get rid of a bug that several thousand users encountered. It's all about the _disposeDrag method. in all references to the drag in this class is used ? , only for onDragUpdate you use !
Flutter generally recommends avoiding bang operators.
You can try to look deeper if you are not sure about the correctness of my wording, but I would ask you not to delay, because maintaining a fork of a third-party library is quite expensive in terms of time and resources. In the current implementation, everything works correctly. Doesn't lead to any additional errors.
EgorK0rshun
commented
1 year ago here is a diagram showing that when switching to a new version of the application, in which the line was corrected, as in the attached request ( https://github.com/fluttercandies/extended_image/pull/620 ) , the error disappears. the smoothness of the graphics is ensured by the gradual rolling out of the new version.
svyatoslavb-digitalchemy
commented
1 year ago Hi, It is not recommended to use the bang operator to interact with gestures. When checking using null-aware access, the method will simply fail if the value is null.
zmtzawqlp
commented
1 year ago Those codes are from https://github.com/flutter/flutter/blob/e8ff2aed31692142fb3494e2ee8b5b11d3e28693/packages/flutter/lib/src/widgets/scrollable.dart#L831
it is already use '?' here, and I believe our modification won't introduce any other issues. Thanks again for your pull request and explanation. LGTM
Version
8.0.2
Platforms
dart, Android, iOS
Device Model
All devices, Galaxy A12, Galaxy A32, Galaxy A13, Redmi 8A, Redmi 9C, Redmi Note 8
flutter info
How to reproduce?
Use
ExtendedImageGesturePageView.builderand try to executeonDraggesture. The exception will be throw foronDragEndcallbackLogs
Example code (optional)
Contact
https://github.com/EgorK0rshun, korshun.dev@gmail.com