GlobalSettingsStore, and other refactors

[gnprice]

This branch makes several refactors related to global settings (following up on #1167) which came up as part of work on #1409. Some of these are things I set out to do; others are things I saw the opportunity for as I got my hands in this code.

In summary:

• There's now a GlobalSettingsStore, moving out from GlobalStore the responsibility for the details of how global settings are managed. This also makes a convenient home for what had been extension members on GlobalSettingsData.
• The initialization of the GlobalSettings table to have a row now happens as a migration (for existing installs) or at database creation time (for new installs), instead of as a check made when fetching from the table at each app launch.
• Several refactors to the API that app code uses for global settings, and added dartdoc.

Selected commit messages

86be5c4 test [nfc]: Inline eg.globalSettings

This isn't really any longer than invoking it, and cuts out a
layer of indirection.

32a6e80 db test: Test updating GlobalSettings

This can be seen as another important aspect of what it means
for ensureGlobalSettings to be functioning properly.

3cf002e settings [nfc]: Drop return value from updateGlobalSettings

We don't currently end up using this anywhere.

0497088 settings [nfc]: Write docs on extension members

f350c19 db [nfc]: Extract _migrationSteps as a static

This helpfully makes it impossible for these migration steps
to accidentally call methods on this database instance directly,
rather than on their Migrator parameters m.

As a bonus, the code gets three levels less indented.

8ab2c98 db: Create GlobalSettings row as part of schema setup

This is a little more complicated in the code overall; but it
simplifies the logic that runs each time the app starts up.

8ed6915 settings [nfc]: Split out a GlobalSettingsStore, and GlobalStoreWidget.settingsOf

This allows widgets to listen separately for changes to settings or
to the rest of the global store's data, which is a bit nice.

More fundamentally, as a matter of code organization, it gives us a
place to put code related to the store of global settings where
there's more room to spread out than if we kept it in GlobalStore,
and where the namespace is already specific to global settings.

Those two points in turn will also help us adjust to be more
ergonomic the API the rest of the app uses for interacting with
these settings.

The TODO comments added in this commit mark places where this API
will be more coherent after upcoming commits in this series.

892691e settings [nfc]: Move the settings data out to GlobalSettingsStore

ed595fb settings [nfc]: Have settingsOf return whole store, not just data

To save code that consults the settings from an extra indirection
".data", we equip GlobalSettingsStore with getters for all the
fields of the data class. This also makes the store now the natural
home for the additional members that had been in an extension on the
data class.

a504266 store [nfc]: Simplify name GlobalStore.settings, from globalSettings

It's natural that the settings found directly on the global store
are the global settings.

4ef3437 store [nfc]: Separate "backend" class from store itself

This commit makes GlobalSettingsStore more self-contained, taking
over from GlobalStore the responsibility for updating settings in
the database at the same time as they get updated in the in-app
cache [GlobalSettingsStore._data].

To do that, it needs to call doUpdateGlobalSettings, the abstract
method that's implemented separately to use a live database in the
real app vs. a vacuous fake backend in tests. But that means that
method can no longer live on GlobalStore: for GlobalStore needs a
GlobalSettingsStore, but then GlobalSettingsStore would need a
GlobalStore, forming a cycle. In particular when calling the
GlobalSettingsStore constructor from within the initializer list of
the GlobalStore constructor, we can't pass this as an argument
because this hasn't yet been constructed.

(If we were determined to make such a cycle work, we could do so by
having a late or nullable field. But let's take the cycle as a
signal that the design can be cleaned up.)

So, make a new class GlobalStoreBackend as the home for this
abstract method. A GlobalStore now "has a" GlobalStoreBackend,
rather than "is a" GlobalStoreBackend as it effectively was before.
As a result we can readily construct a "backend" instance first,
and pass that to the GlobalSettingsStore constructor, before going
on to finish constructing the overall GlobalStore.

Probably most or all of the other functionality that LiveGlobalStore
implements should get moved to the new "backend" class too, but
we'll leave that for possible follow-up changes.

6d438f7 settings [nfc]: Use update method directly on GlobalSettingsStore

This completes the transition to GlobalSettingsStore as the
self-contained store for the global settings.

df70e04 settings [nfc]: Give settings specific setter methods

This makes the call sites significantly nicer to read.

Relatedly, it turns the specifics of how the settings are
represented in the database into more of an implementation detail
of the settings store.

[PIG208]

Thanks! I have read the whole PR and left some comments.

[PIG208]

commit message nit:

settings [nfc]: Split out a GlobalSettingsStore, and GlobalStoreWidget.settingsOf

[…]

Those two points in turn will also help us adjust to be more
ergonomic the API the rest of the app uses for interacting with
these settings.

This part confuses me a little. Is there something missing here?

[gnprice]

How's this?

Those two points in turn will also help us adjust the API the rest
of the app uses for interacting with these settings, so as to make
it more ergonomic.

Same thought but perhaps clearer syntax.

[PIG208]

A reason that effectiveBrowserPreference is separate from this was that we didn't opt into implementing a custom GlobalSettingsData.

Now that GlobalSettingsData _data is nicely hidden as a part of the implementation detail, we can rename effectiveBrowserPreference to browserPreference, replacing it, and skip the part of the dartdoc redirecting to effectiveBrowserPreference.

[PIG208]

I guess keeping browserPreference nullable is consistent with setBrowserPreference, but I think we don't need to expose a way to set it to null.

[gnprice]

Yeah, I think hiding the raw browserPreference behind (a renamed) effectiveBrowserPreference could be a good follow-up change. Then setBrowserPreference would narrow its parameter type to match.

I'll leave that out of this PR, though, because I think it's not on the path to #1409.

[PIG208]

GlobalSettings is the table class; would it be better to refer to GlobalSettingsStore since that's what returned by this method?

[gnprice]

The return type doesn't need a link in the doc, since it's shown directly in the code. By contrast a link to related other types can be helpful for navigating around. (The same reason is why e.g. the doc on the table class Accounts mentions the data class Account, which doesn't otherwise appear in the code there and so wouldn't otherwise be easy to navigate to.)

[PIG208]

This abstraction looks clean! I think it will be helpful for sqlite integration tests that use LiveGlobalStoreBackend with a non-LiveGlobalStore.

[gnprice]

Yeah, that's definitely a possible application of this structure.

[gnprice]

Thanks for the review! Pushed a revision, and replied to subthreads above; PTAL.

[PIG208]

Thanks! LGTM.

[gnprice]

Thanks for the review! Merging.