content: Add distinguishing pill colors for mentions

[Ruhdee]

Fixes #646

Before	After
In mentions narrow:

Changes made:

1. Updated a TODO comment.
2. Added WildcardMentionNode class, even though it is not strictly necessary since user group mentions and wildcard mentions use the same colors, to complete the sealed class MentionNode.
3. Used the same colors as web and updated code to display different colors for the background pills of user mentions vs user group and wildcard mentions.
4. Added tests.

Notes:

1. Named new color as colorGroupMentionBackground to follow web's example(in web, the name is: --color-background-text-group-mention even though the color is used for both user group mentions and wildcard mentions).

[rajveermalviya]

Thanks @Ruhdee! Overall looks good, some small comments below.

[Ruhdee]

@rajveermalviya PTAL!
Changes made:

1. Added links to code in commit message and wrote Fixes #646. according to this comment:
   settings: Make page scrollable #1911 (comment)
   The links in the commit message are quite long, should I break them across lines?
2. Added backgroundPillColor variable and set its value using exhaustive switch.

[rajveermalviya]

Thanks for the revision! LGTM, couple of nits below, moving over to Greg's review.

[rajveermalviya]

nit: Can cut the commit-ID to 9 characters:

…
https://github.com/zulip/zulip/blob/81e0305f9/web/styles/app_variables.css#L2004
https://github.com/zulip/zulip/blob/81e0305f9/web/styles/app_variables.css#L2012
…

[rajveermalviya]

also nit: Indent the links with 2-spaces.

[Ruhdee]

I have addressed the feedback. Just out of curiosity, why shorten the hash to 9 and not 7 (since I just read that 7 is the usual number git shortens the commit hash to)? Is it because the Zulip codebase is too large?

[gnprice]

Thanks @Ruhdee, and thanks @rajveermalviya for the previous reviews!

Comments below, including a couple of questions about what the server's behavior is in terms of what content HTML exists in messages. For getting a definitive answer to those questions, I think this would be a good time for you to make use of our tools/content/check-features survey script for examining a corpus of public Zulip message content.

The script prints a usage message if you run it with --help (or with no arguments). If you have questions that doesn't answer, please ask in the #mobile-team channel on chat.zulip.org. Only a handful of people have used the script so far, so there are probably rough edges in its API or documentation; if you take notes on any obstacles or confusing things you run into, those notes would be helpful to share.

For this purpose, I think running the script on just chat.zulip.org would work well. Run it first on main to get a baseline; then run it with your changes to see how the results change, if they do. The script mainly focuses on content that's unimplemented (as in UnimplementedNode); so to get informative answers, you can edit your code so that any situations you want to investigate (e.g., a user mention that lacks data-user-id) get treated as unimplemented.

Probably #mobile-team is a good place to post your results, since that's better for interactive discussion of the data than GitHub is. Then when the analysis reaches a conclusion, we can bring it back to this PR thread.

[gnprice]

nit: when a TODO item spills onto additional lines, indent to show those lines are part of the TODO rather than an independent comment:

        // TODO(#647) when self-user is non-silently mentioned, make bold, and
        //   distinguish font color between direct and wildcard mentions.

(separately, I think the sentence reads more smoothly without the colon, now that it's one normal sentence not interrupted by a new TODO marker)

[gnprice]

Hmm but the second half isn't related to whether the self-user is mentioned. Or is it? I guess I'm not sure I understand why this is no longer #646 but instead #647 🙂

[Ruhdee]

In the web app code, the different font colors only come into play when the self-user is mentioned. For mentions without the self-user, the font color is black/default for all mentions.
#647 is specifically about mentions containing the self-user, so it would make sense to do the font color changes in that issue. The issue description already contains lines that, in my opinion, encompass the font color change:

Notice the highlighting is done in two ways:

• The text of the mention itself is styled

• The whole message gets an interesting background color

If I try to introduce the changes in this PR, I will have to address the logic for identifying whether the self-user is mentioned. If you want, I can make this PR solve both issues at the same time.

[gnprice]

Here's the key screenshot in #646, to make things concrete:

That's a user mention and a group mention, neither of them including the self-user. (The fact they don't involve the self-user isn't stated in the issue, but it's made clear in the related issue #647 which includes examples of the same mentions.)

I see this TODO comment said "font color", and indeed the font color is black in both of those. But the background color right around the text is quite different. I think the TODO comment probably meant to say "background color", since that's what #646 is about.

The change in background color i.e. #646 gets handled later in this PR, so we can indeed delete that part from the comment. But that should happen in the same commit that implements the change that makes the comment obsolete, i.e. the main commit of this PR.

Then looking closely at the screenshot in #647, I see you're right that when the self-user is pinged, the font color changes too. (That was subtle! I hadn't spotted it when I looked at that issue before.) So the main commit can change this comment to look like this:

        // TODO(#647) when self-user is non-silently mentioned, make bold and
        //   change font color

It doesn't need to specify the details of the color changes; whoever implements that issue will have to look those up anyway, in order to get the exact colors.

[Ruhdee]

The reason I changed the TODO in a separate nfc commit is that this PR doesn't address text styling at all. Won't changing the comment in the main commit make it look like the main commit addresses the TODO (when in reality the TODO isn't addressed, just changed)?
Also, do minor details like these matter, or should I not spend too much time focusing on them?

[gnprice]

To your last question, I'd say yes and yes 🙂. They do matter, because they help us understand what changes we're making and what our code does, and so help us continue building at high quality. But if it feels like it's taking too much time, then best not to let it block you — go ahead and submit a revision (or a PR, etc.), and just mention that there's this point you're not sure about.

The main commit here doesn't touch text styling, but it does change the background color this text appears on, fixing #646. This TODO comment says "font color" but it also says it's for #646, so I think the intended reading of it is that it really meant the background color, the thing #646 is about.

Maybe the most precise version would be:

• A prep NFC commit corrects this TODO item — not "update", but fix, because it's changing it to what it should have been from the beginning. The fixed version looks like what I said above, plus a line for content: Distinguish direct and wildcard mentions #646 that says background color instead of font color.
• Then the main commit deletes the content: Distinguish direct and wildcard mentions #646 line, because it's implementing what that line calls for.

I think the main thing that confused me (at #2168 (comment) above) about the current revision is that it said "Update TODO comment", and "now lies under", both of which are saying that something has changed which makes the comment need to change; and I couldn't tell what that initial change was. I think now what's actually going on is that nothing has changed, and instead the comment was incorrect from the beginning.

[Ruhdee]

Part 1

This TODO comment says "font color" but it also says it's for #646, so I think the intended reading of it is that it really meant the background color, the thing #646 is about.

I don't believe this is the case. As you can see from the lines below, there exists a separate TODO comment (which I remove in the main commit) that specifically targets the background color. Also this separate TODO comment is directly above the field for background color whereas the TODO comment which we are discussing is directly above the text style field.
https://github.com/zulip/zulip-flutter/pull/2168/changes#diff-53a2f53be6653d3af99fed87e1bc1a9f7f33b8450265247362ef7d2a2f284b36L1227

It looks like the comment we are discussing was meant for font color but was misattributed to #646 from the start (as there have not been significant relevant changes to the description of the two issues since opening). Still, your feedback on using the word "fix" remains applicable.

Part 2
Changed the TODO comment according to this discussion (thus, the word "update" along with "fix" in the commit heading):
https://chat.zulip.org/#narrow/channel/101-design/topic/User.20group.20mentions.20inconsistency/near/2389405

[gnprice]

I have addressed the feedback. Just out of curiosity, why shorten the hash to 9 and not 7 (since I just read that 7 is the usual number git shortens the commit hash to)? Is it because the Zulip codebase is too large?

Yeah, good question. That's discussed here:
https://github.com/zulip/zulip-mobile/blob/main/docs/style.md#mentioning-commits

(At some point we should port that style guide over to here from the legacy app's repo. Parts of it are specific to the legacy app's environment, but other parts like that one carry over unchanged.)

In short: yeah, the Zulip server/web codebase is already (and was already when I wrote that paragraph, in probably 2018) large enough that 7 isn't reliably long enough. I think 9 would be a better default for the software to have in general.

[Ruhdee]

Thank you for the review and your patience!

Changes made:

1. Added some missing mention smoke tests. I couldn't find a good reason as to why they were excluded in the first place.
2. Added test for parsing legacy user mention. In the web app code, user mention names are dynamically fetched even for legacy mentions using the email field in legacy mentions. If it is decided that this feature is worth having in the Flutter app, I think a new issue should be created for it.
3. Addressed all the feedback.

Question:

Look at this test:

zulip-flutter/test/widgets/content_test.dart

Lines 1741 to 1748 in 8925073

	testWidgets('header row background color', (tester) async {
	await prepareContent(tester, plainContent(ContentExample.tableWithSingleRow.html));
	final BuildContext context = tester.element(find.byType(Table));
	check(tester.widget<Table>(find.byType(Table))).children.first
	.decoration
	.isA<BoxDecoration>()
	.color.equals(ContentTheme.of(context).colorTableHeaderBackground);
	});

This test uses the brittle implementation to check if the colors match. This test could use a refactor like the one that was proposed for the pill color tests. However, there could be more tests in the codebase that require similar refactors. When I come across such code, should I refactor it individually (in separate commits) as I find it? Or is it better to create a separate issue/PR to address all such types of code changes in the whole codebase at the same time?

[gnprice]

Thanks for the revision! Comments below. (I'm catching up a bit on reviews after getting the implementation of E2EE notifications mostly done: #mobile-team > E2EE notifications @ 💬.)

2. In the web app code, user mention names are dynamically fetched even for legacy mentions using the email field in legacy mentions. If it is decided that this feature is worth having in the Flutter app, I think a new issue should be created for it.

Interesting, thanks for flagging that. I think we'll skip that nuance: it would have been important soon after the switch from that legacy mention format, but today it would often not work even if we added the code for it. For one thing, 9+ years after such messages were sent, many people's email addresses will have changed since then; for another, in many communities most people have hidden their email address from other users (an option we didn't have in 2017).

[gnprice]

This test uses the brittle implementation to check if the colors match. This test could use a refactor like the one that was proposed for the pill color tests. However, there could be more tests in the codebase that require similar refactors. When I come across such code, should I refactor it individually (in separate commits) as I find it? Or is it better to create a separate issue/PR to address all such types of code changes in the whole codebase at the same time?

Grepping for BoxDecoration, it looks like that's the only existing test quite like it. There's another one in a similar spirit in test/widgets/counter_badge_test.dart, but it would require more refactoring to put into this form.

In general the main limitation on doing that sort of refactor is that it costs time for a reviewer as well as you to read the surrounding code and confirm that the refactored test effectively checks all the important things that the old version of the test did.

An issue like this one, where the main problem is that the existing test is brittle, prone to unnecessarily breaking, means that the worst consequence it's likely to have is forcing us to revise the test in the future — it's unlikely that the test would wrongly continue passing and fail to catch a regression. So refactoring such a test now means eagerly paying that cost in order to save potentially needing to pay it in the future. That's a good trade only if there's some reason that the cost of refactoring it is cheaper now than in the future.

The upshot is that this sort of issue, where the risk is the test wrongly breaking rather than wrongly passing, only makes sense to fix when we're already working on the same code, so that loading it into our heads to write and review a change is very cheap.

I think that applies to the test you quoted, which is in this same file content_test.dart and very similar in structure. It wouldn't apply to the other less similar test in counter_badge_test.dart.

[Ruhdee]

Suggested change

	final renderObject = tester.renderObject<RenderBox>(find.byType(Table));
	final headerWidth = renderObject.paintBounds.width;
	final headerHeight = tester.getSize(find.byType(MessageTableCell).first).height;
	final headerRowRect = Rect.fromLTWH(0, 0, headerWidth, headerHeight);
	final contentTheme = ContentTheme.of(tester.element(find.byType(Table)));
	check(renderObject).legacyMatcher(equals(paints..rect(
	rect: headerRowRect,
	style: .fill,
	color: contentTheme.colorTableHeaderBackground)));

After looking into the test in more detail, I could not come up with a straightforward solution to refactor the above test. The issue is that TableRow is not a widget, it is a data class for the Table widget. This means that find.byType() cannot be directly used to get the bounds of TableRow.

The above code snippet is a workaround for it. It creates a custom Rect for the header row using the height of table cell and width of table.
Another solution could be not passing a Rect at all and just checking if the table paints the header color anywhere in the table, not necessarily the header row. However, this might defeat the purpose of the test.

Is either of these implementations worth considering? Or is it not worth the cost right now to refactor this test?

[gnprice]

Cool, thanks for the investigation. Yeah, in that case it sounds like revising that test would be more work than it's worth, given the context in #2168 (comment) .

[Ruhdee]

I have addressed the rest of the feedback. PTAL!

[gnprice]

Thanks for the revision! Looks good; merging.