Skip to content

Conversation

jsade
Copy link
Contributor

@jsade jsade commented Sep 22, 2025

Summary

I needed a windowed presentation option for video calls, so added a new settings toggle and the necessary conditional presentation logic.

Full screen presentations must be explicitly disabled by the user via settings:

Screenshot 2025-09-22 at 15 11 30

Implementation Details

  • Added fullscreenPresentationEnabled setting (default true) with UI toggle and descriptive copy so presenters can disable automatic fullscreen.
  • Presentation extension now tracks presentationUsesFullscreen per session, conditionally requesting fullscreen and setting up modal observer / onfullscreenchange only when fullscreen is requested.
  • Escape key handling added to the wrapper onkeydown listener, invoking endPresentation even in windowed mode; endPresentation now early-returns when already exited to avoid double teardown.
  • Wrapped fullscreen request in try/catch to gracefully fall back to windowed mode in the unlikely event the browser blocks fullscreen, and updated teardown logic to skip fullscreen cleanup when already windowed.

How to Test

  1. Ensure Enter fullscreen while presenting is enabled, start a presentation, confirm Obsidian requests fullscreen and ESC exits both fullscreen and presentation cleanly.
  2. Disable Enter fullscreen while presenting, start a presentation, confirm canvas stays windowed, keyboard navigation still works, and exiting with ESC or the End Presentation command restores canvas state.
  3. Simulate requestFullscreen rejection (this is somewhat tricky to test and a corner case, but e.g. in devtools you can temporarily override the wrapper’s requestFullscreen to throw) and start a presentation; verify it remains windowed, Escape still exits, and teardown has no errors.

@Copilot Copilot AI review requested due to automatic review settings September 22, 2025 12:55
Copilot

This comment was marked as spam.

jsade and others added 2 commits September 22, 2025 16:14
Removes redundant defensives.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Removes redundant defensives.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@Developer-Mike
Copy link
Owner

Thank you for your PR. I'll take a look at it hopefully by next weekend.

I'm sorry for the copilot stuff. I've never enabled anything related to it. Crazy how they shove it down your throat 😕.

@jsade
Copy link
Contributor Author

jsade commented Sep 22, 2025

Eh, I think the copilot review might've been triggered by my default settings which I didn't adjust for this repo, so can only blame myself 😅

No rush with the review though, thanks!

Copy link
Owner

@Developer-Mike Developer-Mike left a comment

Choose a reason for hiding this comment

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

Changes can be made by me 👍🏻. Thank you for your PR!

@Developer-Mike Developer-Mike merged commit 379d096 into Developer-Mike:main Sep 28, 2025
@Developer-Mike Developer-Mike added this to the 5.6.0 milestone Sep 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants