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

Jump to bottom

Fix HTML capture race conditions #580

Merged
merged 10 commits into from
Mar 25, 2020
Merged

Fix HTML capture race conditions #580

merged 10 commits into from
Mar 25, 2020

Conversation

tresf
Copy link
Contributor

@tresf tresf commented Feb 11, 2020 •
edited
Loading

Creating a PR because initial tests are passing.

Closes #547 #32

Test results:

Windows Mac Linux
AdoptOpenJDK11 ✅ ✅ ✅
Oracle Java 8 ✅ ✅ 🚫

@tresf tresf added this to the 2.0.12 milestone Feb 11, 2020
tresf
tresf commented Feb 11, 2020
View reviewed changes
src/qz/printer/action/WebApp.java Outdated
log.debug("Attempting image capture");

SnapshotParameters sparams = new SnapshotParameters();
sparams.setTransform(new Scale(pageZoom, pageZoom));
Copy link
Contributor Author

@tresf tresf Feb 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

On Java 8 and Java 11 using 6x zoom on Mac, it appears to just make the picture bigger without improving quality... :(

image

Berenz added 2 commits March 11, 2020 20:44
Wrap snapshot in animation timer to ensure more stable render
8d8ee82 
+ cleanup unnecessary code
+ address jfx8 default size behavior
Add test class for webapp captures
f0a0a9b
@tresf
Copy link
Contributor Author

tresf commented Mar 12, 2020

@bberenz Tests are looking great so far.

Some requests:

  • Avoid the color white as a background color
  • Test the bottom right pixel to make sure the render is complete

Here's Java 11 + Gluon on macOS. I can't find an issue at a glance with any of the renders.

ezgif-2-7ffa84ff649b

@tresf
Copy link
Contributor Author

tresf commented Mar 12, 2020

I just realized that the zoom is still using the SnapshotParameters logic, which is not correct. #580 (comment) so my test results are not usable.

tresf and others added 2 commits March 12, 2020 12:56
@tresf
Bring back zoom through reflection
33f6e2d 
Remove SnapshotParameters technique for now
Correct starting timeout
d55ee8e
@klabarge
Copy link
Member

I sent over 300 print jobs and all print jobs completed. I didn't hit the race condition using this branch

However, I did notice that the HTML rendering is much more noticeable

Screen Recording 2020-03-24 at 8 48 13 PM

@tresf
Copy link
Contributor Author

tresf commented Mar 25, 2020

However, I did notice that the HTML rendering is much more noticeable

I tried to fix it by making the stage smaller, but it results in more partial snapshots. I guess that won't improve until #576 is completed. 🤞

Merging.

@tresf tresf merged commit 74c4c8e into 2.0 Mar 25, 2020
@tresf tresf deleted the 2.0-html-capture branch March 25, 2020 04:14
@tresf tresf mentioned this pull request Apr 22, 2020
Closed
@tresf tresf mentioned this pull request Apr 30, 2020
Closed
@tresf tresf mentioned this pull request May 7, 2020
Merged
tresf added a commit that referenced this pull request May 26, 2020
@tresf
Add Monocle support (#632)
409be26 
Add Monocle support, performance improvements
This is a 2.1 refactor of #580

Co-authored-by: Berenz <thebrettberenz@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akberenz akberenz akberenz left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.0.12
Development

Successfully merging this pull request may close these issues.
3 participants
@tresf @klabarge @akberenz