1212021 - [meta] Invert Tab Tray / BVC relationship

Status: RESOLVED INACTIVE

(Firefox for iOS :: General, defect)

Description

[Stephan Leroux [:sleroux]]

The current IA of the app is the BVC is the root view and the Tab Tray is a view controller that gets pushed on top of it in the stack. This is backwards from the perspective of the tab tray being the 'list' view of tabs and the BVC being the 'detail' view of a tab. We need to invert this relationship so that it matches our mental model of the app as well as reduce some of the complexity of state we have between tab tray/BVC.

Comment 1

[Stephan Leroux [:sleroux]]

Wanted to put an update on this as I've been on/off trying to get something going here.

Turns out inverting these two view controllers is like pulling the string on a sweater. There's a lot of implicit responsibilities that the BVC does on app startup that makes it difficult to just flip the two. For example, the BVC acts as a singleton that we send requests to for opening links, loading queued tabs and handling restoration. These cases rely on latching onto when the app starts to properly run (load queued tabs that were queued from view later, restoring previous tabs, crash reporter, etc). On top of this, some of the muddiness of how we're using the TabManagerDelegate comes into play. For example, the TabTrayController uses these methods for configuring adding/removing of cells from the collection view which breaks when tab restoration happens on startup.

The more I try to poke the bear I realize it might make sense to think about a more serious refactor of Tab Tray/BVC. I feel like a lot of the complexity of state management in the BVC and the variety of callbacks we have seems needless but that could be my 20/20 hindsight talking. 

Anyways if anyone has additional thoughts on this or alternate strategies let me know!

Comment 2

[Richard Newman [:rnewman]]

Can we simplify the problem by pushing some stuff into the AppDelegate (or creating an intermediate object)?

Stuff like loading the tab queue is *called* from AppDelegate, and the implementation is only in BVC because it needs access to profile.queue and the tabManager.

If the set of open tabs and the profile move out of BVC, then so can queuing, and restoration, and…, and….

Comment 3

[Stephan Leroux [:sleroux]]

I was able to move out the tab queue stuff out of the BVC pretty easily. After some more looking, the key to making everything make sense for the inversion is removing the selectedTab state from the tabManager. The current BVC works by always checking and invalidating this selectedTab property but when we move to the Tray->BVC model, the BVC's selectedTab becomes the tab that was pass to it - making a ton of the conditional statements simpler. We no longer need to map the singleton nature of the BVC to this selectedTab property which frees up the TabManager to be more concise in it's responsibility by focusing on adding/removing of tabs.

I'm going to go down this path a bit and see if the forest clears up.

WIP -> https://github.com/mozilla/firefox-ios/tree/sleroux/Bug1212021-InvertTabTray

Comment 4

[Stephan Leroux [:sleroux]]

Un-assigning and de-prioritizing for now.