Implement stream action sheets #4899

AkashDhiman · 2021-07-15T12:18:10Z

Also involves moving messageActionSheet to src/action-sheet/index.js.

Preview:

WesleyAC · 2021-08-27T00:59:27Z

(went ahead and rebased this, although i haven't looked at the code yet)

WesleyAC

Sorry for taking so long to look at this! Overall, this seems good.

a479b4b seems like it might have been better as three separate commits, although I'm alright with it as-is.

WesleyAC · 2021-08-27T01:12:41Z

src/streams/StreamItem.js

+    streams: getStreamsById(state),
+    streamsByName: getStreamsByName(state), // TODO might need to pass stream by id here.
+    subscriptions: getSubscriptionsById(state),
+    flags: getFlags(state),


I don't think you need this.

alya · 2021-09-02T17:38:11Z

Tiny comment on the screenshot: "Pin to Top" should be "Pin to top".

gnprice

Thanks @AkashDhiman! Generally this looks good. A few comments below.

a479b4b seems like it might have been better as three separate commits, although I'm alright with it as-is.

Indeed, one advantage of splitting those changes would be that it'd make it easy to hold off on the "Enable push notifications" part (which has the only comment that isn't real simple to fix) while merging all the other changes in the PR 😉

gnprice · 2021-09-08T00:11:50Z

src/webview/html/messageHeaderAsHtml.js

-    const stream = subscriptions.find(x => x.name === streamName);
+    const stream = [...subscriptions.values()].find(x => x.name === streamName);


The change at this line risks being significantly slower: the old code is linear (which isn't good), and the new code is still linear, but the new code does a linear amount of allocation, building up this new array, and that's potentially quite a bit more costly than just searching through an array.

And this line is in the code that generates the HTML for the message list, so it's something we potentially do many times if the user is looking at a view with a lot of history (provided it's an interleaved view where the stream name appears in headers -- for example the @-mentions or starred-messages views, or a search view once we have #4859.)

Happily, I think there is an easy solution! We already have the stream ID here, because item is a Message | Outbox with type: 'stream', and those have a stream_id.

So we can just use subscriptions.get(item.stream_id), much like in the spot in action-sheets/index.js that motivates this change.

Hmm, I guess Outbox doesn't actually have stream_id yet: #3998. (Since #4667 it's there on the type as an optional property, but we never actually provide it.) So that simple solution won't yet work.

The most direct solution here would be to use the subscriptions.values() iterator just like you're doing, but use it directly with something like a for/of loop, to avoid allocating a big array of all the elements.

Another solution would be to go ahead and pass down a subscriptionsByName value into this code, gotten from getSubscriptionsByName.

using for ... of.

gnprice · 2021-09-08T00:22:18Z

src/action-sheets/index.js

+    if (sub.push_notifications === true) {
+      buttons.push(disableNotifications);
+    } else {
+      buttons.push(enableNotifications);


This logic isn't quite right. See the existing place we look at this push_notifications property, in StreamSettingsScreen:

const currentValue = subscription?.push_notifications ?? userSettingStreamNotification;

That is, if push_notifications is nullish, then that doesn't necessarily mean the same as false -- it means it falls back to the value the user has chosen as their default, which may be true.

We should probably put that logic in one place; it's already a little awkward that it's duplicated within StreamSettingsScreen.js, and it'd be much more awkward (and much more risk of divergence) to have it duplicated across multiple files.

deduplicated it in StreamSettingsScreen by creating a common function in src/stream; although I am not sure if it's placed at the right location.

Looks good, thanks! See nits at #4899 (comment) . In particular the location I'd put it is in a new file src/subscriptions/subscriptionsModel.js; discussion in that comment.

gnprice · 2021-09-08T00:24:14Z

src/streams/StreamItem.js

+    streams: getStreamsById(state),
+    streamsByName: getStreamsByName(state), // TODO might need to pass stream by id here.


Is this TODO still applicable?

Also, it looks like streamsByName isn't needed for backgroundData, only for finding stream (and thereby the stream's ID.) So it'd be a bit cleaner to have that outside of backgroundData.

That would also then be helpful when we go on to have StreamItem's parent pass it the stream ID, rather than making StreamItem look it up; if streamsByName is on its own outside backgroundData, it'll be easier then to see that it's no longer needed and cut it out.

removed streamsByName.

AkashDhiman · 2021-09-16T15:18:03Z

@gnprice Did the required chances, you may review again.

1 more thing I did on my own in this revision is that I am using api.setSubscriptionProperty instead of dispatch(setSubscriptionProperty) used previously.
is there any difference among them?

This is a better place because the file contains all action sheets and not just `MessageActionSheet`.

gnprice · 2021-10-05T20:41:47Z

(I've just rebased this and resolved conflicts.)

gnprice

Thanks @AkashDhiman for the revision! Sorry for the slow review; comments below.

There's just a few and they're all small things that are quick for me to fix myself. So I think I'm going to go do that and then merge.

1 more thing I did on my own in this revision is that I am using api.setSubscriptionProperty instead of dispatch(setSubscriptionProperty) used previously.
is there any difference among them?

Good question. As you can see in the implementation of the setSubscriptionProperty thunk action, it's a pretty trivial wrapper around api.setSubscriptionProperty -- it's not really adding any new behavior.

I think the answer is that that wrapper (and the similar trivial thunk-action wrappers we have for some other API calls) shouldn't exist, and we should just use the api.foo methods directly, just as you do in this revision.

That's because I don't think there's any meaningful semantics being added by those wrappers. That is, not only do they not add any significant behavior or logic, but there also isn't any way their semantics meaningfully differs from the underlying api.foo methods -- it's not like there's something they're abstracting away, that just happens to have a trivial implementation but where the fact the implementation is trivial is itself a nonobvious fact. (For some examples of functions with trivial implementations that do nevertheless have nontrivial semantics, see src/recipient.js -- particularly streamNameOfStreamMessage or pmTypingKeyFromPmKeyIds.) So they don't gain us anything.

And conversely, the extra wrapper has a real cost -- an extra layer of indirection to jump through when you're reading the code and working out what it does.

gnprice · 2021-10-06T02:06:49Z

src/streams/getIsNotificationEnabled.js

+export default (
+  subscription: Subscription | void,
+  userSettingStreamNotification: boolean,
+): boolean => subscription?.push_notifications ?? userSettingStreamNotification;


Cool, this solves the problem.

Some nits I would do differently:

This is really about a subscription (the user's relationship to a stream, represented by a Subscription value) rather than a stream itself (represented by Stream), so I'd put it in src/subscriptions/ or perhaps src/settings/. (This is a distinction our existing code isn't super consistent about.)

I've found it helpful to shift toward fewer, medium-size files rather than lots of tiny files. So instead of giving this its own file, I would make it the first thing added to a file named src/subscriptions/subscriptionsModel.js ~~or src/settings/settingsModel.js~~ (definitely "subscriptions"; see below). See b21d7c5 (muteModel) for a recent example that's equally tiny for now. See src/unread/unreadModel.js for a larger example, with the reducer in the same file; this function would be a "getter", a lot like the function getUnreadCountForTopic (added in 0b4c2a4) there.

I think my ideal version of this function would take the whole PerAccountSettingsState object, i.e. the return value of getSettings, rather than this one flag from it. That way the details of which piece of the user settings is relevant are internal to this function. That also means that where we have code which is ultimately going to call both this function and a few more like it, that code only needs to pass around a single settings object, and doesn't have to keep adding more bits of settings to pass down through the chain each time it has to call another function like this one.

That is very recent, though -- I might not have felt the same way before splitting up PerAccountSettingsState vs GlobalSettingsState, which just happened in 3d98cf9 / redux: Distinguish per-account vs global state (1/n) #5017 a couple of weeks ago (and after you wrote this revision.) In any case I definitely haven't discussed or demonstrated this idea much in the past.

Also this may involve a bit of refactoring in the layers above where this gets called. Like I said, I think this will lead to less churn there as we add more functions like this, but it means a bit more right now.

I'd like the name to somehow indicate that it's per-stream, as contrasted with something like state.settings.offlineNotification which feels like this same name getIsNotificationEnabled could describe it too. The types already mean there isn't a great danger of outright mixing this up with something that isn't per-stream, but still it'd be good to make it more immediately obvious to the reader.

Something I quite like about this name is that it makes clear it's getting an on/off setting! The first few names I think of that do indicate it's per-stream, plus align with the property name push_notifications, are like getStreamPushNotifications and they sound like they're getting some actual notifications, whatever that would mean. A name that has both these good properties will probably be somewhat wordy, but I think that's OK.

All of those are things I'm happy to take care of after we merge this PR, though -- no need to revise them here. This already solves the really key problem, which is that I want this logic in one place and with its own name.

Oh, but one pair of things I think I would like to have before merge:

A bit of jsdoc saying what this is intended to mean.

A jsdoc comment on the push_notifications property where Subscription is defined, saying to use this function to interpret it.

Those are key because the problem this is meant to solve is, when someone goes to use that property (just as you did in the original revision of this PR), we want the easy natural thing to be that they spot this function and use it so that they get the right behavior. So the comment from the property to help you find this function is a key part of that.

Hmm, and I guess thinking about that answers the question I was waffling on above about whether this goes in a "subscription" model or "settings" model: it should go in a subscriptionsModel.js, because this function is something you need in order to correctly interpret the subscriptions data. Or from another angle, the question this function is an answer to is a question that comes up when you're thinking about a subscription, not when you're thinking about settings.

gnprice · 2021-10-06T02:11:36Z

src/webview/html/header.js

+      let stream = null;
+      for (const sub of subscriptions.values()) {
+        if (sub.name === streamName) {


Let's add a comment here that says TODO(#3339) (with parens just like that -- a consistent format is helpful for grepping.) That's because this spot was and still is an instance of #3339, but in this form with an explicit loop it's not as easy to spot when searching for instances of that issue. So the TODO tag will help make sure we find it when we're closing in on finishing that issue.

gnprice · 2021-10-06T02:14:42Z

static/translations/messages_en.json

-  "Enable notifications": "Enable notifications",
+  "Failed to subscribe": "Failed to subscribe",
+  "Failed to unsubscribe": "Failed to unsubscribe",
+  "Unpin from top": "Unpin from top",
+  "Failed to unpin from top": "Failed to unpin from top",


In this and the next couple of commits, it looks like the changes in this file got a bit messed up.

I think the net changes at the end of the branch look right, though.

Implemented seperately for abstraction and reusability.

Also point to it from the corresponding property on `Subscription`, because when you're looking to use that property you'll need this logic; and mark as unreadable (with `-`) the two similar properties we haven't written similar logic for.

This will help us display actionsheet for streams, wherever necessary. Implemented similar to other action sheets.

This will help us avoid a linear lookup time for any given subscription id. [greg: added TODO-comment at one still-linear spot; see: zulip#4899 (comment) ]

Passed via background data, this will be useful in actions of stream action sheet.

This commit enables stream action sheet at all places.

gnprice · 2021-10-06T03:06:17Z

There's just a few [comments] and they're all small things that are quick for me to fix myself. So I think I'm going to go do that and then merge.

And done. Thanks again @AkashDhiman !

Much nicer! Simpler and more efficient. This takes care of the followup described here: #4899 (comment) which was the remaining open followup specifically mentioned on #3998.

Much nicer! Simpler and more efficient. This takes care of the followup described here: zulip#4899 (comment) which was the remaining open followup specifically mentioned on zulip#3998.

This is just a wrapper for api.setSubscriptionProperty, and one that doesn't add any meaningful semantics. That is, not only does it not add any significant behavior or logic, but there also isn't any way its semantics meaningfully differs from the underlying function. (Sometimes a function with a trivial implementation is nevertheless abstracting something away, so that the fact the implementation is trivial is itself a nonobvious fact. But this isn't one of those.) So, just inline it at its call sites. We already have a series of other sites that call `api.setSubscriptionProperty` directly. Originally noticed this here: zulip#4899 (review)

AkashDhiman requested review from WesleyAC, gnprice and chrisbobbe July 15, 2021 12:18

WesleyAC force-pushed the stream-action-sheet branch from 71a6215 to 39c68e6 Compare August 27, 2021 00:58

WesleyAC reviewed Aug 27, 2021

View reviewed changes

gnprice reviewed Sep 8, 2021

View reviewed changes

This was referenced Sep 11, 2021

Distinguish types PmOutbox vs. StreamOutbox more fully #4998

Merged

Add sender_id and stream_id properties to Outbox #3998

Closed

AkashDhiman force-pushed the stream-action-sheet branch from 39c68e6 to cbc414b Compare September 16, 2021 15:13

actionsheet [nfc]: Move code to src/action-sheet/index.js.

a64be1b

This is a better place because the file contains all action sheets and not just `MessageActionSheet`.

gnprice force-pushed the stream-action-sheet branch from cbc414b to 1a39712 Compare October 5, 2021 20:41

gnprice reviewed Oct 6, 2021

View reviewed changes

AkashDhiman and others added 9 commits October 5, 2021 19:48

settings [nfc]: Implement and use getIsNotificationEnabled.

825e6d5

Implemented seperately for abstraction and reusability.

actionsheet [nfc]: Implement stream action sheet constructors.

99ad7d3

This will help us display actionsheet for streams, wherever necessary. Implemented similar to other action sheets.

actionsheet: Use getSubscriptionsById for topic action sheet.

d689f6e

This will help us avoid a linear lookup time for any given subscription id. [greg: added TODO-comment at one still-linear spot; see: zulip#4899 (comment) ]

actionsheet [nfc]: Pass userSettingStreamNotification.

d773918

Passed via background data, this will be useful in actions of stream action sheet.

actionsheet [nfc]: Implement subscribe/unsubscribe actions.

ad08419

actionsheet [nfc]: Implement Pin to top action.

3400ee1

actionsheet [nfc]: Implement Enabled/Disabled Notification.

46b2670

actionsheet: Enable stream action sheet

42710f8

This commit enables stream action sheet at all places.

gnprice force-pushed the stream-action-sheet branch from 1a39712 to 42710f8 Compare October 6, 2021 03:00

gnprice merged commit 42710f8 into zulip:main Oct 6, 2021

gnprice mentioned this pull request Dec 3, 2021

Inline trivial API-wrapper actions #5149

Merged

		const stream = subscriptions.find(x => x.name === streamName);
		const stream = [...subscriptions.values()].find(x => x.name === streamName);

		streams: getStreamsById(state),
		streamsByName: getStreamsByName(state), // TODO might need to pass stream by id here.

Uh oh!

Implement stream action sheets #4899

Implement stream action sheets #4899

Uh oh!

Conversation

AkashDhiman commented Jul 15, 2021

Uh oh!

WesleyAC commented Aug 27, 2021

Uh oh!

WesleyAC left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alya commented Sep 2, 2021

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!

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!

AkashDhiman commented Sep 16, 2021

Uh oh!

gnprice commented Oct 5, 2021

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!

Choose a reason for hiding this comment

Uh oh!

gnprice commented Oct 6, 2021

Uh oh!

Uh oh!