compose: Add `narrowLink` helper, to write links to Narrows #189

chrisbobbe · 2023-06-14T23:07:54Z

We'll use this for quote-and-reply #116.

This revision sits atop my current revision of #179, which is another quote-and-reply subtask.

gnprice

Thanks! Generally looks great; small comments.

gnprice · 2023-06-15T19:03:07Z

lib/model/compose.dart

+};
+
+// Corresponds to encodeHashComponent in Zulip web;
+// see web/shared/src/internal_url.js.


Suggested change

// see web/shared/src/internal_url.js.

// see web/shared/src/internal_url.ts.

since the source file is in TS now

gnprice · 2023-06-15T19:05:24Z

lib/model/compose.dart

+// see web/shared/src/internal_url.js.
+String encodeHashComponent(String str) {
+  return Uri.encodeComponent(str)
+    .replaceAllMapped(RegExp(r'[%().]'), (Match m) => hashReplacements[m[0]!]!);


nit: pull out the RegExp object as a static final variable (so a final at top level of the file), so that its construction is memoized

This probably isn't a big deal for this particular function as we shouldn't be calling it in a hot path, but best to stick to as a pattern.

gnprice · 2023-06-15T19:12:04Z

lib/model/compose.dart

+    resultBuffer.write('/near/$nearMessageId');
+  }
+
+  return store.account.realmUrl.resolve(resultBuffer.toString());


The relative URL in resultBuffer always goes purely in the fragment — it starts with # — so an equivalent version that's a bit simpler to know what it does is:

Suggested change

return store.account.realmUrl.resolve(resultBuffer.toString());

return store.account.realmUrl.replace(fragment: resultBuffer.toString());

together with

- final resultBuffer = StringBuffer('#narrow'); + final resultBuffer = StringBuffer('narrow');

(And perhaps also rename the local to fragmentBuffer, or just fragment.)

gnprice · 2023-06-15T19:17:51Z

lib/model/compose.dart

+      final operand = resolved.operand;
+      final operandSuffix = operand.length >= 3
+        ? 'group'
+        : (supportsOperatorDm ? 'dm' : 'pm'); // TODO(?) CZO actually gives a slugified full_name here…??


Huh. Looks like there's two implementations of generating a URL for a DM narrow: pm_perma_link and pm_with_url, both in web/src/people.js. The former does what this code does, and the latter does what you're seeing:

if (user_ids.length > 1) { suffix = "group"; } else { const person = get_by_user_id(user_ids[0]); if (person && person.full_name) { suffix = person.full_name.replaceAll(/[ "%/<>`\p{C}]+/gu, "-");

I'm fine with just "dm". I think the fancier thing is most helpful for putting a URL in the location bar, which the user may see all the time when they're looking at that conversation.

Ohh, I think I see why there's both. The thing that's happening is that pm_with_url includes only the other people in the thread, while pm_perma_link includes all including self. Which is necessary for a context like quote-and-reply, where the point of the link is likely for other people to be able to use it.

But then once it's listing all recipients and not just others, that means that in a 1:1 thread it's listing both people. So to use names, you'd have to write both names. Which we don't do even in the location bar for 3-person group DM threads, and so don't do for these permalinks either.

(A shame web is so thin on jsdoc or other comments.)

Anyway, so that just confirms the conclusion: stick with plain dm/pm.

gnprice · 2023-06-15T19:29:12Z

lib/model/compose.dart

+    // TODO(server-7) remove special-casing
+    if (element is ApiNarrowDm) {
+      final supportsOperatorDm = store.connection.zulipFeatureLevel! >= 177; // TODO(server-7)
+      final resolved = element.resolve(legacy: !supportsOperatorDm);


I think this logic can actually be made somewhat cleaner by accepting a bit of mutation:

Suggested change

// TODO(server-7) remove special-casing

if (element is ApiNarrowDm) {

final supportsOperatorDm = store.connection.zulipFeatureLevel! >= 177; // TODO(server-7)

final resolved = element.resolve(legacy: !supportsOperatorDm);

if (element is ApiNarrowDm) {

final supportsOperatorDm = store.connection.zulipFeatureLevel! >= 177; // TODO(server-7)

element = element.resolve(legacy: !supportsOperatorDm);

}

Then the rest of the logic fits into the same structure as the other narrow-element types. The operator gets written the same way as always, and the operand can look like:

case ApiNarrowDmModern(): final suffix = element.operand.length >= 3 ? 'group' : 'dm'; resultBuffer.write('${element.operand.join(',')}-$suffix'); case ApiNarrowPmWith(): final suffix = element.operand.length >= 3 ? 'group' : 'pm'; resultBuffer.write('${element.operand.join(',')}-$suffix'); case ApiNarrowDm(): assert(false, 'ApiNarrowDm should have been resolved');

gnprice · 2023-06-15T19:32:05Z

test/model/compose_test.dart

+      check(narrowLink(store, const AllMessagesNarrow()))
+        .equals(store.account.realmUrl.resolve('#narrow'));
+      check(narrowLink(store, const AllMessagesNarrow(), nearMessageId: 1))
+        .equals(store.account.realmUrl.resolve('#narrow/near/1'));


(resolve is good in these tests, though, because it lets the expected string start with a # which is a helpful cue to the reader.)

chrisbobbe · 2023-06-15T20:23:46Z

Thanks for the review! Pushing to save my work, but no need to re-review until until I've revised #179 and rebased this atop it.

chrisbobbe · 2023-06-15T21:01:43Z

[…] until I've revised #179 and rebased this atop it.

(Done.)

gnprice

Thanks! All looks good with small nits below, and pending #179.

gnprice · 2023-06-16T20:24:00Z

lib/model/compose.dart

+  return resultBuffer.toString();
+}
+
+const hashReplacements = {


nit: should be private

gnprice · 2023-06-16T20:24:06Z

lib/model/compose.dart

+  ".": ".2E",
+};
+
+final encodeHashComponentRegex = RegExp(r'[%().]');


nit: should be private

gnprice · 2023-06-16T20:24:33Z

lib/model/compose.dart

+
+// Corresponds to encodeHashComponent in Zulip web;
+// see web/shared/src/internal_url.ts.
+String encodeHashComponent(String str) {


should probably also be private

chrisbobbe · 2023-06-16T21:12:16Z

Thanks! Revision pushed.

We'll use this soon, for quote-and-reply zulip#116. With the recent commit 4e11ea7, PerAccountStoreTestExtension now has `addStream`, which we use in the tests.

chrisbobbe added the a-compose Compose box, autocomplete, attaching files/images label Jun 14, 2023

chrisbobbe requested a review from gnprice June 14, 2023 23:07

chrisbobbe force-pushed the pr-compose-narrow-link branch 2 times, most recently from 66983a7 to e7d0e85 Compare June 15, 2023 18:51

gnprice reviewed Jun 15, 2023

View reviewed changes

chrisbobbe force-pushed the pr-compose-narrow-link branch from e7d0e85 to 1f208c3 Compare June 15, 2023 20:23

chrisbobbe force-pushed the pr-compose-narrow-link branch from 1f208c3 to 23f5f47 Compare June 15, 2023 21:01

gnprice approved these changes Jun 16, 2023

View reviewed changes

chrisbobbe force-pushed the pr-compose-narrow-link branch from 23f5f47 to 7128b94 Compare June 16, 2023 21:12

chrisbobbe added 4 commits June 16, 2023 15:31

test: Bump recentZulip{Version,FeatureLevel} in example data

c3b9b6c

test: Add stream helper in example data

aab6cad

compose: Add narrowLink helper, to write links to Narrows

4ee0341

We'll use this soon, for quote-and-reply zulip#116. With the recent commit 4e11ea7, PerAccountStoreTestExtension now has `addStream`, which we use in the tests.

compose: Have narrowLink take an optional nearMessageId

26b693a

gnprice force-pushed the pr-compose-narrow-link branch from 7128b94 to 26b693a Compare June 16, 2023 22:33

gnprice merged commit 26b693a into zulip:main Jun 16, 2023

chrisbobbe deleted the pr-compose-narrow-link branch June 16, 2023 22:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

compose: Add `narrowLink` helper, to write links to Narrows #189

compose: Add `narrowLink` helper, to write links to Narrows #189

chrisbobbe commented Jun 14, 2023

gnprice left a comment

gnprice Jun 15, 2023

gnprice Jun 15, 2023

gnprice Jun 15, 2023

gnprice Jun 15, 2023

gnprice Jun 15, 2023

gnprice Jun 15, 2023

gnprice Jun 15, 2023

chrisbobbe commented Jun 15, 2023

chrisbobbe commented Jun 15, 2023

gnprice left a comment

gnprice Jun 16, 2023

gnprice Jun 16, 2023

gnprice Jun 16, 2023

chrisbobbe commented Jun 16, 2023

	// see web/shared/src/internal_url.js.
	// see web/shared/src/internal_url.ts.

	return store.account.realmUrl.resolve(resultBuffer.toString());
	return store.account.realmUrl.replace(fragment: resultBuffer.toString());

compose: Add narrowLink helper, to write links to Narrows #189

compose: Add narrowLink helper, to write links to Narrows #189

Conversation

chrisbobbe commented Jun 14, 2023

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Jun 15, 2023

chrisbobbe commented Jun 15, 2023

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Jun 16, 2023

compose: Add `narrowLink` helper, to write links to Narrows #189

compose: Add `narrowLink` helper, to write links to Narrows #189