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

Fix #5504: invalid warning #5509

Merged

Conversation

thmueller64
Copy link
Contributor

@thmueller64 thmueller64 commented May 20, 2020

Fixes the invalid warning about "dummy-MIDI-client" mentioned in #5504. The implementation is similar to Mixer::tryAudioDevices, like mentioned in the issue, but prints the precise device name that failed.

@LmmsBot
Copy link

@LmmsBot LmmsBot commented May 20, 2020

🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩

Windows

🤖
{"platform_name_to_artifacts": {"Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/aj3l68xie5gpuj5g/artifacts/build/lmms-1.2.1-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/33066770"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/dnd2gcqojxqujikg/artifacts/build/lmms-1.2.1-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/33066770"}]}, "commit_sha": "109cdf6cf3c3ee3c40d228cb694da2521ac5b689"}

@Spekular
Copy link
Contributor

@Spekular Spekular commented May 20, 2020

Tested:

  • ALSA Sequencer (working): Neither version prints any warnings
  • ALSA Raw (failing): Both versions print three lines of warnings, but this PR is more specific on the third
    • Master: Couldn't create MIDI-client, neither with ALSA nor with OSS. Will use dummy-MIDI-client.
    • PR: Couldn't create ALSA Raw-MIDI (Advanced Linux Sound Architecture) MIDI-client. Will use dummy-MIDI-client.
  • Dummy: Master prints a generic warning, this PR prints no warning

So, it makes the error message more specific, and gets rid of it when irrelevant. Seems to fix the issue from what I can tell. Code LGTM as well.

@JohannesLorenz
Copy link
Contributor

@JohannesLorenz JohannesLorenz commented May 22, 2020

Thanks for fixing and reviewing. As this is just a few printfs and some if/else, is everyone OK if we rebase this to stable-1.2?

@thmueller64 thmueller64 changed the base branch from master to stable-1.2 May 23, 2020
@thmueller64 thmueller64 changed the base branch from stable-1.2 to master May 23, 2020
@thmueller64 thmueller64 force-pushed the fix-dummy-midi-client-message branch from 2c022d8 to 109cdf6 Compare May 23, 2020
@JohannesLorenz
Copy link
Contributor

@JohannesLorenz JohannesLorenz commented May 23, 2020

I think you force-pushed right, but we probably still need to tell github that we want to merge into stable-1.2 (by clicking the "edit" button on top right of your PR).

@thmueller64 thmueller64 force-pushed the fix-dummy-midi-client-message branch from 109cdf6 to 2c022d8 Compare May 23, 2020
@thmueller64 thmueller64 force-pushed the fix-dummy-midi-client-message branch from 2c022d8 to eda4f57 Compare May 23, 2020
@thmueller64 thmueller64 changed the base branch from master to stable-1.2 May 23, 2020
@thmueller64 thmueller64 reopened this May 23, 2020
@JohannesLorenz
Copy link
Contributor

@JohannesLorenz JohannesLorenz commented May 23, 2020

CI was passed, see draft PR #5514 . Merging now.

@JohannesLorenz JohannesLorenz merged commit d5a2366 into LMMS:stable-1.2 May 23, 2020
2 of 3 checks passed
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

4 participants