Added "catch" to knob behavior #210
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ny of the remaining 3 CV inputs on the Field
… into electro-smith-master
|
I dig the catch behavior. It can be slightly confusing though when you turn a knob and the page doesn't change. I noticed none of the controls other than reverb work when you have shift enabled. I don't really like that behavior. @stephenhensley any thoughts? |
|
Ben,
I agree. I will fix both those things in this PR.
Thanks,
Gregor
Sent from Mail<https://go.microsoft.com/fwlink/?LinkId=550986> for Windows 10
From: Ben ***@***.***>
Sent: July 26, 2021 12:55 PM
To: ***@***.***>
Cc: Gregor ***@***.***>; ***@***.***>
Subject: Re: [electro-smith/DaisyExamples] Added "catch" to knob behavior (#210)
I dig the catch behavior. It can be slightly confusing though when you turn a knob and the page doesn't change.
Maybe the page should change when the knob moves, but the controls should still use the catch behavior.
Just personal preference.
I noticed none of the controls other than reverb work when you have shift enabled. I don't really like that behavior.
IMO all that should change is whether control 8 maps to Reverb or Feedback.
@stephenhensley<https://github.com/stephenhensley> any thoughts?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub<#210 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAOPXUR525Y3RB75FY5LQPDTZW4SXANCNFSM5A5UMYBA>.
|
|
I haven't had a chance to try it out quite yet, but it changing to the correct page with the knob turn seems like a good UI decision. I could go either way on the shift behavior, but if it's the only thing the shift button does right now then having it toggle only the reverb/feedback control makes sense as well. |
|
Hi @gregornoriskin. Hope you're well. Looks like it was just a small change request remaining on this. Just wanted to see if you were interested in wrapping that up since this has been sitting for a bit. |
|
Stephen,
Apologies, been caught up in a role change at work. I will get this done before the end of the month. It been bugging me too.
Thanks,
Gregor
On Dec 1, 2021, at 1:18 PM, Stephen Hensley ***@***.***> wrote:
Hi @gregornoriskin<https://github.com/gregornoriskin>. Hope you're well. Looks like it was just a small change request remaining on this.
Just wanted to see if you were interested in wrapping that up since this has been sitting for a bit.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#210 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAOPXUWL7JTPGJVLSGXUZHDUO2GJXANCNFSM5A5UMYBA>.
Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
@gregornoriskin thanks for the hasty response. No need to apologize, just wanted to check in. Thanks! Looking forward to merging this :) |
|
Dusting off my code, doing an upstream fetch and merge… perhaps I should abandon my previous PR and create a new one. Don’t see an option to do that though. What is your policy regarding stale PRs?
Thanks,
Gregor
From: Stephen Hensley ***@***.***>
Sent: December 1, 2021 2:16 PM
To: electro-smith/DaisyExamples ***@***.***>
Cc: Gregor Noriskin ***@***.***>; Mention ***@***.***>
Subject: Re: [electro-smith/DaisyExamples] Added "catch" to knob behavior (#210)
@gregornoriskin<https://github.com/gregornoriskin> thanks for the hasty response.
No need to apologize, just wanted to check in.
Thanks! Looking forward to merging this :)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#210 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAOPXUVSOOPSOKY4O4IBDQ3UO2ND3ANCNFSM5A5UMYBA>.
Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
@gregornoriskin we don't have a specific policy in place yet. If there were significant changes to the relevant files, I'd say resubmitting would be the best path forward, but the PR is still looking fairly clean (only 2 files changed), and I typically squash and merge on this repo so the commits will get grouped down to one. So I think it's fine to leave to continue with this PR. Let me know if you have any issues or questions. |
|
Mkay, will just add to the existing PR. Hopefully not too many additional changes to address changes in libdaisy and DaisyDSP.
Thanks,
Gregor
On Dec 7, 2021, at 12:34 PM, Stephen Hensley ***@***.***> wrote:
@gregornoriskin<https://github.com/gregornoriskin> we don't have a specific policy in place yet.
If there were significant changes to the relevant files, I'd say resubmitting would be the best path forward, but the PR is still looking fairly clean (only 2 files changed), and I typically squash and merge on this repo so the commits will get grouped down to one.
So I think it's fine to leave to continue with this PR.
Let me know if you have any issues or questions.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#210 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAOPXUS4GQORSZZE4W6YY3LUPZVTNANCNFSM5A5UMYBA>.
Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
Done and done. Please see the PR. I also built this against the latest submodules so confirmed that it still works 😊
From: Gregor Noriskin ***@***.***>
Sent: December 7, 2021 1:26 PM
To: electro-smith/DaisyExamples ***@***.***>
Cc: electro-smith/DaisyExamples ***@***.***>; Gregor Noriskin ***@***.***>; Mention ***@***.***>
Subject: Re: [electro-smith/DaisyExamples] Added "catch" to knob behavior (#210)
Mkay, will just add to the existing PR. Hopefully not too many additional changes to address changes in libdaisy and DaisyDSP.
Thanks,
Gregor
On Dec 7, 2021, at 12:34 PM, Stephen Hensley ***@***.******@***.***>> wrote:
@gregornoriskin<https://github.com/gregornoriskin> we don't have a specific policy in place yet.
If there were significant changes to the relevant files, I'd say resubmitting would be the best path forward, but the PR is still looking fairly clean (only 2 files changed), and I typically squash and merge on this repo so the commits will get grouped down to one.
So I think it's fine to leave to continue with this PR.
Let me know if you have any issues or questions.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#210 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAOPXUS4GQORSZZE4W6YY3LUPZVTNANCNFSM5A5UMYBA>.
Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
Works well, and the changes look good! I'd say it's ready to merge. 💯 |
|
Thanks again for the contribution @gregornoriskin !! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I added a "catch" behavior to the knobs. Suppressed automatic page changing for some buttons. Fixed a couple of minor bugs.