hm21 / pro_image_editor

The pro_image_editor is a Flutter widget designed for image editing within your application. It provides a flexible and convenient way to integrate image editing capabilities into your Flutter project.
https://hm21.github.io/pro_image_editor/
BSD 3-Clause "New" or "Revised" License
119 stars 69 forks source link

[Bug]: closeWarning() crashes if in a dialog and you close the dialog. #140

Closed sgehrman closed 4 months ago

sgehrman commented 4 months ago

Package Version

4.1.0

Flutter Version

3.22

Platforms

Web

How to reproduce?

Screenshot-20240625212451-2398x1514

Logs (optional)

No response

Example code (optional)

I'm putting a ProImageEditor inside a dialog.  If I make an edit and close the parent dialog before saving, it crashes.

Device Model (optional)

linux

sgehrman commented 4 months ago

I added this so it wouldn't fail, but not sure how to make this work.

I added !didPop, because I think at this point it's too late to do anything about it?

canPop seems to be false, so not sure why it didn't work.

 return Constants(
      child: PopScope(
        canPop:
            disablePopScope || stateManager.position <= 0 || _processFinalImage,
        onPopInvoked: (didPop) {
          if (!didPop &&
              !disablePopScope &&
              stateManager.position > 0 &&
              !_processFinalImage) {
            closeWarning();
          }
        },
hm21 commented 4 months ago

Can you post a minimal example that I can reproduce? In some cases it is necessary to disablePopScope as I do in the example here, but when you post an example will it help to check whether this will resolve it or not.

sgehrman commented 4 months ago

In https://github.com/hm21/pro_image_editor/issues/37

I put the simple dialog code that I use.

hm21 commented 4 months ago

There is no working example in #37, just a small piece of code that is useless. I already tested it when I open the editor with showDialog or BaseDialogRoute but it's not enough to reproduce your problem. If you want me to help you, it would be helpful if you post a minimal code example because I think you close the editor in a different way, but as long as you didn't post an example I can't help you.

sgehrman commented 4 months ago

https://github.com/sgehrman/pro_image_bug

I put some instructions in the readme

sgehrman commented 4 months ago

I noticed that if you click outside the dialog to dismiss it, that seems to work.

My close button calls Navigator.of(context).pop() which doesn't work. What is the difference? I assumed that was the proper way of closing a dialog.

hm21 commented 4 months ago

Thank you for the example code that helped me a lot to reproduce the issue. Below is your code, which I have modified so that there is no problem anymore. In short, we need to make sure that PopScope will have no effect, so you just need to set disablePopScope to true.

import 'package:flutter/foundation.dart';
import 'package:flutter/material.dart';
import 'package:pro_image_editor/pro_image_editor.dart';

void main() {
  runApp(const MyApp());
}

class MyApp extends StatelessWidget {
  const MyApp({super.key});

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      title: 'Flutter Demo',
      theme: ThemeData(
        colorScheme: ColorScheme.fromSeed(seedColor: Colors.deepPurple),
        useMaterial3: true,
      ),
      home: const MyHomePage(title: 'Flutter Demo Home Page'),
    );
  }
}

class MyHomePage extends StatefulWidget {
  const MyHomePage({super.key, required this.title});

  final String title;

  @override
  State<MyHomePage> createState() => _MyHomePageState();
}

class _MyHomePageState extends State<MyHomePage> {
  final int _counter = 0;

  void _incrementCounter() {
    Navigator.of(context).push(
      BaseDialogRoute(const _Editor()),
    );
  }

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        backgroundColor: Theme.of(context).colorScheme.inversePrimary,
        title: Text(widget.title),
      ),
      body: Center(
        child: Column(
          mainAxisAlignment: MainAxisAlignment.center,
          children: <Widget>[
            const Text(
              'You have pushed the button this many times:',
            ),
            Text(
              '$_counter',
              style: Theme.of(context).textTheme.headlineMedium,
            ),
          ],
        ),
      ),
      floatingActionButton: FloatingActionButton(
        onPressed: _incrementCounter,
        tooltip: 'Increment',
        child: const Icon(Icons.add),
      ),
    );
  }
}

// ==================================================================

class BaseDialogRoute<T> extends ModalRoute<T> {
  BaseDialogRoute(this.child) : super();

  Widget child;

  @override
  Duration get transitionDuration => const Duration(milliseconds: 200);

  @override
  bool get opaque => false;

  @override
  bool get barrierDismissible => true;

  @override
  Color get barrierColor => Colors.black.withOpacity(0.5);

  @override
  String get barrierLabel => '';

  @override
  bool get maintainState => true;

  @override
  Widget buildPage(
    BuildContext context,
    Animation<double> animation,
    Animation<double> secondaryAnimation,
  ) {
    final screenSize = MediaQuery.of(context).size;

    return Center(
      child: Container(
        constraints: BoxConstraints(
          maxHeight: screenSize.height * 0.95,
          maxWidth: screenSize.width * 0.9,
        ),
        width: screenSize.width * 0.9,
        decoration: BoxDecoration(
          color: Theme.of(context).scaffoldBackgroundColor,
          borderRadius: const BorderRadius.all(Radius.circular(22)),
        ),
        child: Material(
          type: MaterialType.transparency,
          child: child,
        ),
      ),
    );
  }
}

class _Editor extends StatefulWidget {
  const _Editor();

  @override
  State<_Editor> createState() => _EditorState();
}

class _EditorState extends State<_Editor> {
  final _editorKey = GlobalKey<ProImageEditorState>();

  @override
  Widget build(BuildContext context) {
    return SizedBox(
      height: 400,
      width: 400,
      child: Column(
        children: [
          ElevatedButton(
              onPressed: () {
                /// TODO: You just need to set `disablePopScope` to true
                /// which will fix it. 
                _editorKey.currentState?.disablePopScope = true;
                Navigator.of(context).pop();
              },
              child: const Text('Close')),
          Expanded(
            child: ProImageEditor.network(
              'https://picsum.photos/id/230/2000',
              key: _editorKey,
              configs: ProImageEditorConfigs(
                // gets around the vibration package bug.  may not be needed later
                // to test check the all exceptions debug setting to see it happen
                helperLines: const HelperLines(
                  hitVibration: !kIsWeb,
                ),
                // this is a hack to fix the lines getting offset.  editor must be same size it seems
                // maybe fixed in the future
                imageEditorTheme: ImageEditorTheme(
                  subEditorPage: SubEditorPageTheme(
                    enforceSizeFromMainEditor: true,
                    positionTop: 0,
                    positionLeft: 0,
                    positionRight: 0,
                    positionBottom: 0,
                    barrierDismissible: true,
                    barrierColor: const Color(0x90272727),
                    borderRadius: BorderRadius.circular(10),
                    transitionsBuilder:
                        (context, animation, secondaryAnimation, child) {
                      return child;
                    },
                  ),
                ),
                blurEditorConfigs: const BlurEditorConfigs(maxBlur: 40),
                imageGenerationConfigs: const ImageGeneratioConfigs(
                  outputFormat: OutputFormat.png,
                  // captureOnlyBackgroundImageArea: true,
                  // pngLevel: 6,
                  // processorConfigs: ProcessorConfigs(maxConcurrency: 2),
                  generateImageInBackground: true,
                  // generateInsideSeparateThread: true,
                  maxOutputSize: Size(4096, 4096),
                ),
              ),
              callbacks: ProImageEditorCallbacks(
                onCloseEditor: () {
                  // _mode = _DrawPictureMode.view;

                  // setState(() {});
                },
                onImageEditingComplete: (bytes) {
                  // _imageBytes = bytes;
                  // setState(() {});

                  return Future.value();
                },
              ),
            ),
          ),
        ],
      ),
    );
  }
}
sgehrman commented 4 months ago

Did you figure out why clicking outside the dialog to dismiss it was different than doing a pop()?

This solution seems like a weird hack. I would like to know why pop() breaks it.

hm21 commented 4 months ago

The problem is with the widget PopScope. If we call pop() it will trigger onPopInvoked but ignore canPop. If we tap outside the dialog, it will trigger onPopInvoked and respect canPop. I'm not sure if this is a wanted behavior from Flutter, but in my opinion it is a Flutter issue because I see no sense why it should trigger onPopInvoked but ignore canPop.

sgehrman commented 4 months ago

maybe I should be using maybePop? I'll try that.

sgehrman commented 4 months ago

Yeah, I think maybePop is the answer. Seems to work fine now without that suggestion above. Sorry, I should have known, but I haven't used the PopScope thing in years.

sgehrman commented 4 months ago

But I think your code should have the !didPop in the if statement or it will crash if someone calls pop() instead of maybePop().

          if (!didPop &&
              !disablePopScope &&
              stateManager.position > 0 &&
              !_processFinalImage) {
hm21 commented 4 months ago

Ah! Yes, you are right, maybePop() will have the same behavior as when we click outside the dialog. I thought onPopInvoked would work the same as onWillPop from the old WillPopScope widget, but I see it's not, it's just a callback that is triggered after the decision to close is already made. Anyway, I released version 4.2.2 with your solution to add if(!didPop...) which should fix it. I've also added a new callback onPopInvoked to the MainEditorCallbacks, so it's possible to see what happens from outside the image editor.

sgehrman commented 4 months ago

cool. thanks.