1254599 - Create AppState concept that contains the current state of the app and can be passed into Menu & toolbars

Status: RESOLVED FIXED

(Firefox for iOS :: Menu and Toolbar, defect)

Description

[Emily Toop (:fluffyemily)]

* Open menu in overlay when menu button selected
* overlay should blur out background when presented
* menu should be dismissed when tapping outside the menu or by pressing the menu button again when menu is displaying

Comment 1

[Emily Toop (:fluffyemily)]

(In reply to Emily Toop (:fluffyemily) from comment #0)
> * Open menu in overlay when menu button selected
> * overlay should blur out background when presented
> * menu should be dismissed when tapping outside the menu or by pressing the
> menu button again when menu is displaying

Ignore this comment. The nature of the bug has changed since creation

Comment 2

[Emily Toop (:fluffyemily)]

Attachment: Pull request

Comment 3

[Emily Toop (:fluffyemily)]

:sleroux :jhugman I've asked a question in the PR description that I would appreciate your views on.

Comment 4

[Stephan Leroux [:sleroux]]

Comment on attachment 8735895 [details] [review]
Pull request

Left feedback comment on the PR

Comment 5

[Emily Toop (:fluffyemily)]

Attachment: WIP Feedback Request

Changes to AppState implementation based on rethought approach that was then tweaked and adjusted and moulded into something I actually quite like through discussion with :rnewman and :jhugman.

There is a question in the PR description I'd like some thoughts on, which is:

`Currently I have set up the BrowserViewController to be the app state delegate for the Tab and Home Panels. Should instead, Tab and home panels maintain a list of delegates that it then runs through and notifies, and the Menu is just one of those? I don't really know if I like this idea. If we want to go that way, we are better off sending out an AppStateChanged notification and allowing any components to register as observers. This would enable us to get rid of the delegate entirely. Is this perhaps a better way? I think that what I have implemented here is the more redux-y way of doing things, but maybe I am wrong.`

Comment 6

[Richard Newman [:rnewman]]

The reason to do the chain of delegates is to transform a general app-state into more specific component-level states. For example, the app state might be

  All of my open tabs
  A set of bookmarked URLs
  If there's a network printer in range
  ...

The top-level component (BrowserApp!?) takes that as input. It slices up this state and passes the slices to its sub-components. By the time we get down to refreshing the bookmark star component, we're just passing in the thing it cares about: is the current page bookmarked?

The reason to do this is simple: it makes your components 'dumb' and testable. If every component in the entire app can implicitly depend on anything in the app state, then we're essentially using global immutable state. Better than global mutable state, but still hard to test and hard to understand.


This leads to two lenses through which to reconsider your question.

The first: broadcast or subscribe? If managing the set of subscribers (tab manager -> tabs, for example) is hard, then use broadcasts.

The second: is the data we're sending general or specific? If it's general, don't broadcast it, and I'd expect the data to get smaller and smaller as you move down the chain. If it's specific, then see (1), and simply decide whether to use direct subscription or a specific broadcast at that level..

Comment 7

[Richard Newman [:rnewman]]

Comment on attachment 8737750 [details] [review]
WIP Feedback Request

I'm not going to have enough free time to attend to this :(

Comment 8

[Emily Toop (:fluffyemily)]

Attachment: Pull Request

Re-raised PR merging into main menu branch rather than sub-branch which got all too meta

Comment 9

[Stephan Leroux [:sleroux]]

Comment on attachment 8738472 [details] [review]
Pull Request

Looking really good! Left some discussion points on the PR.

Comment 10

[Brian Nicholson (:bnicholson)]

Comment on attachment 8738472 [details] [review]
Pull Request

Looks good! Just a few nits and naming suggestions.

Comment 11

[Emily Toop (:fluffyemily)]

Adding follow up bugs 

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