Internal_link: Add ApiIsNarrow. #869

PIG208 · 2024-08-06T23:11:16Z

Prep work for #251.

PIG208 · 2024-08-07T05:59:11Z

Should be ready for review now.

chrisbobbe · 2024-08-07T07:19:09Z

Thanks, LGTM! Over to Greg for his review.

PIG208 · 2024-08-08T21:50:29Z

Extracted a bug fix to make Add generic support for is operands nfc.

gnprice

Thanks for doing this refactor! Small comments.

gnprice · 2024-08-09T23:25:53Z

lib/api/model/narrow.dart

@@ -1,3 +1,5 @@
+import '../../model/internal_link.dart';


Generally let's avoid importing in lib/api/ from other parts of the code — I'd like to think of that subtree as a Zulip API binding layer, such that if we wanted to we could easily split it out as its own Dart package.

This is for IsOperand, right? That's part of this API (it's part of https://zulip.com/api/construct-narrow ), so let's just put its definition here in this file.

gnprice · 2024-08-09T23:29:38Z

lib/api/model/narrow.dart

+  @override final String operand;

-class ApiNarrowIsUnread extends ApiNarrowElement {
-  @override String get operator => 'is';
-  @override String get operand => 'unread';
+  ApiNarrowIs(IsOperand operand, {super.negated}) : operand = operand.toString();


I don't really understand this paragraph:

We convert the IsOperand value into a String when constructing ApiNarrowIs, so that we don't need to mingle with ApiNarrowElement.toJson to take care of the enum.

What would happen if we had this class store the enum?

Testing with that again, I think I was mistaken that we needed special handling for ApiNarrowElement.toJson to support the use of the enum field, but actually the fix is to just provide a toJson method on IsOperand.

gnprice · 2024-08-09T23:36:26Z

lib/model/internal_link.dart

+  dm,
+  private, // This is deprecated in FL 177 and equivalent to [dm].


Suggested change

dm,

private, // This is deprecated in FL 177 and equivalent to [dm].

dm, // TODO(server-7) new in FL 177

private, // TODO(server-7) deprecated in FL 177, equivalent to [dm]

The TODO(server-N) format will help us spot in the future that we can drop private.

For the newly-introduced value, having a comment puts us on guard to not try to use it with old servers. Then we need a TODO-server comment to remind us to delete that warning comment… so it may as well be one line that does both.

Somewhat related: how do we deal with deprecated API values? Do we still need to support them post server-7, until they are fully removed in a future version?

When it comes to links, we'll want to accept the old forms forever (as will web). People will have put links to Zulip in all sorts of external places we can't update — for example in GitHub comments, for Zulip users who use GitHub like we do — and so we should keep those links working.

But once we stop supporting the old server versions that lack the new forms, we can stop ever generating the old forms. So for example ApiNarrowDm and its two subclasses will simplify back down to just ApiNarrowDm, and the only wrinkle remaining for the long term will be that we'll accept pm-with in links and translate that to ApiNarrowDm. We'll handle IsOperand.private similarly.

gnprice · 2024-08-09T23:36:42Z

lib/model/internal_link.dart

+  alerted,
+  mentioned,
+  starred,
+  followed,


needs a TODO-server comment, similar to dm above

gnprice · 2024-08-09T23:38:22Z

lib/model/internal_link.dart

+/// See also:
+///   - https://zulip.com/api/construct-narrow
+///   - https://zulip.com/help/search-for-messages#search-your-important-messages
+///   - https://zulip.com/help/search-for-messages#search-by-message-status


Cool, thanks for the thorough collection of links — this made it easy for me to verify the list of values.

gnprice · 2024-08-09T23:40:17Z

lib/model/internal_link.dart

    if (streamElement != null || topicElement != null || dmElement != null) return null;
-    return const MentionsNarrow();
+    switch (isElementOperands.singleOrNull) {


Let's make this more explicit:

Suggested change

switch (isElementOperands.singleOrNull) {

if (isElementOperands.length > 1) return null;

switch (isElementOperands.single) {

I actually started writing a comment asking if the null case was impossible, because this is inside an isNotEmpty conditional. Then I realized there are two different cases that singleOrNull turns to null.

PIG208 · 2024-08-12T18:58:57Z

Thanks for the review @gnprice. Updated the PR.

gnprice · 2024-08-13T03:24:13Z

Thanks for the revision! LGTM.

Would you rebase? It looks like this had a few conflicts with #779 which I just merged.

PIG208 · 2024-08-13T18:58:13Z

Thanks. This should be fixed now.

Previously, `is` elements with operands other than `mentioned` are ignored when parsing internal links. When the remaining elements combined are recognized, some non-null narrows can be returned. Now `is` elements are never ignored, to avoid such false positives. Signed-off-by: Zixuan James Li <[email protected]>

This replaces ApiNarrowIsMentioned and ApiNarrowIsUnread with the generic ApiNarrowIs element. This does not change the internal links behavior, hence the nfc. This is also why this commit hasn't introduced new tests yet. Signed-off-by: Zixuan James Li <[email protected]>

This also helps us with having more thorough test coverage for different `is` operands. Signed-off-by: Zixuan James Li <[email protected]>

This matches the behavior of the webapp. Such narrow links are semantically valid and equivalent to the deduplicated link. Signed-off-by: Zixuan James Li <[email protected]>

gnprice · 2024-08-14T02:35:21Z

Thanks, looks good — merging.

PIG208 force-pushed the is-narrow branch 3 times, most recently from aff00b9 to 1c1d7eb Compare August 7, 2024 05:57

PIG208 changed the title ~~WIP Add ApiIsNarrow.~~ Internal_links Add ApiIsNarrow. Aug 7, 2024

PIG208 changed the title ~~Internal_links Add ApiIsNarrow.~~ Internal_links: Add ApiIsNarrow. Aug 7, 2024

PIG208 changed the title ~~Internal_links: Add ApiIsNarrow.~~ Internal_link: Add ApiIsNarrow. Aug 7, 2024

PIG208 marked this pull request as ready for review August 7, 2024 05:59

PIG208 added the maintainer review PR ready for review by Zulip maintainers label Aug 7, 2024

PIG208 requested a review from chrisbobbe August 7, 2024 05:59

PIG208 added the a-api Implementing specific parts of the Zulip server API label Aug 7, 2024

chrisbobbe 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 Aug 7, 2024

PIG208 mentioned this pull request Aug 8, 2024

narrow: Add starred messages narrow #870

Merged

PIG208 force-pushed the is-narrow branch from 1c1d7eb to a783ced Compare August 8, 2024 21:50

gnprice reviewed Aug 9, 2024

View reviewed changes

PIG208 force-pushed the is-narrow branch from a783ced to 0d9ead8 Compare August 12, 2024 18:57

PIG208 force-pushed the is-narrow branch from 0d9ead8 to cb2e097 Compare August 13, 2024 18:55

PIG208 added 4 commits August 13, 2024 19:34

internal_link [nfc]: Add generic support for is operands.

10d0c83

This also helps us with having more thorough test coverage for different `is` operands. Signed-off-by: Zixuan James Li <[email protected]>

internal_link: Allow duplicates of the same is operand.

f9740c2

This matches the behavior of the webapp. Such narrow links are semantically valid and equivalent to the deduplicated link. Signed-off-by: Zixuan James Li <[email protected]>

gnprice force-pushed the is-narrow branch from cb2e097 to f9740c2 Compare August 14, 2024 02:35

gnprice merged commit f9740c2 into zulip:main Aug 14, 2024
1 check passed

PIG208 deleted the is-narrow branch August 14, 2024 18:05

		dm,
		private, // This is deprecated in FL 177 and equivalent to [dm].

-  dm,
-  private, // This is deprecated in FL 177 and equivalent to [dm].
+  dm, // TODO(server-7) new in FL 177
+  private, // TODO(server-7) deprecated in FL 177, equivalent to [dm]

	switch (isElementOperands.singleOrNull) {
	if (isElementOperands.length > 1) return null;
	switch (isElementOperands.single) {

Internal_link: Add ApiIsNarrow. #869

Internal_link: Add ApiIsNarrow. #869

Uh oh!

Conversation

PIG208 commented Aug 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PIG208 commented Aug 7, 2024

Uh oh!

chrisbobbe commented Aug 7, 2024

Uh oh!

PIG208 commented Aug 8, 2024

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PIG208 Aug 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PIG208 commented Aug 12, 2024

Uh oh!

gnprice commented Aug 13, 2024

Uh oh!

PIG208 commented Aug 13, 2024

Uh oh!

gnprice commented Aug 14, 2024

Uh oh!

Uh oh!

Uh oh!

PIG208 commented Aug 6, 2024 •

edited

Loading

PIG208 Aug 12, 2024 •

edited

Loading