Added copy to clipboard button to external output area

[callmeish]

Addressed the feature request in #845. Testing using python scripts gives identical functionality to hydrogen without these changes, the only difference being the added copy to clipboard button.

[BenRussert]

Thank you for the pr! 🎉

Nice job figuring this out overall! There are a couple things to fix, should be no big deal.

Welcome and let me know if you have questions!

[BenRussert]

The main reason tests are not passing is because flow wants you to handle cases where kernel or kernel.outputStore are undefined.

It should be fine if you do something like:

if (!kernel || !kernel.outputStore) return;

just above this line.

[BenRussert]

Stream outputs, e.g. from print(), have an output.text property. Other execution results have no text property so we will need to look for output.data mime bundles, with 'text/plain' being the lowest/plainest common denominator.

// add up top: 
import { toJS } from "mobx"; 
import _ from "lodash"; 
// ...

const handleClick = () => {
    if (!kernel || !kernel.outputStore) return;
    // output should be converted to a plain js object first:
    const output = toJS(kernel.outputStore.outputs[kernel.outputStore.index]);

    // check for a text property and fall back to data["text/plain"]
    // there are other ways of doing this if you like, but for example:
    const textOrBundle = _.has(output, "text")
      ? output.text
      : output.data["text/plain"];

    atom.clipboard.write(textOrBundle);
    atom.notifications.addSuccess("Copied to clipboard");
  };

[callmeish]

I didn't realize that the output might not be something with an output.text property, thanks for catching that. The style thing was a good idea as well.

I do have a question as far as the case where kernel or kernel.outputStore are undefined. That entire function is (or was, as I moved it out of an anonymous function) nested inside a div that wouldn't even appear if kernel.outputStore.outputs.length returned a value less than or equal to 0. Would that condition not require that both kernel and kernel.outputStore be defined?

[BenRussert]

Minor thing 😄

Since we are using the same style, why not put this into a variable right below EmptyMessage up top:

const buttonStyle = {
  flex: "0 0 auto",
  width: "fit-content"
};

// then in the component:
style={buttonStyle}

[rgbkrk]

kernel isn't defined here so this fails tests. It should come from the store like below.

[BenRussert]

☝️ you can check if you caught the flow errors locally by running npm run flow from the root of your hydrogen project directory.

@rgbkrk I was thinking you could just move handleClick into the outputArea observer function, then add:

  const handleClick = () => {
    if (!kernel || !kernel.outputStore) return;

to satisfy flow?

[rgbkrk]

I just merged in a PR that changed formatting, you'll need to rebase (and likely make sure to upgrade dependencies).

[BenRussert]

@callmeish want to join our slack channel? This is close, but the minor details like flow and the conflicts from our prettier upgrade could be confusing 😆

[callmeish]

Rebased, tested, should be good now. Thanks a bunch for the tip on local flow testing.

[lgeiger]

@callmeish Thanks a lot for the work and the rebase!
Unfortunately after the rebase this includes a lot of commits that are already present on master (take a look at the changed files) so we are unable to merge this.

To fix this a interactive rebase is necessary:

git remote update
git checkout copy-external-output 
git rebase -i upstream/master

Remove the commits unrelated to this PR in the editor that will pop up and you should be good to go.

If you need help with this feel free to join our slack channel.

[lgeiger]

@callmeish Since we're waiting with #978 for this to be merge, I rebase this PR and appended a minor fix for the case where no text/plain mime type is available (e.g. plotly plot).

This should now be good to merge.

Thanks again for digging into this. I invited you to the nteract organisation 🎉 .
We're very happy if you'd like to help us maintaining Hydrogen.

[lgeiger]

I'm not sure if we really need to convert it but it doesn't hurt.

[BenRussert]

I had some error without it (not consistently)

[lgeiger]

An alternative approach would be to get the text directly from the DOM as it is done in the result view component.
Though I kind of prefer the approach taken here because it consistently copies only the plain text representation. The only drawback is that we can't properly handle ANSI codes this way.
In a future PR we should come up with consistent way of handling this.

[BenRussert]

Thanks @callmeish, I hope you stick around!

[BenRussert]

An alternative approach would be to get the text directly from the DOM as it is done in the result view component.
Though I kind of prefer the approach taken here because it consistently copies only the plain text representation.

I like this approach as well, another benefit is it can be done without refs which the react docs say should be used sparingly.

[lgeiger]

We could run anser.ansiToText before copying it to the clipboard since we already have anser as an implicit dependency in our transforms.