obs-websocket-dotnet icon indicating copy to clipboard operation
obs-websocket-dotnet copied to clipboard

Published 20 hours ago • verified publisher icon BarRaider

Massive Refactor

Open Zingabopp opened this issue 4 years ago • 2 comments

This is a fork I've been working on for my own project. There's a lot of changes, so I thought I'd make this draft PR so you can see if it's something you'd want to merge or make a branch off of.

Highlights:

  • Switched from WebSocketSharp to WebSocket4Net.
  • .Net Standard 2.0 target.
  • All requests converted to async Tasks.
  • All events use EventHandler or EventHandler<T>, with T being a class derived from EventArgs.
  • C# 8.0 with nullables.

Zingabopp avatar Aug 25 '20 22:08 Zingabopp

We recently pushed a change to move to .Net Core 3.1 This is way to big a change to be able to review but happy to work with you on multiple smaller PRs

BarRaider avatar Oct 27 '20 05:10 BarRaider

Good changes, but as owner wrote, will be good to have smaller PR with specific changes :) Lets try to write on .Net core @BarRaider do you have already start to work on? Can you give the branch?

alexmurug avatar Jul 22 '21 06:07 alexmurug

Shame this never got any further as I have just come in to see if anyone had any forks making events regular EventHandler<T> and making the connection async/task based, this seem perfect.

@Zingabopp did you ever release this anywhere on nuget under a different name or anything?

grofit avatar Aug 09 '22 10:08 grofit

@grofit I haven't done any actual releases with it. I did the conversion to use in another project of mine, so I just added this as a submodule for that. I may eventually do an update for 5.0, but I'm not currently working on it.

Zingabopp avatar Aug 09 '22 20:08 Zingabopp

For the moment I've just dumped the core files into my repo as it's own project and it's working great so far (.net 6)

As mentioned before I would highly encourage the team working on the v5 branches to adopt some of the patterns here.

The biggest thing for me is the event handler changes, as I'm wrapping all events as IObservables it's trivial to do with EventHandler<T> but much more of a pain to do with custom delegates.

Also the current paradigm for connecting and interactions are not clear on what's happening or how to indicate failure states etc, so it's very nice being able to have a bool returned for confirmation of success as well as it all being Task based so you can await etc.

grofit avatar Aug 10 '22 01:08 grofit

Sorry to communicate with @Zingabopp through here, his fork doesnt have issues enabled, but I am having to make some alterations to your branch around certain JSON fields which may have been added since, are you wanting me to fork yours and PR back in any fixes I add or are you not bothered?

(I appreciate with v5 coming soon most of it will be moot anyway)

grofit avatar Aug 11 '22 12:08 grofit

@grofit feel free to make PRs. I also enabled Issues if there's anything else you want to bring up.

Zingabopp avatar Aug 11 '22 20:08 Zingabopp

Closing this as a PR since there isn't really a way to merge something this big (and was relevant for 4.9.x anyways). We can discuss a incremental approach if the community feels they want these features in the new v5

BarRaider avatar Sep 02 '22 22:09 BarRaider