-
Notifications
You must be signed in to change notification settings - Fork 162
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
login: Support logging out of an account #948
base: main
Are you sure you want to change the base?
Changes from all commits
ed1adbe
325c663
18914a4
6cfe900
2609910
b2cedd5
3e0d499
7aff4d1
f3a3361
ab9ce6d
b4b9569
6bc8bcc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -78,6 +78,8 @@ abstract class GlobalStore extends ChangeNotifier { | |||||||||||
} | ||||||||||||
|
||||||||||||
final Map<int, PerAccountStore> _perAccountStores = {}; | ||||||||||||
|
||||||||||||
int get debugNumPerAccountStoresLoading => _perAccountStoresLoading.length; | ||||||||||||
final Map<int, Future<PerAccountStore>> _perAccountStoresLoading = {}; | ||||||||||||
|
||||||||||||
/// The store's per-account data for the given account, if already loaded. | ||||||||||||
|
@@ -144,7 +146,21 @@ abstract class GlobalStore extends ChangeNotifier { | |||||||||||
/// This method should be called only by the implementation of [perAccount]. | ||||||||||||
/// Other callers interested in per-account data should use [perAccount] | ||||||||||||
/// and/or [perAccountSync]. | ||||||||||||
Future<PerAccountStore> loadPerAccount(int accountId); | ||||||||||||
Future<PerAccountStore> loadPerAccount(int accountId) async { | ||||||||||||
assert(_accounts.containsKey(accountId)); | ||||||||||||
final store = await doLoadPerAccount(accountId); | ||||||||||||
if (!_accounts.containsKey(accountId)) { | ||||||||||||
// [removeAccount] was called during [doLoadPerAccount]. | ||||||||||||
store.dispose(); | ||||||||||||
throw AccountNotFoundException(); | ||||||||||||
} | ||||||||||||
return store; | ||||||||||||
} | ||||||||||||
|
||||||||||||
/// Load per-account data for the given account, unconditionally. | ||||||||||||
/// | ||||||||||||
/// This method should be called only by [loadPerAccount]. | ||||||||||||
Future<PerAccountStore> doLoadPerAccount(int accountId); | ||||||||||||
|
||||||||||||
// Just the Iterables, not the actual Map, to avoid clients mutating the map. | ||||||||||||
// Mutations should go through the setters/mutators below. | ||||||||||||
|
@@ -192,10 +208,26 @@ abstract class GlobalStore extends ChangeNotifier { | |||||||||||
/// Update an account in the underlying data store. | ||||||||||||
Future<void> doUpdateAccount(int accountId, AccountsCompanion data); | ||||||||||||
|
||||||||||||
/// Remove an account from the store. | ||||||||||||
Future<void> removeAccount(int accountId) async { | ||||||||||||
assert(_accounts.containsKey(accountId)); | ||||||||||||
await doRemoveAccount(accountId); | ||||||||||||
if (!_accounts.containsKey(accountId)) return; // Already removed. | ||||||||||||
_accounts.remove(accountId); | ||||||||||||
_perAccountStores.remove(accountId)?.dispose(); | ||||||||||||
unawaited(_perAccountStoresLoading.remove(accountId)); | ||||||||||||
notifyListeners(); | ||||||||||||
} | ||||||||||||
|
||||||||||||
/// Remove an account from the underlying data store. | ||||||||||||
Future<void> doRemoveAccount(int accountId); | ||||||||||||
|
||||||||||||
@override | ||||||||||||
String toString() => '${objectRuntimeType(this, 'GlobalStore')}#${shortHash(this)}'; | ||||||||||||
} | ||||||||||||
|
||||||||||||
class AccountNotFoundException implements Exception {} | ||||||||||||
|
||||||||||||
/// Store for the user's data for a given Zulip account. | ||||||||||||
/// | ||||||||||||
/// This should always have a consistent snapshot of the state on the server, | ||||||||||||
|
@@ -303,6 +335,14 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, ChannelStore, Mess | |||||||||||
final GlobalStore _globalStore; | ||||||||||||
final ApiConnection connection; // TODO(#135): update zulipFeatureLevel with events | ||||||||||||
|
||||||||||||
UpdateMachine? get updateMachine => _updateMachine; | ||||||||||||
UpdateMachine? _updateMachine; | ||||||||||||
set updateMachine(UpdateMachine? value) { | ||||||||||||
assert(_updateMachine == null); | ||||||||||||
Comment on lines
+340
to
+341
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
That way it's clear this only ever gets set to non-null once for a given PerAccountStore. |
||||||||||||
assert(value != null); | ||||||||||||
_updateMachine = value; | ||||||||||||
} | ||||||||||||
|
||||||||||||
bool get isLoading => _isLoading; | ||||||||||||
bool _isLoading = false; | ||||||||||||
@visibleForTesting | ||||||||||||
|
@@ -361,6 +401,10 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, ChannelStore, Mess | |||||||||||
// Data attached to the self-account on the realm. | ||||||||||||
|
||||||||||||
final int accountId; | ||||||||||||
|
||||||||||||
/// The [Account] this store belongs to. | ||||||||||||
/// | ||||||||||||
/// Will throw if called after [dispose] has been called. | ||||||||||||
Account get account => _globalStore.getAccount(accountId)!; | ||||||||||||
|
||||||||||||
/// Always equal to `account.userId`. | ||||||||||||
|
@@ -430,6 +474,8 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, ChannelStore, Mess | |||||||||||
// End of data. | ||||||||||||
//////////////////////////////////////////////////////////////// | ||||||||||||
|
||||||||||||
bool _disposed = false; | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: put this just above (Alternatively it could go up in the first section of data fields, after |
||||||||||||
|
||||||||||||
/// Called when the app is reassembled during debugging, e.g. for hot reload. | ||||||||||||
/// | ||||||||||||
/// This will redo from scratch any computations we can, such as parsing | ||||||||||||
|
@@ -441,14 +487,20 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, ChannelStore, Mess | |||||||||||
|
||||||||||||
@override | ||||||||||||
void dispose() { | ||||||||||||
assert(!_disposed); | ||||||||||||
recentDmConversationsView.dispose(); | ||||||||||||
unreads.dispose(); | ||||||||||||
_messages.dispose(); | ||||||||||||
typingStatus.dispose(); | ||||||||||||
updateMachine?.dispose(); | ||||||||||||
connection.close(); | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit:
In this situation it's not that we will be setting up a new HTTP client instance; rather we already have, which was passed to the new PerAccountStore (and even before that, was used for fetching the new store's initial snapshot). The new store gets constructed before the old one is disposed. |
||||||||||||
_disposed = true; | ||||||||||||
super.dispose(); | ||||||||||||
} | ||||||||||||
|
||||||||||||
Future<void> handleEvent(Event event) async { | ||||||||||||
assert(!_disposed); | ||||||||||||
|
||||||||||||
switch (event) { | ||||||||||||
case HeartbeatEvent(): | ||||||||||||
assert(debugLog("server event: heartbeat")); | ||||||||||||
|
@@ -590,6 +642,8 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, ChannelStore, Mess | |||||||||||
} | ||||||||||||
|
||||||||||||
Future<void> sendMessage({required MessageDestination destination, required String content}) { | ||||||||||||
assert(!_disposed); | ||||||||||||
|
||||||||||||
// TODO implement outbox; see design at | ||||||||||||
// https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.23M3881.20Sending.20outbox.20messages.20is.20fraught.20with.20issues/near/1405739 | ||||||||||||
return _apiSendMessage(connection, | ||||||||||||
|
@@ -682,7 +736,7 @@ class LiveGlobalStore extends GlobalStore { | |||||||||||
final AppDatabase _db; | ||||||||||||
|
||||||||||||
@override | ||||||||||||
Future<PerAccountStore> loadPerAccount(int accountId) async { | ||||||||||||
Future<PerAccountStore> doLoadPerAccount(int accountId) async { | ||||||||||||
final updateMachine = await UpdateMachine.load(this, accountId); | ||||||||||||
return updateMachine.store; | ||||||||||||
} | ||||||||||||
|
@@ -708,6 +762,14 @@ class LiveGlobalStore extends GlobalStore { | |||||||||||
assert(rowsAffected == 1); | ||||||||||||
} | ||||||||||||
|
||||||||||||
@override | ||||||||||||
Future<void> doRemoveAccount(int accountId) async { | ||||||||||||
final rowsAffected = await (_db.delete(_db.accounts) | ||||||||||||
..where((a) => a.id.equals(accountId)) | ||||||||||||
).go(); | ||||||||||||
assert(rowsAffected == 1); | ||||||||||||
} | ||||||||||||
|
||||||||||||
@override | ||||||||||||
String toString() => '${objectRuntimeType(this, 'LiveGlobalStore')}#${shortHash(this)}'; | ||||||||||||
} | ||||||||||||
|
@@ -722,7 +784,9 @@ class UpdateMachine { | |||||||||||
// case of unauthenticated access to a web-public realm. We authenticated. | ||||||||||||
throw Exception("bad initial snapshot: missing queueId"); | ||||||||||||
})(), | ||||||||||||
lastEventId = initialSnapshot.lastEventId; | ||||||||||||
lastEventId = initialSnapshot.lastEventId { | ||||||||||||
store.updateMachine = this; | ||||||||||||
} | ||||||||||||
|
||||||||||||
/// Load the user's data from the server, and start an event queue going. | ||||||||||||
/// | ||||||||||||
|
@@ -772,6 +836,8 @@ class UpdateMachine { | |||||||||||
final String queueId; | ||||||||||||
int lastEventId; | ||||||||||||
|
||||||||||||
bool _disposed = false; | ||||||||||||
|
||||||||||||
static Future<InitialSnapshot> _registerQueueWithRetry( | ||||||||||||
ApiConnection connection) async { | ||||||||||||
BackoffMachine? backoffMachine; | ||||||||||||
|
@@ -858,17 +924,21 @@ class UpdateMachine { | |||||||||||
}()); | ||||||||||||
} | ||||||||||||
|
||||||||||||
if (_disposed) return; | ||||||||||||
|
||||||||||||
final GetEventsResult result; | ||||||||||||
try { | ||||||||||||
result = await getEvents(store.connection, | ||||||||||||
queueId: queueId, lastEventId: lastEventId); | ||||||||||||
} catch (e) { | ||||||||||||
if (_disposed) return; | ||||||||||||
|
||||||||||||
store.isLoading = true; | ||||||||||||
switch (e) { | ||||||||||||
case ZulipApiException(code: 'BAD_EVENT_QUEUE_ID'): | ||||||||||||
assert(debugLog('Lost event queue for $store. Replacing…')); | ||||||||||||
// This disposes the store, which disposes this update machine. | ||||||||||||
await store._globalStore._reloadPerAccount(store.accountId); | ||||||||||||
dispose(); | ||||||||||||
debugLog('… Event queue replaced.'); | ||||||||||||
return; | ||||||||||||
|
||||||||||||
|
@@ -890,6 +960,8 @@ class UpdateMachine { | |||||||||||
} | ||||||||||||
} | ||||||||||||
|
||||||||||||
if (_disposed) return; | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: let's put these checks consistently right after each That's where we want them semantically; so arranging it that way overtly in the source code is I think the only reasonable way to be sure we're doing that. So e.g. this one moves to right after And the one toward the top of the loop moves further up, to right after Then for completeness we can also put an assert at the top of the method itself, like we're doing on other methods. |
||||||||||||
|
||||||||||||
// After one successful request, we reset backoff to its initial state. | ||||||||||||
// That way if the user is off the network and comes back on, the app | ||||||||||||
// doesn't wind up in a state where it's slow to recover the next time | ||||||||||||
|
@@ -911,6 +983,7 @@ class UpdateMachine { | |||||||||||
final events = result.events; | ||||||||||||
for (final event in events) { | ||||||||||||
await store.handleEvent(event); | ||||||||||||
if (_disposed) return; | ||||||||||||
} | ||||||||||||
if (events.isNotEmpty) { | ||||||||||||
lastEventId = events.last.id; | ||||||||||||
|
@@ -926,6 +999,8 @@ class UpdateMachine { | |||||||||||
// TODO(#322) save acked token, to dedupe updating it on the server | ||||||||||||
// TODO(#323) track the addFcmToken/etc request, warn if not succeeding | ||||||||||||
Future<void> registerNotificationToken() async { | ||||||||||||
if (_disposed) return; | ||||||||||||
Comment on lines
1001
to
+1002
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should be an assert, I think — seems like a bug if we get here when it's already disposed. |
||||||||||||
|
||||||||||||
if (!debugEnableRegisterNotificationToken) { | ||||||||||||
return; | ||||||||||||
} | ||||||||||||
|
@@ -939,8 +1014,18 @@ class UpdateMachine { | |||||||||||
await NotificationService.registerToken(store.connection, token: token); | ||||||||||||
} | ||||||||||||
|
||||||||||||
void dispose() { // TODO abort long-poll and close ApiConnection | ||||||||||||
/// Cleans up resources and tells the instance not to make new API requests. | ||||||||||||
/// | ||||||||||||
/// After this is called, the instance is not in a usable state | ||||||||||||
/// and should be abandoned. | ||||||||||||
/// | ||||||||||||
/// To abort polling mid-request, [store]'s [PerAccountStore.connection] | ||||||||||||
/// needs to be closed using [ApiConnection.close], which causes in-progress | ||||||||||||
/// requests to error. [PerAccountStore.dispose] does that. | ||||||||||||
void dispose() { | ||||||||||||
assert(!_disposed); | ||||||||||||
NotificationService.instance.token.removeListener(_registerNotificationToken); | ||||||||||||
_disposed = true; | ||||||||||||
} | ||||||||||||
|
||||||||||||
/// In debug mode, controls whether [fetchEmojiData] should | ||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: paragraph in commit message is no longer relevant in this revision: