Conversation
avacreeth
left a comment
There was a problem hiding this comment.
Just a few high level comments. I'm going to get someone on the desktop team to take a closer look also.
app/services/user-state/index.ts
Outdated
| const sleep = (ms: number) => new Promise(resolve => setTimeout(resolve, ms)); | ||
|
|
||
| @InitAfter('UserService') | ||
| export class UserStateService extends StatefulService<UserStateServiceState> { |
There was a problem hiding this comment.
This feels like a super generic name for a service. Does this have applications outside of vision? If not, it might be good to pick something a bit more specific.
There was a problem hiding this comment.
So this is actually the "old" way to do a stateful service in desktop. We switched to realm for managing state in new services. I can help you get that set up.
There was a problem hiding this comment.
agreed that this is a super generic name for what seems like a pretty specific service
There was a problem hiding this comment.
I agree, the naming is not great here. It does/will have applications outside of vision so I didn't want to lock it in to "vision", since this will help manage all of the user's little data bits that you might see from stream labels, or game data like health, etc.
Igor is working on adding more than just the vision data here, and once it's ready we'll expand this out.
I have some places I'm calling things "user state" and other's where it's "reactive data" -- I would like to narrow it down to one.
Please let me know if you have any good ideas for the naming.
There was a problem hiding this comment.
maybe something like DynamicDataService? just spitballing but if you need a sufficiently vague name that doesn't have namespace collisions (like this one has with UserService and UserAuthService) something like that could work
| sourceProperties={() => this.sourceProperties(sceneNode.id)} | ||
| onEditReactiveData={ | ||
| sceneNode.sourceId && | ||
| this.sourcesService.state.sources[sceneNode.sourceId]?.propertiesManagerType === |
There was a problem hiding this comment.
FYI, this is a non-reactive way to access this state in a React component. I think it's fine because properties manager almost never changes, but FYI.
There was a problem hiding this comment.
Good call, I will make some changes to make it reactive
| WindowsService.actions.closeChildWindow(); | ||
| } | ||
|
|
||
| const [stateFlat, setStateFlat] = useState(UserStateService.state.stateFlat); |
There was a problem hiding this comment.
This is a somewhat unusual thing to do in react because you're not copying the object, you're just adding a reference to the original object in state. If the goal here was to be working off a local copy unique to this component then you would want to copy it when initializing the state. It's not a huge deal since you're not directly mutating the object, just an FYI.
There was a problem hiding this comment.
Good catch 👍 I'll update to make a shallow copy since it's just a simple key-value structure
package.json
Outdated
| "author": "General Workings, Inc.", | ||
| "license": "GPL-3.0", | ||
| "version": "1.19.6", | ||
| "version": "1.19.6-tom", |
There was a problem hiding this comment.
Not sure if we want to update the version here?
| ? (sourceId => () => | ||
| this.sourcesService.actions.showReactiveDataEditorWindow(sourceId))( | ||
| sceneNode.sourceId, | ||
| ) |
There was a problem hiding this comment.
this is kinda a nitpick but idk why you're bothering with all this function currying and invoking instead of just
() => this.sourcesService.actions.showReactiveDataEditorWindow(sceneNode.sourceId)
There was a problem hiding this comment.
I think I did this due to sceneNode.sourceId (ISourceMetadata.sourceId?: string | undefined) potentially being undefined when onEditReactiveData() is called at a future point in time. I put this closure in to ensure it's by value and not reference
I can go your route, I would just need assert the type:
() => this.sourcesService.actions.showReactiveDataEditorWindow(sceneNode.sourceId as string)
Do you like that better? I could also do the checking this way:
() => sceneNode.sourceId && this.sourcesService.actions.showReactiveDataEditorWindow(sceneNode.sourceId)
Let me know which approach you like best 👍
There was a problem hiding this comment.
the second one would work or you could also just add a guard clause in showReactiveDataEditorWindow which would also make it compliant with strict nulls, as far as i can tell the first one still wouldn't be TS kosher as is since folders for example never have a source id
There was a problem hiding this comment.
That's actually perfect, I'll make it allow undefined, since later I was planning to make this ReactiveData window not need to be filtered for a source.
| style={{ opacity: 0.7 }} | ||
| /> | ||
| </Tooltip> | ||
| )} |
There was a problem hiding this comment.
just FYI we put all the optional actions at the top so it doesn't shift alignment for items that always show up
but also... this menu is starting to get extremely crowded
There was a problem hiding this comment.
Sounds good, I will move it up with the others. I originally placed it here for the display order.
I agree, this menu is getting a bit wild 🤠
| textWhite90: 'rgba(255, 255, 255, 0.9)', | ||
| textWhite60: 'rgba(255, 255, 255, 0.6)', | ||
| textWhite40: 'rgba(255, 255, 255, 0.4)', | ||
| textWhite20: 'rgba(255, 255, 255, 0.2)', |
There was a problem hiding this comment.
we shouldn't be using hard coded colors here, it wont match when users shift UI color schemes
there are var colors located in themes.g.less that should be preferred over this
| width: '200px', | ||
| }} | ||
| > | ||
| <input |
There was a problem hiding this comment.
should probably be using the NumberInput component here, makes things a lot easier
| borderTop: index === 0 ? 'none' : `1px solid ${colors.borderWhite5}`, | ||
| alignItems: 'center', | ||
| justifyContent: 'space-between', | ||
| }} |
There was a problem hiding this comment.
all of these styles should probably be extracted into a css module
app/services/vision/index.ts
Outdated
|
|
||
| // | ||
| // | ||
|
|
app/util/dot-tree.ts
Outdated
| return tree; | ||
| } | ||
|
|
||
| export type Prettify<T> = { [K in keyof T]: T[K] } & {}; |
There was a problem hiding this comment.
it's just a handy utility type that I use on occasion, for more complicated types. Not needed, but makes DX a bit better imo. There's a bunch more online about it, but a couple references:
https://timdeschryver.dev/bits/pretty-typescript-types
https://www.totaltypescript.com/concepts/the-prettify-helper
It's handy for cases like:
const dotNotation = toDotNotation(
{ a: { b: { c: 1 } } },
(v): v is number => typeof v === 'number',
);
If you use that Prettify utility type, you get something like this
const dotNotation: {
"a.b.c": number;
}
And if you don't, you get:
const dotNotation: DotMap<{
a: {
b: {
c: number;
};
};
}, number>
There was a problem hiding this comment.
got it, i might be misunderstanding but i think i was thrown because it kind of seems to be a tautilogical type that defeats the purpose of TS? like for type T its enforcing that keys of K return the value of T[K] which would never not be the case? i think maybe something that asserts some sort of typechecking would be more useful, like maybe something like this could work?
for expected string keys
type NestedDictionary<T> = Dictionary<NestedDictionary<T>>
or for something like enum keys
type NestedRecord<K, T> = Record<K, NestedRecord<K, T>>
for whatever solution we end up with i would however recommend that a very general use-case utility type like this should be moved to index.d.ts, types of which can be used anywhere without needing to import/export
app/services/user-state/index.ts
Outdated
| UserService, | ||
| WebsocketService, | ||
| } from 'app-services'; | ||
| import { toDotNotation } from 'components-react/windows/reactive-data-editor/lib/dot-tree'; |
There was a problem hiding this comment.
this is a wild import for something that can just be grabbed from util/dot-tree
There was a problem hiding this comment.
very wild 🤠
I'll work on merging these two files together into one under util/dot-tree
package.json
Outdated
| "author": "General Workings, Inc.", | ||
| "license": "GPL-3.0", | ||
| "version": "1.19.6", | ||
| "version": "1.19.6-tom", |
There was a problem hiding this comment.
we'll need to remove this before shipping
tmaneri
left a comment
There was a problem hiding this comment.
I'll get through the rest of these tomorrow 👍
| sourceProperties={() => this.sourceProperties(sceneNode.id)} | ||
| onEditReactiveData={ | ||
| sceneNode.sourceId && | ||
| this.sourcesService.state.sources[sceneNode.sourceId]?.propertiesManagerType === |
There was a problem hiding this comment.
Good call, I will make some changes to make it reactive
| style={{ opacity: 0.7 }} | ||
| /> | ||
| </Tooltip> | ||
| )} |
There was a problem hiding this comment.
Sounds good, I will move it up with the others. I originally placed it here for the display order.
I agree, this menu is getting a bit wild 🤠
app/components-react/windows/reactive-data-editor/ReactiveDataEditorWindow.tsx
Outdated
Show resolved
Hide resolved
| WindowsService.actions.closeChildWindow(); | ||
| } | ||
|
|
||
| const [stateFlat, setStateFlat] = useState(UserStateService.state.stateFlat); |
There was a problem hiding this comment.
Good catch 👍 I'll update to make a shallow copy since it's just a simple key-value structure
| if (!schemaFlat || !stateFlat) { | ||
| return ( | ||
| <ModalLayout bodyStyle={{ padding: '20px' }} hideFooter={true}> | ||
| {!schemaFlat ? <div>Waiting for schema...</div> : <div>Waiting for state...</div>} |
There was a problem hiding this comment.
I'll wrap in $t. State is async, it starts out as undefined until it's fetched from the API.
Schema is... supposed to be coming from the API and not from this PoC file lib/schema so thank you for bringing it up. I'll update it 👍
| @@ -0,0 +1,3 @@ | |||
| export type ReactiveDataEditorProps = { | |||
| stateKeysOfInterest: string[]; | |||
There was a problem hiding this comment.
Sounds good, I'll move it to ReactiveDataEditorWindow.tsx
| * This enforces a consistent hostname across all reactive browser sources, | ||
| * enabling centralized testing and deployment of new reactive overlay features. | ||
| */ | ||
| const REACTIVE_SOURCES_ORIGIN = 'https://alpha-sl-dynamic-overlays-demo.streamlabs.workers.dev'; |
There was a problem hiding this comment.
Yeah, when this goes live I'll need to update this to the live URL. I just made this a draft PR for that reason.
Is there a better approach for this kind of thing? The TL;DR is that I want this origin to be one way for testing the next version of the rive wrapper I made, and another way when we go live or want to test the live rive wrapper.
app/services/user-state/index.ts
Outdated
| UserService, | ||
| WebsocketService, | ||
| } from 'app-services'; | ||
| import { toDotNotation } from 'components-react/windows/reactive-data-editor/lib/dot-tree'; |
There was a problem hiding this comment.
very wild 🤠
I'll work on merging these two files together into one under util/dot-tree
app/services/user-state/index.ts
Outdated
| const sleep = (ms: number) => new Promise(resolve => setTimeout(resolve, ms)); | ||
|
|
||
| @InitAfter('UserService') | ||
| export class UserStateService extends StatefulService<UserStateServiceState> { |
There was a problem hiding this comment.
I agree, the naming is not great here. It does/will have applications outside of vision so I didn't want to lock it in to "vision", since this will help manage all of the user's little data bits that you might see from stream labels, or game data like health, etc.
Igor is working on adding more than just the vision data here, and once it's ready we'll expand this out.
I have some places I'm calling things "user state" and other's where it's "reactive data" -- I would like to narrow it down to one.
Please let me know if you have any good ideas for the naming.
| // subscribe to websocket events to keep state updated | ||
| this.socketSub = this.websocketService.socketEvent.subscribe(e => { | ||
| if (['visionEvent', 'userStateUpdated'].includes(e.type)) { | ||
| this.log(e); |
There was a problem hiding this comment.
This is to help debug any issues with the new reactive sources. We utilize websocketService.socketEvent.subscribe in some other places, this was placed to write things out to the user's log file if there is an issue with a reactive source not updating.
…ry new ideas after this commit
|
/Azurepipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
No description provided.