-
Notifications
You must be signed in to change notification settings - Fork 1
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
Conversation
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. |
@gmaclennan i believe you wrote this in mapeo-mobile. Do you have any context for this? |
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 |
There was a problem hiding this 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)
There was a problem hiding this 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.
I believe the current maintained fork for blob download is https://github.com/RonRadtke/react-native-blob-util |
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
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. |
@CDFN does this work for share? I had understood that you cannot pass an |
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 |
There was a problem hiding this 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.
- You have 2 exported function to produce one result:
useAttachmentUrlQueries
anduseAttachmentsBase64Query
. TheuseAttachmentUrlQueries
is not being used anywhere else, so I think we should encapsulate the logic ofuseAttachmentUrlQueries
insideuseAttachmentsBase64Query
- 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.
|
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. |
Sorry, i was confusing the queries and confusing their implementations. Thanks for clearing that up |
There was a problem hiding this 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
Closes #198