Fixed the screenshot flickering issue #740
Conversation
WalkthroughThis update optimizes the browser setup by dynamically adjusting window size and position according to screen resolution and platform-specific offsets. It includes a new utility module for screen resolution, modifies context creation to disable viewport for better anti-detection, and refactors screenshot tests for enhanced reliability and compatibility across operating systems. Changes
Entelligence.ai can learn from your feedback. Simply add 👍 / 👎 emojis to teach it your preferences. More shortcuts belowEmoji Descriptions:
Interact with the Bot:
|
| from screeninfo import get_monitors # Cross-platform library | ||
| monitor = get_monitors()[0] # Get primary monitor | ||
| return {"width": monitor.width, "height": monitor.height} |
There was a problem hiding this comment.
Using get_monitors()[0] without checking list length could raise IndexError if no monitors are detected. Should add length check before accessing first element.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| from screeninfo import get_monitors # Cross-platform library | |
| monitor = get_monitors()[0] # Get primary monitor | |
| return {"width": monitor.width, "height": monitor.height} | |
| from screeninfo import get_monitors # Cross-platform library | |
| monitors = get_monitors() | |
| if not monitors: | |
| return {"width": 0, "height": 0} # Default fallback if no monitors | |
| monitor = monitors[0] # Get primary monitor | |
| return {"width": monitor.width, "height": monitor.height} |
There was a problem hiding this comment.
Is the default 0, 0 though? I mean it's better than an index error, I guess, but if we pass back 0, 0 is that just going to break things or will it actually default to the pre-existing 1280x720? If not, does it cause an error? If so, then could/should we not just return 1280x720 for the sake of debugging logic flow?
There was a problem hiding this comment.
default screen size is return {"width": 1920, "height": 1080}, it is standard for most laptops and monitors.
here's a list of standard screen sizes:
- 1920 × 1080 (Full HD, 1080p) Most laptops & monitors (standard)
- 1366 × 768 Older laptops, budget displays
- 2560 × 1440 (1440p, QHD) High-end monitors, gaming setups
- 3840 × 2160 (4K UHD) Premium displays, large screens
- 1280 × 800 Some older MacBooks
- 2560 × 1600 Modern MacBooks (MacBook Pro)
- 3440 × 1440 Ultrawide monitors for gaming/productivity
but yes i'll add better error handling because in virtual environments, we can get indexerror.
fyi, 0,0 is default return for get_window_adjustments() function. it is the default one already present in code.
|
@gregpr07 @MagMueller pls review this PR |
|
@svaze30 I am glad it worked for you. I hope this PR gets merged. |
|
This works perfectly. 👍 |
|
|
| f'--window-position={offset_x},{offset_y}', | ||
| f'--window-size={screen_size["width"]},{screen_size["height"]}', |
There was a problem hiding this comment.
| f'--window-position={offset_x},{offset_y}', | |
| f'--window-size={screen_size["width"]},{screen_size["height"]}', | |
| '--start-maximized', |
Why not just start maximized?
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
@PaperBoardOfficial for context they are only 2 guys and they got nearly 50 thousand stars and hundreds of thousands of messages within the span of a month, while actively fundraising setting up their company. I am working with them to go through all the PRs and give everyone the responses they deserve currently!
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Ok I understand, I know it sucks when you put in a bunch of work and get no recognition. All I can say is don't take it too personally, no one is trying to ghost you intentionally, there are just tons of competing priorities.
Issue link:
#673
#663
If the user's monitor size is different from the default viewport size of playwright(which is 1280x720, then the playwright would resize the browser viewport to take screenshot.
I added the screen_resolution.py to dynamically set the playwright window size and window position using screen_info library.
Also, the screenshot_test.py file wasn't working, so i fixed it.