-
-
Notifications
You must be signed in to change notification settings - Fork 538
Feat streamdeck plus swipe #3721
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
base: main
Are you sure you want to change the base?
Conversation
- Horizontal swipe changes pages - Vertical swipe sends rotate actions - (Added a swipe action to `SurfaceHandler` to handle the page change.)
- Options are added to `SurfaceGroupConfig` - Modify *EditPanel.tsx* to add the new options - Modify `SurfaceGroup.setCurrentPage` to handle page restrictions - Import/Export new fields (pages are converted similarly to `startup_page_id` and `last_page_id` - (Modify `InternalPageIdDropdown` to allow multiple values)
Just thinking about future growth here, would it not be better for touchscreen inputs to have their own full set of gestures that can be linked to actions, rather than trying to shoehorn them in to existing functionality? For example, the SD+ LCD may just have swipe up, down, left, and right, but what if future touchscreen inputs we also want to include gestures such as rotating, or other such things? Would it not make more sense to build it out properly now, rather than take a shortcut to rig it force horizontal swipe to do page changes, and vertical for rotation actions, which could be fine for some users but not everyone wants to have an LCD screen have to span multiple Companion pages for different functionality |
|
My take is that the api of this should be made a little more generic, so that the terminology 'swipe' remains inside the elgato device implementation. I can imagine other surfaces where they have buttons which make sense to be dedicated page up and down (I feel like there was a request for this from a satellite user at one point). That would also mean that this is not a start for any more generic touch interactions, as the touch becomes an implementation detail of the elgato surfaces. |
docs/3_config/surfaces/groups.md
Outdated
| This helps setups where you need to be able to swap out a Stream Deck quickly, and also need actions to reference specific Stream Decks. | ||
| You can create your groups and program the system ahead of time, then later add or remove surfaces to the groups without needing to update any buttons. | ||
|
|
||
| ## Use Case 3: Establishing "Permission Groups" with restricted access |
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'm not sure this is a separate use case for groups.
From my reading of the implementation, the page restriction is possible for 'standalone' surfaces too.
And from reading this, it sounds like I should be able to add multiple surfaces to a group to limit them to a set of pages, but have each of them on separate pages; which is not the case.
I think this bit about restricted access should maybe be its own page in the docs?
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.
Well, I compromised, adding "when it applies" comments and also adding a new page. Feel free to modify as you see fit.
Also, because "security" settings are scattered in the UI, I added a help page with links to them all...
|
Thanks @Julusian & @thedist. I'm short on time, right now but perhaps you could help with the following? Something in the WebUI code/config changed recently and is causing my WSL to crash when running |
|
Well, a bit of sleuthing identifies the monaco library as the culprit for my crashing. This appears to be one of the known cases for the longstanding Vite "memory leak" issue. To be precise when running My temporary workaround is to allow WSL to use more than 50% of system RAM (yes, I don't have "enough" RAM, but it has worked fine until now). But it's very frustrating that relatively small changes cause such huge jumps in memory usage. Have you considered moving away from Vite? For example, the React folks themselves recommend Next.js and go so far as saying that it is the closest thing to "the React team’s full-stack architecture vision" I know nothing about Next.js but this is the second time a small change has blown up Vite's memory consumption (during build). And the issue itself is several years old, with the developers apparently saying that they're not going to/can't fix it (last comment there). |
I am not sure if any of those frameworks are going to be a good fit, as we are a slightly unconventional app. Companion has a lot of state loaded upon connection with continual syncing (very much a SPA), and while next can technically maybe handle that it wants to be building a more traditional webapp. I suspect that some kind of forced chunking or maybe even not bundling monaco during dev would be the way to go (monaco tries to load from a cdn, so it should be pretty trivial to let it do that during dev) |
|
Thanks Julian, a fix to the memory issues would be awesome. (I started reading rollup documentation but it seems like going down a rabbit hole, which at least at present I'm trying to avoid...) Ok. Now back to this PR... (Should I copy this part of the conversation out into a new issue?) |
|
Before I go changing code, (well OK, I actually started, which led to the following thorughts) there may be a slight mismatch in our "visions". On the one hand I like the idea of tucking "swipe page" in with the Elgato surface implementation, especially regarding the config options, but on the other hand, I don't agree that adding a "page swipe" event is a "slippery slope", since its purpose is really quite different (these numbers do not correspond to your comment numbers, above, Julian):
|
I sencond that concern. But I think mapping to rotary actions in the third row of that surface would be perfectly fine and doesn't do any harm for now. I would not introduce new configuration or a new page change logic where the surface manages it's own page. I'd prefer everything of this being handled by existing set surface page actions, so the button will invoke a page change and there is no need to add any new surface methods. I think this will be a dead end street and much more complicated to upgrade later. |
I don't follow that. I think you are not thinking open enough. Just because Elgato uses the swipe gesture for page change doesn't mean a Companion user wants that too. |
|
Thanks for the feedback @dnmeid It sounds like we all agree that Julian's recommended code changes address any future-proofing, and I've decided to accept his code advice, so no need to comment further on that. Let me clarify some details that may help with our discussion:
So given that information, is the user really losing anything by not having full flexibility on horizontal swipes? This PR already gives them four new rotational buttons, and to reiterate: I'd say that insisting that the swipe maps to some "hidden" location on each page, would be a disservice to the vast majority of users. A simple compromise is to allow mapping to a button if "swipe to change page" is turned off. I'm not strongly opposed to this, though I do question its value considering the SD+ API/firmware limitations discussed next. To a large extent, I suspect that doing so would just open up the advanced user to frustration with the "feature". Here's what I found to be the Stream Deck+ swipe capabilities and limitations:
Since the swipe is not quantitative, a user trying to map horizontal swipes to an incremental change (i.e. rotational actions) would quickly become frustrated with the having to swipe horizontally over and over again to get a desired change. So, again, while not opposed to allowing it, I question whether it's a good idea. In terms of what users want: let's not forget that this PR is addressing is a three year old issue, so there is clearly a longstanding desire for a solution. In fact, I recently posted a short video on FB demonstrating what this PR does and soliciting feedback: it got a large and overwhelmingly positive response, so I think it's safe to say that Companion users are eager to have the functionality added by this PR. (In fact the only "complaints" in the comments were that they want the same thing for their favorite surface 😀.) |
Maybe, but I see this PR as being 2 features; one for restricting page changes in general, the other specifically about the swipes on the sd+. I'm not opposed to this being something more generic. Perhaps the best approach is to a proeprty on the I was about to say that at it is just the one surface using it, then it might not be worth that effort. Then realised that in order for this to work from satellite (and the surface api rework), it probably does need to become a property like that.
The whole job of surface groups is to link surfaces to be always on the same page.
No, I am not suggesting this.
On a technical level I kind of agree, but on a UX level having to manually setup 8 actions per page to get the same page change behaviour is not going to be nice. And yeah, maybe we will redo it later, I dont see a problem with having this until we have something better. Of course, that is if we can even map the lcd on this into something better, seeing as how limited the events are I am wondering if when this page swipe stuff is disabled, perhaps left and right swipes should trigger the same rotation events on the lcd strip buttons. |
- ensure as best as possible that page restrictions are enforced (a) at startup and (b) even if enabled after visiting now-restricted pages.
* Ensure all `configFields` properties are added to to panelConfig * Ensure that defaults specified in `configFields` are honored * Ensure that missing fields are added when importing a preexisting export. * Deal with TypeScript oddities
|
These are more changes than I had anticipated, so I broke it up into separate commits to make it a bit "modular"
This should address all comments prior to your new comment today, Julian, but doesn't address, yet, your new idea, which I either need time to digest or more clarity on how you see it used. On that note:
Thanks again for all your advice, input and encouragement! |
This branch is now strictly focused on SD+ swipe actions
|
Ari said:
I absolutely don't follow that argumentation. Forcing everybody to add some actions to a "hidden" button is not as bad as you are trying to make us believe. I follow you with it being easier to have a page change functionality built into the surface like you propose for exactly that one purpose: mapping page changes to the horizontal swipe. But as I said, that is not the Companion way of doing things. This would be a paradigm shift and introduce a new interaction style in Companion that is currently not there. As I see it there will definitely be some users that want to do exactly that, triggering page changes from swipe, but there will also be users that want to do something else with horizontal swipe. You didn't address that concern. Julian said:
Why 8 actions? Isn't it the same two actions like you have to set up on every other surface when using regular buttons?
I have a different assesment of the maintenance cost. I'm greatly remembered with this feature at the introduction of pin code locks. By that time we had exactly the same discussion. Someone had a working PR for introducing pin codes like we have now. I voted against the implementation as completely independent drawn buttons and input logic. Still today we are fighting with the quirks of this implementation because now there are completely different surfaces and users always want more functionality like individual lock-outs and individual landing pages and so on. Everything would have been straight-forward and easy to understand with an action-based lockout. And today many users do this anyway and use the dataentry module for it. If we now introduce page control from the surface without using some element that lives on the grid and is configurable with our standard methods and at the place where it really happens (or at least close), that would completely not be inline with the Companion style again and would be hard to remove in the future. For me that feels 100% like the same mistake we see over and over again from module developers. It is an implementation of exactly one workflow exactly the way the developer would use it. That is the reason why we have so many checkboxes and options only used by a few people. So ultimately we agreed on discussing such feature additions with the board before adding and that is what we should do. |
|
Not mad, @dnmeid: you have valid concerns and they should be addressed. Let me suggest that we already agree much more than we disagree and that there's a simple solution. In fact, I was going to start with a long list of "precedent" arguments, but you've already covered them! I understand that you don't like those precedents, but considering that you yourself have used them and even recommended them in your first comment, it's hardly cause for invoking a "nuclear option"! If I understand you correctly, your main concern is that (a) people should have the option to do something other than page change for horizontal swipe and (b) that it should be mapped to the button grid. Just to review what I believe we can agree on:
So what this PR does uncontroversially is: LCD strip swipes fully mapped to a grid and 100% consistent with current practices. The question is then what to do with the "bonus" horizontal swipe, since the grid is currently fully mapped.
Put another way, the advanced user will have to jump through extra hoops regardless, the question therefore is: Do we force everyone to jump through hoops just because a small minority will want to? So the simple solution is to tie the horizontal swipe to a first- or second-row button if "Swipe to Change Page" is disabled. This way, the advanced user has to do exactly the same amount of work as they would have had to do anyway, whereas the typical user gets what they want without frustration. (And considering the established precedents you have mentioned, without compromising the integrity of Companion's core tenets.). A somewhat more difficult/awkward solution would be to expand the SD+ grid to allow assigning horizontal swipe to a new button for the advanced users. (But of course that's a rather disruptive change relative to its benefit.) Dorian wrote:
This is an important point, so worth elaborating on specifically.
To rephrase that last point: Companion clearly recognizes that page-changing is a special case/universal need. This PR follows in that tradition. So the thing that frees up the most buttons and is the least frustrating for users is enabled by the new independent mechanism of page changes. And again, there is plenty of precedent for surface actions that work without using the button grid. So I still believe that it would be a great disservice to the majority of users to skip this functionality and as I've pointed out above, it can be done without imposing any restrictions on the few who will want to map it to a specific button. I certainly welcome any alternative suggestions you might have for how to make things simple for the majority of users without restricting advanced users -- or indicating your preference of the two alternatives I suggested here for accommodating advanced users. |
|
There's nothing like hitting the "send" button to provide extra insight 😀, but this one is probably worth its own space anyway: It worth noting that the TCP/UDP API has predefined commands for Page Set and Page-Up/Page-Down that is independent of button-presses or the button grid, so that is yet another precedent for adding the same functionality to the surfaces API. |
Yeah, but to replicate what this PR does (assuming the general rotation parts are merged), it requires adding a rotate left and a rotate right action to each of the 4 buttons that the lcd strip will translate to. As explained in this comment, that is going to be really tedious and manual to do:
Yes and no. There are many things we do for convenience (often phrased as UX), that add complexity. I could argue that presets are exactly the same, as being purely convenience focused.
Sure, but we are no closer to a better solution now than we were 3 years ago when rotary actions were first added. So maybe this swipe stuff isn't the ultimate and final solution, but it will do until there is a plan and some momentum on a bigger solution.
I cant say if I agree or disagree with this tbh. I don't have any ideas on how this could translate onto the grid in a nice way. Because to me this means being able to have some lcd interactions being done as if it were 1 control, and some interactions as if it were 4, and I cant see how to make that work in a sensible way that isnt either really space inefficient, a confusing layout or horribly complicated Or we could do as we did recently for the old xkeys layout and simply remove them. I havent yet seen anyone complain/notice that we did that.
No objection from me on doing that. With various people having been rather quiet recently I havent felt it worth trying to arrange anything on this.
I wouldnt agree on 'overwhelming majority', but would say that some users of the sd+ would. That is enough to justify its value for me. |
This completes my proposal in Issue #2227 , as modified by Julian's requests...
swipePageaction toSurfaceHandlerto handle the page change.)SurfaceGroupConfigSurfaceGroup.setCurrentPageto handle page restrictionsstartup_page_idandlast_page_idInternalPageIdDropdownto allow multiple values)