model [nfc]: Comment on lack of [AutocompleteViewManager.dispose] #645

chrisbobbe · 2024-05-06T20:58:01Z

No description provided.

gnprice · 2024-05-07T00:12:31Z

Thanks. I prefer a different way of putting it, so I'll push a version that does that. LMK whether the revised version seems clear to you, and we can iterate from there.

chrisbobbe · 2024-05-07T00:29:12Z

Sure, LGTM!

PerAccountStore shouldn't be an owner of the MessageListView elements. Its relationship to MessageListView is similar to that of AutocompleteViewManager to MentionAutocompleteView (zulip#645). With two owners, the MessageListView can be disposed twice: 1. before the frame is rendered, `removeAccount` disposes the `PerAccountStoreWidget`, which disposes the `MessageListView`; `_MessageListState` is not yet disposed; 2. during build, because `store` is set to `null`, `PerAccountStoreWidget` gets rebuilt. `_MessageListState`, a descendent of it, is no longer in the render tree; 3. during finalization, `_MessageListState` tries to dispose the `MessageListView`. This removes regression tests added for zulip#810, because `MessageStoreImpl.dispose` no longer exists. `MessageListView` does not get disposed unless there is a `_MessageListState` owner. Signed-off-by: Zixuan James Li <zixuan@zulip.com>

PerAccountStore shouldn't be an owner of the MessageListView elements. Its relationship to MessageListView is similar to that of AutocompleteViewManager to MentionAutocompleteView (zulip#645). With two owners, the MessageListView can be disposed twice: 1. before the frame is rendered, `removeAccount` disposes the `PerAccountStoreWidget`, which disposes the `MessageListView`; `_MessageListState` is not yet disposed; 2. during build, because `store` is set to `null`, `PerAccountStoreWidget` gets rebuilt. `_MessageListState`, a descendent of it, is no longer in the render tree; 3. during finalization, `_MessageListState` tries to dispose the `MessageListView`. This removes regression tests added for zulip#810, because `MessageStoreImpl.dispose` no longer exists. `MessageListView` does not get disposed unless there is a `_MessageListState` owner. See discussion: https://chat.zulip.org/#narrow/channel/243-mobile-team/topic/.60MentionAutocompleteView.2Edispose.60/near/2083074 Signed-off-by: Zixuan James Li <zixuan@zulip.com>

PerAccountStore shouldn't be an owner of the MessageListView elements. Its relationship to MessageListView is similar to that of AutocompleteViewManager to MentionAutocompleteView (zulip#645). With two owners, the MessageListView can be disposed twice: 1. before the frame is rendered, `removeAccount` disposes the `PerAccountStoreWidget`, which disposes the `MessageListView`; `_MessageListState` is not yet disposed; 2. during build, because `store` is set to `null`, `PerAccountStoreWidget` gets rebuilt. `_MessageListState`, a descendent of it, is no longer in the render tree; 3. during finalization, `_MessageListState` tries to dispose the `MessageListView`. This removes regression tests added for zulip#810, because `MessageStoreImpl.dispose` no longer exists. `MessageListView` does not get disposed unless there is a `_MessageListState` owner. Signed-off-by: Zixuan James Li <zixuan@zulip.com>

PerAccountStore shouldn't be an owner of the MessageListView objects. Its relationship to MessageListView is similar to that of AutocompleteViewManager to MentionAutocompleteView (zulip#645). With two owners, MessageListView can be disposed twice: 1. before the frame is rendered, `GlobalStore.removeAccount` disposes the `PerAccountStore`, which disposes the `MessageListView` (via `MessageStoreImpl`), notifying listeners of `GlobalStore`. At this point `_MessageListState` is not yet disposed. 2. As a dependent of `GlobalStore`, a rebuilt is triggered for `PerAccountStoreWidget`. This time, the `MessageList` Element is no longer in the element tree 2. during build, because `store` is set to `null`, `PerAccountStoreWidget` gets rebuilt. `_MessageListState`, a descendent of it, is no longer in the render tree; 3. during finalization, `_MessageListState` tries to dispose the `MessageListView`. This removes regression tests added for zulip#810, because `MessageStoreImpl.dispose` no longer exists. `MessageListView` does not get disposed unless there is a `_MessageListState` owner. See discussion: https://chat.zulip.org/#narrow/channel/243-mobile-team/topic/.60MentionAutocompleteView.2Edispose.60/near/2083074 Signed-off-by: Zixuan James Li <zixuan@zulip.com>

PerAccountStore shouldn't be an owner of the MessageListView elements. Its relationship to MessageListView is similar to that of AutocompleteViewManager to MentionAutocompleteView (zulip#645). With two owners, the MessageListView can be disposed twice: 1. before the frame is rendered, `removeAccount` disposes the `PerAccountStoreWidget`, which disposes the `MessageListView`; `_MessageListState` is not yet disposed; 2. during build, because `store` is set to `null`, `PerAccountStoreWidget` gets rebuilt. `_MessageListState`, a descendent of it, is no longer in the render tree; 3. during finalization, `_MessageListState` tries to dispose the `MessageListView`. This removes regression tests added for zulip#810, because `MessageStoreImpl.dispose` no longer exists. `MessageListView` does not get disposed unless there is a `_MessageListState` owner. Signed-off-by: Zixuan James Li <zixuan@zulip.com>

`PerAccountStore` shouldn't be an owner of the `MessageListView` objects. Its relationship to `MessageListView` is similar to that of `AutocompleteViewManager` to `MentionAutocompleteView` (zulip#645). With two owners, `MessageListView` can be disposed twice: 1. Before the frame is rendered, after removing the `PerAccountStore` from `GlobalStore`, `removeAccount` disposes the `PerAccountStore` , which disposes the `MessageListView` (via `MessageStoreImpl`). `removeAccount` also notifies the listeners of `GlobalStore`. At this point `_MessageListState` is not yet disposed. 2. Being dependent on `GlobalStore`, `PerAccountStoreWidget` is rebuilt. This time, the StatefulElement corresponding to the `MessageList` widget, is no longer in the element tree because `PerAccountStoreWidget` cannot find the account and builds a placeholder instead. 3. During finalization, `_MessageListState` tries to dispose the `MessageListView`, and fails to do so. We couldn't've kept `MessageStoreImpl.dispose` with an assertion `_messageListView.isEmpty`, because `PerAccountStore` is disposed before `_MessageListState.dispose` (and similarily `_MessageListState.onNewStore`) is called. Fixing that will be a future follow-up to this, as noted in the TODO comment. A regression test for zulip#810 has been appropriated. The original issue is relevant because the scenario this integration test targeted still applies after this change. See discussion: https://chat.zulip.org/#narrow/channel/243-mobile-team/topic/MessageListView.20lifecycle/near/2083074 Signed-off-by: Zixuan James Li <zixuan@zulip.com>

`PerAccountStore` shouldn't be an owner of the `MessageListView` objects. Its relationship to `MessageListView` is similar to that of `AutocompleteViewManager` to `MentionAutocompleteView` (zulip#645). With two owners, `MessageListView` can be disposed twice when `removeAccount` is called (when the user logs out, for example): 1. Before the frame is rendered, after removing the `PerAccountStore` from `GlobalStore`, `removeAccount` disposes the `PerAccountStore` , which disposes the `MessageListView` (via `MessageStoreImpl`). `removeAccount` also notifies the listeners of `GlobalStore`. At this point `_MessageListState` is not yet disposed. 2. Being dependent on `GlobalStore`, `PerAccountStoreWidget` is rebuilt. This time, the StatefulElement corresponding to the `MessageList` widget, is no longer in the element tree because `PerAccountStoreWidget` cannot find the account and builds a placeholder instead. 3. During finalization, `_MessageListState` tries to dispose the `MessageListView`, and fails to do so. We couldn't've kept `MessageStoreImpl.dispose` with an assertion `_messageListView.isEmpty`, because `PerAccountStore` is disposed before `_MessageListState.dispose` (and similarily `_MessageListState.onNewStore`) is called. Fixing that will be a future follow-up to this, as noted in the TODO comment. See discussion: https://chat.zulip.org/#narrow/channel/243-mobile-team/topic/MessageListView.20lifecycle/near/2083074 Signed-off-by: Zixuan James Li <zixuan@zulip.com>

`PerAccountStore` shouldn't be an owner of the `MessageListView` objects. Its relationship to `MessageListView` is similar to that of `AutocompleteViewManager` to `MentionAutocompleteView` (zulip#645). With two owners, `MessageListView` can be disposed twice when `removeAccount` is called (when the user logs out, for example): 1. Before the frame is rendered, after removing the `PerAccountStore` from `GlobalStore`, `removeAccount` disposes the `PerAccountStore` , which disposes the `MessageListView` (via `MessageStoreImpl`). `removeAccount` also notifies the listeners of `GlobalStore`. At this point `_MessageListState` is not yet disposed. 2. Being dependent on `GlobalStore`, `PerAccountStoreWidget` is rebuilt. This time, the StatefulElement corresponding to the `MessageList` widget, is no longer in the element tree because `PerAccountStoreWidget` cannot find the account and builds a placeholder instead. 3. During finalization, `_MessageListState` tries to dispose the `MessageListView`, and fails to do so. We couldn't've kept `MessageStoreImpl.dispose` with an assertion `_messageListView.isEmpty`, because `PerAccountStore` is disposed before `_MessageListState.dispose` (and similarily `_MessageListState.onNewStore`) is called. Fixing that will be a future follow-up to this, as noted in the TODO comment. See discussion: https://chat.zulip.org/#narrow/channel/243-mobile-team/topic/MessageListView.20lifecycle/near/2086893 Signed-off-by: Zixuan James Li <zixuan@zulip.com>

chrisbobbe requested a review from gnprice May 6, 2024 20:58

chrisbobbe added the a-model Implementing our data model (PerAccountStore, etc.) label May 6, 2024

chrisbobbe force-pushed the pr-comment-autocomplete-view-manager-dispose branch from 2ef7274 to 7dd19bb Compare May 6, 2024 21:00

autocomplete [nfc]: Explain why no [AutocompleteViewManager.dispose]

ebf41f2

gnprice force-pushed the pr-comment-autocomplete-view-manager-dispose branch from 7dd19bb to ebf41f2 Compare May 7, 2024 00:12

gnprice merged commit ebf41f2 into zulip:main May 7, 2024

chrisbobbe deleted the pr-comment-autocomplete-view-manager-dispose branch May 7, 2024 00:29

PIG208 mentioned this pull request Feb 8, 2025

store: Ensure sole ownership of MessageListView #1340

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

model [nfc]: Comment on lack of [AutocompleteViewManager.dispose] #645

model [nfc]: Comment on lack of [AutocompleteViewManager.dispose] #645

chrisbobbe commented May 6, 2024

gnprice commented May 7, 2024

chrisbobbe commented May 7, 2024

model [nfc]: Comment on lack of [AutocompleteViewManager.dispose] #645

model [nfc]: Comment on lack of [AutocompleteViewManager.dispose] #645

Conversation

chrisbobbe commented May 6, 2024

gnprice commented May 7, 2024

chrisbobbe commented May 7, 2024