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

JogWheelBasic component doesn't take into account deck switching #11867

Open
miwig opened this issue Aug 25, 2023 · 6 comments
Open

JogWheelBasic component doesn't take into account deck switching #11867

miwig opened this issue Aug 25, 2023 · 6 comments

Comments

@miwig
Copy link
Contributor

miwig commented Aug 25, 2023

Bug Description

As seen here the JogWheelBasic component doesn't take into account the selected deck/group when scratching.

I could try a PR if you want. I assume the fix would be to add special handling here similar to how it's done for EffectAssignmentButton.

Version

2.3.5

OS

Arch Linux btw

@Swiftb0y
Copy link
Member

Hey there. Thank you for filing the issue. This is indeed an oversight. I'd be very happy if you could file a PR for that. I see two possible solutions:

  1. The special handling as you suggested
  2. instead of relying on the .deck, we instead extract the deck number from the .group on each call.

The second solution is more robust, but likely very much lower-performant... Not sure what solution is the most optimal one. What do you think?

@miwig
Copy link
Contributor Author

miwig commented Aug 27, 2023

instead of relying on the .deck, we instead extract the deck number from the .group on each call.

Couldn't JogWheelBasic override connect to extract the deck number from the group?

@Swiftb0y
Copy link
Member

That... could work... Not sure how robust that is though.

Another option would to do the regex extraction on each call, but speed it up by memoizing the call each time.

@miwig
Copy link
Contributor Author

miwig commented Aug 27, 2023

That... could work... Not sure how robust that is though.

It seems more robust than the special case solution to me. The docs also suggest that overriding connect is allowed, but give a different reason for doing it.

@Swiftb0y
Copy link
Member

Right, overwriting connect is completely reasonable and encouraged. I'm just not 100% confident that consumers will always call connect after changing the group. On the other hand, they should, otherwise that would probably result in bugs in all other components as well. So yeah, overwriting connect and extracting the deck from the group is probably the best solution. Can you open a PR with that?

@miwig
Copy link
Contributor Author

miwig commented Aug 27, 2023

I can probably get around to it this week, yeah.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants