Message List from Same User is now Distinguishable via Surface Tint on Long Press

[Gaurav-Kushwaha-1225]

Changes Made

This issue resolved by:

• Wrapping the MessageWithPossibleSender with a Container
• Then, giving it a color property on a condition that when isMessageActionSheetOpen is true show the surface tint, else remain Colors.transparent.

For this,

• I shifted the MessageWithPossibleSender from StatelessWidget to StatefulWidget.
• Added a nullable VoidCallback? onDismiss method in parameters of showMessageActionSheet or BottomSheet for updating the Surface Tint of message.

Fixes #1142

Screenshots

Light Theme	Dark Theme
video_2024-12-13_13-16-02.mp4	video_2024-12-13_13-16-08.mp4

[alya]

Please post screenshots of your changes in the PR description, as videos are hard to review.

[Gaurav-Kushwaha-1225]

Please post screenshots of your changes in the PR description, as videos are hard to review.

Changes

1st

• shifting the MessageWithPossibleSender widget from StatelessWidget to StatefulWidget.
• adding isMessageActionSheetOpen and pressedTint as parameters of MessageWithPossibleSender for making surface value tint to transparent after bottom sheet is closed.
• Screenshot:

2nd

• Wrapped with Container with color property on isMessageActionSheetOpen condition.
• Updating the function of onLongPress of MessageWithPossibleSender widget. That, when isMessageActionSheetOpen is true show the surface tint.
• Screenshot:

3rd

• Just adding a nullable VoidCallback? onDismiss method in parameters of showMessageActionSheet and _showActionSheet for getting response of closing of bottom sheet.
• Screenshots:
• 
• 

[chrisbobbe]

Alya means before/after screenshots of the app's UI. We can read your code easily without screenshots :)

[chrisbobbe]

Also I see there's a failing check; please run tests locally (tools/check) and fix any failures.

[Gaurav-Kushwaha-1225]

Alya means before/after screenshots of the app's UI. We can read your code easily without screenshots :)

I am sorry for the confusion.
Here, are the screenshots:

Before	After

Also I see there's a failing check; please run tests locally (tools/check) and fix any failures.

I have updated the changes and there are no failing checks and conflicts now.
Thank you.

[alya]

Are those before/after scheenshots the same? The point is to show the differences from your PR's changes.

[Gaurav-Kushwaha-1225]

Are those before/after scheenshots the same? The point is to show the differences from your PR's changes.

@alya,
In the screenshots, the message on which the user has long pressed, that message has appeared a surface tint color separating from rest other messages, in both light and dark theme.
This was the issue to resolve i.e. to give a surface tint to those messages where long press event has occurred.

[alya]

Oh, I see it now.

Is it happening in the dark theme screenshot? I don't see a difference, so if the effect is there, it's too subtle.

[alya]

It's also best to take screenshots that are identical other than the change your PR makes, to make them easy to compare.

[Gaurav-Kushwaha-1225]

Oh, I see it now.

Is it happening in the dark theme screenshot? I don't see a difference, so if the effect is there, it's too subtle.

@alya
Yeah, actually I took the same tint color for both themes. I saw in the figma file both are different for different themes.
I will update the PR in few minutes.
Sorry, for my inconvenience.

[alya]

Sure, please be mindful to review your own work carefully next time. Posting screenshots is one of the things that should help you identify any problems yourself, before asking someone else to take a look.

[Gaurav-Kushwaha-1225]

@alya

Here, is the updated PR.

Requirements according to Figma File

• 
• 

Before and After Screenshots

NOTE: In after screenshots, you may see the colors appearing to be a bit different (or less light/dark) from actual figma. This is because the action sheet is opened in my after screenshots which are not in figma file.

Before	After

Both pressedTint colors in light and dark theme are according to the Figma File i.e. RGBA(0, 0, 0, 0.04) for light theme and RGBA(1, 1, 1, 0.04) for dark theme.

[PIG208]

Hi @Gaurav-Kushwaha-1225! Thanks for the updates. For the PR to be reviewable, we need tests for this: https://github.com/zulip/zulip-flutter?tab=readme-ov-file#submitting-a-pull-request

Please also pay attention to the point on commit style guide from that link, and organize the commit history coherently.

[chrisbobbe]

I see you're adding merge commits; Zulip doesn't use those.

[Gaurav-Kushwaha-1225]

Hi @Gaurav-Kushwaha-1225! Thanks for the updates. For the PR to be reviewable, we need tests for this: https://github.com/zulip/zulip-flutter?tab=readme-ov-file#submitting-a-pull-request

Please also pay attention to the point on commit style guide from that link, and organize the commit history coherently.

Hi @PIG208, I have added the necessary tests for these changes in test/widgets/message_list_test.dart file.
Please review the changes and feature added.
Thank you.

[PIG208]

Looks like this update still hasn't addressed my previous comment on the commit style:

Please also pay attention to the point on commit style guide from that link, and organize the commit history coherently.

As Chris said, we don't use merge commits. Could you please also review the commit style guide linked in our README?

[Gaurav-Kushwaha-1225]

Looks like this update still hasn't addressed my previous comment on the commit style:

Please also pay attention to the point on commit style guide from that link, and organize the commit history coherently.

As Chris said, we don't use merge commits. Could you please also review the commit style guide linked in our README?

I’ve updated the commits, improving the commit style and making other enhancements.
I believe it’s ready for review now.

Please let me know if any further changes are required.

[chrisbobbe]

Thanks! Comments below. Please also squash these two commits into one, so the tests land in the same commit as the code they're testing.

[gnprice]

In addition to Chris's comments above, one other high-level comment. (I've only done an initial skim of this PR, since it's still in maintainer review.)

[gnprice]

I'll update your PR branch with a quick prep commit

Done. @Gaurav-Kushwaha-1225 As the first step when you start work on your next revision, be sure to pull down the updated version of this branch — use git fetch me or git fetch origin, whatever name you use for the remote pointing at your GitHub fork of this repo. (If you're unsure how to do this, the #git help channel is a good place for questions.)

[Gaurav-Kushwaha-1225]

Hi @chrisbobbe, I have updated the commits as per your request.

• Changed return type of showMessageActionSheet and _showActionSheet to ModalStatus
• Used the logic as you said in above comments for pressedTint

video_2025-01-21_19-39-25.mp4

[chrisbobbe]

Thanks! Small comments below.

[Gaurav-Kushwaha-1225]

Hi @chrisbobbe,
I have updated this PR according to your review.

Please have a look at it and let me know if anything else is required.
Thank you! 😊

[Gaurav-Kushwaha-1225]

Hi @chrisbobbe,
I have updated this feat. PR accordingly.

Please take a review and let me know if something else is required.

[Gaurav-Kushwaha-1225]

Screenshots:

Below are the screenshots of added pressedTint feature. Unlike in previous screenshots of this PR, the outer padding is also included now as asked in above review comments.

Before	After

[gnprice]

Thanks @Gaurav-Kushwaha-1225 and @chrisbobbe for your work on this so far!

The issue this addresses is an M6 Post-launch issue, and we're now focusing more strictly on launch issues until the launch is ready. But given that a substantial amount of review work has gone into this and it seems like it's relatively close to reaching a state we're able to merge, this can be one of the exceptions that we continue work on nonetheless.

[chrisbobbe]

Hi @Gaurav-Kushwaha-1225, GitHub is saying there are some merge conflicts; would you resolve those please?

[Gaurav-Kushwaha-1225]

Hi @Gaurav-Kushwaha-1225, GitHub is saying there are some merge conflicts; would you resolve those please?

I've updated the commits, and there are no more merge conflicts. However, the current failing CI checks are due to a separate issue in tools/check android, as discussed in Zulip Chat.

This CI failure is android specific, we can ignore the android suite for this PR as it's not touching any android specific files.

[chrisbobbe]

Thanks! Ah, but I see now there are some new merge conflicts; would you resolve those please?

[Gaurav-Kushwaha-1225]

I've updated the commits, and there are no more merge conflicts.

I've rebased the commits, and there are no more merge conflicts.
Hopefully, there won't be any conflicts when you review the PR again. 😓

[chrisbobbe]

Thanks for the revision! Here's another review, this time with a new thought about a case that needs test coverage.

[chrisbobbe]

The visual feedback is also given on long-press gestures that are canceled and don't cause the action sheet to open. How about just 'background-color tint'?

[chrisbobbe]

Ah, there's another important case that we should test that isn't covered yet: the onLongPressCancel case. The user puts their finger down, then quickly drags because they want to scroll instead of opening the action sheet. The UI recognizes this as cancelling the long-press gesture, and we should test that the feedback disappears at that time.

Rather than just adding another testWidgets, I think a neat way to organize the test coverage is in just two tests, each with a clear storyline:

• 'long-press opens open action sheet':
  • Check transparent
  • Begin long-press
  • Check tinted
  • End long-press
  • Check tinted (it didn't flicker to transparent here)
  • wait through the bottom-sheet entrance animation (for examples, search for 'default duration of bottom-sheet enter animation')
  • Check tinted
  • Dismiss action sheet
  • Check transparent
• 'long-press canceled':
  • Check transparent
  • Begin long-press
  • Check tinted
  • Cancel long-press, for example by dragging
  • Check transparent

[Gaurav-Kushwaha-1225]

Hi @chrisbobbe,

The above comment #1152 (comment) and this comment are referring to the same issue?

If yes, then np.
I have updated the PR with both the required tests and changes.

Thank You.

[chrisbobbe]

Thanks! Comments below, including some substantive comments on the tests.

[chrisbobbe]

This is missing much of what I outlined at #1152 (comment) . In fact, if I intentionally remove onLongPressDown and onLongPressUp in the implementation, this test still passes, meaning it wouldn't catch user-facing bugs that might sneak in in the future.

Quoting what I outlined, with annotations:

'long-press opens open action sheet':

• Check transparent
• Begin long-press
• Check tinted ← This is missing; (a)
• End long-press
• Check tinted (it didn't flicker to transparent here) ← This isn't working as intended; (b)
• wait through the bottom-sheet entrance animation (for examples, search for 'default duration of bottom-sheet enter animation') ← We should explicitly wait for 250ms; please search for those examples I mentioned; (c)
• Check tinted
• Dismiss action sheet
• Check transparent

(a) should be possible by calling tester.longPress and storing the returned Future in a variable instead of immediately awaiting it. Then pump for some duration that's shorter than would trigger a long-press. I see the following in the implementation of tester.longPress:

await pump(kLongPressTimeout + kPressTimeout);

So how about awaiting a pump of half that duration? Then to proceed to the end of the long-press gesture, pump that duration again, and await the stored Future.

(b) is subtle but also good to test. You can test whether this check is working by temporarily breaking the implementation in a way that should make the check fail, and confirm that the check does fail. Here's a way to do that (i.e. delaying the "selected" state by a frame):

--- lib/widgets/message_list.dart
+++ lib/widgets/message_list.dart
@@ -2,6 +2,7 @@ import 'dart:math';
 
 import 'package:collection/collection.dart';
 import 'package:flutter/material.dart';
+import 'package:flutter/scheduler.dart';
 import 'package:flutter_color_models/flutter_color_models.dart';
 import 'package:intl/intl.dart' hide TextDirection;
 
@@ -1428,7 +1429,8 @@ class _MessageWithPossibleSenderState extends State<MessageWithPossibleSender> {
     return GestureDetector(
       behavior: HitTestBehavior.translucent,
       onLongPress: () async {
-        statesController.update(WidgetState.selected, true);
+        SchedulerBinding.instance.addPostFrameCallback(
+          (_) => statesController.update(WidgetState.selected, true));
         ModalStatus status = showMessageActionSheet(context: context,
           message: message);
         await status.closed;

It might be as easy as doing the check before instead of after a pump().

For (c), see existing tests, as mentioned.

[Gaurav-Kushwaha-1225]

According to the above comments, I think this would be the updated version. I also added comments so that I can describe what I think is happening via those respective lines.

  testWidgets('long-press opens action sheet', (tester) async {
    await setupMessageListPage(tester, messages: [message]);

    // Check starts as transparent
    check(getBackgroundColor(tester)).equals(Colors.transparent);

    // Begin long-press - part (a)
    final gestureFuture = tester.longPress(find.byType(MessageWithPossibleSender));
    await tester.pump(const Duration(milliseconds: 300));

    final expectedTint = DesignVariables.of(tester.element(find.byType(MessageWithPossibleSender)))
      .pressedTint;

    // Check tinted after short pump
    check(getBackgroundColor(tester)).equals(expectedTint);

    // Complete long-press
    await tester.pump(const Duration(milliseconds: 300));
    await gestureFuture;

    // Check tinted after long-press 
    // (part b - Not yet completed, as it is not failing a particular case when implemented that it should fail.)
    check(getBackgroundColor(tester)).equals(expectedTint);

    // Wait explicitly for bottom-sheet entrance animation (250ms)
    await tester.pump(const Duration(milliseconds: 250));

    // Check still tinted
    check(getBackgroundColor(tester)).equals(expectedTint);

    // Dismiss action sheet
    await tester.tapAt(const Offset(0, 0));
    await tester.pumpAndSettle();

    // Check returns to transparent
    check(getBackgroundColor(tester)).equals(Colors.transparent);
  });

Breakdown of Changes

1. For part (a)

• It's now implemented as requested.
• Used 300 milliseconds because await pump(kLongPressTimeout + kPressTimeout); summed up to 600ms.
• However, an error occurs while running the test because await should be there after a future method call:

The following assertion was thrown running a test:
Guarded function conflict.
You must use "await" with all Future-returning test APIs.
The guarded method "longPressAt" from class WidgetController was called from
Then, the "pump" method from class WidgetTester was called from
The first method (WidgetController.longPressAt) had not yet finished executing at the time that the
second method (WidgetTester.pump) was called. Since both are guarded, and the second was not a
nested call inside the first, the first must complete its execution before the second can be called.
Typically, this is achieved by putting an "await" statement in front of the call to the first.

2. For part (b)

• I completely understood the request, butt.
• This check isn't failing, but after making the suggested changes (in details part discussion link), as it should fail.
• I am trying to figure out where I am missing in this case of checking tint after long press.

3. For part (c)

• Used 250ms for waiting, as I couldn't find a default duration for bottom-sheet enter animation.
• The Flutter documentation states that the default value is null.
• References:
  • Bottom Sheet Animation Controller
  • Bottom Sheet Animation Controller Constructor

I will also ask regarding part (b) in the mobile-dev-help group for help.
Thank You.