Skip to content

Edit-message (5/n): Fix bug where compose progress can be lost on new event queue #1482

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 25, 2025
Merged

Edit-message (5/n): Fix bug where compose progress can be lost on new event queue #1482

merged 1 commit into from
Apr 25, 2025

Conversation

chrisbobbe
Copy link
Collaborator

And factor out a helper for the part that reads widget.narrow and sets a new controller accordingly; we'll use this helper for the edit-message UI, to reset the compose box at the end of an edit-message session.

Fixes: #1470

@chrisbobbe chrisbobbe added the maintainer review PR ready for review by Zulip maintainers label Apr 24, 2025
@chrisbobbe chrisbobbe requested a review from PIG208 April 24, 2025 02:33
@chrisbobbe chrisbobbe mentioned this pull request Apr 24, 2025
Closed
PIG208
PIG208 reviewed Apr 24, 2025
View reviewed changes
Copy link
Member

@PIG208 PIG208 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! The fix looks good to me with manual testing. Left some comments.

test/widgets/compose_box_test.dart Outdated
await tester.pump();

// new store replaces the old one
final newStore = testBinding.globalStore.perAccountSync(eg.selfAccount.id)!;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about this?

Suggested change
final newStore = testBinding.globalStore.perAccountSync(eg.selfAccount.id)!;
final newStore = testBinding.globalStore.perAccountSync(store.accountId)!;

This way we don't rely on prepareComposeBox using a specific value for self-account ID.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooh, thanks for thinking of this.

test/widgets/compose_box_test.dart Outdated
Comment on lines 1275 to 1279
// new store has the same basic boring data so the compose box
// has its content input instead of no-posting-permission error banner
// TODO instead, add data by preparing new store's initial snapshot
await newStore.addStream(channel);
await newStore.addUser(eg.selfUser);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the TODO just calls for moving setup code into the initial snapshot in prepareComposeBox?

     await testBinding.globalStore.add(selfAccount, eg.initialSnapshot(
+      realmUsers: [selfUser, ...otherUsers],
+      streams: streams,
       zulipFeatureLevel: zulipFeatureLevel,
       realmMandatoryTopics: mandatoryTopics,
     ));
 
     store = await testBinding.globalStore.perAccount(selfAccount.id);
-
-    await store.addUsers([selfUser, ...otherUsers]);
-    await store.addStreams(streams);

The initial call to testBinding.globalStore.add sets the initial snapshot that will also be used when reloading the store.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The initial call to testBinding.globalStore.add sets the initial snapshot that will also be used when reloading the store.

Ohh, I didn't realize; thanks for pointing this out.

lib/widgets/compose_box.dart Outdated
Comment on lines 1528 to 1530
if (_controller == null) {
_setNewControllerForNarrow(newStore);
} else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: perhaps we can return early here and de-indent the else-block

@chrisbobbe
Copy link
Collaborator Author

Thanks for the review! Revision pushed.

@PIG208
Copy link
Member

PIG208 commented Apr 24, 2025

Thanks! Marking for Greg's review.

@PIG208 PIG208 added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Apr 24, 2025
@PIG208 PIG208 assigned gnprice and unassigned PIG208 Apr 24, 2025
@chrisbobbe chrisbobbe requested a review from gnprice April 25, 2025 00:46
gnprice
gnprice reviewed Apr 25, 2025
View reviewed changes
Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks both! Looks good; small comments.

lib/widgets/compose_box.dart Outdated
Comment on lines 1533 to 1535
switch (_controller) {
case StreamComposeBoxController():
(_controller as StreamComposeBoxController).topic.store = newStore;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be simplified by extracting a local variable as a copy of the field, so that type promotion works:

Suggested change
switch (_controller) {
case StreamComposeBoxController():
(_controller as StreamComposeBoxController).topic.store = newStore;
final controller = _controller;
switch (controller) {
case StreamComposeBoxController():
controller.topic.store = newStore;

lib/widgets/compose_box.dart Outdated
Comment on lines 1538 to 1539
case null:
assert(false);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Similarly if the local is used for the initial null check, then the analyzer sees this case is impossible.

lib/widgets/compose_box.dart Outdated
}
}

void _setNewControllerForNarrow(PerAccountStore store) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: drop "for narrow"? Seems like it's just the new controller that's appropriate for this compose box.

@chrisbobbe
compose: Fix bug where compose progress can be lost on new event queue
2683df0 
And factor out a helper for the part that reads `widget.narrow` and
sets a new controller accordingly; we'll use this helper for the
edit-message UI, to reset the compose box at the end of an
edit-message session.

Fixes: zulip#1470
@chrisbobbe
Copy link
Collaborator Author

Thanks! Revision pushed.

@gnprice
Copy link
Member

gnprice commented Apr 25, 2025

Thanks! Looks good; merging.

@gnprice gnprice merged commit 2683df0 into zulip:main Apr 25, 2025
1 check passed
@chrisbobbe chrisbobbe deleted the pr-edit-message-5 branch April 25, 2025 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gnprice gnprice gnprice left review comments

@PIG208 PIG208 PIG208 left review comments

Assignees

@gnprice gnprice

Labels
integration review Added by maintainers when PR may be ready for integration
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

compose: In topic or DM narrow, compose progress is lost when event queue expires
3 participants
@chrisbobbe @PIG208 @gnprice