jonataslaw / getx

Open screens/snackbars/dialogs/bottomSheets without context, manage states and inject dependencies easily with Get.
MIT License
10.39k stars 1.63k forks source link

Controller is not initialized correctly when you quickly navigate between screens with the same controller #1880

Open pinguluk opened 3 years ago

pinguluk commented 3 years ago

Describe the bug For example: You have a chat list and each element navigates to a ConversationPage via Get.toNamed + arguments (eg you pass the chat title).

The ConversationPage has a ConversationController which is binded in the routes, via Get.lazyPut<ConversationController>(() => ConversationController());.

conversation_page.dart

import 'package:flutter/material.dart';
import 'package:get/get.dart';
import 'package:getx_binding_error/app/modules/conversation/conversation_controller.dart';

class ConversationPage extends GetView<ConversationController> {
  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(title: Text(controller.title)),
      body: SafeArea(
        child: controller.obx(
          (state) {
            return Column(
              children: [
                Expanded(
                  child: ListView.builder(
                    itemCount: controller.messages.length,
                    itemBuilder: (context, index) => Padding(
                      padding: const EdgeInsets.all(25.0),
                      child: Text(controller.messages[index]),
                    ),
                  ),
                ),
              ],
            );
          },
        ),
      ),
    );
  }
}

conversation_controller.dart

import 'package:get/get.dart';

class ChatsController extends GetxController with StateMixin {
  List<String> chats = [];

  @override
  void onInit() {
    super.onInit();
    fillChats();
  }

  Future<void> fillChats() async {
    for (int i = 0; i < 10; i++) {
      chats.add('Chat $i');
    }
    change(null, status: RxStatus.success());
  }
}

conversation_binding.dart

import 'package:get/get.dart';
import 'package:getx_binding_error/app/modules/conversation/conversation_controller.dart';

class ConversationBinding implements Bindings {
  @override
  void dependencies() {
    Get.lazyPut<ConversationController>(() => ConversationController());
  }
}

routes.dart

import 'package:get/get.dart';
import 'package:getx_binding_error/app/modules/chats/chats_binding.dart';
import 'package:getx_binding_error/app/modules/chats/chats_page.dart';
import 'package:getx_binding_error/app/modules/conversation/conversation_binding.dart';
import 'package:getx_binding_error/app/modules/conversation/conversation_page.dart';
import 'package:getx_binding_error/app/routes/routes.dart';

class AppPages {
  static List<GetPage>? allPages = [
    GetPage(name: AppRoutes.chats, page: () => ChatsPage(), binding: ChatsBinding()),
    GetPage(name: AppRoutes.conversation, page: () => ConversationPage(), binding: ConversationBinding()),
  ];
}

chats_page.dart

import 'dart:developer';

import 'package:flutter/material.dart';
import 'package:get/get.dart';
import 'package:getx_binding_error/app/modules/chats/chats_controller.dart';
import 'package:getx_binding_error/app/routes/routes.dart';

class ChatsPage extends GetView<ChatsController> {
  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: Text("Chats"),
      ),
      body: controller.obx(
        (state) {
          return Column(
            children: [
              Expanded(
                child: ListView.builder(
                  itemCount: controller.chats.length,
                  itemBuilder: (context, index) => InkWell(
                    onTap: () => Get.toNamed(
                      AppRoutes.conversation,
                      arguments: controller.chats[index],
                    ),
                    child: Padding(
                      padding: const EdgeInsets.all(25.0),
                      child: Text(controller.chats[index]),
                    ),
                  ),
                ),
              ),
            ],
          );
        },
      ),
    );
  }
}

Now if you navigate to the first conversation and then quickly go back and navigate to another conversation, you'll see that the title is the one from the previously conversation and you'll get a red error message:

"ConversationController" not found. You need to call "Get.put(ConversationController())" or "Get.lazyPut(()=>ConversationController())"

Basically when you go back, ConversationController is being deleted from the memory and if you quickly navigate to another conversation screen, it is trying to find the ConversationController. Therefore, because the ConversationController is not being initialized again or fast enough before you navigate to another conversation page, you'll get the error.

If you wait for the ConversationController to be deleted from the memory and only then navigate to another chat, the ConversationController is initialized again and everything works fine. But if you don't wait to be complety deleted from the memory, you'll get the error.

Also, I noticed that the functions are still being processed, even if the controller is deleted from the memory. I guess they are still queued somehow in the memory? Not sure if this is a bug or not, but it's a bit weird that if you dispose the controller while doing a function, the function doesn't stop automatically and it continues until the end.

[GETX] "ConversationController" onDelete() called
[GETX] "ConversationController" deleted from memory
I/flutter (20348): Messages filled! // <--- THIS SHOULD NOT BE PRINTED IF YOU DELETE THE CONTROLLER BEFORE THE FUNCTION REACHES THE PRINT METHOD (I guess that's how it should work)

Reproduction code Download: lib.zip

To Reproduce As described earlier

Expected behavior ConversationController to be created and initialized correctly when you navigate quickly between screens that use the same controller.

I tried to replace Get.lazyPut with the Get.put and all the smartManagement options, but they don't fix the issue.

Screenshots Video: https://user-images.githubusercontent.com/16257821/134400126-19f7d7ab-940c-4695-8bd1-53767fe32fa6.mp4

Flutter Version: 2.2.3

Getx Version: ^4.3.8

Describe on which device you found the bug: Pixel 3a API 30

Minimal reproduce code As described & attached above

MrCsabaToth commented 3 years ago

You write you replaced lazyPut with put but I'd look for some more async operations. Just a guess

pinguluk commented 3 years ago

You write you replaced lazyPut with put but I'd look for some more async operations. Just a guess

Tried both but dunno. What do you suggest?

XuanTung95 commented 3 years ago

Same problem, after close a page, the controller will be deleted but after some delay. If open the page again before executing delete, it will later delete the controller while it is being used. SmartManagement.full is so buggy and should not be the default option.

pinguluk commented 3 years ago

Same problem, after close a page, the controller will be deleted but after some delay. If open the page again before executing delete, it will later delete the controller while it is being used. SmartManagement.full is so buggy and should not be the default option.

One solution would be to add a controller variable and check if it's null => init the controller manually

XuanTung95 commented 3 years ago

One solution would be to add a controller variable and check if it's null => init the controller manually

Still does not work with GetBuilder, after deleting the controller, cannot update GetBuilder. The best option is permanent = true so it won't delete the controller automatically.

pinguluk commented 3 years ago

@jonataslaw can you please check it out?

pinguluk commented 3 years ago

hey @jonataslaw, can you please review it? it's kinda urgent

ziakhan110 commented 2 years ago

I am having same issue, its caused big issue on production code, on debug it sometimes works correctly.

ghost commented 2 years ago

I having same issue, I workaround this case with used page as tag and isRegistered before router

extension GetxExtention on GetInterface {
  S? findOrNull<S>({String? page}) => Get.isRegistered<S>(tag: page) ? Get.find<S>(tag: page) : null;

  Future<T?>? sureToNamed<T>(String page, {dynamic arguments, int? id, bool preventDuplicates = true, Map<String, String>? parameters}) {
    if (findOrNull<T>(page: page) == null) {
      return Get.toNamed(page, preventDuplicates: preventDuplicates, id: id, arguments: arguments, parameters: parameters);
    } else {
      return null;
    }
  }
} 

hope this help ^o^

SameerChorge94 commented 2 years ago

Any Update on this issue?? I'm facing the same issue for controller gets deleted delayed and if tried to open same screen with controller again then UI break with controller not found issue

minh-dai commented 2 years ago

I having same issue

vrman commented 2 years ago

same issue here , and there is no solution to fix this correctly

pinguluk commented 2 years ago

Hey @jonataslaw, can you please look into it?

XuanTung95 commented 2 years ago

Not tested but I think it could be fixed by deleting the controller on onDispose(). Then hopefully it will not auto-delete the controller when re-open the page.

Edit: tested but not works.

vrman commented 2 years ago

Not tested but I think it could be fixed by deleting the controller on onDispose(). Then hopefully it will not auto-delete the controller when re-open the page.

Tested , but not works , u need to put at least 1 second delay before next navigation , then Getx will create controller again .

XuanTung95 commented 2 years ago

Tested , but not works , u need to put at least 1 second delay before next navigation , then Getx will create controller again .

Yes. After some debugging, Controller is deleted here in default_route.dart

  @override
  void dispose() {
    super.dispose();
    if (Get.smartManagement != SmartManagement.onlyBuilder) {
      WidgetsBinding.instance!.addPostFrameCallback((_) {
        if (Get.reference != reference) {
          GetInstance().removeDependencyByRoute("$reference");
        }
      });
    }

    // if (Get.smartManagement != SmartManagement.onlyBuilder) {
    //   GetInstance().removeDependencyByRoute("$reference");
    // }

    final middlewareRunner = MiddlewareRunner(middlewares);
    middlewareRunner.runOnPageDispose();
  }

Maybe moving it to didPop() will fix this.

  /// A request was made to pop this route. If the route can handle it
  /// internally (e.g. because it has its own stack of internal state) then
  /// return false, otherwise return true (by returning the value of calling
  /// `super.didPop`). Returning false will prevent the default behavior of
  /// [NavigatorState.pop].
  ///
  /// When this function returns true, the navigator removes this route from
  /// the history but does not yet call [dispose]. Instead, it is the route's
  /// responsibility to call [NavigatorState.finalizeRoute], which will in turn
  /// call [dispose] on the route. This sequence lets the route perform an
  /// exit animation (or some other visual effect) after being popped but prior
  /// to being disposed.
  ///
  /// This method should call [didComplete] to resolve the [popped] future (and
  /// this is all that the default implementation does); routes should not wait
  /// for their exit animation to complete before doing so.
  ///
  /// See [popped], [didComplete], and [currentResult] for a discussion of the
  /// `result` argument.
  @mustCallSuper
  bool didPop(T? result) {
    didComplete(result);
    return true;
  }
vrman commented 2 years ago

@XuanTung95 cant find your code in default_route.dart , what is your getx version ?

XuanTung95 commented 2 years ago

@vrman get-4.2.0\lib\get_navigation\src\routes\default_route.dart

You can put breakpoint on onClose() of GetXController

vrman commented 2 years ago

i remove Getx from my project its not stable , and going to riverpod . there is no help after 1 year for this topic from developers. so many bug report without answer . i think Getx is dead. and its so risky to use it in production.

pinguluk commented 2 years ago

i remove Getx from my project its not stable , and going to riverpod . there is no help after 1 year for this topic from developers. so many bug report without answer . i think Getx is dead. and its so risky to use it in production.

I think there's just only one developer and that's @jonataslaw

kauemurakami commented 2 years ago

According to @Nipodemos' answer in this issue

"Just to add some context for people who might see this issue in the future: GetX by default doesn't let you go to a route you're already on. This helps when you accidentally call the push method twice or the user presses the button to the next screen too fast multiple times.

But since there might be use cases where you want to reopen the same screen again (i.e. reopen the page with different parameters), GetX has a preventDuplicates option that you can change when submitting the new route."

So I was facing the same error in the latest version, when the user changes the page quickly in a bottom navigation of a Navigator with onGenerateRoutes.

To make sure I didn't duplicate controllers and/or stack pages, I navigated this way. await Get.offNamed(pages[index.value], id: 1);

Now I navigate this way await Get.toNamed(pages[index.value], id: 1, preventDuplicates: true);

And now everything works fine, even if the user flips pages in my bottom navigator mindlessly, with the guarantee of not navigating to a route that is already on the stack, as per @Nipodemos' comment.

You can see this by adding displaying the tag or hashCode of the controller, and you'll see that it's only instantiated once. Hope this helps to find this issue in the future.

This will not call your controller again, so oninit will only be called the first time.

j-j-gajjar commented 2 years ago

@kauemurakami https://github.com/jonataslaw/getx/issues/1784

kauemurakami commented 2 years ago

@j-j-gajjar don't work for you ?

j-j-gajjar commented 2 years ago

@kauemurakami Yes, it's not working for me in 4.6.5 version

j-j-gajjar commented 2 years ago

@kauemurakami any update? cc: @jonataslaw

chenchat commented 1 year ago

Any progress? I'm having the same problem

BloodLucia commented 1 year ago

Hey, I solved the problem, you just need to set fenix to true in lazyput.

please see:

Get.lazyPut<NewsDetailController>(() => NewsDetailController(),
        fenix: true);
knottx commented 1 year ago

Same issue here, now i put 500ms delayed before next navigation in same controller

louislamlam commented 1 year ago

I have a bottomSheet with lots of listed items inside and experience the same issue. The bottomSheet will get closed if I navigate to same page & controllers when relative controllers aren't delete on time.

Tried all suggestions but no luck.

induwarar commented 1 year ago

Inside the initState(), delete the controller using Get.delete<> then call Get.put() again.

late YourController controller;

@override
void initState() {
    super.initState();

    Get.delete<YourController>();
    controller = Get.put(YourController());
}

Find out this one. https://stackoverflow.com/a/70376462/9519005

suljich commented 10 months ago

In my case, it was related to a FutureBuilder I was using inside GetView. As far as I could tell, the FutureBuilder's future subscription would last through the deletion of the controller, and when navigating to the same page again, SmartManagement would get confused, and delete the controller while building the view. The solution was removing the FutureBuilder, and finding an alternate approach.

Asif-shah786 commented 6 months ago

Certainly! Here's a refined version of your message:

There is a workaround that helps!

Create a temporary string variable in the controller, and call it in the build method of the widget where you are using this controller.

I'm using the ProfileController in GetView of the ProfileView, so I'm calling print(controller.temp) in the build method of ProfileView.

It's working fine now.

bunpotks commented 1 month ago

I had the same issue when switching screens too quickly.

My solution is to check the controller's operation and add a delay before proceeding

I'm not sure if this is the best method

if (Get.isRegistered<MyController>()) {
  DialogService.showLoading();
  await Future.delayed(Durations.long2);
  DialogService.hide();
}
Get.toNamed('MyRoute');
pinguluk commented 1 month ago

I had the same issue when switching screens too quickly.

My solution is to check the controller's operation and add a delay before proceeding

I'm not sure if this is the best method

if (Get.isRegistered<MyController>()) {
  DialogService.showLoading();
  await Future.delayed(Durations.long2);
  DialogService.hide();
}
Get.toNamed('MyRoute');

The best method is to switch to another state management library, lol.