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.

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

/dash-preview: Fix window size #250

Merged
merged 8 commits into from
Sep 30, 2019
Merged

/dash-preview: Fix window size #250

Show file tree
Hide file tree
Changes from 6 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
4 changes: 4 additions & 0 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 @@ -57,6 +57,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
4 changes: 2 additions & 2 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 @@ -51,8 +51,8 @@ tap.test('parse:', t => {

// height/width are converted from microns to pixels:
t.same(result.browserSize, {
height: 3.779527559055118,
width: 3.779527559055118
height: 4,
width: 4
})
t.same(result.pdfOptions.pageSize, {
height: 1000,
Expand Down