jellyfin / jellyfin-roku

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

Improve entering of server port number on setup screen #366

Closed Germs2004 closed 11 months ago

Germs2004 commented 3 years ago

Is your feature request related to a problem? Please describe. Adding a server to Jellyfin Roku is kind of tedious when using a remote since you have to use the on-screen keyboard. Since the setup page doesn't make it clear that the user does not have to specify a port if they're using the default 8096, users may assume they have to specify the port, which adds more seconds of tedium.

Describe the solution you'd like Give the user 2 textboxes. One for specifying the address, and another for the port. The port textbox could be prefilled with 8096. That will make it clear which port will be used and the user won't have to hunt for the colon character if they want to change the port number.

Describe alternatives you've considered Or, just add a text tip to the screen that tells the user "You only have to specify the port number if you are using a non-default port."

cewert commented 3 years ago

There used to be a separate textbox for port. This was changed to one textbox to match other clients - #115

I also think UX would improve by having the port in a separate box. This would allow the default port to already be filled in which would make it easier and faster overall i.e. local users no longer have to go searching for the colon, some users could just ignore port since they use default etc.

anthonylavado commented 3 years ago

I could argue it either way - all the clients do have a single box now, including Android TV. For my personal setup,I don't have to specify a port at all (though adding it wouldn't break anything).

On the other hand, having at least better help text would make it clearer.

sevenrats commented 1 year ago

Roku right now has relatively simple and deterministic url standardization for urls relating to the server: https://github.com/jellyfin/jellyfin-roku/blob/5dbbc98c0fbfd2fd79c8eb9772ed507747b3dc5f/source/utils/misc.brs#L169 I've opened this feature request regarding this: https://github.com/jellyfin/jellyfin-roku/issues/1278 - Closed this dupe and just commented below.

Edit: This was initially supposed to be a response to jellyfin-meta 35 but since that conversation is mostly about SRV records at the project level, we need a unified place to discuss this specific enhancement for Roku. This channel can be it if nobody is in disagreement.

sevenrats commented 1 year ago

right now we normalize the url deterministically into either 8096 or 8920 based on whether or not tls is used. We should break this loop out to check for the server at a list of inferred addresses in order of priority, such that, for example, when we type in 192.168.1.2, we would check for a server at http://192.168.1.2:8096/, then http://192.168.1.2:80, then https://192.168.1.2:8920/, then https://192.168.1.2:443, continuing until we find a jellyfin server. Or, if https is declared, then we will simply begin with https guesses. If proto and port are declared, we will use the string and not guess at all. Caveats: Obviously this could cause problems if people are running multiple servers over multiple protocols at the same place. Do we care? It would be trivial to avoid, but not intuitive. This is an extremely edge case anyway. It may also potentially lengthen the process time if we dont use concurrent requests, but it looks like we do have asynchronous gets in brightscript so it should not be impossible to do so.

cewert commented 1 year ago

@sevenrats I think having a separate textbox for port and forcing the user to input a port would be a better solution than guessing at the port, shooting off API requests, and hoping one comes back with data. What if none of them reply? What if more than one reply? How long do we wait? What does that error message look like? Do we tell the user all the ports we tried to connect to? "Could not connect to Jellyfin server @ http://192.168.1.2:8096 or http://192.168.1.2:80 or https://192.168.1.2:8920 or https://192.168.1.2:443 etc."

I propose we create a separate text box for port and prevent the submit button from working if the port is blank (or enable the submit button but show an error message about port on submit), have the port filled in with the default 8096, and use a different keyboard for inputting port that only shows numbers (to improve UX).

We could even take it a step further and have a dropdown for HTTPS/HTTP with HTTP selected by default. If the user selects HTTPS and port still equals 8096 then we change port to the default HTTPS port of 8920. This would force all server URLs to have a protocol and a port, prevent us from having to reformat the server string and make guesses about ports, and would be the best UX (at least IMO).

sevenrats commented 1 year ago

well, I normally love your ideas but I do not love the idea of a second box.

What if none reply: Then we are in the same state as when a single server fails to respond now. The user needs to try different input. This will only happen less with my proposed changes. What if more than one reply: We take the first one. The requests will all fire asynchronously so the first message to hit the port will be our winner. It will be an extremely unlikely case where multiple servers are running at different ports in unpredictable ways. usually we will see one port or standard http->https redirects. How long do we wait? We wait the same time we wait now. However long our timeout is is out of scope. My proposed implementation won't change it. What does the error message look like/Do we tell the user all the ports we tried? This is interesting and I think I could argue either way. On the one hand, I think we should inform them what we tried, on the other hand, I feel like we can just use our current message. "Could not connect." Essentially, youve not provided me enough info to proceed, enter something else. I am working on the "library" I mentioned in chat rn and once its dont my plan will be crystal clear. I honestly feel like just checking these four cases (our two if either port or proto are declared, or just one if both are declared) is not that big of a deal, request wise, and maybe we don't need to tell the user much because it will feel intuitive. certainly more intuitive than having to manually enter the proto's default port.

cewert commented 1 year ago

I don't think I did a good job of explaining the reasoning behind my proposal so let me try this again.

I think your proposal about attempting to connect on the default port is a good idea and an improvement to what we have now but I think having a separate textbox for port is an even better improvement and should be the ultimate goal as far as this issue is concerned ("improving entering of server port on setup screen").

I don't like the idea of attempting ports other than default because I think it only helps the user if they know we are going to try other ports. Attempting the default ports is intuitive to me but once we start trying other ports now you need to update the GUI to tell the user and you need to add that info to the error message. If you don't tell me you're trying port 443 why would I assume the app will try it? Only a portion of our users will be on port 443 and of those users how many of them will assume we are going to try that port? I don't think we would be helping many people in the end and only complicating things.

Reasons I think a textbox is better than guessing:

I believe the added transparency and UX improvement for servers on strange ports makes the second text box the better solution. The only downside vs your proposal would be users on port 443/80 having to type out their port. I think this is a fair trade off especially when considering that the UI would need to be updated so these users know they don't need to type their port in the first place. If we don't tell them they will probably still type out the port which would be worse UX than the separate textbox since they'd need to find and use the colon.

sevenrats commented 1 year ago

yeah when I was half way through writing my PR I realized our ideas didn't even conflict. I don't hate the idea of mitigating the ridiculous entry pattern of hunting down the colon that is ten miles away. I'm open to solutions to that. A second text box is a good one. Maybe another is a custom keyboard that puts everything where it should be. I would prefer a solution that was aesthetically in line with other 10-ft apps, I don't think a separate box for port is a big deal. I dont really agree that we need to keep the user so up to date. If we find a server at 443 and we connect, the user is happy probably. If we don't find a server there, we can just do a "Your input wasn't enough to get us there" type error like we do now. I think on roku the error says "The server could not be reached. Is it online?" The behavior in my new PR is heavily motivated by the kotlin sdk and the android tv app. We should just use whatever error string they use.

cewert commented 1 year ago

Apparently using port 80/443 is more common than I thought so it makes sense to me now that we try and make things as easy as possible for those users.

The only problem is that in order to make life easiest for them we have to eliminate the textbox for port - that way they don't need to type 80/443. But by eliminating the textbox we alienate our users that use strange ports - they will always have to hunt down the colon and numbers using the same keyboard.

Possible solutions:

sevenrats commented 1 year ago

now that you lay it out so clearly, I want every single thing you just said. lets do it all. special keyboard on the input if we can make it fit, keep full parsing on the string so current behaviors keep working, and also add your "advanced features" with specialized keyboards, and also run all these entries through the guessing logic. I think thats all fantastic.

question though. what if the user enters a port in the main url field and in the port field? i think it would be easiest to just use the port-awareness from 1280 and include checking the special port field as part of the branch when a port was not included in the main url field.

edit: We should not pre-populate the port text box. let it be blank and we will check mainurl with regex, then check the new port box, then guess if both did not yield a port. i will put a comment in 1280 indicating where i mean when i address your comments on it.

cewert commented 1 year ago

what if the user enters a port in the main url field and in the port field?

I think this is the best solution yet. Let me know what you think.

We have a textbox for port with a default value of "auto" that is always shown to the user. If the user doesn't specify a port in their URL string and port = "auto" then we try default ports + 80/443. If the user does specify a port in their URL string then we remove the port from the URL string on submit (not to submit the page but when they submit the string and leave the keyboard) and then show the port number in the port textbox instead of "auto".

This would prevent multiple ports being specified as well as make life easiest for users with port 80/443 as well as making life easiest for users with strange ports. After selecting the port textbox, we could explain that leaving the port blank will result in us trying to connect on default ports and 80/443.

sevenrats commented 1 year ago

if using the string "auto" as a port does not break the urlregex then I think this is great see edit. this particular regex is used elsewhere in the code and should probably be produced by a module now. i think it would be alot easier to implement the usually-hidden advanced features because we could even just drop a standarddialog and be done in seconds with existing code. redoing the setserver scene will be much more complex. ultimately its your code to write, so maybe my opinion will matter to you or maybe they wont. 1280 should just plug into the existing logic and not be too much in your way when you go to change it. I do think there is a lot to be said for just doing what kotlin and android tv do, which is just transparently make these guesses and not worry to much about it. Ulitmately that is my vote.

i thought about it more and I think your proposal is too much just to solve the two problems of "entering a port sucks" and "you have to enter the full proto:domain:port phrase every time". 1280 should solve the second problem, and reduce the first to extremely rare cases. so we are left with "entering a port sucks in extremely rare cases" and I think what you are describe is a very heavy ui change just to solve that remaining problem. I suggest we solve the remnants of problem one with a custom keyboard.

cewert commented 1 year ago

My latest proposal is the only way I can think to solve the main problem I just mentioned:

The only problem is that in order to make life easiest for them we have to eliminate the textbox for port - that way they don't need to type 80/443. But by eliminating the textbox we alienate our users that use strange ports - they will always have to hunt down the colon and numbers using the same keyboard.

Do you disagree? How else can we help our users with strange ports?

i thought about it more and I think your proposal is too much just to solve the two problems of "entering a port sucks" and "you have to enter the full proto:domain:port phrase every time". 1280 should solve the second problem, and reduce the first to extremely rare cases.

You are saying that users with strange ports are "extremely rare"? I disagree. Do you have data to back that up? Even if they are rare I don't understand why you would be against a solution that makes life better for everyone. Yes, it would require changing the GUI and be much more difficult to write than your current PR but so what? I thought we were discussing the best solution possible not the easiest one to implement or the one that complements 1280 the best.

From what I can gather, your criticism of my latest proposal is that it isn't compatible with 1280 and would require changing the GUI. Is that all? I don't think either one of those are a good enough reason to can the idea.

sevenrats commented 1 year ago

"You are saying that users with strange ports are "extremely rare"? I disagree..." : No I have no data to back this up and you are right that we should improve their lives. I just mean that they are rare in the sense that I assume that the four default ports are probably the vast majority of cases. Certainly paging over to the colons and slashes when you have to is a nightmare we need to fix for those people. I just think that problem could be solved lighter ways that what you've proposed. Like an improved keyboard.

"My latest proposal is the only way... Do you disagree? How else can we help our users with strange ports?": I don't have the golden ticket for this but I agree this is where we should focus our thought, for sure.

"From what I can gather, your criticism of my latest proposal is that it isn't compatible with 1280...": No, i agree that would be a bad reason. I think your proposal is perfectly compatible with 1280, or at least, it is as long as you don't prepopulate the port field (this is a worse idea because it can only ever make one guess, while the logic in 1280 will make 2 or 4 guesses). I just think it's a very heavy solution to insert the new scene components and the string construction logic. The impact on 1280 is essentially just a single branch as long as you do not insist on prepopulating the port field. "auto" would probably work, but this would still fall under my criticism of "its just too much change versus the problem".

ultimately this is your decision, obviously, unless you ask me to write it. 1280 will be ready to land tomorrow and it would be really easy for you to plug either of your ideas into it, except for prepopulating the port.

can you help me understand what you don't like about just silently succeeding based on the minimum info, treating all urls as complete and concealing the entire hooha from the user? thats what happens elsewhere in jellyfin land. to be absolutely clear, my solution for people who must enter the port is to just make that simpler, hopefully with a custom keyboard that we can keep in npm off the repo entirely, and thereby add no code at all.

sevenrats commented 1 year ago

the android app and the androidtv app, by virtue of the kotlin sdk, both notify the user of all the url's they inferred and tried. I still don't want to tho. Or if we do, do it in a succinct way in the error string field thats already there (our users have to be used to unhelpful data being crammed into that space by now) "Failed to find server with any standard port or protocol." Or the low effort solution is to use the existing, already translated string which is something along the lines of "Failed to connect to server. Is it online?"