Skip to content
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

Feature/streamlink and live #659

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

CMorrison82z
Copy link

Implements #658

See annotated code changes. I expect what I've done can be improved.

@CMorrison82z
Copy link
Author

CMorrison82z commented Dec 13, 2024

Beware that I've left a few things a bit of a mess. Wasn't sure how you'd want things organized / implemented so I left them as is.

@CMorrison82z
Copy link
Author

Config for what player to use and stream quality would be good

@Xithrius Xithrius self-requested a review December 13, 2024 23:16
@Xithrius
Copy link
Owner

Xithrius commented Dec 13, 2024

Beware that I've left a few things a bit of a mess. Wasn't sure how you'd want things organized / implemented so I left them as is.

Understandable.

I've had a quick look over the PR, and it conflicts about my intuition for how this could be implemented. The following widget takes from the twitch API, so based on a config/state value the API call could probably just be switched out and the UI could be changed a little to show a tab/header based on if you're currently searching by who you're following and who's live. I see that you already have the logic for that, but I don't think this requires changes of higher level component structures. TLDR change following widget to do a different API call based on config/state value.

Config for what player to use and stream quality would be good

I like that idea. If you want to make a tracking issue for that, go ahead.

@CMorrison82z
Copy link
Author

I can't remember exactly, but TwitchAction being in TerminalAction was causing me issues in SearchWidget when I was messing with how to search & return the selected search item. Also I needed to create a matching struct for the API response to a different call. I'm sure there's a more elegant / straightforward way of implementing this change, this is just what I saw after skimming the source code myself. Feel free to implement this how you see fit.

Also, you probably want to checkout what I'm doing with spawning streamlink and holding onto that process handle, because I didn't really know what I was doing.

@Xithrius
Copy link
Owner

I'll take a longer stare in the near future, thank you for the initial changes!

@Xithrius
Copy link
Owner

Xithrius commented Jan 1, 2025

Hey, sorry I haven't gotten around to this (IRL stuff). Hope to get a proper review going soon.

@CMorrison82z
Copy link
Author

CMorrison82z commented Jan 5, 2025

No worries. Like you said, I may have over-complicated it a bit, so it's a bit extra work to review than it should be :/

@Xithrius
Copy link
Owner

Xithrius commented Mar 8, 2025

@CMorrison82z Sorry that it's been a bit of a while, was focusing on IRL stuff. Could you fix the merge conflicts? Once that's done I think this should be rewritten a bit, could you re-visit this comment that I posted earlier? I feel like there is a simpler way to do this. Also if you want me to take a stab at implementing this instead of this PR, let me know. Thanks!

@CMorrison82z
Copy link
Author

CMorrison82z commented Mar 10, 2025

In my PR, I changed the SearchWidget implementation to be more general over what can be searched for.
I think this ultimately will be a useful change for if/when search widget would be used for searching other things.
And of course, changing the search widget (and TerminalAction) meant propogating those changes throughout all of those other modules.

I think the main issues with my PR are:

  • How streamlink process is created and handled.
  • Lack of configuration options for searching followed channels, and how to launch streamlink
  • Perhaps some style issues

I'll work on getting the merge conflict resolved once we've figured these things out.

@Xithrius Xithrius force-pushed the main branch 3 times, most recently from 0a9a190 to 1e4fbdd Compare March 15, 2025 04:18
@Xithrius
Copy link
Owner

Xithrius commented Mar 21, 2025

Alright, I've poked around a bit and I think I've reduced the complexity on retrieving followed vs followed & live. Made a branch for it, let me know what you think. If it looks good, perhaps we can merge this into your branch and then see what we can do for the streamlink feature?

main...followed-and-live

For now don't worry about styling issues and configuration, those can be handled later. The base functionality of streamlink and followed (and/or live) without crashing and all is the state I'd like this PR to be in when it's merged so we don't end up doing scope creep. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants