Implement image sending from sender #113

bitfriend · 2022-05-23T12:10:59Z

I tested image sending.
I confirmed message history appears in chat.
But image is not displayed.
Because the feature that fetches the received image is not implemented on flutter & rust.
Now implementing it.

bitfriend · 2022-05-23T12:38:10Z

Reformatted by rust code

bitfriend · 2022-05-23T12:54:44Z

Reformatted by rust code

gnunicorn

we don't want to send using the newly, yet to be approved MSC but instead use the existing "attachment" types.

gnunicorn · 2022-05-23T14:19:54Z

native/effektio/Cargo.toml

@@ -51,6 +52,9 @@ features = [
    "anyhow",
 ]

+[dependencies.ruma-common]
+features = ["unstable-msc3552"]


I appreciate you finding out there is an open msc... but there's also an already implemented version we should be doing instead...

Noted. Will check it.

Just pushed #0eb3936393570afddc6ba244aaa9ab4ba7809277 (Replace msc3552 with standard on image send)

gnunicorn · 2022-05-23T14:22:02Z

native/effektio/src/api/room.rs

+                    Some(val) => UInt::new(val),
+                    None => None,
+                };
+                let file = FileContent::plain(url, Some(Box::new(info)));


rather than sending over a URI, we should be handing over the actual source-content to the SDK and use room.send_attachment to upload it to the server and send an appropriate image message...

Noted. Will check it.

Just pushed #0eb3936393570afddc6ba244aaa9ab4ba7809277 (Replace msc3552 with standard on image send)

gnunicorn

code looks fine, and seems to work, but the UI shows the now-outdated error message still:

gnunicorn · 2022-05-24T19:36:46Z

native/effektio/src/api/room.rs

@@ -205,9 +203,11 @@ impl Room {
        &self,
        message: String,


message isn't used anymore.

Suggested change

message: String,

gnunicorn · 2022-05-24T19:37:17Z

native/effektio/api.rsh

@@ -139,7 +139,7 @@ object Conversation {
    /// received over timeline().next()
    fn send_plain_message(text_message: string) -> Future<Result<string>>;

-    fn send_image_message(message: string, uri: string, name: Option<string>, mimetype: Option<string>, size: Option<u64>) -> Future<Result<string>>;
+    fn send_image_message(message: string, uri: string, name: string, mimetype: string, size: u32, width: u32, height: u32) -> Future<Result<string>>;


message isn't used anymore.

bitfriend · 2022-05-26T19:01:50Z

The get_media_content function of matrix sdk returns image binary not file URL.
So I couldn't display image with URL on flutter side.
I implemented custom image message builder using Image.memory.

The user credential is needed before encrypted image can be decrypted.
So I implemented CURRENT_CLIENT variable globally, it keeps active matrix client when user does login.

And on rust side, I implemented the class that keeps image-specific info, excluding standard message info.
It is ImageDescription.

I used string_validator library to check if image URL is valid on flutter side.
Because the image that user just sent is displayed from local not remote.

bitfriend · 2022-05-26T22:00:00Z

RUNTIME.block_on must be replaced with RUNTIME.spawn, before we can avoid deadlock when opening ChatScreen page.
I replaced it in bin_data of ImageDescription.
But I didn't replace in image_description of RoomMessage.
Now I can see bug that image is not displayed if above function replaced.
This is problem that should be fixed as soon.

gnunicorn

Please revert from using a global client and instead pass a client: self.client.clone() when creating messages from the room.

Further comments inline.

gnunicorn · 2022-05-26T22:04:22Z

native/effektio/src/api/client.rs

@@ -47,6 +47,8 @@ impl std::ops::Deref for Client {
    }
 }

+static mut CURRENT_CLIENT: Option<MatrixClient> = None;


we can't do that, sorry. The client is hold by the surrounding SDK and the code may not assume that there is a global to store the current client. Why? Because the SDK is meant to allow for multi-client support (even if the UI doesn't implement it yet).

Yes, correct.
It doesn't look smart for me too.
But I guess on rust side there is no way to manage the multiple clients and set active to some client.
Alternatively on flutter side there is currentClient of EffektioSdk.
If we go and back between rust and flutter, we can get delay.
Before flutter can deliver current client to rust, flutter must pass token to rust and rust must login with it, so it takes a bit of time to perform this login, because login is operation via network.
Really I tested it and confirmed delay when opening ChatScreen page.
So I just wanted to finish all operations in rust, without go and back between rust and flutter.

Okay.
Will work from currentClient of EffektioSdk of flutter side.

Did you tested it in release build?

No, I tested in debug build.
In that testing, 10 logins were needed when 10 image messages loaded.
Anyway, will find solution.

gnunicorn · 2022-05-26T22:07:55Z

native/effektio/src/api/messages.rs

+    _bin_data: Vec<u8>,
+    _name: String,
+    _mimetype: Option<String>,
+    _size: u64,
+    _width: Option<u64>,
+    _height: Option<u64>,


in rust all fields are private, unless prefixed with pub. _-prefixes are only used for internal variables, that aren't actually needed. Let's stick to that model and stay with regular names instead.

Suggested change

_bin_data: Vec<u8>,

_name: String,

_mimetype: Option<String>,

_size: u64,

_width: Option<u64>,

_height: Option<u64>,

bin_data: Vec<u8>,

name: String,

mimetype: Option<String>,

size: u64,

width: Option<u64>,

height: Option<u64>,

Yes, rust recognize _ as private property in struct.
By the way, ImageDescription must keep the getter functions about these fields.
So I appended the prefix _ to properties.

Sure, _-prefixes are acceptable, but are unnecessary and make it needlessly harder to read, so I'd prefer we dropped them. Of course that doesn't mean we don't need the getter functions, that's totally okay. Was just talking about the style of these names...

Understood.
Will change property name and keep getter function name.

gnunicorn · 2022-05-26T22:08:17Z

native/effektio/src/api/messages.rs

+}
+
+impl ImageDescription {
+    pub async fn bin_data(&self) -> Result<api::FfiBuffer<u8>> {


this shouldn't copy here but only on request actually call to client and fetch the image and return it.

gnunicorn · 2022-05-26T22:08:31Z

native/effektio/src/api/messages.rs

+}
+
+pub struct ImageDescription {
+    _bin_data: Vec<u8>,


We shouldn't hold this constantly in memory here.

gnunicorn · 2022-05-26T22:09:04Z

native/effektio/src/api/messages.rs

+                    let bin_data = client
+                        .get_media_content(
+                            &MediaRequest {
+                                source: content.source.clone(),
+                                format: MediaFormat::File,
+                            },
+                            false,
+                        )
+                        .await?;


we shouldn't do this here but only upon request of the actual function call.

gnunicorn · 2022-05-26T22:10:35Z

native/effektio/src/api/messages.rs

+    pub async fn image_description(&self) -> Result<ImageDescription> {
+        match &self.inner.content.msgtype {
+            MessageType::Image(content) => {
+                let client = Client::current_client().unwrap();


rather than assuming a global, we should put client onto the room-message (and the image description), so we can do:

Suggested change

let client = Client::current_client().unwrap();

let client = self.client.clone();

gnunicorn · 2022-05-26T22:11:31Z

native/effektio/api.rsh

+
+object ImageDescription {
+
+    fn bin_data() -> Future<Result<buffer<u8>>>;


can we have a better name for this function? How about image_binary or something?

gnunicorn · 2022-05-26T22:12:07Z

native/effektio/api.rsh

+
+    fn msgtype() -> string;
+
+    fn image_description() -> Future<Result<ImageDescription>>;
+}
+
+object ImageDescription {
+
+    fn bin_data() -> Future<Result<buffer<u8>>>;
+
+    fn name() -> string;
+
+    fn mimetype() -> Option<string>;
+
+    fn size() -> u64;
+
+    fn width() -> Option<u64>;
+
+    fn height() -> Option<u64>;


Please add doc comments to all new functions in this file.

bitfriend · 2022-05-28T00:15:00Z

On flutter side:

In case that _insertMessage is wrapped into setState.
What if the 1st image binary reaches before initState completes in ChatScreen page?
In case that _insertMessage is not wrapped into setState.
What if the fetching image binary reaches after initState completed in ChatScreen page?

Without mutex, it is impossible to solve these problems atomically.
So I added mutex to dependency and implemented mutex for state updating on flutter side.

PS:
As you see, now this app is handling the image caching in low level directly.
So mutex and image custom builder was introduced.
If it is possible to use remote URL (that means image is not encrypted), we could remove all these complicated things.
But we are targeting the encrypted images, so we must handle the image caching directly.

bitfriend · 2022-05-28T10:31:16Z

Hi, Ben.
Your commit (211b4f5) to my branch is working well.
Thank you for great help again.

bitfriend · 2022-06-02T12:30:39Z

I used path.join to prepare file download path on flutter side.
This function depends on libandroidicu.so and I included this library into jniLibs.
I could get this library for only x86_64, not x86/arm64-v8a/armeabi-v7a.

I used fluttertoast to show error in case that user clicked the downloaded file and there is no associated with that file.

I moved the image/file reading from message to room.
Because room on rust side keeps client and ChatScreen.dart contains room object.
So we can keep messages.rs simple as original (I had introduced matrix client to read image/file in messages.rs).

gnunicorn · 2022-06-02T12:44:53Z

I used fluttertoast to show error in case that user clicked the downloaded file and there is no associated with that file.

we've just removed fluttertoast (see #119 ), please use the default Material Snackbar.

I used path.join to prepare file download path on flutter side.
This function depends on libandroidicu.so and I included this library into jniLibs.

Let's not. Please use rust's path-stuff for anything localised - just add a new method.

bitfriend · 2022-06-02T14:46:13Z

I used fluttertoast to show error in case that user clicked the downloaded file and there is no associated with that file.

we've just removed fluttertoast (see #119 ), please use the default Material Snackbar.

I used path.join to prepare file download path on flutter side.
This function depends on libandroidicu.so and I included this library into jniLibs.

Let's not. Please use rust's path-stuff for anything localised - just add a new method.

Great idea.
Will do.

bitfriend · 2022-06-14T22:09:16Z

ubuntu-latest was upgraded from 20220529 to 20220605.
Now we are getting the error unable to find library -lgcc.
As of 2.7.0, cargo-ndk stopped the support of NDK r22.
NDK r23+ occurs the error unable to find library -lgcc.
Two changes made to fix this bug.

Downgrade cargo-ndk. Replace cargo install cargo-ndk with cargo install cargo-ndk --version 2.6.0 in Makefile.toml of root directory.
Add this step prior to cargo make --profile release android-arm

      - uses: nttld/setup-ndk@v1
        id: install-ndk-22
        with:
          # FIXME: cargo-ndk doesn't work with latest
          ndk-version: r22
--- NEW <
      - name: Copy ndk utilities for cargo make
        run: |
          rmdir -r /usr/local/lib/android/sdk/ndk-bundle/toolchains
          mkdir -p /usr/local/lib/android/sdk/ndk-bundle/toolchains
          cp -R ${{ steps.install-ndk-22.outputs.ndk-path }}/toolchains/llvm /usr/local/lib/android/sdk/ndk-bundle/toolchains
--- NEW >

gnunicorn

There's a bunch of minor API design and internal style remarks. Please fix at your own peril and merge.

gnunicorn · 2022-06-15T20:35:30Z

native/effektio/api.rsh

+    /// m.audio, m.emote, m.file, m.image, m.location, m.service_notice, m.text, m.video or m.key.verification.request
+    fn msgtype() -> string;


we might want to consider switching this into an Enum-type... but later...

let's make that into an issue for later, once the PR is merged.

gnunicorn · 2022-06-15T20:36:47Z

native/effektio/api.rsh

+    fn image_description() -> Result<ImageDescription>;
+
+    /// contains source data, name, mimetype and size
+    fn file_description() -> Result<FileDescription>;


What if there is no image/file? I assume this is then None? Shouldn't this be Option rather than Result then?

In fact, I was planning to user these functions for only file/image.
Will fix.

I wanted to abstract image description and file description into a single struct.
But they have different fields, so I couldn't abstract them.

gnunicorn · 2022-06-15T20:37:32Z

native/effektio/api.rsh

+    /// file size
+    fn size() -> u64;


... in byte?

Suggested change

/// file size

fn size() -> u64;

/// file size in byte

fn size() -> u64;

Yes, you are correct.

gnunicorn · 2022-06-15T20:37:40Z

native/effektio/api.rsh

+    /// MIME
+    fn mimetype() -> Option<string>;
+
+    /// file size


Suggested change

/// file size

/// file size in byte

Yes, you are correct

gnunicorn · 2022-06-15T20:38:25Z

native/effektio/api.rsh

+    fn send_image_message(uri: string, name: string, mimetype: string, size: u32, width: u32, height: u32) -> Future<Result<string>>;
+
+    /// decrypted image file data
+    fn image_binary(event_id: string) -> Future<Result<buffer<u8>>>;


why is this function not on image or RoomMessage?

Flutter keeps room as member variable in UI.
Flutter doesn't keep room message as variable, because it may have to load so many messages in screen.
So I wanted to get image binary data from member variable.
Getting temporary variable for each message may be slow.

yeah, fair enough then. keep it the way it is.

gnunicorn · 2022-06-15T20:46:50Z

native/effektio/src/api/messages.rs

    },
 };
+use std::{fs::File, io::Write};


I don't think these are needed ...

Suggested change

use std::{fs::File, io::Write};

gnunicorn · 2022-06-15T20:48:04Z

native/effektio/src/api/room.rs

+        size: u32,
+        width: u32,
+        height: u32,


shouldn't these be optionals, too? Or can we be sure that any flutter will always provide them for us?

gnunicorn · 2022-06-15T20:51:28Z

native/effektio/src/api/room.rs

+                                [room.room_id().as_str().as_bytes(), event_id.as_bytes()].concat();
+                            client
+                                .store()
+                                .set_custom_value(&key, path.to_str().unwrap().as_bytes().to_vec())


please don't unwrap in actual code (tests are fine). Instead always use .expect(MSG) with a message stating, why we can be sure this never panics. e.g. :

Suggested change

.set_custom_value(&key, path.to_str().unwrap().as_bytes().to_vec())

.set_custom_value(&key, path.to_str().expect("Path was generated from strings. Must be string").as_bytes().to_vec())

gnunicorn · 2022-06-15T20:51:58Z

native/effektio/src/api/room.rs

+                            let path = client.store().get_custom_value(&key).await?;
+                            match path {
+                                Some(value) => {
+                                    let text = std::str::from_utf8(&value).unwrap();


same as above. please don't unwrap. always expect.

gnunicorn · 2022-06-15T20:52:44Z

native/effektio/src/api/room.rs

+                                None => Ok("".to_owned()),
+                            }
+                        }
+                        _ => Ok("".to_owned()),
+                    },
+                    _ => Ok("".to_owned()),


Pretty sure, these should all be None rather than Ok of an empty string.

bitfriend closed this May 23, 2022

bitfriend force-pushed the issue77_file-transfter-in-chat branch from 25303cf to 269bfd7 Compare May 23, 2022 12:33

bitfriend reopened this May 23, 2022

bitfriend closed this May 23, 2022

bitfriend force-pushed the issue77_file-transfter-in-chat branch from 5c0c00b to 269bfd7 Compare May 23, 2022 12:51

Implement image sending from sender

e0e8df1

bitfriend reopened this May 23, 2022

gnunicorn requested changes May 23, 2022

View reviewed changes

bitfriend added 2 commits May 23, 2022 18:50

Replace msc3552 with standard on image send

0eb3936

Merge branch 'main' into issue77_file-transfter-in-chat

bc7d22a

gnunicorn approved these changes May 24, 2022

View reviewed changes

bitfriend added 3 commits May 26, 2022 11:44

Implement function that reads the received image

972957b

Merge branch 'main' into issue77_file-transfter-in-chat

b70001d

Support both remote image and local image

ef7eb3e

Replace runtime block_on with spawn for only bin_data

ca68454

gnunicorn requested changes May 26, 2022

View reviewed changes

bitfriend and others added 5 commits May 26, 2022 18:27

Append comment

80375ef

replace global client with local client

211b4f5

Fetch image binary via local client in message

558bf00

Remove deadlock on loading of images

7a9961d

Implement mutex for state updating

1972b38

Merge branch 'main' into issue77_file-transfter-in-chat

a1ba21b

bitfriend added 2 commits May 28, 2022 06:32

Fix merge conflict

3ed8780

Perform rust format

70a0fae

bitfriend added 2 commits June 2, 2022 05:55

Remove lint errors

e17ec0e

Remove lint error

a12adaa

Replace toast with snackbar

c17d147

bitfriend added 19 commits June 2, 2022 09:06

Rename variables

50b8a16

Move path concat from flutter to rust

ede06bb

Merge branch 'main' into issue77_file-transfter-in-chat

e959742

Fix clang error "unable to find library -lgcc" on git CI/CD

d53d118

Fix clang error "unable to find library -lgcc" on git CI/CD

b85b648

Fix clang error "unable to find library -lgcc" on git CI/CD

cc4d43f

Fix clang error "unable to find library -lgcc" on git CI/CD

d7f645b

Fix clang error "unable to find library -lgcc" on git CI/CD

3cb6001

Fix clang error "unable to find library -lgcc" on git CI/CD

fa08bc4

Fix clang error "unable to find library -lgcc" on git CI/CD

7f469bc

Fix clang error "unable to find library -lgcc" on git CI/CD

4e1ef28

Fix clang error "unable to find library -lgcc" on git CI/CD

8460db4

Fix clang error "unable to find library -lgcc" on git CI/CD

714e1b4

Fix clang error "unable to find library -lgcc" on git CI/CD

5a89729

Fix clang error "unable to find library -lgcc" on git CI/CD

95dff48

Fix clang error "unable to find library -lgcc" on git CI/CD

ad626d9

Fix clang error "unable to find library -lgcc" on git CI/CD

3091093

Fix clang error "unable to find library -lgcc" on git CI/CD

980ddc4

Fix clang error "unable to find library -lgcc" on git CI/CD

d01df0d

Fix clang error "unable to find library -lgcc" on git CI/CD

67ac8e6

gnunicorn approved these changes Jun 15, 2022

View reviewed changes

bitfriend closed this Jun 15, 2022

bitfriend deleted the issue77_file-transfter-in-chat branch June 15, 2022 23:41

	let client = Client::current_client().unwrap();
	let client = self.client.clone();


		object ImageDescription {

		fn bin_data() -> Future<Result<buffer<u8>>>;

		/// m.audio, m.emote, m.file, m.image, m.location, m.service_notice, m.text, m.video or m.key.verification.request
		fn msgtype() -> string;

	.set_custom_value(&key, path.to_str().unwrap().as_bytes().to_vec())
	.set_custom_value(&key, path.to_str().expect("Path was generated from strings. Must be string").as_bytes().to_vec())

Implement image sending from sender #113

Implement image sending from sender #113

Conversation

bitfriend commented May 23, 2022

bitfriend commented May 23, 2022

bitfriend commented May 23, 2022

gnunicorn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnunicorn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bitfriend commented May 26, 2022

bitfriend commented May 26, 2022

gnunicorn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gtalha07 May 27, 2022 • edited Loading

Choose a reason for hiding this comment

bitfriend May 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnunicorn May 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bitfriend commented May 28, 2022 • edited Loading

bitfriend commented May 28, 2022

bitfriend commented Jun 2, 2022 • edited Loading

gnunicorn commented Jun 2, 2022

bitfriend commented Jun 2, 2022

bitfriend commented Jun 14, 2022 • edited Loading

gnunicorn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gtalha07 May 27, 2022 •

edited

Loading

bitfriend May 27, 2022 •

edited

Loading

gnunicorn May 26, 2022 •

edited

Loading

bitfriend commented May 28, 2022 •

edited

Loading

bitfriend commented Jun 2, 2022 •

edited

Loading

bitfriend commented Jun 14, 2022 •

edited

Loading