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.
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
Ability for controller to share data at runtime #12199
Ability for controller to share data at runtime #12199
Changes from 6 commits
966e572
3fcb107
276ff7f
957a427
172be8c
56e2d9f
c0410fd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not share one single complex blob. This makes it really difficult to understand for developers and we can't print meaningful error messages, when two mappings do no match together.
The later can especially happen, when you edit the variable type at runtime of Mixxx, because only one mapping is reloaded at a time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if, in your namespace
foobar
, you want to share just aTypedArray
or a string, like in the tests? I think adding variables don't add any values to the feature and just another level.This isn't possible anyway. I thought we agreed that introducing namespace was a good enough option to prevent conflict.
I also don't see how this would work anyway either since any controllers part of a namespace can get/edit values as well: this means that it is acceptable for
MappingA
to doget("MappingBRunning")
(orget().MappingBRunning
currently) and find no values.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Namespacing is good enough to prevent unintended iterference of completly unrelated mappings.
But this does not address compatibility between the related scripts using the same namespace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this request is consistent with my original namespacing proposal above: #12199 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How your solution will compatibility between the related scripts compare to the current one? Do you mean I should also implement the API change mentioned here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be one approach. But also without an explicit declaration, it would be possible to print warnings like:
Line 1234 - Shared variable "deckSwitch" changed type in call of engine.setSharedVariable from number to string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is all javascript, there is no schema / static typing, nor is it really necessary. If there is no technical reason why variables could not change their type I don't see a good reason to introduce one (especially when it would only result in extra complexity).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I understand you are using type change as a way to detect a potential problem, this lefts out other usecase where a type change wouldn't occur but yet doesn't occur - eg wrong unit.
I think it all boils down to understanding what you are sharing, and making sure producers and consumers expect the same data AND don't conflict with each other.
I think this type of detection is a nice to have, but I cannot really see how this is required. Realistically, only a hand full of official mappings will use this features, so I think it is a matter of making sure this is well document (e.g using shared context class for example).
If you really feel like there should be a key for data set/get, I will add it, but lets agree first this is the last significant change I'm being asked to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for the record, I don't think a schema is necessary. exchanging an untyped/dynamic object is completely fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JoergAtGithub is that a hard requirement in your opinion, or are you happy to go with the untyped dynamic data as it is currently?