search

jellyfin-archive / jellyfin-desktop

Desktop Client for Jellyfin
https://jellyfin.org
GNU General Public License v2.0
180 stars 45 forks source link

[WIP] Overhaul #26

Closed cromefire closed 4 years ago

cromefire commented 4 years ago

Goals of this PR:

This was the original bug to be fixed (fullscreen): image

It would be nice if some one confirms that typescript, prettier and yarn are okay (also " or ', I would prefer ")

BLOCKED BY: https://features.jellyfin.org/posts/148/extend-nativeshell-for-jellyfin-theater

dkanada commented 4 years ago

I've started to like yarn more recently, so that choice is excellent. I can't say I have an opinion on TypeScript yet, but since no one else has stepped up yet to modernize this client for Jellyfin I'd say the rest of your choices are fine as well. Double quotes are best in my opinion.

If you keep your commit messages detailed we can merge the entire history to master, but fixed another error is a bit vague as a description.

cromefire commented 4 years ago

If you keep your commit messages detailed we can merge the entire history to master, but fixed another error is a bit vague as a description.

Yes I'm generally trying that

I've started to like yarn more recently, so that choice is excellent. I can't say I have an opinion on TypeScript yet, but since no one else has stepped up yet to modernize this client for Jellyfin I'd say the rest of your choices are fine as well. Double quotes are best in my opinion.

Well then I'll try to do what I can

cromefire commented 4 years ago

One last question, is there a minimum node version or can I just target node 12?

dkanada commented 4 years ago

Does it just get bundled with electron, or is it an external dependency? As long as most modern systems would work fine with node 12 I'd say that's fine.

cromefire commented 4 years ago

It's bundled, but the developer has to have it

dkanada commented 4 years ago

Node 12 sounds fine then!

cromefire commented 4 years ago

Ramped up the communication (at least for main -> renderer, other way coming soon) to use comlink (A lot more user friendly than postMessage() and removes the need for all the sendJavascript("Random, I bet exploitable, code"))

cromefire commented 4 years ago

And mpv is currently broken

cromefire commented 4 years ago

Is there a svg of jellyfin?

dkanada commented 4 years ago

We have vector graphics here for all the clients.

cromefire commented 4 years ago

Bocked on jellyfin/jellyfin-web#392

joshuaboniface commented 4 years ago

Hey @cromefire I just wanted to touch base on this one. I'm definitely grateful for the work, but it might be a better idea to split this up a bit to reduce the burden of a major rewrite all at once. Any thoughts on this?

cromefire commented 4 years ago

I think once I'm able to have an api to work with it's like a few days to get this done

cromefire commented 4 years ago

The infrastructure is mostly in place I'm only waiting to interface with the web ui (Most things aside from mpv do already work)

JustAMan commented 4 years ago

@cromefire https://github.com/jellyfin/jellyfin-web/issues/392 is there, can you please continue this PR?

cromefire commented 4 years ago

I'm on it, I'm waiting on the API

JustAMan commented 4 years ago

I'm on it, I'm waiting on the API

What API are you waiting for specifically @cromefire ?

cromefire commented 4 years ago

See jellyfin/jellyfin-web#392

JustAMan commented 4 years ago

Oh, I saw that but, as it was closed, I didn't look too close into that. Correct link would be this: https://features.jellyfin.org/posts/148/extend-nativeshell-for-jellyfin-theater