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.

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

feat: Observation sharing #335

Merged
merged 24 commits into from
May 24, 2024
Merged

feat: Observation sharing #335

merged 24 commits into from
May 24, 2024

Conversation

CDFN
Copy link
Collaborator

@CDFN CDFN commented May 10, 2024

Closes #198

@CDFN CDFN self-assigned this May 10, 2024
@CDFN CDFN added the swmansion label May 10, 2024
@CDFN CDFN requested review from achou11 and ErikSin and removed request for achou11 May 13, 2024 09:09
@CDFN
Copy link
Collaborator Author

CDFN commented May 13, 2024

Also, for markdown rendering I'm not quite sure how should I implement it. And what would happen if some app doesn't support rich text. If you have any docs on how it works with What's App it would be great if you could provide me with some. I understand you used turndown for this purpose before but I'm not quite sure what format What's App expects.

@ErikSin
Copy link
Contributor

ErikSin commented May 13, 2024

@gmaclennan i believe you wrote this in mapeo-mobile. Do you have any context for this?

@gmaclennan
Copy link
Member

Also, for markdown rendering I'm not quite sure how should I implement it. And what would happen if some app doesn't support rich text. If you have any docs on how it works with What's App it would be great if you could provide me with some. I understand you used turndown for this purpose before but I'm not quite sure what format What's App expects.

I think for now just using the same logic as we have in Mapeo Mobile should be ok: https://github.com/digidem/mapeo-mobile/blob/develop/src/frontend/screens/Observation/ObservationShare.js. This is not the most efficient way of doing things, but it's not a performance bottleneck I don't think. The format used here works in WhatsApp (docs are here). For apps that don't support rich text, I think, for now, this is ok, since the formatting used works in plain text too. If we receive issues from users that they don't like this format for apps that don't support rich text (e.g. text surrounded by * or _), then we can address this in the future.

Copy link
Contributor

@ErikSin ErikSin left a comment

Choose a reason for hiding this comment

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

Good job. I left a comment about needing to add a loading state (and potentially combining the queries)

src/frontend/hooks/server/media.ts Outdated Show resolved Hide resolved
src/frontend/hooks/server/media.ts Show resolved Hide resolved
src/frontend/screens/Observation/Buttons.tsx Outdated Show resolved Hide resolved
src/frontend/screens/Observation/Buttons.tsx Outdated Show resolved Hide resolved
Copy link
Member

@gmaclennan gmaclennan left a comment

Choose a reason for hiding this comment

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

Sorry for jumping in here, but I was having a look at this given the context of originally developing the share function for Mapeo Mobile.

As written, this will have a significant impact on memory usage and performance, even when a user is not sharing observations, because it loads all attachments into memory as base64 strings whenever an observation is viewed, before it is shared. Original resolution photo attachments can be 5Mb+, and base64 has a 33% memory overhead, so opening an observation with 5 attached photos would use 30Mb+ of memory, plus the buffers that are fetched which would need to be garbage collected - 5 * 5 = 25Mb+. This will result in large amounts of garbage collection when viewing observations, which I believe will have a significant effect on app performance, particularly on low-end devices.

Attachments should only be prepared for sharing when the user clicks the share button. This would also address the issue that @ErikSin raised about the photos possibly not having loaded in time.

However, in addition it would be better to not share as base64, because this has a large amount of memory overhead. In Mapeo Mobile photos were stored on disk so we could directly share content:// urls for attachments, but unfortunately we can't do that here because you can't share a localhost URL in a share intent. Instead the solution is to download the attachment to cache storage, generate a content URL, and share from there, then cleanup cache storage when complete, e.g. see https://react-native-share.github.io/react-native-share/docs/share-remote-file. This way the attachments do not have to be loaded into memory.

In a perfect world we would write a Content Provider for the backend, however that is beyond our capacity right now.

I recommend that there is some kind of UI state between the user pressing "share" and the share sheet opening (e.g. while images are downloaded to cache storage) so that the user is aware that something is happening and they do not attempt to press the share button more than once.

@gmaclennan
Copy link
Member

I believe the current maintained fork for blob download is https://github.com/RonRadtke/react-native-blob-util

@ErikSin
Copy link
Contributor

ErikSin commented May 19, 2024

Just for some clarity. It seems like you are able to serve the blob directly from the backend (via the url). Does this mean that we are not putting the images into memory?

Secondly, I think we should follow what Gregor said and don't load the images until after (sorry I told you contrary). The flow should be

  1. User presses share
  2. Share button turns into a while the images are being retrieved (don't retrieve them before user presses share)
  3. When the images have completed being retrieved, turn off the loader, and open the share sheet

The difference is that we are not retrieving the images right away. We are only retrieving the images if the user clicks the share button.

@gmaclennan
Copy link
Member

@CDFN does this work for share? I had understood that you cannot pass an http:// URL to share, you need a content:// URL, however reading through the android docs for share intents, they don't seem to specify anything other than the need for this to be a URI that the receiving application can read, so I guess it might work?

@CDFN
Copy link
Collaborator Author

CDFN commented May 20, 2024

It didn't work for sharing, I went back to base64 solution + lazy fetching base64 strings when sharing. Displaying photos in scroll view is done via blob url. Tried to make sharing work with react-native-blob-util, but apparently react-native-share doesn't work well with file:// uris. Their docs state that urls should be list of base64 strings.

@CDFN CDFN requested a review from ErikSin May 20, 2024 13:02
Copy link
Contributor

@ErikSin ErikSin left a comment

Choose a reason for hiding this comment

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

Gregor and I had a discussion this morning about this. Unfortunately it does seem like the best thing to do is use base 64.

I do think there are a couple design things that need to change.

  1. You have 2 exported function to produce one result: useAttachmentUrlQueries and useAttachmentsBase64Query. The useAttachmentUrlQueries is not being used anywhere else, so I think we should encapsulate the logic of useAttachmentUrlQueries inside useAttachmentsBase64Query
  2. We should only load the photos as base 64 if the user presses the share button (not befeorehand). This avoids us having to load them in memory if it is not necessary. We can do this by disabling the query and using the 'refetch' function. The flow should be: User presses the share button, share button turn into a loader while the images are converted to base 64, on completion the share sheet pops up.

@CDFN
Copy link
Collaborator Author

CDFN commented May 20, 2024

  1. useAttachmentUrlQueries is used 3 times: in share button, in observation screen and in observation edit screen. And base64 is only used when sharing, however I removed the query as it's not used anymore.
  2. It is currently done like that - there is promise with blob fetches that's called only when user presses share button. I don't think we need to do this via tanstack query as this will cache results which we don't want (it'll be persisted in memory for long time and images are quite memory intensive)

@CDFN CDFN requested a review from ErikSin May 20, 2024 19:35
@ErikSin
Copy link
Contributor

ErikSin commented May 20, 2024

There is a subtly that is being missed in your current implementation.

The user open the screen, the urls (not the base 64) are retrieved. So the button is in a loading state. Url get retrieved and then button gets out of the loading state.

The user then presses the share button. When the user presses the share button the urls are turned into base 64. But there is no loading state for when the urls are are turned into base64 (which if there are several photos on a slow device, this can take several seconds). So we need to capture this loading state.

If we add a loading state as is, it will lead to 2 loading state. Once when the page is first loaded, and the second time when the user clicks the button

I think the better UI would be to have 1 loading state, when the user clicks the button. the user clicks the button, and the urls are retrieved then the base 64 are retrieved. So i think taking both queries outside of tanstack would make sense, because you are right, it is going to put them into a cache which may be memory intensive.

@ErikSin
Copy link
Contributor

ErikSin commented May 20, 2024

  1. useAttachmentUrlQueries is used 3 times: in share button, in observation screen and in observation edit screen. And base64 is only used when sharing, however I removed the query as it's not used anymore.

Sorry, i was confusing the queries and confusing their implementations. Thanks for clearing that up

Copy link
Contributor

@ErikSin ErikSin left a comment

Choose a reason for hiding this comment

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

some minor changes, but can merge

src/frontend/screens/Observation/Buttons.tsx Outdated Show resolved Hide resolved
src/frontend/screens/Observation/Buttons.tsx Outdated Show resolved Hide resolved
src/frontend/screens/Observation/Buttons.tsx Outdated Show resolved Hide resolved
src/frontend/screens/Observation/Buttons.tsx Outdated Show resolved Hide resolved
src/frontend/screens/ObservationEdit/index.tsx Outdated Show resolved Hide resolved
src/frontend/sharedComponents/ThumbnailScrollView.tsx Outdated Show resolved Hide resolved
@CDFN CDFN merged commit 3be5639 into main May 24, 2024
3 checks passed
@CDFN CDFN deleted the feat/observation-sharing branch May 24, 2024 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Observation Share
3 participants