WIP: OBS as source of truth for hotkeys #1054

blackxored · 2018-11-27T00:40:18Z

Initial support for using OBS as the source of truth for hotkeys.

app/services/hotkeys.ts

AppVeyorBot · 2018-12-12T18:10:46Z

❌ build failed

AppVeyorBot · 2018-12-12T19:49:50Z

❌ build failed

AppVeyorBot · 2018-12-12T20:45:08Z

❌ build failed

AppVeyorBot · 2018-12-12T22:52:02Z

❌ build failed

AppVeyorBot · 2018-12-12T22:57:59Z

❌ build failed

avacreeth · 2018-12-12T23:21:55Z

app/services/scene-collections/scene-collections.ts

@@ -476,6 +476,7 @@ export class SceneCollectionsService extends Service

        await this.loadDataIntoApplicationState(data);
      } catch (e) {
+        console.error('Error while loading collection, restoring backup', e);


gettinToasty · 2018-12-13T17:36:04Z

app/services/hotkeys.ts


-    HOTKEY_ACTIONS.GENERAL.forEach(action => {
+    Object.values(GENERAL_ACTIONS).forEach(action => {


AFAIK this is the only place you're using the *_ACTIONS objects so i'm kind of wondering why you refactored them into objects? if you only use them to turn back into arrays it just seems like it's adding in an extra step, but i might be missing something here

We will peel them off, one by one. Easier to have them separate to differentiate logic (such as general being run first and not having to duplicate-check), does that make sense?

i understand breaking them up into chunks, but the data transformation from array into object -> array is where i'm lost

We need these indexed for getActionFromName and normalization, in the future, this could be a way to improve performance in lookups as well.

gettinToasty · 2018-12-13T17:53:21Z

app/services/hotkeys.ts

-    Object.keys(hotkeySet.sources).forEach(sourceId => hotkeys.push(...hotkeySet.sources[sourceId]));
+    Object.keys(hotkeySet.sources).forEach(sourceId =>
+      hotkeys.push(...hotkeySet.sources[sourceId]),
+    );


are these all automatic linter changes? i feel like we should change the config because we use single line ifs and anonymous functions all the time

For style issues, I believe we should focus our effort on #1129. I believe it'd be wasteful to change something back to just change it again (or not) in the other branch. This might've been before we merged #1043, so I apologize if it was a result of my editor and switching branches.

gettinToasty · 2018-12-13T17:57:11Z

app/services/hotkeys.ts

+
+const getActionFromName = (actionName: string) => {
+  return ACTIONS[actionName] || ACTIONS[getMigrationMapping(actionName)];
+};


not sure why this and the above are using manual returns

I've changed getActionFromName. I believe getMigrationMapping should stay this way due to a somewhat complex expression and the fact we're using an object.

gettinToasty · 2018-12-13T17:59:33Z

app/services/hotkeys.ts

+  }
+};
+
+const normalizeActionName = (actionName: string) => actionName.split('.')[0];


i feel like utility functions should be defined above the class, we already have several examples of that in this file. open to putting them after but either way i think it should be consistent.

I agree 100% with this, however I've mixed feelings about it. There wasn't an easy choice, as I believe the most important thing for us looking at this file would be
Types -> Consts -> Class -> Utils
Unfortunately, the functions you see above Class are there because of no-use-before-define. The rest is supporting and I believe should stay out of the way at the end. Unless we really want them in our way before we get to read the class, that is.

gettinToasty · 2018-12-13T18:01:57Z

app/services/scenes/scenes.ts

-  ISceneCreateOptions,
-  IScenesServiceApi
+  Scene,
+  SceneItem,


not sure why these imports change order? it just seems like dirtying the diff for no actual reason

I think it'd be great if we started sorting imports, it's recommended by pretty much everyone. Relevant rule: https://eslint.org/docs/rules/sort-imports.

Changes of this file have to be excluded from this PR

How do you mean? We need the scene dedup that's in here.

gettinToasty · 2018-12-13T18:04:33Z

app/services/scenes/scenes.ts

-        height: 250
-      }
+        height: 250,
+      },
    });
  }
 }


it doesn't look like anything was actually changed in this file apart from style changes? i think we should keep those in separate PRs to reduce diff size and cognitive load when reviewing

We've added scene dedup and typed a few functions. While I see your point about having style diffs here, these kinds of style changes wouldn't be an issue anymore, shortly.

yea that was more a future consideration than something to change for this PR

holiber · 2018-12-13T18:08:17Z

app/services/hotkeys.ts

-      isActive: () => getTransitionsService().state.studioMode
+    isActive: () => {
+      const streamingService = getStreamingService();
+      return !streamingService.isStreaming;


Don't see a reason why you replaced one line expression to the 2 lines

Can you please paste the line? I'm not entirely sure what you're referring to in this particular case.

holiber · 2018-12-13T18:14:40Z

app/services/hotkeys.ts

 interface IHotkeysServiceState {
  hotkeys: IHotkey[]; // only bound hotkeys are stored
 }

+type OBSHotkey = {


We usually use T prefix for types

type TOBSHotkey

Only to disambiguate, no?

holiber · 2018-12-13T18:41:51Z

app/services/hotkeys.ts

  upHandler?(): void;
 }

+type HotkeyGroup = {
+  [x: string]: IHotkeyAction;


what is x? I assume it must be actionName here

This is conventional indexer syntax in TS, do you think having actionName would really increase readability?

holiber · 2018-12-13T18:46:59Z

app/services/scenes/scenes.ts

-  ISceneCreateOptions,
-  IScenesServiceApi
+  Scene,
+  SceneItem,


Changes of this file have to be excluded from this PR

holiber · 2018-12-13T18:51:15Z

app/services/hotkeys.ts


-type THotkeyType = 'GENERAL' | 'SCENE' | 'SCENE_ITEM' | 'SOURCE';
+const isAudio = (sourceId: string) => {
+  const source = getSourcesService().getSource(sourceId);


Is it really possible to get source = null in this function?

Yes, in my testing

AppVeyorBot · 2018-12-13T20:30:42Z

❌ build failed

holiber · 2018-12-13T20:44:01Z

app/services/scenes/scenes.ts

-    return this.state.displayOrder.map(id => {
-      return this.getScene(id);
-    });
+    return uniqBy(this.state.displayOrder.map(id => this.getScene(id)), x => x.id);


That's looks like a bug symptom fix. Can you please describe this bug in the TODO comment before this line and add a task to Asana with steps how to reproduce this issue?

I've added it closer to where the bug happens.

holiber · 2018-12-13T20:45:03Z

app/services/scenes/scenes.ts

@@ -1,4 +1,6 @@
 import Vue from 'vue';
+import { uniqBy } from 'lodash';
+import { Subject } from 'rxjs';


Unused import

Which one exactly? Both uniqBy and Subject are used.

AppVeyorBot · 2018-12-13T21:02:23Z

❌ build failed

* support display capture hotkeys. * fix issues with duplicate scenes. * support sending keyup events to OBS backend. * log error when scene collection fails to load. Now we can process `SHOW_SCENE_ITEM` and `HIDE_SCENE_ITEM` without getting duplicate scenes in `SceneSelector`, coming from the service. An uniqueness check was added to the getter for scenes that checks based on scene ID. If we fail to load an scene collection, the behavior appears to be trying to restore from backup. This causes the loading of application state to be invoked a second time, with unexpected side-effects like duplicate scenes. This commit addresses the visibility part by displaying an error message for developers as opposed to swallowing the error entirely. Prior to this, we addressed the second part of one of those visible effects by adding uniqueness checks in the scenes API. * Fortifies checking for a source's type in case it cannot be found in the service (it has happened). * Fix action names for `TOGGLE_MUTE` and `TOGGLE_UNMUTE`. * Track added hotkeys on a set for the time being, ensuring we don't add duplicates coming from OBS that we're already handling. * Add explicit check for whether the returned item belongs to a scene hotkey. Please notice that currently identifying which kind of hotkey we got from the backend is factorial as we don't seem to have a better way to get this info from the backend. The number of hotkeys affected by this is minimal (the main reason we're sticking to the former loops for now) but could still be significant. I have not, however, profiled this.

blackxored requested review from EddyGharbi and avacreeth November 27, 2018 00:40

blackxored force-pushed the refactor/hotkeys branch from 18e1c4d to 898611b Compare November 27, 2018 01:03

avacreeth reviewed Nov 27, 2018

View reviewed changes

app/services/hotkeys.ts Outdated Show resolved Hide resolved

blackxored force-pushed the refactor/hotkeys branch 3 times, most recently from 01c7b61 to 54a2f8e Compare November 29, 2018 20:23

blackxored added the work in progress label Nov 30, 2018

blackxored force-pushed the refactor/hotkeys branch from 54a2f8e to 60c8390 Compare December 12, 2018 18:05

blackxored force-pushed the refactor/hotkeys branch from 60c8390 to deca03f Compare December 12, 2018 19:16

blackxored force-pushed the refactor/hotkeys branch 2 times, most recently from 53c7fcd to 4f43d1d Compare December 12, 2018 21:59

avacreeth reviewed Dec 12, 2018

View reviewed changes

gettinToasty reviewed Dec 13, 2018

View reviewed changes

blackxored force-pushed the refactor/hotkeys branch from 4f43d1d to 4e98113 Compare December 13, 2018 18:09

holiber reviewed Dec 13, 2018

View reviewed changes

blackxored force-pushed the refactor/hotkeys branch from 7e60211 to 3afdb19 Compare December 13, 2018 20:24

holiber reviewed Dec 13, 2018

View reviewed changes

holiber approved these changes Dec 13, 2018

View reviewed changes

holiber reviewed Dec 13, 2018

View reviewed changes

blackxored force-pushed the refactor/hotkeys branch from 59309a0 to 4b5fcb8 Compare December 13, 2018 21:11

blackxored added 3 commits December 13, 2018 15:52

chore: add comment about dup items

c183c62

refactor: rename indexer to actionName

9ae2ca5

blackxored force-pushed the refactor/hotkeys branch from 4b5fcb8 to 9ae2ca5 Compare December 13, 2018 23:52

avacreeth merged commit bced379 into staging Dec 15, 2018

avacreeth deleted the refactor/hotkeys branch December 15, 2018 01:22

EddyGharbi mentioned this pull request Jan 5, 2022

Update osn to v0.19.27 #3971

Merged


		HOTKEY_ACTIONS.GENERAL.forEach(action => {
		Object.values(GENERAL_ACTIONS).forEach(action => {

WIP: OBS as source of truth for hotkeys #1054

WIP: OBS as source of truth for hotkeys #1054

Conversation

blackxored commented Nov 27, 2018

AppVeyorBot commented Dec 12, 2018

AppVeyorBot commented Dec 12, 2018

AppVeyorBot commented Dec 12, 2018

AppVeyorBot commented Dec 12, 2018

AppVeyorBot commented Dec 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blackxored Dec 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AppVeyorBot commented Dec 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AppVeyorBot commented Dec 13, 2018

blackxored Dec 13, 2018 •

edited

Loading