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

Add Windows Media Player Warning #2683

Open
wants to merge 13 commits into
base: dev
Choose a base branch
from

Conversation

onesounds
Copy link
Contributor

@onesounds onesounds commented May 1, 2024

What's the PR

image

  • Fix BUG: Sound Effect broken on v1.18 #2682

  • If WMP doesn't exist on your system and can't adjust volume, it will warn and guide you beforehand.

  • Check if the executable of Windows Media Player(Legacy) exists by reading registry and turn it on/off.

    • Fallback to use SoundPlayer to play sound if there no WMP

More Info

  • After merging feat: Add sound effect volume #2488, Multiple “sound not playing” issues reported
  • I investigated and found that these are users who have uninstalled WMP.
  • I looked into it and found that many people were uninstalling wmp. (Many posts offer this as a tip - the system warns you that there may be a problem, but many people ignore it)
    • soundplay() is not volume adjustable, and mediaplayer() is WMP-based, so it won't play without WMP.
    • I decided this was better than adding a library for WAV playback.
    • WMPlib is in system32 and there seems to be a way to utilize it, but I don't want to bother investigating further.

I am upset with the following situation 🤬

  • .NET uses WMP for sound play.
  • .NET doesn't provide a volume property for soundplay(). Who in the world decides not to put a volume property in a sound play function?
  • There are many posts telling people to delete WMP that is utilized in their system.
  • Despite the warnings on the system, people delete WMP.
  • We are the ones who suffer from all these problems.

@Yusyuriv Yusyuriv self-assigned this May 2, 2024
@Garulf
Copy link
Member

Garulf commented May 2, 2024

Would this be better?

"Windows Media Player is unavailable and is required for sound effects. Please check your installation."

Co-authored-by: VictoriousRaptor <10308169+VictoriousRaptor@users.noreply.github.com>
@taooceros
Copy link
Member

maybe try something like this https://github.com/mobiletechtracker/NetCoreAudio?

@Yusyuriv
Copy link
Member

Yusyuriv commented May 3, 2024

Adding a new dependency to play a single sound sounds wrong to me.

@onesounds
Copy link
Contributor Author

onesounds commented May 15, 2024

  • Change to use soundplay to play sound if there no WMP
  • Change the warning message to "Windows Media Player is unavailable and is required for volume adjust. Please check your WMP installation".
  • Warning is hidden by default. If there is no WMP, it will be visible.

@onesounds onesounds marked this pull request as ready for review May 16, 2024 06:17
@onesounds onesounds added bug Something isn't working kind/ui related to UI, icons, themes, etc labels May 16, 2024
@onesounds
Copy link
Contributor Author

  • Add disable the volume control when without WMP
    • Disabled only the control, when it should have correctly output as disabling the entire card. Apply this after completing the SPLIT SETTINGS. (I don't know if there is a disable style)
  • Issue reporters have reported that this PR is working.

@taooceros
Copy link
Member

@onesounds do you want to abstract this piece out to a usercontrol?

@jjw24 jjw24 added this to the 1.19.0 milestone May 17, 2024
@jjw24
Copy link
Member

jjw24 commented May 17, 2024

I am going to enable squash and merge into one commit in case we need to get this out sooner.

@jjw24 jjw24 enabled auto-merge (squash) May 17, 2024 05:37
@onesounds
Copy link
Contributor Author

@onesounds do you want to abstract this piece out to a usercontrol?

not now.

Copy link
Member

@jjw24 jjw24 left a comment

Choose a reason for hiding this comment

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

@VictoriousRaptor can we move the check media player even higher, rather than when main window initializes maybe when we actually starting up the application? Put the check result as a constant variable maybe in https://github.com/Flow-Launcher/Flow.Launcher/blob/dev/Flow.Launcher.Infrastructure/Constant.cs or user settings class (but don't persist it to disk).

If user reinstalls the media player to fix the problem they can restart flow.

@onesounds
Copy link
Contributor Author

Need to work on UI to match split setting pr. Will be worked on after split setting merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working kind/ui related to UI, icons, themes, etc
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

BUG: Sound Effect broken on v1.18
6 participants