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. 😓