Skip to content

/dash-preview: Fix window size #250

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Sep 30, 2019
Merged

/dash-preview: Fix window size #250

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f119b14
Dockerfile: add package-lock.json before npm install, copy Orca last …
antoinerg Sep 17, 2019
46d6bc5
Merge pull request #251 from plotly/ci-docker-caching
antoinerg Sep 23, 2019
407fe05
Enable additional options.
tarzzz Sep 24, 2019
fdb0a35
Use "math.ceil" for browserSize as well.
tarzzz Sep 24, 2019
c67d171
update test.
tarzzz Sep 24, 2019
f5a736f
Use default xvfb window size.
tarzzz Sep 24, 2019
0943a79
dash-preview: properly parse (pdf_options.)pageSize and :lock: with test
antoinerg Sep 26, 2019
156a35c
dash-preview: turn mock object into a function returning a mock
antoinerg Sep 30, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 16 additions & 16 deletions deployment/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,22 +82,6 @@ RUN apt-get update -y && \
RUN curl -L https://github.com/plotly/plotly.js/archive/master.tar.gz \
| tar -xvzf - --strip-components=3 plotly.js-master/dist/extras/mathjax

####################
# Copy and set up Orca

RUN wget -q -O - https://dl-ssl.google.com/linux/linux_signing_key.pub | apt-key add - && \
sh -c 'echo "deb [arch=amd64] http://dl.google.com/linux/chrome/deb/ stable main" >> /etc/apt/sources.list.d/google-chrome.list' && \
apt-get update -y && \
apt-get install -y google-chrome-stable xvfb poppler-utils git && \
rm -rf /var/lib/apt/lists/* && apt-get clean

COPY package.json /var/www/image-exporter/
COPY bin /var/www/image-exporter/bin
COPY src /var/www/image-exporter/src

WORKDIR /var/www/image-exporter
RUN npm install && mkdir build

####################
# Install and configure monit
COPY deployment/monitrc /etc
Expand Down Expand Up @@ -127,6 +111,22 @@ RUN wget https://raw.githubusercontent.com/plotly/plotly.js/master/dist/plotly-g

COPY deployment/ImageMagickPolicy.xml /etc/ImageMagick-6/policy.xml

####################
# Copy and set up Orca

RUN wget -q -O - https://dl-ssl.google.com/linux/linux_signing_key.pub | apt-key add - && \
sh -c 'echo "deb [arch=amd64] http://dl.google.com/linux/chrome/deb/ stable main" >> /etc/apt/sources.list.d/google-chrome.list' && \
apt-get update -y && \
apt-get install -y google-chrome-stable xvfb poppler-utils git && \
rm -rf /var/lib/apt/lists/* && apt-get clean

COPY package.json /var/www/image-exporter/
COPY package-lock.json /var/www/image-exporter/
WORKDIR /var/www/image-exporter
RUN npm install && mkdir build
COPY bin /var/www/image-exporter/bin
COPY src /var/www/image-exporter/src

####################
# Add entrypoint script
COPY deployment/entrypoint.sh /
Expand Down
2 changes: 1 addition & 1 deletion deployment/run_server
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ pkill -9 Xvfb
pkill -9 node
pkill -9 electron

xvfb-run --auto-servernum --server-args '-screen 0 640x480x24' /var/www/image-exporter/bin/orca.js serve $REQUEST_LIMIT --safe-mode --verbose $PLOTLYJS_ARG $ORCA_IGNORECERTERRORS_ARG $@ &
xvfb-run --auto-servernum --server-args '-screen 0 1024x768x24' /var/www/image-exporter/bin/orca.js serve $REQUEST_LIMIT --safe-mode --verbose $PLOTLYJS_ARG $ORCA_IGNORECERTERRORS_ARG $@ &
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this really necessary? I thought this line: createBrowserWindowOpts['enableLargerThanScreen'] = true would make the result independent of the screen resolution 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was under the same impression, but apparently it doesn't. Should I remove that option, or let it be there?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That is unfortunate... I wonder if that is fixed in recent versions of electron (see branch pr-bump-electron-6).

Anyway, changing the resolution of the screen seems like a short-term solution: what if a user needs a bigger browser size than 1024 by 768... We're essentially putting a limit on how big a page we can print. If so, it should be clearly stated somewhere. By the way, what do you mean when you say it "works" (ie. how/what are you testing exactly)?

echo \$! > \$PIDFILE

EOF
Expand Down
28 changes: 20 additions & 8 deletions src/component/plotly-dash-preview/parse.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,17 +36,25 @@ function parse (body, req, opts, sendToRenderer) {
result.timeOut = body.timeout
result.tries = Number(result.timeOut * 1000 / cst.minInterval)

if (cst.sizeMapping[result.pdfOptions.pageSize]) {
result.browserSize = cst.sizeMapping[result.pdfOptions.pageSize]
} else if (body.pageSize && isPositiveNumeric(body.pageSize.width) &&
isPositiveNumeric(body.pageSize.height)) {
var pageSize
if (result.pdfOptions.pageSize) {
pageSize = result.pdfOptions.pageSize
} else if (body.pageSize) {
pageSize = body.pageSize
}

if (cst.sizeMapping[pageSize]) {
result.browserSize = cst.sizeMapping[pageSize]
result.pdfOptions.pageSize = pageSize
} else if (pageSize && isPositiveNumeric(pageSize.width) &&
isPositiveNumeric(pageSize.height)) {
result.browserSize = {
width: body.pageSize.width * cst.pixelsInMicron,
height: body.pageSize.height * cst.pixelsInMicron
width: pageSize.width * cst.pixelsInMicron,
height: pageSize.height * cst.pixelsInMicron
}
result.pdfOptions.pageSize = {
width: Math.ceil(body.pageSize.width),
height: Math.ceil(body.pageSize.height)
width: Math.ceil(pageSize.width),
height: Math.ceil(pageSize.height)
}
} else {
return errorOut(
Expand All @@ -57,6 +65,10 @@ function parse (body, req, opts, sendToRenderer) {
)
}

// BrowserWindow only accepts integer values:
result.browserSize['width'] = Math.ceil(result.browserSize['width'])
result.browserSize['height'] = Math.ceil(result.browserSize['height'])

sendToRenderer(null, result)
}

Expand Down
2 changes: 2 additions & 0 deletions src/component/plotly-dash-preview/render.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ function render (info, opts, sendToMain) {
const result = {}

let createBrowserWindowOpts = info.browserSize ? info.browserSize : {}
createBrowserWindowOpts['enableLargerThanScreen'] = true
createBrowserWindowOpts['useContentSize'] = true
createBrowserWindowOpts['show'] = opts.debug

let win = remote.createBrowserWindow(createBrowserWindowOpts)
Expand Down
103 changes: 67 additions & 36 deletions test/unit/plotly-dash-preview_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ const tap = require('tap')
const sinon = require('sinon')

const _module = require('../../src/component/plotly-dash-preview')
const constants = require('../../src/component/plotly-dash-preview/constants')
const remote = require('../../src/util/remote')
const { createMockWindow } = require('../common')

Expand Down Expand Up @@ -30,49 +31,79 @@ tap.test('parse:', t => {
t.end()
})
})
t.test('should error when pageSize is not given', t => {
fn({
t.test('pageSize options:', t => {
const mock = () => ({
url: 'https://dash-app.com',
selector: 'dummy'
}, {}, {}, (errorCode, result) => {
t.equal(errorCode, 400)
t.same(result.msg, 'pageSize must either be A3, A4, A5, Legal, Letter, ' +
'Tabloid or an Object containing height and width in microns.')
t.end()
})
})
t.test('should parse properly when pageSize is given', t => {
fn({
url: 'https://dash-app.com',
selector: 'dummy',
pageSize: { height: 1000, width: 1000 }
}, {}, {}, (errorCode, result) => {
t.equal(errorCode, null)

// height/width are converted from microns to pixels:
t.same(result.browserSize, {
height: 3.779527559055118,
width: 3.779527559055118

t.test('should error when not given', t => {
fn(mock(), {}, {}, (errorCode, result) => {
t.equal(errorCode, 400)
t.same(result.msg, 'pageSize must either be A3, A4, A5, Legal, Letter, ' +
'Tabloid or an Object containing height and width in microns.')
t.end()
})
t.same(result.pdfOptions.pageSize, {
height: 1000,
width: 1000
})

function assertEqualSize (browserSize, pageSize) {
// Browser size is always integer pixels
var bW = browserSize.width
var bH = browserSize.height
t.ok(Number.isInteger(bW), 'browserSize.width is not an integer')
t.ok(Number.isInteger(bH), 'browserSize.height is not an integer')
var pW, pH
if (constants.sizeMapping[pageSize]) {
var equivalentPixelSize = constants.sizeMapping[pageSize]
pW = equivalentPixelSize.width
pH = equivalentPixelSize.height
} else {
pW = pageSize.width * constants.pixelsInMicron
pH = pageSize.height * constants.pixelsInMicron
}
// Round
pW = Math.ceil(pW)
pH = Math.ceil(pH)
t.equal(bW, pW, 'browser and page should have the same width')
t.equal(bH, pH, 'browser and page should have the same height')
}

// Browser size and page size should be the same assuming a DPI of 96
// to make sure plotly.js figures are appropriately sized right away for print
[
[true, { height: 1000, width: 1000 }],
[true, 'Letter'],
[false, { height: 1000, width: 1000 }],
[false, 'Letter']
].forEach(arg => {
var toplevel = arg[0]
var pageSize = arg[1]
t.test(`should size window and page properly when ${toplevel ? '' : 'pdf_options.'}pageSize is given`, t => {
var body = mock()
if (toplevel) {
body.pageSize = pageSize
} else {
body.pdf_options = { pageSize: pageSize }
}
fn(body, {}, {}, (errorCode, result) => {
t.equal(errorCode, null)
t.same(result.pdfOptions.pageSize, pageSize)
assertEqualSize(result.browserSize, result.pdfOptions.pageSize)
t.end()
})
})
t.end()
})
})
t.test('should parse properly when pdf_options are given', t => {
fn({
url: 'https://dash-app.com',
selector: 'dummy',
pdf_options: { pageSize: 'Letter', marginsType: 1 }
}, {}, {}, (errorCode, result) => {
t.equal(errorCode, null)
// height/width are converted to pixels from page-type:
t.same(result.browserSize, { height: 1056, width: 816 })
t.same(result.pdfOptions, { pageSize: 'Letter', marginsType: 1 })
t.end()

t.test('should passthrough pdf_options', t => {
var body = mock()
body.pdf_options = { pageSize: 'Letter', marginsType: 1, crazyOptions: true }
fn(body, {}, {}, (errorCode, result) => {
t.equal(errorCode, null)
t.same(result.pdfOptions, { pageSize: 'Letter', marginsType: 1, crazyOptions: true })
t.end()
})
})
t.end()
})

t.end()
Expand Down