[Enhancement] Export plot as PDF, JPG, or PNG #1164

hudsonfoo · 2016-09-01T17:52:13Z

Changes address original issue?
Unit tests included and/or updated with changes?
Command line build passes?
Changes have been smoke-tested?

For issue #967.

I'm reviewing methods for adding testing, and there are two minor issues left, but I would appreciate a code review. I'm doing my best to make updates the same way you would internally but it may take a few swings to get it right.

Remaining issues are trivial:

Bottom of plot gets cut off on export
User should be given option to name the file prior to download (?)

This will be used for exporting an image of the plot.

Extends PlotController by adding three new scoped methods: exportPDF, exportPNG, exportJPG. All three methods use basically the same steps. The HTML node of the plot is passed through html2canvas which generates a canvas. From the canvas we export a blob, PNG, or JPG then save the file.

VWoeltjen · 2016-09-01T18:49:42Z

platform/features/plot/src/PlotController.js

+            $scope.exportJPG = function () {
+                PlotController.prototype.exportJPG(self.$element, 'plot.jpg');
+            };
+


PlotController is exposed as plot in the Angular scope already, so I believe these could be omitted by instead having ng-click="plot.exportPDF()" in the plot template. This would make the code and changeset simpler.

VWoeltjen · 2016-09-01T19:08:35Z

I'm reviewing methods for adding testing, and there are two minor issues left, but I would appreciate a code review. I'm doing my best to make updates the same way you would internally but it may take a few swings to get it right.

Sure! Left a couple of comments in-line.

Remaining issues are trivial:

Bottom of plot gets cut off on export

User should be given option to name the file prior to download (?)

I agree that a user-specified name is better UX, but think this can also be deferred (particularly if collecting that input proves non-obvious/non-trivial for some reason); saving as "plot.png" or "plot.pdf" is sufficient to meet the use case. A user can always rename the file after it's downloaded.

Fixes #967 WIP to be integrated with work from hudsonfoo in PR #1164

Set export functions on the ExportImageService prototype. Insantiated ExportImageService in the PlotController for better dependcy injection.

Fixes nasa#967 WIP to be integrated with work from hudsonfoo in PR nasa#1164 (cherry picked from commit ebadad6)

It's worth noting that I changed the .gl-plot position from relative to absolute in an attempt to match the results to a similar requirement in MCT Table. Setting to absolute caused no regressions as far as I could tell, but I have not attempted browsers outside of Chrome.

Buttons temporarily hide until export completes.

…injection

I would prefer this be passed in via the bundle, but it continues to fail saying "Unknown Provider". I have chosen to require them into the module the old-fashioned way, then allow an injectible dependency to override.

hudsonfoo · 2016-09-04T14:26:57Z

@VWoeltjen unit test has been added. I did try to set up dependency injection with html2canvas, jsPDF, and saveAs, which in the end I did successfully, but I am dissatisfied with my method. For whatever reason, adding any of those to the dependencies in plot/bundle.js wind up with "Unknown provider" errors. If you have time for one more code review, all I have left is to make sure the plot never gets cut off at the bottom (almost certainly a CSS issue) and we're good to go.

Based on updates from 0457f7b these styles are no longer necessary and have been removed.

Issue nasa#1164. Based on updates from 0457f7b these styles are no longer necessary and have been removed.

hudsonfoo · 2016-09-08T02:14:55Z

@charlesh88 cool. I cherry-picked 0457f7b and merged.

+1 on moving this to the context menu long-term.

charlesh88 · 2016-09-08T20:15:33Z

@hudsonfoo Hi Hudson: The changes in _plots-main.scss lines 25 - 39 shouldn't be there - we actually want the export buttons to always display when the user is viewing the object directly to aid discoverability. We hide and show them when that object is in a Layout because often it's a smaller size; that's what the work in #1167 was about generalizing. Sorry to have to correct this after the fact; I'm pretty behind on getting our style guide together which should include this info.

Also: a bit nervous about changing .gl-plot from position: relative to absolute, but I assume that was done for the needs of exporting?

hudsonfoo · 2016-09-09T03:15:05Z

@charlesh88 cool no prob. After some additional testing with the new cherry-pick, looks like those styles are no longer required, including the .gl-plot change. I've put the stylesheet back to its original state and fixed a minor typo.

As for style guides, I purchased several hardcopy NASA Graphics Standards manuals awhile back and distributed them throughout my office as a reference point for our own :) Pretty cool.

VWoeltjen · 2016-09-09T16:37:56Z

platform/features/plot/bundle.js

@@ -84,12 +88,30 @@ define([
                    ]
                }
            ],
+            "services": [
+                {
+                    "key": "ExportImageService",


A nit, but exportImageService would be more consistent with naming conventions used elsewhere. (Same goes for its usage in PlotController; we use UpperCamelCase as a convention to identify constructor functions.)

Issue nasa#1164

VWoeltjen · 2016-09-09T16:46:51Z

platform/features/plot/src/services/ExportImageService.js

+            }
+        }
+
+        ExportImageService.prototype.exportPDF = function (element, filename) {


Could use some brief JSDoc here for this and the other export methods

hudsonfoo · 2016-09-09T16:47:43Z

Another good catch @VWoeltjen, thanks.

VWoeltjen · 2016-09-09T16:50:07Z

platform/features/plot/src/GLChart.js

@@ -54,7 +54,7 @@ define(
         * @throws {Error} an error is thrown if WebGL is unavailable.
         */
        function GLChart(canvas) {
-            var gl = canvas.getContext("webgl") || canvas.getContext("experimental-webgl"),
+            var gl = canvas.getContext("webgl", { preserveDrawingBuffer: true }) || canvas.getContext("experimental-webgl", { preserveDrawingBuffer: true }),


Would like to break this up to reduce line length, e.g.

var gl = canvas.getContext("webgl", { preserveDrawingBuffer: true }) || canvas.getContext("experimental-webgl", { preserveDrawingBuffer: true }),

Issue nasa#1164

VWoeltjen · 2016-09-09T17:01:55Z

platform/features/plot/test/services/ExportImageServiceSpec.js

+                    mockSaveAs
+                );
+            });
+


I think the verification of high-level interactions is good here, but would also like to pass in a testElement (don't think there's anything that needs to be mocked, so this could just be some object) to the exportXYZ methods, in order to expect(mockHtml2Canvas).toHaveBeenCalledWith(testElement, { onrendered: jasmine.any(Function) })

Testing of the onrendered callback itself would also be good. In particular, it would be nice to check interactions with the canvas object to make sure exportPNG isn't producing JPGs and so forth.

Issue nasa#1164

charlesh88 · 2016-09-09T17:58:46Z

@hudsonfoo Thanks! I'm good to go on this PR with my eval.

Issue nasa#1164

Issue nasa#967

hudsonfoo · 2016-09-12T08:03:43Z

@VWoeltjen please see updated tests which look for proper html2canvas call as well as ensuring canvas.toBlob requests the correct image type from the canvas.

Issue nasa#967

VWoeltjen · 2016-09-12T17:56:05Z

Everything's looking really good, thanks for working on this! Double bonus extra gratitude for including licensing info for the new run-time dependencies.

Reviewer Checklist

Changes appear to address issue? Y
Appropriate unit tests included? Y
Code style and in-line documentation are appropriate? Y
Commit messages meet standards? Y

David Hudson added 7 commits August 31, 2016 23:03

[Enhancement] Quick prototype of PDF export

852ac3c

Merge branch 'master' of github.com:nasa/openmct into open967

4d3afb8

[Formatting] Remove test code

5e0975d

[Enhancement] Pass element to plot controller

06a4533

This will be used for exporting an image of the plot.

[Frontend] Add export plot buttons

219301a

Merge branch 'master' of github.com:nasa/openmct into open967

a8900f3

hudsonfoo changed the title ~~[Enhancement] Export plot as PDF, JPG, or PNG~~ [Enhancement] Export plot as PDF, JPG, or PNG for issue #967 Sep 1, 2016

hudsonfoo changed the title ~~[Enhancement] Export plot as PDF, JPG, or PNG for issue #967~~ [Enhancement] Export plot as PDF, JPG, or PNG Sep 1, 2016

[Formatting] Linting

3a19890

VWoeltjen reviewed Sep 1, 2016
View reviewed changes

VWoeltjen self-assigned this Sep 1, 2016

charlesh88 added a commit that referenced this pull request Sep 2, 2016

[Frontend] Markup for #967

ebadad6

Fixes #967 WIP to be integrated with work from hudsonfoo in PR #1164

David Hudson and others added 14 commits September 2, 2016 11:54

[Code Review] Updates based on code review by @VWoeltjen

e370271

Set export functions on the ExportImageService prototype. Insantiated ExportImageService in the PlotController for better dependcy injection.

[Formatting] Switched to double quote across the board for consistency

3e3d3eb

Merge branch 'master' of github.com:nasa/openmct into open967

e38cf1e

[Frontend] Markup for nasa#967

e7a7025

Fixes nasa#967 WIP to be integrated with work from hudsonfoo in PR nasa#1164 (cherry picked from commit ebadad6)

[Enhancement] Hide export buttons during export

de85f79

Buttons temporarily hide until export completes.

[Tests] Add jsPDF and html2canvas

40ad9c4

[Tests] Mock new argument in PlotController

92bfea1

[Formatting] Fix styling errors

26c14d2

[Testing] Move ExportImageController into constructor for dependency …

fecf419

…injection

[Testing] Add ExportImageService mock

a566265

[Testing] Add better dependency injection and error handling

9bb647e

[Testing] Add test spec for ExportImageService

a49eebf

David Hudson added 3 commits September 8, 2016 11:03

[Frontend] Remove styles that are no longer necessary

222b442

Based on updates from 0457f7b these styles are no longer necessary and have been removed.

[Frontend] Remove styles that are no longer necessary

911ece7

Issue nasa#1164. Based on updates from 0457f7b these styles are no longer necessary and have been removed.

Merge branch 'open967' of github.com:hudsonfoo/openmct into open967

990c653

David Hudson added 3 commits September 9, 2016 12:02

Merge branch 'master' of github.com:nasa/openmct into open967

da23bba

[Frontend] Remove unnecessary export button styles

c6caae8

[Frontend] Fix typo

72ecbb0

VWoeltjen reviewed Sep 9, 2016
View reviewed changes

[Formatting] Rename export image service variables

a6386b1

Issue nasa#1164

VWoeltjen reviewed Sep 9, 2016
View reviewed changes

[Formatting] Add inline JSDoc

b51ffca

Issue nasa#1164

VWoeltjen reviewed Sep 9, 2016
View reviewed changes

[Formatting] Reduce line length of gl declaration

98deac0

Issue nasa#1164

David Hudson added 2 commits September 10, 2016 03:12

[Formatting] Rename export image service key

8e39da6

Issue nasa#1164

[Formatting] Rename export image service key

d62989b

Issue nasa#1164

VWoeltjen added this to the Pratchett milestone Sep 9, 2016

David Hudson added 4 commits September 12, 2016 15:46

Merge branch 'master' of github.com:nasa/openmct into open967

0f1d685

[Testing] Convert blob to proper format. Inject FileReader.

3093ab8

Issue nasa#967

[Formatting] Fix missing space

487ec79

Issue nasa#967

[Testing] Add tests to ensure correct image type

bb34528

Issue nasa#967

[Typo] Add reference to self

bad901a

Issue nasa#967

VWoeltjen merged commit e73bb4f into nasa:master Sep 12, 2016

VWoeltjen mentioned this pull request Sep 19, 2016

[Sub-object] Prototype sub-object selection #1126

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Enhancement] Export plot as PDF, JPG, or PNG #1164

[Enhancement] Export plot as PDF, JPG, or PNG #1164

hudsonfoo commented Sep 1, 2016 •

edited

Loading

VWoeltjen Sep 1, 2016

VWoeltjen commented Sep 1, 2016

hudsonfoo commented Sep 4, 2016

hudsonfoo commented Sep 8, 2016

charlesh88 commented Sep 8, 2016

hudsonfoo commented Sep 9, 2016 •

edited

Loading

VWoeltjen Sep 9, 2016

VWoeltjen Sep 9, 2016

hudsonfoo commented Sep 9, 2016

VWoeltjen Sep 9, 2016

VWoeltjen Sep 9, 2016

charlesh88 commented Sep 9, 2016

hudsonfoo commented Sep 12, 2016

VWoeltjen commented Sep 12, 2016

[Enhancement] Export plot as PDF, JPG, or PNG #1164

[Enhancement] Export plot as PDF, JPG, or PNG #1164

Conversation

hudsonfoo commented Sep 1, 2016 • edited Loading

VWoeltjen Sep 1, 2016

Choose a reason for hiding this comment

VWoeltjen commented Sep 1, 2016

hudsonfoo commented Sep 4, 2016

hudsonfoo commented Sep 8, 2016

charlesh88 commented Sep 8, 2016

hudsonfoo commented Sep 9, 2016 • edited Loading

VWoeltjen Sep 9, 2016

Choose a reason for hiding this comment

VWoeltjen Sep 9, 2016

Choose a reason for hiding this comment

hudsonfoo commented Sep 9, 2016

VWoeltjen Sep 9, 2016

Choose a reason for hiding this comment

VWoeltjen Sep 9, 2016

Choose a reason for hiding this comment

charlesh88 commented Sep 9, 2016

hudsonfoo commented Sep 12, 2016

VWoeltjen commented Sep 12, 2016

Reviewer Checklist

hudsonfoo commented Sep 1, 2016 •

edited

Loading

hudsonfoo commented Sep 9, 2016 •

edited

Loading