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

Multi monitor feature for video #153

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

xaprier
Copy link

@xaprier xaprier commented Jun 8, 2024

Added feature for issue #46

  • Added right click context menu for items in iconview and for apply button.
    • Each monitor added to context menu to set which monitor to set and also "Set All" option.
  • Added option for mute audio when there is maximized window(just mute, no stop)
  • Used from screeninfo import get_monitors for getting monitor infos. Indexes same as videos from configs.
  • Updated default config for setting multi monitor. JSON file kept like
    "data_source": [
       "/home/xaprier/Videos/Hidamari/ironman2.mp4",
       "/home/xaprier/Videos/Hidamari/ironman1.mp4"
    ],
    

- Created a menu when user clicked on apply to set to which monitor
- Updated VideoPlayer to work well for multi monitor support.
Only tested with specific version of screeninfo
- Added option to mute audio when focused maximized/fullscreen instance
- Added right click context menu to selection of monitor for each item
- Update meson file to add monitor.py
@xaprier
Copy link
Author

xaprier commented Jun 8, 2024

1 monitor and 3 monitor should be tested.

- Fixed bug when mute when maximized option is enabled. There was no
  voice although it minimized window on first startup.
@jeffshee
Copy link
Owner

jeffshee commented Jun 9, 2024

Hi @xaprier, thanks for the PR! 🤗
I'll do some code review and testing. It might take some time until the merge so please bear with me~

requirements.txt Outdated Show resolved Hide resolved
src/monitor.py Outdated Show resolved Hide resolved
src/monitor.py Outdated Show resolved Hide resolved
@xaprier xaprier requested a review from jeffshee June 9, 2024 13:05
@xaprier
Copy link
Author

xaprier commented Jun 9, 2024

I tested last implementation of monitors and it worked for me.

@xaprier
Copy link
Author

xaprier commented Jun 14, 2024

I found a bug with "mute when maximized" option, but i cannot understand why and how to solve it for right now. Fixing if we apply wallpaper again but if starts automatically when launched its not working.

@xaprier
Copy link
Author

xaprier commented Jun 15, 2024

Also if we use stream and webpage feature, it will broke with changing config.

@jeffshee
Copy link
Owner

Yes, you are right. There are more aspects that need to be reconsidered regarding the changes in the config, including:

  • The migration from the previous config format to the current one, so that users don't need to reconfigure everything after the update.
  • The behavior of audio playback: should all videos output sound when there are multiple sources configured?
  • The behavior when the number of monitors changes:
    • Is the player spawned/terminated properly when a monitor is added/removed?
    • When a new monitor is added, which video source should it use?
  • The impact/regression of this PR
  • etc

I might push some commits directly to the PR branch if necessary 😃

@jeffshee
Copy link
Owner

The current implementation doesn't work when the monitor is added dynamically.
Perhaps the video sources in config should be a dict instead of an array, where the key is the name of the monitor (except the default, which is the video source used when a new (unconfigured) monitor is added):

{
  "default": "/path/to/video_set_as_default.mp4",
  "eDP1": "/path/to/video_set_as_edp1.mp4",
  "eDP2": "/path/to/video_set_as_edp2.mp4",
}

Just an idea in my head right now 😉

@xaprier
Copy link
Author

xaprier commented Jun 16, 2024

  • The behavior of audio playback should be the primary monitor if sound is not muted. The others will not play like now.
  • The last applied video must be default i think, and if there will be added monitors, automatically it will set to default. We must watch the system to keep track of adding monitors.
  • Config approach will be better with your saying. But we should consider if there will no monitor with key of config also.
    • Stream and web player should set default value i think, or we can add this feature to them also which is i would not recommend it.

@xaprier
Copy link
Author

xaprier commented Jun 16, 2024

I will updating code with our discussion also. I'm just busy nowadays. But I will try to take a time

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.

None yet

2 participants