Page MenuHomePhabricator
Differential D5063

[lib] Introduce new `sendMultimediaMessage` and consume in `native`
ClosedPublic
Actions

Authored by atul on Sep 5 2022, 10:52 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Mar 15, 9:21 PM
Unknown Object (File)
Thu, Mar 13, 3:18 AM
Unknown Object (File)
Tue, Mar 11, 5:45 PM
Unknown Object (File)
Sat, Mar 8, 5:55 AM
Unknown Object (File)
Sat, Mar 8, 1:06 AM
Unknown Object (File)
Sat, Mar 8, 1:06 AM
Unknown Object (File)
Sat, Mar 8, 1:06 AM
Unknown Object (File)
Sat, Mar 8, 1:06 AM
View All Files
Subscribers
ashoat

Details

Reviewers
• abosh
tomek
marcin
Commits
rCOMM88eabcde44a5: [lib] Introduce new `sendMultimediaMessage` and consume in `native`
Summary

Introduce a new sendMultimediaMessage in lib/actions/message-actions that hits the create_multimedia_message endpoint with the updated payload (mediaMessageContents instead of mediaIDs).

This endpoint is consumed by input-state-container on native, but not on web. We continue to use the legacySendMultimediaMessage on web to avoid breaking things.

IMPORTANT: While the changes made to keyserver in this stack are backwards compatible, this change to native depends on keyserver being deployed before a release is made. To keep things simple I'll avoid landing this diff until all necessary changes on the keyserver side are made and tested. I'll hold back on landing this diff (and a few others) until we decide to "flip the switch." I'll probably just keep this commit in my local environment as I'm working, but figured it might be helpful for reviewers if they wanted to arc patch or test any part of the stack in their local dev environment.

(As of this diff, the keyserver has everything it needs in multimediaMessageCreationResponder(...) to properly associate uploads with media with messages)

Depends on D5062

Test Plan
  1. Able to send image(s) on native as before without issue
  2. Able to send image(s) on web as before without issue

Also set breakpoints at key parts of the multimedia message flow and ensured that values appeared as expected.

(I'll do some a bunch of additional testing once things are "working" end-to-end)

Diff Detail

Repository
rCOMM Comm
Branch
master
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

atul created this revision.Sep 5 2022, 10:52 PM
Herald added a subscriber: ashoat. · View Herald TranscriptSep 5 2022, 10:52 PM
atul edited the summary of this revision. (Show Details)Sep 5 2022, 11:00 PM
Harbormaster completed remote builds in B11903: Diff 16346.Sep 5 2022, 11:02 PM
atul requested review of this revision.Sep 5 2022, 11:02 PM
atul added a reviewer: marcin.Sep 5 2022, 11:02 PM
atul edited the summary of this revision. (Show Details)
atul removed a reviewer: • jacek.Sep 7 2022, 5:33 PM
atul added 1 blocking reviewer(s): tomek.Sep 7 2022, 10:22 PM
tomek requested changes to this revision.Sep 8 2022, 8:27 AM

The new and old API differ in only one type, so maybe it would be a good idea to simply have a method that works for both. We're going to throw away the old API so copying code might be a good idea... but now I'm not sure if introducing the new method made sense for legacy shape. We could simply have a function that accepts either type and at the end of process we would delete the old type.

I'm not necessarily requesting to change the code now, but I'm curious about your opinion.

lib/actions/message-actions.js
133 ↗(On Diff #16346)

It's confusing to see db type in an interface between client and server.

This revision now requires changes to proceed.Sep 8 2022, 8:27 AM
atul added inline comments.Sep 8 2022, 9:13 AM
lib/actions/message-actions.js
133 ↗(On Diff #16346)

That's totally fair. I couldn't think of a better name, but open to changing it to whatever might be clearer. I thought of just MediaMessageContent but that might imply that it contains the "contents" (eg blobs) of the MediaMessage?

Idk, I just re-used the naming scheme that I used for the message-reducer stuff a while ago but very open to changing name to something clearer

atul updated this revision to Diff 16525.Sep 8 2022, 2:26 PM

rebase

Harbormaster completed remote builds in B12028: Diff 16525.Sep 8 2022, 2:36 PM
atul added a comment.Sep 8 2022, 2:38 PM

The new and old API differ in only one type, so maybe it would be a good idea to simply have a method that works for both.

We kind of have that on the keyserver side. I put a good amount of thought into supporting old clients with the new keyserver... but didn't think we were too worried about supporting old keyservers with newer clients at this point since there's only one keyserver.

With regards to sendMultimediaMessage and legacySendMultimediaMessage in lib being largely redundant on the client-side, I didn't want to spend too much time refactoring that since legacySendMultimediaMessage will be removed altogether pretty soon in this stack. These functions are only used from the client-side (native and web), so we don't have to worry about "keeping it around" like we have to for functionality on the keyserver side (to support old clients).

We're going to throw away the old API so copying code might be a good idea... but now I'm not sure if introducing the new method made sense for legacy shape. We could simply have a function that accepts either type and at the end of process we would delete the old type.

Yeah we totally could have merged the functionality to handle both types within one function, but I thought it was easier to just split them apart and give them different names rather than add functionality/logic to handle both. If we were keeping both around, I'd certainly refactor to reduce repetition, but going to remove legacy... very quickly so I guess I'm being lazy and this was easier to work with as I'm developing.

(Let me know if I correctly understood your questions, or if there's something I'm missing/should expand on)

atul added a child revision: D5085: [keyserver] Introduce `fetchMediaFromMediaMessageContent(...)`.Sep 8 2022, 11:37 PM
tomek accepted this revision.Sep 9 2022, 8:37 AM

(Let me know if I correctly understood your questions, or if there's something I'm missing/should expand on)

I think we're on the same page

but didn't think we were too worried about supporting old keyservers with newer clients at this point since there's only one keyserver.

Yeah, that's right. But at some point it could be necessary to version the keyserver code. Probably the same applies to services. We can't assume that everything will be always up-to-date, because we should have a way to rollback. So that seems like a quite complicated issue. But as we agreed, in this diff we're fine.

Yeah we totally could have merged the functionality to handle both types within one function, but I thought it was easier to just split them apart and give them different names rather than add functionality/logic to handle both. If we were keeping both around, I'd certainly refactor to reduce repetition, but going to remove legacy... very quickly so I guess I'm being lazy and this was easier to work with as I'm developing.

Sounds reasonable.

lib/actions/message-actions.js
133 ↗(On Diff #16346)

I think that adding e.g. descriptor at the end of the name could differentiate between content and just the ids. What do you think about MediaMessageContentDescriptor? (or description, not sure which one sounds more appropriate)

This revision is now accepted and ready to land.Sep 9 2022, 8:37 AM
ashoat added a comment.Sep 15 2022, 12:53 PM

Now that D5062 has been deployed to production, can this be landed?

atul added a comment.Sep 15 2022, 1:58 PM
In D5063#150700, @ashoat wrote:

Now that D5062 has been deployed to production, can this be landed?

Yeah. I'll patch this in, connect to prod, and send some photo messages as a sanity check... but this should be good to land.

atul updated this revision to Diff 16877.Sep 19 2022, 3:22 PM

rebase before landing

Harbormaster completed remote builds in B12293: Diff 16877.Sep 19 2022, 3:32 PM
Closed by commit rCOMM88eabcde44a5: [lib] Introduce new `sendMultimediaMessage` and consume in `native` (authored by atul). · Explain WhySep 20 2022, 3:44 AM
This revision was automatically updated to reflect the committed changes.
atul added a commit: rCOMM88eabcde44a5: [lib] Introduce new `sendMultimediaMessage` and consume in `native`.

Revision Contents
Changeset List

PathSize
lib/
actions/
38 lines
native/
input/
33 lines

Diff 16525

View Options

lib/actions/message-actions.js

  • This file was added.
div.containerViewerLiked {
padding: 4px;
border-radius: 4px;
background-color: var(--shades-black-80);
}
div.container:hover {
cursor: pointer;
}
tomekUnsubmitted
Not Done
Inline Actions

For other tooltip actions we don't need to manually set this style. Can we use the mechanism we already have, or are there any reasons for not doing so?

tomek: For other tooltip actions we don't need to manually set this style. Can we use the mechanism we…
ginsuAuthorUnsubmitted
Done
Inline Actions

I believe the other icons don't need to set this style because it is built into IcomoonReact. When I switched the Like action tooltip button with one of the icons the hover style just works out of the box. Wondering if @atul has more context since he is pretty familiar with the icons stuff

ginsu: I believe the other icons don't need to set this style because it is built into `IcomoonReact`.
tomekUnsubmitted
Not Done
Inline Actions

It wouldn't make too much sense for Icomoon to provide hover effects for icons because they could be used in other places - not only as buttons. It seems like the style on hover is applied in message-tooltip.css

div.messageActionButtons svg:hover {
  cursor: pointer;
  color: var(--fg);
}

so it should be enough to apply this style also to the new button.

tomek: It wouldn't make too much sense for `Icomoon` to provide hover effects for icons because they…
ginsuAuthorUnsubmitted
Done
Inline Actions

Ahhhhh that makes sense, thank you for clarifying, I will make the necessary changes

ginsu: Ahhhhh that makes sense, thank you for clarifying, I will make the necessary changes
View Options

native/input/input-state-container.react.js

  • This file was added.
div.containerViewerLiked {
padding: 4px;
border-radius: 4px;
background-color: var(--shades-black-80);
}
div.container:hover {
cursor: pointer;
}
tomekUnsubmitted
Not Done
Inline Actions

For other tooltip actions we don't need to manually set this style. Can we use the mechanism we already have, or are there any reasons for not doing so?

tomek: For other tooltip actions we don't need to manually set this style. Can we use the mechanism we…
ginsuAuthorUnsubmitted
Done
Inline Actions

I believe the other icons don't need to set this style because it is built into IcomoonReact. When I switched the Like action tooltip button with one of the icons the hover style just works out of the box. Wondering if @atul has more context since he is pretty familiar with the icons stuff

ginsu: I believe the other icons don't need to set this style because it is built into `IcomoonReact`.
tomekUnsubmitted
Not Done
Inline Actions

It wouldn't make too much sense for Icomoon to provide hover effects for icons because they could be used in other places - not only as buttons. It seems like the style on hover is applied in message-tooltip.css

div.messageActionButtons svg:hover {
  cursor: pointer;
  color: var(--fg);
}

so it should be enough to apply this style also to the new button.

tomek: It wouldn't make too much sense for `Icomoon` to provide hover effects for icons because they…
ginsuAuthorUnsubmitted
Done
Inline Actions

Ahhhhh that makes sense, thank you for clarifying, I will make the necessary changes

ginsu: Ahhhhh that makes sense, thank you for clarifying, I will make the necessary changes