react-native-sketch icon indicating copy to clipboard operation
react-native-sketch copied to clipboard

Initial Image and/or Custom Image Data Prop

Open HankieCodes opened this issue 8 years ago • 6 comments

This is a rebuild of my coworker @gensc004's work to allow initial images to the Sketchpad. I have more past experience with ObjC code so I worked on upgrading it to work inline with the change that happened in this library since #27 was opened.

The implementation I built here is a new prop, imageData that is passed into the native component. Here's what happens:

  1. <Sketch is passed imageData as a prop at first render.
  2. Prop is copied to state of the JS component, and sent to the native component.
  3. (later): Native component calls onChange() when a line is drawn.
  4. JS component updates state and reapplies imageData to the native component <- this causes double rendering of the line...once immediately after the line is drawn, and again once the JS code "accepts" the line.
  5. (opt): At any time, a new imageData value passed to <Sketch will overwrite the current value rendered in native code. If the parent component calls setState() within the onChange() function's callback, the change will be deduplicated by the React reconciler and only the parent component's value will be applied.

For me, the double rendering resolves an issue where the JS state is out-of-sync with the ObjC state for this component. Ideally, we would only need to render once...but the solution to this could be in a few different ways...I would vote to either leave the double rendering as there's minimal performance impact, or have the end-of-line-drawn not trigger the render, and rely on the JS state for accurate imageData information.

Remaining Tasks

  • [ ] Discuss & agree on double-rendering ObjC code
  • [ ] Add documentation for the new property

HankieCodes avatar Oct 05 '17 15:10 HankieCodes

Thanks for your hard work @hnryjms ! I'll have a play with it over the weekend. I'm slightly concerned about the double rendering issue though... What was your other idea around it?

jgrancher avatar Oct 06 '17 10:10 jgrancher

What I was thinking is that as long as JS code keeps pushing updates back to ObjC, we wouldn't need to render at -touchesEnded:withEvent:, because we can assume the JS will send the new imageData value once it's processed in the JS code of the app (triggering the rerender). -touchesEnded:withEvent: would instead act the same as -touchesMoved:withEvent: for placing the last point onto the line.

The only thing I'm unsure on without testing this approach is if -drawingToString will send the updated imageData value, or if it won't include the most recent line.

HankieCodes avatar Oct 06 '17 13:10 HankieCodes

Hey @hnryjms! I'm currently having a look at your PR. Can you give me an example of how I should be using imageData? Seems that I can't render any initial image right now.

jgrancher avatar Nov 14 '17 01:11 jgrancher

So it takes in base64 encoded image data, in the same format that is returned as the onChange() function. Here's the code my team is using to pass an initial image in:

RNFS.readFile(initialSketchImageFile.path, 'base64')
    .then((base64) => {
        const imageData = 'data:image/jpg;base64,' + base64;
        this.setState({ imageData });
    })

HankieCodes avatar Nov 14 '17 14:11 HankieCodes

^ I'm not in love with that whole base64 thing....a path would seem more practical. My only thought is that when it takes base64 data, it is consistent inputs and outputs for the component.

Maybe we want a helper function like the one below? thoughts??

RNSketch.getImageDataFromFilePath(initialSketchImageFile.path)
    .then((imageData) => {
        this.setState({ imageData });
    });

HankieCodes avatar Nov 14 '17 14:11 HankieCodes

Hey @jgrancher I just updated this to add a helper function. I'm not in love with making this Sketch project force react-native-fs upon everyone, but I think it's clearer than forcing the caller of that helper function to pass in pre-loaded base64 encoded image data. Unsure about the thoughts on this....it might be better to write the one operation (RNFS.readFile( ... , 'base64')) into the Sketch ObjC code itself, then there's no new dependency.

Maybe give this a shot, and we can go from there. I'll also try to spin back up here and build that -[RNSkecth readImageDataFile:] ObjC function so we don't need RNFS if I see this PR is sitting too long a second time.

HankieCodes avatar Jul 13 '18 21:07 HankieCodes