!feat(components): add support for remote-images #796

lessp · 2020-03-22T11:56:37Z

A new PR to reflect changes from Skia.

Closes #537

BREAKING CHANGE:
When using the <Image ... />-primitive, you now have to pass a variant of either (`Url or `File) to src.

In other words, where you previously passed <Image src="example.png" /> you'll now have to pass

<Image src=`File("example.png") />

or

<Image src=`Url("https://example.com/example.png") />

Et7f3 · 2020-03-22T13:19:21Z

Oups GHA removed lockdir because esy can't solve dependencies.

…mages

lessp · 2020-03-22T17:30:33Z

There's now a working example pushed, for now I created a getContextRemote for along with getContext for performance-reasons.

I'm guessing we probably want a different API, so that will have to be handled either now or in the future. E.g.

<Image src=Url("..", options) />

lessp · 2020-03-23T15:06:02Z

Can you see why this fails @Et7f3?

info install 0.6.2 (using package.json)
info checking https://github.com/ocaml/opam-repository for updates...
info checking https://github.com/esy-ocaml/esy-opam-override for updates...
error: File exists
  reading package metadata from github:esy-packages/esy-openssl:package.json#b7eab4ff97f6e61ec4523e771176e3aadc25cdd8
  resolving metadata @opam/conf-libssl@github:esy-packages/esy-openssl:package.json#b7eab4ff97f6e61ec4523e771176e3aadc25cdd8
  resolving @reason-native-web/[email protected]
  resolving @reason-native-web/[email protected]
  resolving fetch-native-lwt@github:lessp/fetch:fetch-native-lwt.json#0774bc989493f78f87e1689731f41f2a83ea0b78
esy: exiting due to errors above

##[error]Bash exited with code '1'.

lessp · 2020-03-24T18:18:27Z

Ok, so still stuck at the above error.

info install 0.6.2 (using package.json)
info checking https://github.com/ocaml/opam-repository for updates...
info checking https://github.com/esy-ocaml/esy-opam-override for updates...
error: File exists
  reading package metadata from github:esy-packages/esy-openssl:package.json#b7eab4ff97f6e61ec4523e771176e3aadc25cdd8
  resolving metadata @opam/conf-libssl@github:esy-packages/esy-openssl:package.json#b7eab4ff97f6e61ec4523e771176e3aadc25cdd8
  resolving @reason-native-web/[email protected]
  resolving @reason-native-web/[email protected]
  resolving @reason-native-web/[email protected]
  resolving fetch-native-lwt@github:lessp/fetch:fetch-native-lwt.json#0774bc9
esy: exiting due to errors above
##[error]Process completed with exit code 1.

Did you try to run it @Et7f3? (on your Windows-machine, if yes) 🙂

Et7f3 · 2020-03-24T18:32:25Z

 rd /S/Q esy.lock _esy & esy i
info install 0.6.2 (using package.json)
info checking https://github.com/ocaml/opam-repository for updates...
info checking https://github.com/esy-ocaml/esy-opam-override for updates...
info resolving esy packages: done
info solving esy constraints: done
info resolving npm packages: done
.... fetching @opam/conf-libssl@archive:https://github.com/openssl/openssl/archive/OpenSSL_1_1_1d.tar.gz#sha1:df0ee4811c87c209ebadb4e6                                                                                                                                      info fetching: done
.... installing @opam/conf-libssl@archive:https://github.com/openssl/openssl/archive/OpenSSL_1_1_1d.tar.gz#sha1:df0ee4811c87c209ebadb4                                                                                                                                      info installing: done

Work ok on my computer it manage to solve. I try to build.

src/Core/Environment.rei

glennsl · 2020-06-05T12:44:57Z

src/Draw/CanvasContext.re

+    LetOperators.(
+      {
+        let.map image = ImageRenderer.fromUrl(url);
+
+        switch (image) {
+        | None => ()
+        | Some(img) =>
+          Canvas.drawImageRect(
+            v.canvas,
+            img,
+            None,
+            Rect.makeLtrb(x, y, x +. width, y +. height),
+            paint,
+          )
+        };
+      }


This seems like the wrong place to do I/O, and asynchronous I/O in particular. How would you recover from error, provide fallback using a local file and such, for example, with this logic being here? This ought to be moved up to the component, I think.

glennsl · 2020-06-05T13:46:58Z

src/UI_Primitives/Image.re

-      node#setStyle(styles);
-      node#setResizeMode(resizeMode);
-      Obj.magic(node);
+module Asset = {


Yeah, I agree with this. Other props could be passed through the variant as well, if needed, e.g. Remote({url, timeout}).

glennsl · 2020-06-05T13:48:23Z

src/Draw/LetOperators.re

+let (let.map) = (promise, fn) => Lwt.map(fn, promise);
+let (let.mapOk) = (promise, fn) =>
+  Lwt.map(
+    fun
+    | Ok(response) => fn(response)
+    | Error(e) => e,
+    promise,
+  );
+
+let (let.flatMapOk) = (promise, fn) =>
+  Lwt.bind(
+    promise,
+    fun
+    | Ok(response) => fn(response)
+    | Error(e) => Lwt.return(Error(e)),
+  );


This ought to be moved to Utility I think.

glennsl · 2020-06-05T14:02:09Z

src/Draw/ImageRenderer.re

+  let download = url => {
+    Fetch.(
+      {
+        Log.info("Fetching image: " ++ url);
+
+        let.flatMapOk {Response.body, headers, _} = get(url);
+
+        let data = Body.toString(body);
+        let mediaType =
+          headers
+          |> List.find_opt(((k, _v)) =>
+               String.lowercase_ascii(k) == "content-type"
+             )
+          |> Option.map(((_k, v)) => v)
+          |> Option.value(~default="");
+
+        Lwt.return(Ok({data, mediaType}));
+      }
+    );
+  };


This is also the wrong place to do I/O I think. A renderer should render, not do all kinds of other things as well. I can see how the earlier decision to have fromAssetPath here lead to this, but adding network I/O and asynchrony makes the spaghetti bowl significantly deeper. I think this needs a proper refactoring to separate file I/O, asset handling, network access and rendering.

Yes, that probably makes sense! Any pointers on where you'd start? drawImage takes a Skia.Image.t and that we handle IO in the component? Would that mean that we need to create a wrapper-%component for the Image-primitive perhaps?

... or would ImageNode work?

Maybe to begin with, I could start to put all the IO-specific things in a separate module and let drawImage take:

let drawImage = (~x, ~y, ~width, ~height, ~paint=?, data: Skia.Image.t, v: t) => { Canvas.drawImageRect( v.canvas, data, None, Rect.makeLtrb(x, y, x +. width, y +. height), paint, ); };

Yeah, that seems like a good start. Not sure what you mean with the wrapped component, but I don't think the component API needs or ought to change.

I've pushed some progress, let me know if you think this is the right direction and what else we could do to tidy things up. Really appreciate the feedback @glennsl! 👌

Looks great! I think this already addresses the bulk of my concerns. It would be nice to have the asset handling and networking separated out and to have proper interface files for the new modules. I'm also not sure about the idea of Image living in IO, but that's more of a cognitive dissonance issue than an actual problem I think.

Good job and thanks for doing this!

Cool, I'll give another round soon just to tie things up. Thanks!

I added some interface-files and left a note that the Image-module residing in IO could be moved and/or have another API-surface. Agree with you on that!

After your feedback, I think this at least leaves it in a little better state than previously while still having room for improvements!

As for this PR to not have to compete against time and getting stale, I'm thinking we can remove the use of it in the examples (as well as the call to startEventLoop), in order to enable Timber again, but still get the functionality in. What do you think?

Yeah, I think that's a good idea if this can't be solved now. If possible, it might be a good idea to try to run Lwt the same way we run Luv in Oni2, polling on every tick so we're still in full control of the event loop:

https://github.com/onivim/oni2/blob/f5094e83c944108bd8fea9d0a7744ff3e5a33912/src/bin_editor/Oni2_editor.re#L208-L228

I added some interface-files and left a note that the Image-module residing in IO could be moved and/or have another API-surface. Agree with you on that!

Good call. Thanks for adding the interfaces and comments! I find them very helpful.

Co-authored-by: Glenn Slotte <[email protected]>

…mages

package.json

glennsl · 2020-06-05T19:55:01Z

src/IO/Image.rei

+/**
+ * fromUrl
+ *
+ * Given a network file-path this returns a promise,
+ * holding either a Ok(option(Skia.Image.t)) or an Error(string).
+ *
+ * Examples:
+ * let result = Image.fromUrl("https://example.com/hello.png");
+ */
+let fromUrl: string => Lwt.t(result(option(Skia.Image.t), string));


Why an option inside a result? What does Ok(None) mean?

Thanks! Changed in: 07edf87

lessp · 2020-06-07T10:12:34Z

Before bringing this in, as to not bring another breaking change too soon. The two things I thought about were:

Control over caching etc

Images downloaded from online would probably have an option for folder to cache in, delete/keep files after download, caching etc while these would not apply for images loaded from disk since they're already there. I don't think settings like these make a lot of sense as props, but better put in a record.

If so, we can leave as is and the breaking change would be to go from:

`Url(string)

to something like:

`Url({ path: string, ...options })

Which might be OK as a breaking change for the future? Unless we want to take pre-caution and let \Url` take a record from the beginning.

Component while loading/error etc

Does images over network and file-images take the same props, I think they could, right?

<Image 
  src=`Url({ ...Image.Url.defaults, url: "https://example.com/example.png" }) 
  // src=`File("example.png") 
  onLoading={<View />} 
  onError={<View />}  
/>

If so, we can leave as is because it'll just be additional niceties/features in the future.

@glennsl @zbaylin @Et7f3

lessp · 2020-06-07T10:13:18Z

I saw that you merged it @glennsl, but I think it's fine, the conclusions I came to are above 🙂

tl;dr I think the breaking change might go from Url(string) -> Url(Image.Url.t), rest of the props can be shared

glennsl · 2020-06-07T10:26:02Z

Ah, sorry, that was terrible timing.

Images downloaded from online would probably have an option for folder to cache in, delete/keep files after download, caching etc while these would not apply for images loaded from disk since they're already there. I don't think settings like these make a lot of sense as props, but better put in a record.

This seems to me like an application-wide setting, not one that would need to be different for each component instance. Or to have the flexibility of having some instances with different settings, you could provide a functor interface for example.

Does images over network and file-images take the same props, I think they could, right?

For any optional props that aren't outright contradictory for file-images, I think that's absolutely fine.

lessp · 2020-06-07T10:27:49Z

No worries at all, just hit me while discussing a bit with Zach!

This seems to me like an application-wide setting, not one that would need to be different for each component instance. Or to have the flexibility of having some instances with different settings, you could provide a functor interface for example.

Yep, that's true!

lessp and others added 4 commits March 11, 2020 14:13

chore: remote images skia

ef9bf7a

Merge branch 'master' into remote-images

679431b

Update to reflect Skia-changes

6a4d7a0

Update lockdir

9be7030

lessp mentioned this pull request Mar 22, 2020

[WIP] feat: support remote images #642

Closed

lessp and others added 3 commits March 22, 2020 13:00

Lockfiles for good measure

d57f38d

Formatting

615215d

Update lockdir

f08d5e4

lessp added 2 commits March 22, 2020 18:27

Working example

7ee8829

Merge branch 'remote-images' of github.com:lessp/revery into remote-i…

5071d24

…mages

lessp and others added 3 commits March 22, 2020 18:58

esy.lock

5236ede

Lockfiles

5ec88c4

Update lockdir

9c0f48b

Et7f3 and others added 8 commits March 23, 2020 16:23

Update package.json

de5947c

Some refactoring

f6bfe58

Lockfiles

f75df28

Update lockdir

907cbf4

Lockfiles, package.json

8bc92e1

Update lockdir

20b4ada

Lockfiles, package.json

211eec0

Update lockdir

bb71079

Et7f3 and others added 4 commits March 24, 2020 19:40

remove github: prefix.

e5cb40a

solve it

d94c081

solve also for other sandbox

d47629a

Update lockdir

079d1c9

lessp changed the title ~~feat: add support for remote-images~~ feat(components): add support for remote-images Jun 5, 2020

lessp changed the title ~~feat(components): add support for remote-images~~ !feat(components): add support for remote-images Jun 5, 2020

glennsl reviewed Jun 5, 2020

View reviewed changes

lessp and others added 10 commits June 5, 2020 16:05

chore: wording getTempDirectory

6977cfe

Co-authored-by: Glenn Slotte <[email protected]>

refactor: use variant for image-src

d7d2ce1

Merge branch 'remote-images' of github.com:lessp/revery into remote-i…

05fb4e4

…mages

refactor: move IO-things to an IO-module

999be7c

chore: remove leftovers

3795690

chore: push changs for hello-example

8a3b43e

chore: minor clean-up

5281160

chore: remove unused library for Draw

e5890cb

Merge branch 'master' into remote-images

6ef4c95

chore: merge master

94b266e

Et7f3 reviewed Jun 5, 2020

View reviewed changes

package.json Outdated Show resolved Hide resolved

lessp added 3 commits June 5, 2020 20:38

chore: add interface-files, minor cleanup

5e64c01

Merge branch 'master' into remote-images

12f89e1

chore: merge master

1d32a31

glennsl reviewed Jun 5, 2020

View reviewed changes

lessp and others added 5 commits June 5, 2020 22:33

refactor: remove result-type from fromUrl-call

07edf87

chore: remove Url-image for now in order to keep Timber-logs

213be13

chore: fix typo in IO.Image-interface

6aaba0f

chore: remove unused open

e8248bb

Merge branch 'master' into remote-images

7c39286

glennsl approved these changes Jun 6, 2020

View reviewed changes

glennsl merged commit f2dce86 into revery-ui:master Jun 7, 2020

lessp deleted the remote-images branch June 7, 2020 10:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

!feat(components): add support for remote-images #796

!feat(components): add support for remote-images #796

lessp commented Mar 22, 2020 •

edited

Loading

Et7f3 commented Mar 22, 2020

lessp commented Mar 22, 2020

lessp commented Mar 23, 2020 •

edited

Loading

lessp commented Mar 24, 2020 •

edited

Loading

Et7f3 commented Mar 24, 2020

glennsl Jun 5, 2020 •

edited

Loading

glennsl Jun 5, 2020

glennsl Jun 5, 2020

glennsl Jun 5, 2020 •

edited

Loading

lessp Jun 5, 2020

lessp Jun 5, 2020

lessp Jun 5, 2020

glennsl Jun 5, 2020 •

edited

Loading

lessp Jun 5, 2020 •

edited

Loading

glennsl Jun 5, 2020

lessp Jun 5, 2020

lessp Jun 5, 2020

glennsl Jun 5, 2020

glennsl Jun 5, 2020

lessp Jun 5, 2020 •

edited

Loading

lessp commented Jun 7, 2020

lessp commented Jun 7, 2020 •

edited

Loading

glennsl commented Jun 7, 2020

lessp commented Jun 7, 2020 •

edited

Loading

!feat(components): add support for remote-images #796

!feat(components): add support for remote-images #796

Conversation

lessp commented Mar 22, 2020 • edited Loading

Et7f3 commented Mar 22, 2020

lessp commented Mar 22, 2020

lessp commented Mar 23, 2020 • edited Loading

lessp commented Mar 24, 2020 • edited Loading

Et7f3 commented Mar 24, 2020

glennsl Jun 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glennsl Jun 5, 2020 • 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

glennsl Jun 5, 2020 • edited Loading

Choose a reason for hiding this comment

lessp Jun 5, 2020 • 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

lessp Jun 5, 2020 • edited Loading

Choose a reason for hiding this comment

lessp commented Jun 7, 2020

lessp commented Jun 7, 2020 • edited Loading

glennsl commented Jun 7, 2020

lessp commented Jun 7, 2020 • edited Loading

lessp commented Mar 22, 2020 •

edited

Loading

lessp commented Mar 23, 2020 •

edited

Loading

lessp commented Mar 24, 2020 •

edited

Loading

glennsl Jun 5, 2020 •

edited

Loading

glennsl Jun 5, 2020 •

edited

Loading

glennsl Jun 5, 2020 •

edited

Loading

lessp Jun 5, 2020 •

edited

Loading

lessp Jun 5, 2020 •

edited

Loading

lessp commented Jun 7, 2020 •

edited

Loading

lessp commented Jun 7, 2020 •

edited

Loading