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(cli): smarter info window hiding logic #482

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

PeculiarProgrammer
Copy link
Contributor

@PeculiarProgrammer PeculiarProgrammer commented Oct 22, 2024

Fixes Issue

  • The full-screen option used to break with the info window, which this fixes.

Closes #327

Description

  • The --hide-info-window argument now accepts three options: auto, always, and never. Auto hides the info window if there is only one monitor.
  • By default, the presentation will now go to monitor 0, and the info window will go to monitor 1, preventing them from being placed on the same monitor.

Check List

Check all the applicable boxes:

  • I understand that my contributions needs to pass the checks;
  • If I created new functions / methods, I documented them and add type hints;
  • If I modified already existing code, I updated the documentation accordingly;
  • The title of my pull request is a short description of the requested changes.

Note to reviewers

In full-screen mode prior to this, the info window would not be in full screen. It turns out that the method putting the info window into full-screen was being executed prior to the window appearing, so it wouldn't take effect. I fixed this by making the info window full-screen in the player-window's scope instead. Noting this because the change seems unnecessary otherwise.

PeculiarProgrammer and others added 2 commits October 21, 2024 20:11
This changes the ``--hide-info-window-`` command to accept three options: auto, always, and never. Auto will hide the info window if there is only one screen. This also fixes the --full-screen option by moving the info window to monitor 1 and the presentation to monitor 0 by default.
Copy link

codecov bot commented Oct 22, 2024

Codecov Report

Attention: Patch coverage is 46.42857% with 15 lines in your changes missing coverage. Please review.

Project coverage is 80.36%. Comparing base (988011f) to head (8329a88).

Files with missing lines Patch % Lines
manim_slides/present/player.py 14.28% 12 Missing ⚠️
manim_slides/present/__init__.py 78.57% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #482      +/-   ##
==========================================
- Coverage   80.96%   80.36%   -0.60%     
==========================================
  Files          23       23              
  Lines        1886     1905      +19     
==========================================
+ Hits         1527     1531       +4     
- Misses        359      374      +15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Owner

@jeertmans jeertmans left a comment

Choose a reason for hiding this comment

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

Hi @PeculiarProgrammer, thanks for your contribution! It looks excellent!

I just had a small comment, but it looks fine otherwise :-)

manim_slides/present/__init__.py Outdated Show resolved Hide resolved
@jeertmans jeertmans changed the title Adds features from #327 feat(cli): smarter info window hiding logic Oct 22, 2024
@jeertmans jeertmans added enhancement New feature or request present Related to the main "present" feature cli Related to the command line interface labels Oct 22, 2024
@jeertmans
Copy link
Owner

@PeculiarProgrammer I have pushed some fixes / cleanups.

However, I have an issue when I try to set the screen, mainly problems:

  • if I run in full-screen -F, then both window are displayed on two different screens, but not when I don't toggle -F, even though it is still supposed to be displayed on two screens;
  • if I run in full-screen and choose a --screen number, the main and info windows are sometimes put on the same screen (even though the code says the opposite...);

In general, it looks like the logic to put windows on specific screens is broken.

@PeculiarProgrammer
Copy link
Contributor Author

Those issues you described only appear in the code you recently pushed. Reverting back to the older logic fixes them. I combined some of your additions with the older logic and refactored my code.

@jeertmans
Copy link
Owner

Those issues you described only appear in the code you recently pushed. Reverting back to the older logic fixes them. I combined some of your additions with the older logic and refactored my code.

I will check that tomorrow when I have access to a second monitor :-)

@PeculiarProgrammer
Copy link
Contributor Author

I will check that tomorrow when I have access to a second monitor :-)

Have you checked yet?

@jeertmans
Copy link
Owner

My apologies for the late reply, and thanks for the ping @PeculiarProgrammer!

It looks like your patch works, except for one thing: the main video seems to be frozen.

When I check out the main (jeertmans/main) branch, I have no issue, see screenshot below.

image

However, on your branch, the video is stuck at some frame, and skipping the slide does not change anything. The info window does update correctly.

image

Do you also observe this bug? I am using the BasicExample from example.py.

@PeculiarProgrammer
Copy link
Contributor Author

I can't seem to replicate that. I was also using the BasicExample from example.py but on Windows. Perhaps it's an issue with your Linux distro? I'll try to replicate it on Ubuntu later today.

@jeertmans
Copy link
Owner

I can't seem to replicate that. I was also using the BasicExample from example.py but on Windows. Perhaps it's an issue with your Linux distro? I'll try to replicate it on Ubuntu later today.

Maybe, I had some issues when upgrading to Ubuntu 24.04, also setting CUDA libraries (which could have broken some QT video-related features I think), and only pyside6==6.5.2 works.

I will see when I have more time to see if I can first fix installation issues, to later verify that this patch doesn't introduce new bugs.

@PeculiarProgrammer
Copy link
Contributor Author

PeculiarProgrammer commented Nov 6, 2024

I am currently testing this on Ubuntu (22.04.3) and found that the slides are freezing on both main and enhance-presenter-view when pyside6 is anything above 6.5.2.

Edit: I have since found the error occurring inconsistently. It seems to happen most frequently when the full screen flag is given (either branch).

@jeertmans
Copy link
Owner

I am currently testing this on Ubuntu (22.04.3) and found that the slides are freezing on both main and enhance-presenter-view when pyside6 is anything above 6.5.2.

Edit: I have since found the error occurring inconsistently. It seems to happen most frequently when the full screen flag is given (either branch).

Yeah, it is a bit of a curse that changing Qt versions just seems to break the presenter, while there is nothing documented about that releases that could help us anticipate this.

I will see what I can do, but it is hard to maintain support for Qt :-/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli Related to the command line interface enhancement New feature or request present Related to the main "present" feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] Enhance presenter view
2 participants