jellyfin / jellyfin-roku

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

Rewrite GUI/layout code for 1080p #74

Closed cewert closed 1 year ago

cewert commented 5 years ago

As mentioned in #70 the current code base is written for 720p and 1080p. Rewriting for 1080p only and letting Roku auto scale should allow for cleaner easier to read code and prevent future bugs.

As mentioned in the Roku docs - try to make all spacing and layout sizing divisible by 3, which will allow Roku to resize to a lower resolution without glitches.

cfunseth commented 4 years ago

I'd like to contribute to this project and this seems like a fairly simple issue to start on. I'm not familiar with Roku development so I just want to be sure I'm understand your request. Would this change be as simple as updating the associated XML files for each screen, or are there changes needed for the BrightScript files as well?

bisby commented 4 years ago

Originally some of the code was written assuming there would be variable sizes, so things were written as dimensions * 0.15 (or something like that) to get widths of elements.

If we are writing everything to be a fixed 1080p, we can assert things more confidently, just width = 300 and let Roku do the scaling.

So basically finding all the places where we try to cleverly set sizes on the fly, and then set that statically.

For example:

https://github.com/jellyfin/jellyfin-roku/blob/9e84a7b8c790f0f752ce15025dde1ab83ed692c1/components/Pager.brs#L49

Here, we use the dimensions to place the paginator centered at the bottom of the page. dimensions/2 could easily just become 960 instead of doing a calculation every time. (having the values set specifically also makes it easier to tweak)

cfunseth commented 4 years ago

Excellent, that helps get me started.

What would the preference for you all be in this instance? Would we want to hard code dimensions in each function, or use references to other scenes like shown in the Pager.brs example with the assumption that the parent node is already following the Roku guidelines?

bisby commented 4 years ago

In most cases hard coded values. "Dimensions" can be assumed.

There might be a few edge cases where something needs to be abstract (if we re-use a component in multiple places and need different params each time, calculating size on the fly makes sense).

But in the Pager example saying translation = [960, 1022] would be enough. And since it's a static value, it can just go into the init() function and we could throw away the entire updateLayout function.

If the [960, 1022] is confusing as to "how we got these numbers", including a comment like "centered full width horizontally, and centered vertically in the bottom 115 pixels" might be useful.

n76 commented 4 years ago

What is the status on this? If I take this as a task would I be in conflict with someone else working on it?

cewert commented 4 years ago

@n76 I don't think anyone is working on this

182 made things difficult. The GUI should still be written designed for 1080p but dynamic enough to handle slightly different resolutions (1080p-ish)

cfunseth commented 4 years ago

@n76 I updated the config screen late last year but got so confused by how BrightScript works that I was too nervous to attempt any of the other screens. I haven't worked on it any further so there shouldn't be anything to conflict with.

n76 commented 4 years ago

Okay. No guarantees, no time line, etc. But I'll start playing around with this.

From my first look at things, it seems that if a 1080 screen size is used then a lot of the calculations that are in the BrightScript turn in to constants. And that, in turn means that a fair amount of the object definitions can be moved to the XML which should aid in separating layout design from logic and make things easier in the future.

With respect to issue 182, I think following Roku's advice on safe zones will be needed. That requires some UI design considerations, a bit more than just mechanically replacing computation with constants and moving display objects to XML if it makes things cleaner.

I suspect though that having the UI object definitions cleaned up would make looking at the UI design changes needed to address 182 a bit easer.

cewert commented 4 years ago

I never seen that link on safe zones good find.

Well if we follow the safe zone advice we should still be able to hard code for 1080p. Exceptions being things like ItemList() being worked on in #206 which should stay dynamic since they are reused for multiple screens.

Edit: Got distracted by your amazing link sorry. Separating logic from layout when possible sounds like a great idea