Honor "useRootNavigator" arg when "Navigator.pop" is called

[paolovalerdi]

Currently, the "useRootNavigator" field is ignored when the 'Navigator.pop' method is called by pressing either the 'cancel' or 'ok' button, which causes some unexpected issues in certain use cases like having nested navigators in a bottom navigation bar configuration.

[rydmike]

Hi @paolovalerdi you are absolutely right, this needs to be added. Good find, thanks!

I think I even experienced the issue you describe myself in a use case, that I then forgot to look into. Now when I see this issue report/PR I'm sure this was the reason for it.

I can and will merge this, but I also noticed I need to move useRootNavigator into the the ColorPicker itself or into ColorPickerActionButtons. Why? Well, the optional ColorPickerToolbar also use the same problematic pop's in its optional OK and Close icon buttons, so they will need to know about this preference too, to work correctly as well.

The reason why those toolbar OK/close buttons are in the Widget itself and not in the dialog, is because I wanted to be able to show them easily on the same top row as the title and copy and paste buttons, but doing so kind of forced me to move them into the widget itself. Not an ideal design, as they will be problematic of course if configured to be shown/used when the widget is not used in a Dialog, ie on main surface. All I could do about that was to document it and say don't do that, lol.

Anyway, to make it possible for them to also access the useRootNavigator same setting, it needs to be in a place where it can be accessed by their pop functions too. And of course we should only have the value for this setting in one place, having two conflicting ones would be really bad.

So I need to deprecate it in the showPickerDialog with the advice to move the property setting into its new place. With the deprecation I can keep the property around for a few versions, so it does not break the build, and explain why it has moved in deprecation message, and explain where to set it instead.

Now I'm just pondering if it should be in the ColorPicker itself or tuck it all the way into the ColorPickerActionButtons config class. Easier to use directly in the ColorPicker class of course, but it does not really belong there, it would kind of fit better in ColorPickerActionButtons.

What do you think? I would love to hear some preferences on it.

It is not yet a very common use case to need it. So maybe it is cleaner in the ColorPickerActionButtons. On the other hand, the need for using root for dialogs is growing. Nested navigators that may need them are getting more common with Nav2 and Web/desktop usage too.

[paolovalerdi]

Thanks for such detailed explanation! 😄

I also noticed that but decided to keep 'as is' because, as you said, I did not think that argument should be part of the ColorPicker itself so I think moving it to ColorPickerActionButtons would be better since it is really the place where the "pop" happens.

[rydmike]

I will merge your contribution (so you get the due attribution for this find) and then rework the useRootNavigator property to be in the ColorPickerActionButtons and deprecate the old one. I think the property makes more sense there too, it is not perfect there either, but at least I can explain it in the docs and API docs, and motivate why it is there and not in the dialog method.

Might be until tomorrow before I have time to implement the fix. I'll let you know when it is done, in case you want to try/test it via GitHub before I publish it as an update on pub.dev.

[rydmike]

@paolovalerdi Your PR and fix was merged, and then converted to be as prop via ColorPickerActionButtons instead so that all pop:s that need it can and do use it. This fix is now included in latest release on pub.dev.

Let me know if you still have any issues with it. Thanks for this contribution!

[paolovalerdi]

Awesome! I'll check it out and let you know. Thank you so much 👍