Skip to content

Add Windows Media Player Warning #2683

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 17 commits into from
May 23, 2024
Merged

Add Windows Media Player Warning #2683

merged 17 commits into from
May 23, 2024

Conversation

@onesounds
Copy link
Contributor

@onesounds onesounds commented May 1, 2024 •
edited by VictoriousRaptor
Loading

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
VictoriousRaptor
VictoriousRaptor reviewed May 2, 2024
View reviewed changes
@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."

@taooceros
Copy link
Member

taooceros commented May 3, 2024

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 •
edited
Loading

  • 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

onesounds commented May 16, 2024

  • 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

taooceros commented May 17, 2024

@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 commented May 17, 2024

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

not now.

@VictoriousRaptor VictoriousRaptor disabled auto-merge May 19, 2024 11:56
jjw24
jjw24 reviewed May 20, 2024
View reviewed changes
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

onesounds commented May 22, 2024

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

Yusyuriv
Yusyuriv previously approved these changes May 23, 2024
View reviewed changes
VictoriousRaptor
VictoriousRaptor approved these changes May 23, 2024
View reviewed changes
Flow.Launcher/SettingWindow.xaml.cs Outdated
{
if (Keyboard.FocusedElement is not TextBox textBox)
{
if (Keyboard.FocusedElement is not TextBox textBox)
Copy link
Contributor

@VictoriousRaptor VictoriousRaptor May 2, 2024

Choose a reason for hiding this comment

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

Is this intentional?

Flow.Launcher/MainWindow.xaml.cs Outdated
private void SoundPlay()
{

if (!File.Exists(Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.ProgramFiles), "Windows Media Player", "wmplayer.exe")))
Copy link
Contributor

@VictoriousRaptor VictoriousRaptor May 19, 2024

Choose a reason for hiding this comment

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

Do we really need to check on every toggle? IMO just to check on startup.

@VictoriousRaptor VictoriousRaptor merged commit 5afce95 into Flow-Launcher:dev May 23, 2024
@jjw24 jjw24 mentioned this pull request Aug 14, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jjw24 jjw24 jjw24 left review comments

@VictoriousRaptor VictoriousRaptor VictoriousRaptor approved these changes

@Yusyuriv Yusyuriv Yusyuriv left review comments

@taooceros taooceros Awaiting requested review from taooceros

Assignees

@Yusyuriv Yusyuriv

@onesounds onesounds

@VictoriousRaptor VictoriousRaptor

Labels

bug Something isn't working kind/ui related to UI, icons, themes, etc

Projects

Work In Progress
Status: Done

Milestone

1.19.0

Development

Successfully merging this pull request may close these issues.

BUG: Sound Effect broken on v1.18

6 participants

@onesounds @Garulf @taooceros @Yusyuriv @jjw24 @VictoriousRaptor