Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Handle Zulip internal links #305

Merged
merged 8 commits into from
Oct 13, 2023
Merged

Handle Zulip internal links #305

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2f944a0
internal_link [nfc]: Move narrowLink out of compose into a new intern…
sirpengi Sep 12, 2023
1cfa5dc
lint: Suppress constant_identifier_names in generated files
sirpengi Sep 22, 2023
c65bab8
store: Add streamsByName mapping
sirpengi Sep 12, 2023
664f4a5
narrow: Add DmNarrow.withUsers
sirpengi Sep 22, 2023
2cf1b36
test: Add optional Account and InitialSnapshot to store() factory
sirpengi Oct 3, 2023
9a5ec69
internal_link: Add tryResolveOnRealmUrl
sirpengi Oct 3, 2023
4beb7bb
internal_link: Add decodeHashComponent function
sirpengi Oct 4, 2023
e3a2dcc
internal_link: Parse internal links into narrows
sirpengi Oct 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions build.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,6 @@ targets:
source_gen:combining_builder:
options:
ignore_for_file:
- constant_identifier_names
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

lint [nfc]: Suppress constant_identifier_names in generated files

This prevents us from making private `JsonEnum` classes
as the generated `EnumMap` does not follow the rule of
being a lowerCamelCase identifier.

This sounds at first like it's describing what the commit does. (After all, lots of lint config changes are precisely to "prevent[] us from" doing something, making some kind of mistake.)

One way to disambiguate is to use the past tense when describing things that stopped happening with this commit. So "This was preventing us …", and "was triggering".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Separate commit-message nit: leave out "[nfc]"

This change is indeed NFC from the perspective of our app code, but the prefix already says it's about the linter; and it does change the behavior of the linter.

A "lint [nfc]:" change, by contrast, would be one that's refactoring our lint config itself. These aren't that common, but if you try git log --oneline --grep 'lint .nfc.:' in zulip-mobile you'll see a burst of them a few years ago, as our lint config there has a lot more to it.

Similarly a test-only change may say "test:", or "foo test:"; and a pure refactor of the tests will say "test [nfc]:" or "foo test [nfc]:". For examples, try the commands git log --stat -p --grep 'test:' and git log --stat -p --grep 'test .nfc.:'.

# https://github.com/google/json_serializable.dart/issues/625
- unnecessary_cast
2 changes: 1 addition & 1 deletion lib/api/model/events.g.dart
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion lib/api/model/initial_snapshot.g.dart
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion lib/api/model/model.g.dart
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion lib/api/model/reaction.g.dart
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion lib/api/route/account.g.dart
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion lib/api/route/events.g.dart
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion lib/api/route/messages.g.dart
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion lib/api/route/realm.g.dart
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion lib/api/route/users.g.dart
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

78 changes: 1 addition & 77 deletions lib/model/compose.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import 'dart:math';

import '../api/model/model.dart';
import '../api/model/narrow.dart';
import 'internal_link.dart';
import 'narrow.dart';
import 'store.dart';

Expand Down Expand Up @@ -101,82 +101,6 @@ String wrapWithBacktickFence({required String content, String? infoString}) {
return resultBuffer.toString();
}

const _hashReplacements = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nits in commit message:

lib [nfc]: refactor narrowLink out of compose into a new internal_link package
  • "lib" is extremely broad as a prefix; "model" is better
    • perhaps better yet, use the same prefix "internal_link" as for subsequent changes to the new file
  • use sentence case, capitalizing the first word after the colon
  • "package" in the Dart world means the thing that has a pubspec.yaml and a lib/ directory, so e.g. our whole repo is one package; the noun here is "library", or simply "file"
  • for the verb, can say "move" instead of "refactor" — it's shorter (useful in these summary lines) and also more specific

"%": ".",
"(": ".28",
")": ".29",
".": ".2E",
};

final _encodeHashComponentRegex = RegExp(r'[%().]');

// Corresponds to encodeHashComponent in Zulip web;
// see web/shared/src/internal_url.ts.
String _encodeHashComponent(String str) {
return Uri.encodeComponent(str)
.replaceAllMapped(_encodeHashComponentRegex, (Match m) => _hashReplacements[m[0]!]!);
}

/// A URL to the given [Narrow], on `store`'s realm.
///
/// To include /near/{messageId} in the link, pass a non-null [nearMessageId].
// Why take [nearMessageId] in a param, instead of looking for it in [narrow]?
//
// A reasonable question: after all, the "near" part of a near link (e.g., for
// quote-and-reply) does take the same form as other operator/operand pairs
// that we represent with [ApiNarrowElement]s, like "/stream/48-mobile".
//
// But unlike those other elements, we choose not to give the "near" element
// an [ApiNarrowElement] representation, because it doesn't have quite that role:
// it says where to look in a list of messages, but it doesn't filter the list down.
// In fact, from a brief look at server code, it seems to be *ignored*
// if you include it in the `narrow` param in get-messages requests.
// When you want to point the server to a location in a message list, you
// you do so by passing the `anchor` param.
Uri narrowLink(PerAccountStore store, Narrow narrow, {int? nearMessageId}) {
final apiNarrow = narrow.apiEncode();
final fragment = StringBuffer('narrow');
for (ApiNarrowElement element in apiNarrow) {
fragment.write('/');
if (element.negated) {
fragment.write('-');
}

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

fragment.write('${element.operator}/');

switch (element) {
case ApiNarrowStream():
final streamId = element.operand;
final name = store.streams[streamId]?.name ?? 'unknown';
final slugifiedName = _encodeHashComponent(name.replaceAll(' ', '-'));
fragment.write('$streamId-$slugifiedName');
case ApiNarrowTopic():
fragment.write(_encodeHashComponent(element.operand));
case ApiNarrowDmModern():
final suffix = element.operand.length >= 3 ? 'group' : 'dm';
fragment.write('${element.operand.join(',')}-$suffix');
case ApiNarrowPmWith():
final suffix = element.operand.length >= 3 ? 'group' : 'pm';
fragment.write('${element.operand.join(',')}-$suffix');
case ApiNarrowDm():
assert(false, 'ApiNarrowDm should have been resolved');
case ApiNarrowMessageId():
fragment.write(element.operand.toString());
}
}

if (nearMessageId != null) {
fragment.write('/near/$nearMessageId');
}

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

/// An @-mention, like @**Chris Bobbe|13313**.
///
/// To omit the user ID part ("|13313") whenever the name part is unambiguous,
Expand Down
2 changes: 1 addition & 1 deletion lib/model/database.g.dart
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading