search

jellyfin / jellyfin-roku

The Official Roku Client for Jellyfin
https://jellyfin.org
GNU General Public License v2.0
412 stars 129 forks source link

Server lookup doesn't support base URL #95

Closed bisby closed 4 years ago

bisby commented 4 years ago

Describe the bug

The base URL changes that came in 10.4.0 and 10.4.1 mean that more people have host paths like http://myserver.local:8080/jellyfin ... however we form our API requests based off of hostname and port alone.

To Reproduce

  1. Set a base URL in your jellyfin management dashboard.
  2. Try to connect to server in Roku app
  3. "Server not online"

Expected behavior

The server is found and connectable.

Logs

Screenshots

Additional context

piajesse commented 4 years ago

I also can not log in, I get the following from the logs...

BRIGHTSCRIPT: ERROR: ParseJSON: Unknown identifier '<!DOCTYPE html>
<html data-culture="en-US" lang="en" class="preload">
<head>

    <meta name="viewport" content="width=device-width, initial-scale=1, minimum-scale=1, maximum-scale=1, user-scalable=no">
    <link rel="manifest" href="m: pkg:/source/api/baserequest.brs(70)
Server not found, is it online? New values / Retry

I'm gonna play around and see if I can come up with a patch

piajesse commented 4 years ago

I fixed the issue, once I figure out how to submit a patch I'll pull request it

bisby commented 4 years ago

Unless you need/want /jellyfin/ as your base URL, youc an also go to /web/index.html#!/networking.html and change "Base URL" to be empty.

piajesse commented 4 years ago

Thanks, I had no idea, I currently run rokus, so I've been stuck with Plex, but the whole update where you can stream free content made the interface really nasty, and a pain to get to my media. So I've been trying to make a full on approach to run Jellyfin :D

Now, onto this issue, if you can give me an example of what should be implemented on the roku app so I can tweak it to get it right (I'm a developer, not a designer :stuck_out_tongue:). Here is what I think I will work on until this issue is thought out...

piajesse commented 4 years ago

I think we should follow the route jellyfin android went. Just a single box with domain/IP and port Screenshot_20191214-195726_Jellyfin

bisby commented 4 years ago

Also a developer not a designer :(.

We need a whole rework of the "enter server info" screen in general. But for this specific thing, we would need just an extra box ("hostname", "port", "baseURL"), so that we could map it onto API requests, if it is set: if settings(baseURL), then API = API + "/baseURL/" kind of thing.

Or maybe we do some sort of auto discovery of baseURL based on /jellyfin/ or not. or we ditch the hostname/port idea and you just have to input the whole server URI in a single field (https://myserver:8096/jellyfin/) to avoid adding any extra questions on the login screen.

unsetting the base URL in the server networking dashboard has been good enough for me that I havent put too much thought into it.

and seeing the screenshot. I would 100% condone that. :+1:

piajesse commented 4 years ago

I'll lookup how the app version does it logic wise and make it work the same way (client side, not codebase).

Thanks for the opinion!

piajesse commented 4 years ago

so I think the codebase can use a lot of rework, right now when ever it requests something from the api (every single time), it calls a function called APIRequest which calls buildURL and server_is_https which both use get_setting a couple times. Now I don't know how fast that method is or if it even reads from the storage on the roku every time, but I think we should store these values and read them from a single variable, and only do all the "server_is_https" logic on first call.

Opinions matter to me, so please share what you think. I'm gonna look at more of the abilities of bright script before I try this though.

bisby commented 4 years ago

I dont know enough about where Roku stores everything. I'm 99% sure though that this isn't a bottle neck, so I haven't bothered.

The main other way I could think of doing this would be to instantiate things so that m. contains the information you want. but then you have to make sure you keep the scope lined up properly.

This isn't the greatest code, but other methods I've seen in Roku didn't seem to be any cleaner, and this explicit way seems to be the most straightforward.

But by all means, feel free to look into this and propose alternatives.