Bug 1845409 - File RFC 0010 proposing handling navigation through middleware #4126

MatthewTighe · 2023-10-18T00:05:06Z

This patch includes an RFC for a new architectural goal of handling navigation through middleware moving forward. Currently, there are two commits.

Old proposal for observing navigation state and reacting accordingly.
New proposal for handling navigation through middleware.

The first proposal seemed suboptimal after offline conversations with the team, so I've reworked it accordingly. It's included here for context.

Tentative deadline is Feb 2nd.

GitHub Automation

https://bugzilla.mozilla.org/show_bug.cgi?id=1845409

jonalmeida

This seems fair to me!

I reviewed the code in Focus to see how it connects the AppStore to an activity that has a lifetime, and I think that flow makes sense for Fenix too for only navigations that that are coming from HomeActivity.

I'm curious to see a prototype of this though and if it plays well with the navgraph library.

MozillaNoah

This all makes sense to me. What would be the next steps? Do you have v0.1 or v1 of what this effort looks like?

MatthewTighe · 2023-10-26T18:34:57Z

I would like to get a small prototype together in the next few days focused on tackling the bug this is filed against. I'll announce more broadly when I get something together

MatthewTighe · 2023-11-01T23:30:18Z

I worked up a basic prototype that can be found at #4323

MatthewTighe · 2024-01-19T19:23:12Z

I've updated the proposal based on offline conversations with the team in our weekly sync meeting. I've left the original proposal as a separate commit for now for context.

MozillaNoah · 2024-01-19T22:07:55Z

@MatthewTighe, should this be moved out of draft?

Did the Debug Drawer navigation help inform some of this direction?

Also, will the prototype PR be updated at all?

MatthewTighe · 2024-01-22T17:46:51Z

@MatthewTighe, should this be moved out of draft?

Yeah, I will move this out of draft shortly. Was getting a draft pass from @boek offline.

Did the Debug Drawer navigation help inform some of this direction?

The Debug Drawer, Review Checker, and History fragment refactor all helped inform this, yes.

Also, will the prototype PR be updated at all?

Yes, I can update the prototype PR

MatthewTighe · 2024-01-22T20:04:30Z

@MozillaNoah Instead of dealing with a messy diff to update the previous prototype, I've drafted a new one

Approval on first proposal

MozillaNoah

Nice job with the updates!

rocketsroger · 2024-01-24T22:19:14Z

docs/rfcs/0010-add-state-based-navigation.md

+        next(action)
+        when (action) {
+            is HomeButtonClicked -> getNavController().navigate(R.id.home_fragment)
+			is HistoryGroupClicked -> getNavController().navigate(R.id.history_metadata_fragment)
+        }


nit: indent missing? I think the original example have the same issue. Maybe the override fun invoke( part have too much indent.

Thanks, apparently Android Studio used actual unexpanded tab characters any time I copied a line with a tab in it. Haven't run into a tabs v spaces problem in a longggggg time 😆

t-p-white · 2024-01-31T10:06:43Z

docs/rfcs/0010-add-state-based-navigation.md

+
+## Summary
+
+Following the acceptance of [0009 - Remove Interactors and Controllers](0009-remove-interactors-and-controllers), Fenix should have a method of navigation that is tied to the `lib-state` model to  provide a method of handling navigation side-effects that is consistent with architectural goals.


that is consistent with architectural goals.

nit: Is there documentation that you can link here to outline the architectural goals?

Good call, updated!

t-p-white · 2024-01-31T10:12:00Z

docs/rfcs/0010-add-state-based-navigation.md

+}
+```
+
+### 3. The special case of `openToBrowserAndLoad`


t-p-white · 2024-01-31T10:52:40Z

Looks good, no objections from me 🙂 having a consistent method for handling the various aspects of navigation would certainly be beneficial. Appreciate the demo provided too. My only immediate concern is around the defined use cases and how clear the boundaries of responsibility are e.g handling telemetry and other various side effects, but I don't think it's reasonable for you to anticipate all use cases upfront

…dleware

github-actions bot added the 🕵️‍♀️ needs review PRs that need to be reviewed label Oct 18, 2023

Bug 1845409 - File RFC 0010 proposing adding nav State to AppState

1c8de2d

MatthewTighe force-pushed the rfc-0010-state-based-navigation branch from f31d616 to 1c8de2d Compare October 18, 2023 00:06

jonalmeida previously approved these changes Oct 19, 2023

View reviewed changes

gabrielluong added approved PR that has been approved and removed 🕵️‍♀️ needs review PRs that need to be reviewed labels Oct 24, 2023

MozillaNoah previously approved these changes Oct 26, 2023

View reviewed changes

MatthewTighe mentioned this pull request Nov 1, 2023

Nav proto #4323

Closed

github-actions bot assigned jonalmeida and MozillaNoah Jan 19, 2024

MatthewTighe changed the title ~~Bug 1845409 - File RFC 0010 proposing adding nav State to AppState~~ Bug 1845409 - File RFC 0010 proposing handling navigation through middleware Jan 19, 2024

MatthewTighe marked this pull request as draft January 19, 2024 19:23

github-actions bot added work in progress Not ready to land yet. Work in progress (WIP). and removed approved PR that has been approved labels Jan 19, 2024

MatthewTighe added 🕵️‍♀️ needs review PRs that need to be reviewed and removed work in progress Not ready to land yet. Work in progress (WIP). labels Jan 22, 2024

MatthewTighe marked this pull request as ready for review January 22, 2024 20:04

github-actions bot added approved PR that has been approved and removed 🕵️‍♀️ needs review PRs that need to be reviewed labels Jan 22, 2024

MatthewTighe added 🕵️‍♀️ needs review PRs that need to be reviewed and removed approved PR that has been approved labels Jan 22, 2024

MozillaNoah approved these changes Jan 23, 2024

View reviewed changes

rocketsroger reviewed Jan 24, 2024

View reviewed changes

MatthewTighe force-pushed the rfc-0010-state-based-navigation branch from e7f3daa to 5ccfb67 Compare January 24, 2024 23:45

github-actions bot added approved PR that has been approved and removed 🕵️‍♀️ needs review PRs that need to be reviewed labels Jan 24, 2024

MatthewTighe force-pushed the rfc-0010-state-based-navigation branch from 5ccfb67 to 16e91ee Compare January 24, 2024 23:49

t-p-white reviewed Jan 31, 2024

View reviewed changes

docs/rfcs/0010-add-state-based-navigation.md

}

```

### 3. The special case of `openToBrowserAndLoad`

Copy link

Contributor

t-p-white Jan 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌

t-p-white approved these changes Jan 31, 2024

View reviewed changes

Amejia481 approved these changes Jan 31, 2024

View reviewed changes

Bug 1845409 - File RFC 0010 proposing handling navigation through mid…

b985a89

…dleware

MatthewTighe force-pushed the rfc-0010-state-based-navigation branch from 16e91ee to b985a89 Compare January 31, 2024 18:42

github-actions bot assigned Amejia481 and t-p-white Jan 31, 2024

MatthewTighe added the 🛬 needs landing PRs that are ready to land label Feb 27, 2024

Merge branch 'main' into rfc-0010-state-based-navigation

22dc954

mergify bot merged commit 12e13e2 into mozilla-mobile:main Feb 27, 2024
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1845409 - File RFC 0010 proposing handling navigation through middleware #4126

Bug 1845409 - File RFC 0010 proposing handling navigation through middleware #4126

MatthewTighe commented Oct 18, 2023 •

edited by github-actions bot

jonalmeida left a comment

MozillaNoah left a comment

MatthewTighe commented Oct 26, 2023

MatthewTighe commented Nov 1, 2023

MatthewTighe commented Jan 19, 2024

MozillaNoah commented Jan 19, 2024 •

edited

MatthewTighe commented Jan 22, 2024

MatthewTighe commented Jan 22, 2024

MozillaNoah left a comment

rocketsroger Jan 24, 2024 •

edited

MatthewTighe Jan 24, 2024

t-p-white Jan 31, 2024 •

edited

MatthewTighe Jan 31, 2024

t-p-white Feb 1, 2024

t-p-white Jan 31, 2024

t-p-white commented Jan 31, 2024 •

edited


		## Summary

		Following the acceptance of [0009 - Remove Interactors and Controllers](0009-remove-interactors-and-controllers), Fenix should have a method of navigation that is tied to the `lib-state` model to provide a method of handling navigation side-effects that is consistent with architectural goals.

Bug 1845409 - File RFC 0010 proposing handling navigation through middleware #4126

Bug 1845409 - File RFC 0010 proposing handling navigation through middleware #4126

Conversation

MatthewTighe commented Oct 18, 2023 • edited by github-actions bot

GitHub Automation

jonalmeida left a comment

Choose a reason for hiding this comment

MozillaNoah left a comment

Choose a reason for hiding this comment

MatthewTighe commented Oct 26, 2023

MatthewTighe commented Nov 1, 2023

MatthewTighe commented Jan 19, 2024

MozillaNoah commented Jan 19, 2024 • edited

MatthewTighe commented Jan 22, 2024

MatthewTighe commented Jan 22, 2024

MozillaNoah left a comment

Choose a reason for hiding this comment

rocketsroger Jan 24, 2024 • edited

Choose a reason for hiding this comment

MatthewTighe Jan 24, 2024

Choose a reason for hiding this comment

t-p-white Jan 31, 2024 • edited

Choose a reason for hiding this comment

MatthewTighe Jan 31, 2024

Choose a reason for hiding this comment

t-p-white Feb 1, 2024

Choose a reason for hiding this comment

t-p-white Jan 31, 2024

Choose a reason for hiding this comment

t-p-white commented Jan 31, 2024 • edited

MatthewTighe commented Oct 18, 2023 •

edited by github-actions bot

MozillaNoah commented Jan 19, 2024 •

edited

rocketsroger Jan 24, 2024 •

edited

t-p-white Jan 31, 2024 •

edited

t-p-white commented Jan 31, 2024 •

edited