Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug report] onDragEnd gesture -> Null check operator used on a null value #601

Closed
EgorK0rshun opened this issue Aug 16, 2023 · 14 comments · Fixed by #620
Closed

[Bug report] onDragEnd gesture -> Null check operator used on a null value #601

EgorK0rshun opened this issue Aug 16, 2023 · 14 comments · Fixed by #620

Comments

@EgorK0rshun
Copy link
Contributor

EgorK0rshun commented Aug 16, 2023

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

flutter doctor -v
[✓] Flutter (Channel stable, 3.10.6, on macOS 13.0.1 22A400 darwin-arm64, locale ru-BY)
    • Flutter version 3.10.6 on channel stable at /Users/k/develop/flutter
    • Upstream repository https://github.com/flutter/flutter.git
    • Framework revision f468f3366c (5 weeks ago), 2023-07-12 15:19:05 -0700
    • Engine revision cdbeda788a
    • Dart version 3.0.6
    • DevTools version 2.23.1

[✓] Android toolchain - develop for Android devices (Android SDK version 33.0.1)
    • Android SDK at /Users/k/Library/Android/sdk
    • Platform android-33, build-tools 33.0.1
    • Java binary at: /Applications/Android Studio.app/Contents/jre/Contents/Home/bin/java
    • Java version OpenJDK Runtime Environment (build 11.0.13+0-b1751.21-8125866)
    • All Android licenses accepted.

[✓] Xcode - develop for iOS and macOS (Xcode 14.2)
    • Xcode at /Applications/Xcode.app/Contents/Developer
    • Build 14C18
    • CocoaPods version 1.11.3

[✓] Chrome - develop for the web
    • Chrome at /Applications/Google Chrome.app/Contents/MacOS/Google Chrome

[✓] Android Studio (version 2021.3)
    • Android Studio at /Applications/Android Studio.app/Contents
    • Flutter plugin can be installed from:
      🔨 https://plugins.jetbrains.com/plugin/9212-flutter
    • Dart plugin can be installed from:
      🔨 https://plugins.jetbrains.com/plugin/6351-dart
    • Java version OpenJDK Runtime Environment (build 11.0.13+0-b1751.21-8125866)

[✓] Android Studio (version 2021.3)
    • Android Studio at /Users/k/Library/Application Support/JetBrains/Toolbox/apps/AndroidStudio/ch-0/213.7172.25.2113.9123335/Android Studio.app/Contents
    • Flutter plugin can be installed from:
      🔨 https://plugins.jetbrains.com/plugin/9212-flutter
    • Dart plugin can be installed from:
      🔨 https://plugins.jetbrains.com/plugin/6351-dart
    • Java version OpenJDK Runtime Environment (build 11.0.13+0-b1751.21-8125866)

[✓] VS Code (version 1.81.0)
    • VS Code at /Applications/Visual Studio Code.app/Contents
    • Flutter extension version 3.56.0

[✓] Connected device (3 available)
    • SM A137F (mobile) • RF8T80AXGPY • android-arm    • Android 13 (API 33)
    • macOS (desktop)   • macos       • darwin-arm64   • macOS 13.0.1 22A400 darwin-arm64
    • Chrome (web)      • chrome      • web-javascript • Google Chrome 115.0.5790.170

[✓] Network resources
    • All expected network resources are available.

• No issues found!

How to reproduce?

Use ExtendedImageGesturePageView.builder and try to execute onDrag gesture. The exception will be throw for onDragEnd callback

Logs

UnhandledFlutterException: Null check operator used on a null value
       at extended_image.src.gesture.page_view.gesture_page_view.dart.ExtendedImageGesturePageViewState.onDragEnd(gesture_page_view.dart:426)
       at extended_image.src.gesture_detector.drag.dart.ExtendedDragGestureRecognizer._checkEnd.<fn>(drag.dart:623)
       at flutter.src.gestures.recognizer.dart.GestureRecognizer.invokeCallback(recognizer.dart:253)
       at extended_image.src.gesture_detector.drag.dart.ExtendedDragGestureRecognizer._checkEnd(drag.dart:623)
       at extended_image.src.gesture_detector.drag.dart.ExtendedDragGestureRecognizer.didStopTrackingLastPointer(drag.dart:526)
       at flutter.src.gestures.recognizer.dart.OneSequenceGestureRecognizer.stopTrackingPointer(recognizer.dart:446)
       at extended_image.src.gesture_detector.drag.dart.ExtendedDragGestureRecognizer._giveUpPointer(drag.dart:535)
       at extended_image.src.gesture_detector.drag.dart.ExtendedDragGestureRecognizer.handleEvent(drag.dart:449)
       at flutter.src.gestures.pointer_router.dart.PointerRouter._dispatch(pointer_router.dart:98)
       at flutter.src.gestures.pointer_router.dart.PointerRouter._dispatchEventToRoutes.<fn>(pointer_router.dart:143)
       at collection-patch.compact_hash.dart._LinkedHashMapMixin.forEach(compact_hash.dart:625)
       at flutter.src.gestures.pointer_router.dart.PointerRouter._dispatchEventToRoutes(pointer_router.dart:141)
       at flutter.src.gestures.pointer_router.dart.PointerRouter.route(pointer_router.dart:127)
       at flutter.src.gestures.binding.dart.GestureBinding.handleEvent(binding.dart:460)
       at flutter.src.gestures.binding.dart.GestureBinding.dispatchEvent(binding.dart:440)
       at flutter.src.rendering.binding.dart.RendererBinding.dispatchEvent(binding.dart:336)
       at flutter.src.gestures.binding.dart.GestureBinding._handlePointerEventImmediately(binding.dart:395)
       at flutter.src.gestures.binding.dart.GestureBinding.handlePointerEvent(binding.dart:357)
       at flutter.src.gestures.binding.dart.GestureBinding._flushPointerEventQueue(binding.dart:314)
       at flutter.src.gestures.binding.dart.GestureBinding._handlePointerDataPacket(binding.dart:295)
       at async.zone.dart._rootRunUnary(zone.dart:1414)
       at async.zone.dart._CustomZone.runUnary(zone.dart:1307)
       at async.zone.dart._CustomZone.runUnaryGuarded(zone.dart:1216)
       at ui.hooks.dart._invoke1(hooks.dart:166)
       at ui.platform_dispatcher.dart.PlatformDispatcher._dispatchPointerDataPacket(platform_dispatcher.dart:361)
       at ui.hooks.dart._dispatchPointerDataPacket(hooks.dart:91)

Example code (optional)

child: Stack(
              children: <Widget>[
                ValueListenableBuilder<Map<String, ImageDownloadingState>>(
                  valueListenable: context.read<Model>().imageDownloadingStatesValueNotifier,
                  builder: (_, Map<String, ImageDownloadingState> imageDownloadingStates, __) {
                    return ExtendedImageGesturePageView.builder(
                      controller: _extendedPageController,
                      physics: _ImageScrollPhysics(),
                      itemCount: _imagePaths.length,
                      onPageChanged: (int index) {
                   _onPageChanged();
                        _startPrecachingImages(index);
                      },
                      canScrollPage: (GestureDetails? details) {
                        if (details?.totalScale == 1.0) {
                          return true;
                        }
                        return false;
                      },
                      itemBuilder: (BuildContext context, int index) {
                        final Widget image = AnimatedScale(
                          key: Key(_imagePaths[index]),
                          duration: _deleteAnimationDuration,
                          scale: _someConditionalForImage[index] ? 0 : 1,
                          child: _someConditional
                              ? const FractionallySizedBox(
                                  widthFactor: 0.75,
                                  child: ImagePreviewProgress(
                                    size: double.infinity,
                                  ),
                                )
                              : ExtendedImage(
                                  key: Key(_imagePaths[index] + _shouldBeFullscreen.toString()),
                                  image: ExtendedFileImageProvider(File(_imagePaths[index])),
                                  fit: BoxFit.contain,
                                  mode: ExtendedImageMode.gesture,
                                  initGestureConfigHandler: (ExtendedImageState state) {
                                    return GestureConfig(
                                      animationMinScale: 1.0,
                                      minScale: 1,
                                      inPageView: true,
                                      cacheGesture: false,
                                    );
                                  },
                                  loadStateChanged: (ExtendedImageState state) {
                                    switch (state.extendedImageLoadState) {
                                      case LoadState.loading:
                                        return null;
                                      case LoadState.completed:
                                        _imageLoaded = true;
                                        return null;
                                      case LoadState.failed:
                                        _imageLoaded = false;
                                        // remove memory cached
                                        state.imageProvider.evict();
                                        return const FractionallySizedBox(
                                          widthFactor: 0.75,
                                          child: ImagePreviewError(
                                            size: double.infinity,
                                          ),
                                        );
                                    }
                                  },
                                ),
                        );
                        ...
}
                       
    void _startPrecachingImages(int currentImageIndex) {
    final List<String> filesToPrecache = <String>[];
    if (currentImageIndex < _imagePaths.length - 1) {
      filesToPrecache.add(_imagePaths[currentImageIndex + 1]);
    }
    if (currentImageIndex != 0) {
      filesToPrecache.add(_imagePaths[currentImageIndex - 1]);
    }
    for (String path in filesToPrecache) {
      precacheImage(ExtendedFileImageProvider(File(path)), context);
    }
  } 
                   

Contact

https://github.com/EgorK0rshun,
[email protected]

@zmtzawqlp
Copy link
Member

could you provide a simple runnable demo to reproduce your issue?

@EgorK0rshun
Copy link
Contributor Author

EgorK0rshun commented Aug 16, 2023

@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
Copy link
Member

zmtzawqlp commented Aug 17, 2023

_drag should not be null at that moment , it's better to reproduce it so that i can bug it.

@EgorK0rshun
Copy link
Contributor Author

EgorK0rshun commented Aug 17, 2023

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
Copy link
Member

zmtzawqlp commented Aug 18, 2023

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

i mean that it should not be null at that moment(onDragEnd). _disposeDrag callback is called after onDragEnd

@EgorK0rshun
Copy link
Contributor Author

@zmtzawqlp , shouldn't, but there is. analytics and stacktrace of the exception says that this place is null

@zmtzawqlp
Copy link
Member

@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
Copy link
Contributor Author

@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
Copy link
Contributor Author

@zmtzawqlp , please review
#620

@zmtzawqlp
Copy link
Member

@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
Copy link
Contributor Author

EgorK0rshun commented Sep 6, 2023

@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
Copy link
Contributor Author

EgorK0rshun commented Sep 6, 2023

Снимок экрана 2023-09-06 в 15 24 28 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//pull/620 ) , the error disappears. the smoothness of the graphics is ensured by the gradual rolling out of the new version.

@svyatoslavb-digitalchemy

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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants