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.

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

feat: add a config to enable retina quality images in screenshots #17409

Merged
merged 3 commits into from
Nov 15, 2021

Conversation

eschutho
Copy link
Member

@eschutho eschutho commented Nov 11, 2021

SUMMARY

Adding support for configuring image quality for screenshots in both Chrome and Firefox.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

Update the config override with a pixel_density of 2. Create a report with an email and check the filesize for a 2x size. It should still be captured at the same width as declared in the config, but at a higher resolution. The email will also display the image at 1000px as per the html.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@@ -409,12 +410,13 @@ def _try_json_readsha(filepath: str, length: int) -> Optional[str]:
# This could cause the server to run out of memory or compute.
"ALLOW_FULL_CSV_EXPORT": False,
"UX_BETA": False,
"SCREENSHOTS_USE_RETINA_HIRES": False
Copy link
Member Author

@eschutho eschutho Nov 11, 2021

Choose a reason for hiding this comment

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

this is the only change in this file... the rest is automated linting.

Copy link
Member

Choose a reason for hiding this comment

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

Nit, trailing comma:

Suggested change
"SCREENSHOTS_USE_RETINA_HIRES": False
"SCREENSHOTS_USE_RETINA_HIRES": False,

@eschutho eschutho changed the title feature: add a feature flag to enable retina quality images in screenshots feat: add a feature flag to enable retina quality images in screenshots Nov 11, 2021
@codecov
Copy link

codecov bot commented Nov 11, 2021

Codecov Report

Merging #17409 (131467e) into master (aa8040e) will decrease coverage by 0.31%.
The diff coverage is 55.65%.

❗ Current head 131467e differs from pull request most recent head 8604cad. Consider uploading reports for the commit 8604cad to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17409      +/-   ##
==========================================
- Coverage   77.04%   76.73%   -0.32%     
==========================================
  Files        1041     1042       +1     
  Lines       56079    56255     +176     
  Branches     7742     7784      +42     
==========================================
- Hits        43204    43165      -39     
- Misses      12617    12831     +214     
- Partials      258      259       +1     
Flag Coverage Δ
hive ?
mysql 81.95% <80.50%> (+0.01%) ⬆️
postgres 81.96% <80.50%> (+0.01%) ⬆️
presto ?
python 82.04% <80.50%> (-0.40%) ⬇️
sqlite 81.64% <80.50%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
.../src/dashboard/components/gridComponents/Chart.jsx 78.37% <ø> (ø)
...ols/FilterControl/AdhocFilterEditPopover/index.jsx 64.91% <ø> (-1.19%) ⬇️
...ols/MetricControl/AdhocMetricEditPopover/index.jsx 86.42% <ø> (-0.20%) ⬇️
...filters/components/GroupBy/GroupByFilterPlugin.tsx 0.00% <0.00%> (ø)
superset/common/query_actions.py 92.85% <ø> (ø)
superset/common/query_context.py 91.85% <ø> (+0.90%) ⬆️
superset/connectors/base/models.py 88.67% <ø> (+0.31%) ⬆️
superset/db_engine_specs/hive.py 69.49% <0.00%> (-16.99%) ⬇️
...olumnSelectControl/DndColumnSelectPopoverTitle.jsx 3.84% <3.84%> (ø)
...ols/DndColumnSelectControl/ColumnSelectPopover.tsx 12.96% <10.52%> (-3.17%) ⬇️
... and 30 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa8040e...8604cad. Read the comment docs.

Copy link
Member

@betodealmeida betodealmeida left a comment

Choose a reason for hiding this comment

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

LGTM, but what's the workflow here? Doesn't this depend on the device that's loading the screenshot? Is there a way to send both resolutions (1x and 2x) in a report and let the device choose one?

@@ -409,12 +410,13 @@ def _try_json_readsha(filepath: str, length: int) -> Optional[str]:
# This could cause the server to run out of memory or compute.
"ALLOW_FULL_CSV_EXPORT": False,
"UX_BETA": False,
"SCREENSHOTS_USE_RETINA_HIRES": False
Copy link
Member

Choose a reason for hiding this comment

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

Nit, trailing comma:

Suggested change
"SCREENSHOTS_USE_RETINA_HIRES": False
"SCREENSHOTS_USE_RETINA_HIRES": False,

@eschutho
Copy link
Member Author

eschutho commented Nov 11, 2021

LGTM, but what's the workflow here? Doesn't this depend on the device that's loading the screenshot? Is there a way to send both resolutions (1x and 2x) in a report and let the device choose one?

Yeah, good question, so I'm using the term retina very loosely. It's basically a higher res image. The rendered image size in the html is 1000px, and the slice for example defaults at 3000px wide image capture, which will place a 3x resolution image in the html. For people who are trying to read the image in the email itself, the text will likely be so small that it's unreadable, so they'll opt to open the image as an attachment and zoom in.

If you want to take a screenshot at a lower window size so that you can maximize text readability, but still allow people to zoom into the image (some email applications allow you to zoom in in the html and some force you to download the attachment, while some don't let you zoom in at all) then you would set your WEBDRIVER_WINDOW config to something smaller like 1400px or 1700px and then increase the screen capture resolution (2x).

Another option instead of a feature flag is to make this a config and set it at 1x as a default and allow people to set the ratio themselves. At 2x we're seeing fair quality results but as devices get more pixels (I think we may be up to 4x now) we may want to increase the resolution.

But to answer the question, no there's no way (that I know of) with email to let the device decide which one it wants to load.

@eschutho eschutho force-pushed the elizabeth/retina-feature-flag branch from 0036eea to 7b183fd Compare November 12, 2021 18:12
@@ -1084,7 +1094,8 @@ def SQL_QUERY_MUTATOR( # pylint: disable=invalid-name,unused-argument
WEBDRIVER_TYPE = "firefox"

# Window size - this will impact the rendering of the data
WEBDRIVER_WINDOW = {"dashboard": (1600, 2000), "slice": (3000, 1200)}
WEBDRIVER_WINDOW = {"dashboard": (
1600, 2000), "slice": (3000, 1200), "pixel_density": 1}
Copy link
Member Author

Choose a reason for hiding this comment

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

this is the only manual change.. the rest is automated linting.

@eschutho eschutho changed the title feat: add a feature flag to enable retina quality images in screenshots feat: add a config to enable retina quality images in screenshots Nov 12, 2021
@eschutho eschutho force-pushed the elizabeth/retina-feature-flag branch from 7b183fd to 1e3ed3c Compare November 12, 2021 23:17
@eschutho
Copy link
Member Author

So after leaving the last comment, I convinced myself to go with the config route, b/c it will be more flexible.

@eschutho eschutho force-pushed the elizabeth/retina-feature-flag branch from 1e3ed3c to d5af66a Compare November 13, 2021 00:42
@eschutho eschutho force-pushed the elizabeth/retina-feature-flag branch 2 times, most recently from e568c1a to bcaaa19 Compare November 13, 2021 01:02
@eschutho eschutho force-pushed the elizabeth/retina-feature-flag branch from bcaaa19 to 42027f1 Compare November 13, 2021 01:49
Copy link
Member

@betodealmeida betodealmeida left a comment

Choose a reason for hiding this comment

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

This is great!

@eschutho eschutho force-pushed the elizabeth/retina-feature-flag branch from 3954e7d to 8604cad Compare November 15, 2021 18:39
@pull-request-size pull-request-size bot added size/S and removed size/M labels Nov 15, 2021
@eschutho eschutho merged commit 3ee9e11 into apache:master Nov 15, 2021
@eschutho eschutho deleted the elizabeth/retina-feature-flag branch November 15, 2021 20:47
sadpandajoe pushed a commit to preset-io/superset that referenced this pull request Nov 17, 2021
…ache#17409)

* add feature flag for retina screenshot support

* use config for pixel density

* run black

(cherry picked from commit 3ee9e11)
@sadpandajoe
Copy link
Member

🏷️ 2021.46

AAfghahi pushed a commit that referenced this pull request Jan 10, 2022
…7409)

* add feature flag for retina screenshot support

* use config for pixel density

* run black
@eschutho
Copy link
Member Author

This fixes a portion of #13954

@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels preset:2021.46 preset-io size/S 🚢 1.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants