Skip to content
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

Feature/snapshot #1

Closed
wants to merge 26 commits into from
Closed

Feature/snapshot #1

wants to merge 26 commits into from

Conversation

timelyportfolio
Copy link
Owner

Better Snapshot

Reference: plotly #83

Objectives

  1. Attach a toImage method directly to Plotly.
  2. Use promises
  3. Allow resize options for the snapshot in the new toImage and/or the snapshot modebar button.

1) Attach toImage

The easiest way to attach toImage would be to piggyback off the existing snapshot modebar button, since the snapshot button click method best handles this from start to finish.

See codepen for how we can currently accomplish this.

Plotly.toImage = function(){
  gd._fullLayout._modeBar.buttons.filter(
    function(btn){return btn[0].name==="toImage"
  })[0][0].click(gd)
}

While this works, this is an unworkable approach. First, we assume that the modebar exists and contains the snapshot button. To get around this, perhaps we could generalize the snapshot click handler and make it available outside of the modebar context in snapshot. Currently, the click handler is specified in these lines, so its broader usefulness is limited. In cff741a, I took a first pass at a new toSnapshot method in Plotly.Snapshot.

I misunderstood the objective. We want to attach toImage to Plotly, so let's try again. Wait to do this until the other steps are complete.

2) Use promises

I forgot that we want toImage to return a promise. It seems that we should replace the EV/EventEmitter with Promises. I see two places where this replacement needs to happen in the pipeline svgtoimg and toimage. I was able to get this "working" in 937340c and ac86931. Although it works, I am not sure the changes follow Plotly's conventions. Here is a quick example of what we can do now with promises using codepen example.

var trace1 = {
  y: [5, 4, 3, 2, 1, 0, 1, 2, 3, 4, 5],
  mode: 'markers',
  marker: {
    size: 40,
     color: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10],
    colorscale: 'Viridis'
  }
};

var data = [trace1];

var layout = {
  title: 'Scatter Plot with a Color Dimension'
};

var img = document.createElement('img');
document.body.append(img);

Plotly.newPlot('myDiv', data, layout)
  .then(Plotly.Snapshot.toImage)
  .then(function(url){img.src=url});

3) Allow resize

We'll need to do two things to accomplish this task. One, I think is fairly easy, and requires the ability to pass height and width in the opts argument of toImage to clonePlot. 760a93b is my first try at this. It seems to work. Second, we'll need to come up with the UI to specify height and width. I looked around the Plotly codebase for a modal or similar UI that I could mimic, but I did not find anything. If we do a modal, it seems like we should put it in Lib similar to notifier. At a minimum, the UI should have textareas for height and width. If possible, though, it would be nice to get a live updating preview to reflect the changes to the height and width. While this is nice, I don't think it is necessary.

@timelyportfolio
Copy link
Owner Author

I just ran the jasmine tests, and the old tests pass even with our changes, but this is because the tests only test clone. I'll look for opportunities to test once I collect opinions and thoughts.

var Lib = Plotly.Lib;

// check for undefined opts
opts = (opts) ? opts : {};

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we would usually write opts = opts || {}; in situations like this one.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

guessing same for the next line as well

opts.format = opts.format || 'png';

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

@etpinard
Copy link

@timelyportfolio I feel like our discussion here and on plotly#83 hasn't put forward any hard requirements for this issue, let me write them down here:

  • Add a Plotly.toImage (official) method. This method is essentially the current Plotly.Snapshot.toImage but with options handlers for format (either 'svg', 'png', 'jpg' and 'webp'), width (which should default to the current layout.width) and height (which should default to layout.height).
  • Plotly.toImage should be able to be in promise chain as such:
var img = document.createElement('img');
document.body.append(img);

Plotly.newPlot('myDiv', data, layout)
  .then(Plotly.toImage)
  .then(function(url){ img.src = url });
  • Plotly.toImage should be defined in src/plot_api/to_image.js
  • The Plotly.Snapshot should keep the same default behavior as their current version. Unfortunately, these methods are unofficially exposed at the moment, and this PR shouldn't make any breaking changes to the library.

@timelyportfolio
Copy link
Owner Author

I believe I made all the changes mentioned in the line notes. However, I need to return the Snapshot back to the way it was, and move the new toImage and the new downloadImage to attach directly to Plotly.

@etpinard, do you envision a 1.9.1 for this?

@timelyportfolio
Copy link
Owner Author

@etpinard, I'll try to summarize the current status of this pull, since a lot has been circular.

  • still need to add tests, but want to make sure that I understood the requirements first

Plotly.Snapshot

Back to the way it was except

  1. svgToImg will return an EventEmitter or optionally a Promise if opts contains promise: true .
  2. We have a new downloadImage function that programatically does what the snapshot modebar button does. downloadImage supports multiple formats (png, jpeg, and webp) and also supports height and width.
  3. Since the snapshot button and downloadImage do the same thing, the click handler for the snapshot modebar now just uses downloadImage.

Plotly.toImage

The new promise-based toImage is attached directly to Plotly and not hidden behind the unofficial Plotly.Snapshot. Note, Plotly.toImage is different from Plotly.Snapshot.toImage. We retained the Plotly.Snapshot.toImage for backward compatibility in case some users have taken advantage of the functionality in the past. Plans are to remove the Plotly.Snaphot unofficial API in 2.0.0.

Here is a simple example use of the new toImage.

var img = document.createElement('img');
document.body.appendChild(img);

Plotly.plot()
    .then(Plotly.toImage)
    .then(function(url){
         img.src = url;
    })

... but we also get to specify format, height, and width, which would look like this.

var img = document.createElement('img');
document.body.appendChild(img);

Plotly.plot()
    .then(function(gd){
         return Plotly.toImage(gd,{format:'webp', height:400, width:400});
    })
    .then(function(url){
         img.src = url;
    })

@etpinard
Copy link

@timelyportfolio you're on the right track for sure. 👍 for points 1 to 3.

As Plotly.toImage will be part of our official API, we should make sure that it handles bad arguments in a sensible way. Looking over the plot_api_test.js suite is a good place to start.

I'll make a few styling comments on the modified lines directly.


ev.clean();
});
click: function(gd){

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

function(gd) {

}

gd._snapshotInProgress = true;
Lib.notifier('Taking snapshot - this may take a few seconds', 'long');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this notifier should be omitted in Plotly.downloadImage.

var promise = toImage(gd, opts);

var filename = gd.fn || 'newplot';
var filename = opts.filename || gd.fn || 'newplot';

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice 👍

@timelyportfolio
Copy link
Owner Author

I rebased to the current master and squashed all the commits into one in this feature/snapshot-pull branch. I'll try to think of how best to test downloadImage, rebase/squash again, and then submit a proper pull.

@timelyportfolio
Copy link
Owner Author

timelyportfolio commented Apr 19, 2016

@etpinard, I just made some tests for downloadImage, but unfortunately, the really negative side effect is we get a download image on the local machine each time the test runs, so if watching, then you might get >10 new files downloaded to your drive. Any thoughts on whether or not to include? I condensed the tests so that we only run downloadImage once.

var Plotly = require('@lib/index');
var createGraphDiv = require('../assets/create_graph_div');
var destroyGraphDiv = require('../assets/destroy_graph_div');
var textchartMock = require('../../image/mocks/text_chart_arrays.json');

// minimize tests since each downloadImage
//  will result in a new file on the machine
//  this might mean we actually just do not test

describe('Plotly.downloadImage', function() {
    'use strict';
    var gd;
    var originalTimeout;

    beforeEach(function() {
        gd = createGraphDiv();

        // downloadImage can take a little longer
        //  so give it a little more time to finish
        originalTimeout = jasmine.DEFAULT_TIMEOUT_INTERVAL;
        jasmine.DEFAULT_TIMEOUT_INTERVAL = 10000;
    });

    afterEach(function() {
        destroyGraphDiv();
        jasmine.DEFAULT_TIMEOUT_INTERVAL = originalTimeout;    
    });

    it('should be attached to Plotly', function() {
        expect(Plotly.downloadImage).toBeDefined();
    });

    it('should create link, remove link, accept options', function(done) {
        //use MutationObserver to monitor the DOM
        //for changes
        //code modeled after
        //https://developer.mozilla.org/en-US/docs/Web/API/MutationObserver
        // select the target node
        var target = document.body;
        var domchanges = [];

        // create an observer instance
        var observer = new MutationObserver(function(mutations) {
          mutations.forEach(function(mutation) {
            domchanges.push(mutation);
          });
        });

        Plotly.plot(gd, textchartMock.data, textchartMock.layout).then(function(gd) {
            // start observing dom
            // configuration of the observer:
            var config = { childList: true };

            // pass in the target node and observer options
            observer.observe(target, config);

            return Plotly.downloadImage(gd, {format: 'jpeg', height: 300, width: 300, filename: 'plotly_download'});
        }).then(function(){
            // stop observing
            observer.disconnect();
            // look for an added and removed link
            var linkadded = domchanges[domchanges.length-2].addedNodes[0].outerHTML;
            var linkdeleted = domchanges[domchanges.length-1].removedNodes[0].outerHTML;

            // check for a <a element and proper file type
            expect(linkadded.substr(0,25)).toBe('<a href="data:image/jpeg;');
            // check that filename option handled properly
            expect(linkadded.substr(linkadded.length-26,15)).toBe('plotly_download');

            // check that link removed
            expect(linkadded).toBe(linkdeleted);
            done();
        });
    });
});

@etpinard
Copy link

@timelyportfolio to ensure that we test everything about downloadImage except the click call, we could add in the downloadImage suite:

var createElement = document.createElement;

beforeAll(function() {
  document.createElement = function(args) {
     var el = createElement.call(document, args);
     el.click = function() {};
     return el;
  }
});

afterAll(function() {
  document.createElement = createElement;
});

@timelyportfolio
Copy link
Owner Author

close in favor of plotly#446

timelyportfolio pushed a commit that referenced this pull request Aug 3, 2016
Legend item wrap with layout.legend.orientation = h
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants