Forbid from displaying pop-up windows during image export #66

JamesCropcho · 2018-04-11T17:49:29Z

Hello,

This pull requested created at the behest of @jackparmer (at #65 (comment)).

How does it look?

―James

…2988

n-riesco · 2018-04-11T17:55:12Z

@JamesCropcho @etpinard shall we add show: false to the server too?
https://github.com/plotly/image-exporter/blob/1c164bb2a4cb69f67a0f842aaf371bcb7721ca55/src/app/server/coerce-opts.js#L26

JamesCropcho · 2018-04-11T18:02:36Z

@n-riesco @etpinard I think we should do exactly what @jmmease did. I was hoping he could confirm that this PR achieves that.

etpinard · 2018-04-11T18:03:51Z

Can someone please tell me what show: false does exactly and how it differs from using xvfb? Thanks.

n-riesco · 2018-04-11T18:08:46Z

@etpinard My understanding is that when show: false is used, the BrowserWindow is created minimised (in other words, I think xvfb is still needed).

etpinard · 2018-04-11T18:14:59Z

src/app/runner/coerce-opts.js

@@ -15,7 +15,7 @@ function coerceOpts (_opts = {}) {
  const opts = {}

  opts.debug = !!_opts.debug
-  opts._browserWindowOpts = {}
+  opts._browserWindowOpts = { show: false }


Probably best to set show: opts.debug for debugging purposes.

Okay, I have done this.

etpinard · 2018-04-11T18:15:46Z

shall we add show: false to the server too?

Yep, good catch 🎣

JamesCropcho · 2018-04-11T18:22:14Z

Yep, good catch

Okay, I have done this. I did not do so initially because @jmmease claimed he did not. It is there now.

etpinard · 2018-04-11T18:24:10Z

src/component/plotly-graph/render.js

@@ -154,7 +154,8 @@ function toPDF (imgData, imgOpts, bgColor) {
  return new Promise((resolve, reject) => {
    let win = remote.createBrowserWindow({
      width: wPx,
-      height: hPx
+      height: hPx,
+      show: false


there's a few more createBrowserWindow calls:

Moreover, here too we should set show: opts.debug.

@etpinard Okay, I have addressed all createBrowserWindow() calls, as well as leveraged opts.debug.

I am unsure if my changes are sound. May they please be evaluated?

I won't have time to test this out today. Hopefully tomorrow morning.

Hello, @etpinard. I was wondering if you managed to evaluate these aforementioned changes of mine, which by own admission, I am unsure if they are sound.

No. But looks like @jmmease did.

@etpinard It does not appear to me, at least, as though @jmmease did. Rather, he appears to have manually tested Image-Exporter's functionality. If that will suffice for you, of course at your discretion you need not perform the evaluation.

Well from #66 (comment) and #66 (comment) you agreed to incorporate a few more things in this PR, correct?

I'll test this out when all the pieces are here.

Yes, that is correct, @etpinard. I am working on it now, in fact, and I can happily replicate some of @jmmease's examples of behavior.

I fear I am being unclear. I'm saying that when I made these referenced changes I felt like I was on very unsure footing. This concern is independent of the manually-testable behavior of the my changes.

Once again, you may perform only manual functionality testing at your discretion, of course. I'm replying to you here, to ensure that you're doing so with the understanding of where I was coming from.

I will continue to work on this pull request and I will strive to continue to make progress.

… should set show: opts.debug."

jonmmease · 2018-04-12T14:55:17Z

When I perform a png conversion from the command line I see no window

bin/plotly-graph-exporter.js '{"data":[{"y": [3, 2, 5, 3]}],"layout":{}}' -o out --format=png

But for a PDF I still get a window

bin/plotly-graph-exporter.js '{"data":[{"y": [3, 2, 5, 3]}],"layout":{}}' -o out --format=pdf

If I hardcode show: false in src/component/plotly-graph/render.js the window goes away during PDF conversion

https://github.com/plotly/image-exporter/blob/c8db9df30280aeacf01941804df5554e21ad3e4b/src/component/plotly-graph/render.js#L158

Is there anything I should do from the command line to cause imgOpts.debug to be false here?

jonmmease · 2018-04-12T15:39:11Z

I have two additional related suggestions for your consideration in this PR @JamesCropcho @jackparmer @etpinard @chriddyp

The electron BrowserWindow has a skipTaskbar option with description

skipTaskbar Boolean (optional) - Whether to show the window in taskbar. Default is false.

On Windows and Linux, setting this to true should keep the electron window from displaying in the taskbar, which I believe to be desirable for making image export a seamless experience for Windows and Linux Python/R users. I think this change would just involve adding the skipTaskbar option everywhere the show option is added.

Note: I'm on OS X so I can't test this directly, as the dock is not considered a taskbar (See below)

To hide the electron icon from the dock on OS X it is recommended (Add ability to hide dock icon immediately electron-userland/electron-builder#1456) to add "extendInfo": {"LSUIElement": 1} to the top-level mac configuration settings (which I believe are in package.json for this project). e.g.

"mac": {
  "extendInfo": {
    "LSUIElement": 1
  }
}

Note: I believe these settings only take effect after the application is bundled, so I don't expect it to change the behavior of running bin/plotly-graph-exporter.js out of the development directory.

Thanks for considering. I know I'm being a perfectionist here :-)

jackparmer · 2018-04-12T17:13:03Z

Great finds @jmmease !

@JamesCropcho can you please incorporate Jon's feedback into this PR?

JamesCropcho · 2018-04-12T20:00:33Z

@jackparmer Yes, certainly. I will do so.

…efined` as true. This commit performs like casting to a Boolean.

…window from displaying in the taskbar, which I believe to be desirable for making image export a seamless experience for Windows and Linux Python/R users. I think this change would just involve adding the skipTaskbar option everywhere the show option is added."

…lectron-userland/electron-builder#1456) to add "extendInfo": {"LSUIElement": 1} to the top-level mac configuration settings"

JamesCropcho · 2018-04-12T23:36:20Z

@jackparmer I have incorporated all of Jon's feedback into this PR, as per your request.

@jmmease, could you kindly examine my recent commits and ensure they adhere to both the letter and spirit of your proposals?

jonmmease · 2018-04-13T10:30:54Z

Everything is working well on OS X. No windows popup during export to at least png, pdf, and eps.

Also, I merged this branch with #63, called npm run pack, installed the .dmg, and ran export commands using the /Applications/Plotly Graph Exporter.app/Contents/MacOS/Plotly Graph Exporter executable and I can confirm that the dock icon is suppressed, without even a flicker!

I looked through the code, and my only comment is I'm curious if there was a reason not to include skipTaskbar alongside all of the new uses of show: false. As I mentioned above, this option relevant only Linux/Windows so I can confirm if it's working.

Can someone on Linux/Windows confirm that the electron application doesn't show (or even flicker) in the taskbar during export of png, pdf, and eps?

cc: @jackparmer @etpinard

n-riesco · 2018-04-13T11:32:10Z

After merging #63 into #66 and running npm install && npm run pack, I tested the AppImage version on Ubuntu 16.04, and it runs without flicker (tested --format pdf, png and svg).

n-riesco · 2018-04-13T11:34:12Z

I've noticed that running the AppImage version now prints out the message:

installed: X-AppImage-BuildId=5a6b0d10-3f0d-11a8-3934-f7a37a902d22 image: X-AppImage-BuildId=5a6b0d10-3f0d-11a8-3934-f7a37a902d22

I don't remember this happening before.

JamesCropcho · 2018-04-13T14:52:31Z

Thank you for your review, Jon.

I looked through the code, and my only comment is I'm curious if there was a reason not to include skipTaskbar alongside all of the new uses of show: false.

Presumably skipTaskBar is in fact being used the way you proposed, via: https://github.com/plotly/image-exporter/pull/66/files#diff-6b824d79a3398a233ff5db1d7f07a115R15. Unlike show: false, which is conditional upon being in debug mode, skipTaskBar would simply always be true, per your recommendation. Kindly let me know what you think.

I've noticed that running the AppImage version now prints out the message:

installed: X-AppImage-BuildId=5a6b0d10-3f0d-11a8-3934-f7a37a902d22 image: X-AppImage-BuildId=5a6b0d10-3f0d-11a8-3934-f7a37a902d22

I am fine with that, if everyone else is.

@jmmease @etpinard @n-riesco

jonmmease · 2018-04-13T15:05:21Z

@JamesCropcho Yes, I see that now. Thanks for explaining.

I too noticed an error/warning message (I can't reproduce it at the moment). For me, it only occurred the very first time I ran the command after building it, and it didn't appear to interfere with the export itself. That doesn't trouble me personally.

So this lgtm. Thanks to all for being open to my feedback (and for choosing to open source this in the first place!). I'm really excited to have this on the Python side, it fills in a missing piece that has prevented some people (perhaps many people) from settling on plotly.py as their primary visualization library.

etpinard · 2018-04-13T16:11:10Z

This is looking amazing. show: false and skipTaskBar: true are working out beautifully on my Ubuntu 16.04 machine. Big ups to @jmmease for bringing this up in #65 and shoutout to @n-riesco for suggesting show: false in #65 (comment)! I was foolishing waiting for first-class support of the --headless chromium flag in Electron.

It might be nice to ask someone to test this out on Windows before merging this thing. Does anyone know someone with access to a Windows machine?

JamesCropcho · 2018-04-13T16:16:40Z

@etpinard I am able to dual-boot into Windows 10 Pro on my development machine, and I can perform specific tests in that environment upon request. I do not believe I have Git, Powershell or other such tooling present, however.

n-riesco · 2018-04-13T16:21:36Z

@JamesCropcho I'd merge this PR into #63 and test the Windows installer that AppVeyor builds.

JamesCropcho · 2018-04-13T16:27:46Z

@n-riesco Could you give me a command(s) you'd like me to execute, specifying which shell to do so in, and optionally specify anything in particular I should look for?

n-riesco · 2018-04-13T16:47:52Z

@JamesCropcho First, you need a JSON file that defines a plotly chart (I used this one.

After running the Windows installer, I'm guessing, an executable named Plotly Graph Exporter.exe will be accessible from the command line. If that's the case, the command to export 20.json into pdf would be:

Plotly\ Graph\ Exporter.exe 20.json --format pdf

Ping me on slack if you need more help.

JamesCropcho · 2018-04-13T17:24:09Z

@n-riesco I will use the build at https://ci.appveyor.com/project/JamesCropcho/image-exporter/build/0.1.270/artifacts, and the JSON file you suggested.

I hope to report back to you within the hour.

JamesCropcho · 2018-04-13T17:47:28Z

Hello @n-riesco, I'll recap what happened here.

I ran the installer. I got this:

...however all was otherwise seemingly okay.,

The installation did not add an executable to the path, at least in the out-of-box Command Prompt. I still found the executable, and ran it. It worked, even if I had to press enter one additional time, to get the prompt back:

C:\Users\James\Downloads>"C:\Users\James\AppData\Local\Programs\image-exporter\Plotly Graph Exporter.exe" --help

C:\Users\James\Downloads>
plotly-graph-exporter

  Usage:

    $ plotly-graph-exporter [path/to/json/file(s), URL(s), glob(s), '{"data":[],"layout":{}}'] {options}

    $ cat plot.json | plotly-graph-exporter {options} > plot.png

  Options:

  --help [or -h]
    Displays this message.

  --version [or -v]
    Displays package version.

  --output-dir [or -d, --outputDir]
    Sets output directory for the generated images. Defaults to the current working directory

  --output [or -o]
    Sets output filename. If multiple inputs are provided, then their item index will be appended to the filename.

  --plotly [or --plotlyjs, --plotly-js, --plotly_js, --plotlyJS, --plotlyJs]
    Sets the path to the plotly.js bundle to use.
    This option can be also set to 'latest' or any valid plotly.js server release (e.g. 'v1.2.3'),
    where the corresponding plot.ly CDN bundle is used.
    By default, the 'latest' CDN bundle is used.

  --mapbox-access-token [or --mapboxAccessToken]
    Sets mapbox access token. Required to export mapbox graphs.
    Alternatively, one can set a `mapboxAccessToken` environment variable.

  --topojson
    Sets path to topojson files. By default topojson files on the plot.ly CDN are used.

  --mathjax [or --MathJax]
    Sets path to MathJax files. Required to export LaTeX characters.

  --format [or -f]
    Sets the output format (png, jpeg, webp, svg, pdf, eps). Applies to all output images.

  --scale
    Sets the image scale. Applies to all output images.

  --width
    Sets the image width. If not set, defaults to `layout.width` value. Applies to all output images.

  --height
    Sets the image height. If not set, defaults to `layout.height` value. Applies to all output images.

  --parallel-limit [or --parallelLimit]
    Sets the limit of parallel tasks run.

  --verbose
    Turn on verbose logging on stdout.

  --debug
    Starts app in debug mode and turn on verbose logs on stdout.


C:\Users\James\Downloads>

I then ran a command analogous to your recommendation:

C:\Users\James\Downloads>"C:\Users\James\AppData\Local\Programs\image-exporter\Plotly Graph Exporter.exe" 20.json --format pdf

C:\Users\James\Downloads>

C:\Users\James\Downloads>

Sadly, the command seems to have not created no output file, (nor any in-terminal output no displayed above).

I must offer the caveat, that I really do not know Windows well at all. In hindsight, I'm not suitable to test these things, because I could have done wrong some complete beginners' mistake.

Still, here is my report.

n-riesco · 2018-04-13T17:51:37Z

@JamesCropcho Does it work with other formats? svg? png?

JamesCropcho · 2018-04-13T17:58:22Z

@n-riesco I experienced identical behavior when attempting to create PNG and SVG output.

etpinard · 2018-04-13T21:10:59Z

Ok well, we probably don't have to have a working Windows executable for our 0.0.1 release. I just logged an issue here -> #70

@JamesCropcho would you mind merge master into this branch and fixing the merge conflict?

…2988

… should set show: opts.debug."

…efined` as true. This commit performs like casting to a Boolean.

…window from displaying in the taskbar, which I believe to be desirable for making image export a seamless experience for Windows and Linux Python/R users. I think this change would just involve adding the skipTaskbar option everywhere the show option is added."

…lectron-userland/electron-builder#1456) to add "extendInfo": {"LSUIElement": 1} to the top-level mac configuration settings"

…er into forbid-popup-windows

JamesCropcho · 2018-04-14T00:39:06Z

@etpinard

@JamesCropcho would you mind merge master into this branch and fixing the merge conflict?

Happily. I have done as requested.

etpinard · 2018-04-16T13:54:25Z

OK @JamesCropcho not sure what happened when you merge master. We have another mess of a commit history in our hands. I'll try squashing and merging again.

etpinard · 2018-04-16T14:13:59Z

Hold on. I'm seeing changes from #63 in https://github.com/plotly/image-exporter/pull/63/files

This will mess up our commit history. @JamesCropcho can you open up a another PR with just the changes you made in src/ before you (tried to) merge master into this branch?

JamesCropcho · 2018-04-16T21:25:49Z

@etpinard

This will mess up our commit history. @JamesCropcho can you open up a another PR with just the changes you made in src/ before you (tried to) merge master into this branch?

I believe I did successfully merge master. As always, if there's some different way you'd like me to do something, let me know.

More to the point of your request: okay, I have found that this is the commit on this branch/PR which is immediately prior the merge commit: f81ff05. Next I have found the diff of that revision of that branch relative to master: master...f81ff05. According to it, the list of changed files:

package.json
+1 −1 src/app/runner/coerce-opts.js
+1 −1 src/app/server/coerce-opts.js
+5 −2 src/component/plotly-dash-preview/render.js
+3 −2 src/component/plotly-dashboard-preview/render.js
+2 −1 src/component/plotly-dashboard-thumbnail/render.js
+3 −2 src/component/plotly-dashboard/render.js
+2 −1 src/component/plotly-graph/render.js
+1 −0 src/util/remote.js

All are in /src other than package.json.

Manually reverting the changes in package.json in a branch off this revision mentioned at the top, the difference between that and my new branch is indeed just the package.json changes: f81ff05...just-changes-in-src-from-forbid-popup-windows

The PR as described above has been created, at #72.

etpinard · 2018-04-16T21:29:08Z

I believe I did successfully merge master.

Yes you did. But you also added 7 commits with no new content in the process. git merge origin master should have been enough.

JamesCropcho · 2018-04-16T21:32:16Z

@etpinard

Yes you did. But you also added 7 commits with no new content in the process. git merge origin master should have been enough.

Okay, please pardon me there. Ironically, due to a recent uptick in concern about signal in revision history, I decided to rebase rather than merge, to avoid creating the merge commit. Again ironically, it sounds like that would have in fact been the cleaner method.

etpinard · 2018-04-16T21:36:39Z

Ironically, you find adding 7 commits new-content-less commits better for revision history than one single merge commit.

Moreover, please never rebase (and git push -f) on branches that have open PRs attached to them. If you feel the need to rebase, do that on a different branch/PR.

JamesCropcho · 2018-04-17T01:20:55Z

@etpinard Will do!

Again ironically, it sounds like that would have in fact been the cleaner method.

Please pardon again: by the "that" I refer to above, I'm trying to say the merge (your way) would have been cleaner than a rebase (my way) after all.

https://github.com/plotly/image-exporter/issues/65#issuecomment-38053…

f6d32e5

…2988

JamesCropcho requested a review from etpinard April 11, 2018 17:49

JamesCropcho mentioned this pull request Apr 11, 2018

Headless? #65

Closed

etpinard reviewed Apr 11, 2018

View reviewed changes

JamesCropcho added 2 commits April 11, 2018 14:16

"Probably best to set show: opts.debug for debugging purposes."

5420e46

"shall we add show: false to the server too?"

e1138a9

etpinard reviewed Apr 11, 2018

View reviewed changes

"there's a few more createBrowserWindow calls" "Moreover, here too we…

c8db9df

… should set show: opts.debug."

JamesCropcho added 3 commits April 12, 2018 19:17

remote.createBrowserWindow's show parameter appears to treat `und…

b7adf1c

…efined` as true. This commit performs like casting to a Boolean.

"To hide the electron icon from the dock on OS X it is recommended (e…

d185fcf

…lectron-userland/electron-builder#1456) to add "extendInfo": {"LSUIElement": 1} to the top-level mac configuration settings"

Enable CI platforms to build Linux, Windows and OS X releases (#63)

433d619

JamesCropcho added 8 commits April 13, 2018 20:28

https://github.com/plotly/image-exporter/issues/65#issuecomment-38053…

5debb1e

…2988

"Probably best to set show: opts.debug for debugging purposes."

0a634d3

"shall we add show: false to the server too?"

7a6586e

"there's a few more createBrowserWindow calls" "Moreover, here too we…

5616ec1

… should set show: opts.debug."

remote.createBrowserWindow's show parameter appears to treat `und…

de73f03

…efined` as true. This commit performs like casting to a Boolean.

"To hide the electron icon from the dock on OS X it is recommended (e…

f81ff05

…lectron-userland/electron-builder#1456) to add "extendInfo": {"LSUIElement": 1} to the top-level mac configuration settings"

Merge branch 'forbid-popup-windows' of github.com:plotly/image-export…

8d0782d

…er into forbid-popup-windows

JamesCropcho mentioned this pull request Apr 16, 2018

Just changes in /src from branch forbid-popup-windows #72

Merged

etpinard closed this Apr 16, 2018

etpinard deleted the forbid-popup-windows branch April 16, 2018 21:36

etpinard mentioned this pull request Apr 18, 2018

Standalone app logs "Installed ..." on exec #77

Closed

jonmmease mentioned this pull request Jul 30, 2018

Dock icon not hidden on OS X #102

Closed

Forbid from displaying pop-up windows during image export #66

Forbid from displaying pop-up windows during image export #66

Conversation

JamesCropcho commented Apr 11, 2018

n-riesco commented Apr 11, 2018

JamesCropcho commented Apr 11, 2018

etpinard commented Apr 11, 2018 • edited Loading

n-riesco commented Apr 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard commented Apr 11, 2018

JamesCropcho commented Apr 11, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonmmease commented Apr 12, 2018 • edited by JamesCropcho Loading

jonmmease commented Apr 12, 2018

jackparmer commented Apr 12, 2018

JamesCropcho commented Apr 12, 2018

JamesCropcho commented Apr 12, 2018

jonmmease commented Apr 13, 2018

n-riesco commented Apr 13, 2018

n-riesco commented Apr 13, 2018

JamesCropcho commented Apr 13, 2018

jonmmease commented Apr 13, 2018

etpinard commented Apr 13, 2018

JamesCropcho commented Apr 13, 2018

n-riesco commented Apr 13, 2018

JamesCropcho commented Apr 13, 2018

n-riesco commented Apr 13, 2018 • edited Loading

JamesCropcho commented Apr 13, 2018

JamesCropcho commented Apr 13, 2018

n-riesco commented Apr 13, 2018

JamesCropcho commented Apr 13, 2018

etpinard commented Apr 13, 2018 • edited Loading

JamesCropcho commented Apr 14, 2018

etpinard commented Apr 16, 2018

etpinard commented Apr 16, 2018

JamesCropcho commented Apr 16, 2018

etpinard commented Apr 16, 2018

JamesCropcho commented Apr 16, 2018

etpinard commented Apr 16, 2018

JamesCropcho commented Apr 17, 2018

etpinard commented Apr 11, 2018 •

edited

Loading

jonmmease commented Apr 12, 2018 •

edited by JamesCropcho

Loading

n-riesco commented Apr 13, 2018 •

edited

Loading

etpinard commented Apr 13, 2018 •

edited

Loading