Add point of interest support

[Hofferic]

The tvOS video list contains not only a acessibilityLabel but also a pointsOfInterest list mapping timestamps to resource id's. After some poking around in the resources-15.tar it was pretty easy to find the localised strings referenced by these id's and display them instead of the Location label if available and requested.

Steps to get at the Strings: download .tar bundle get binary plist .strings files from TVIdleScreenStrings.bundle/<LOCALE>.lproj/Localizable.nocache.strings convert to json using plutil -convert json *.strings on macOS (wildcard works) move/rename files to take advantage of android locale resolution

[theothernt]

Thank you for all the work you've done on this feature, it looks really good!

What I'm going to do spend a day or two running the screensaver with the changes, review the code then get back to you with feedback, etc.

[Hofferic]

First fixed issue: Some of the descriptions contain (at most) one linebreak. This is (judging from the screenshots of the "original") there on purpose, so instead of stripping those I adjusted the layout to account for up to two lines of text when using poi data.

[Hofferic]

Apologies for the force-push, I just realized I was using my home-office keys from work and I would rather not have my work profile authoring commits in hobby projects 🤦

[theothernt]

I have a few suggestions...

Prefs screen - GIven that we have 3 states for the location text (simple, poi or none), should be used radio buttons instead? (eg. Settings > Apple TV Videos > Quality)

Video screen - I had been meaning to clean up the aerial_activity and video_view for a while, so I think there are a few changes that can be made to make the logic simpler. There are currently two clocks, clock and altClock. I think there should only be one in video_view so both the location text and clock are in the one file/view.

Then the location text can be set the bottom left or right corner, with the clock relative so it's position on top (ie. no need to use the padding anymore). This will also help in future as I want to add the option to adjust the size of the text (small, medium, large, etc)

Also, the location text should be 1 line for short, 2 lines for poi - which you've already done - but I think the width should be fixed for 2 lines so the text wraps like on the Apple TV version.

Anyway, let me know what you think - and I can help out with some of the changes if you like.

[Hofferic]

Regarding the preferences making it a three-way selection sounds sensible, I am just not quite sure how to best implement that based on the used preference Library. Also how important is it to respect the previous versions Settings when replacing a toggle with a new Setting?

I considered simplifying the layout and logic within but that will require moving away from FrameLayout as the base to something like CoordinatorLayout.

For the text wrapping I am not quite sure how to best implement this - give it too much space and there will be situations where nothing or a single word is line-wrapped; give it too little and some long descriptions (especially in more verbose languages) will fail to fit inside. Some texts already come pre-wrapped with line-breaks, by the way. Any ideas how to solve this? (Also I would prefer to keep that optional too, on my giant tv it looks quite alright the way it is :)

[theothernt]

Prefs screen - To avoid settings issues, the simple solution is to leave show_location as it is, then have an option below that with 2 options: short title OR points of interest. I can use an enum type so store the choice, like with AppleVideoQuality

Layout - Agreed about the change to CoordinatorLayout or similar, if it's something you'd like to do.

Text - Good point, and I didn't realise the POI text had line breaks. In that case, no width restrictions - just 1 line for short desc, 2 lines for POI text.

Defaults - I will be leaving the defaults the same: Show Location enabled, Short title enabled

[Hofferic]

Prefs Screen - I moved some options around to help make connections more obvious, but should be close to what you suggested. Especially the connection KotprefModel-SettingsScreen-Databinding was a neat excercise.

Layout - CoordinatorLayout was a brain fart, I meant ConstraintLayout. I Re-implemented video_view using that. Specifically the text views are arranged in a Flow, which is configured with the appropriate paddings/margins. While doing so I also pared down the aerial_activity.

The Text sizes can now be changed at will and the Layout will "Just Work(tm)". Same goes for the margin Text <-> Screen Border and the (separate) padding between the text elements, all on the androidx.constraintlayout.helper.widget.Flow android:id="@+id/flow".

To Adjust the margins, change the properties

android:layout_marginStart="@dimen/screen_border_padding"
android:layout_marginEnd="@dimen/screen_border_padding"
android:layout_marginBottom="@dimen/screen_border_padding"

to adjust the padding between text blocks adjust

app:flow_verticalGap="@dimen/text_block_Padding"

Those could easily be replaced by a "margin enum" to also allow those to be user-selectable.

[theothernt]

That's looking really good!

And I have just one more suggestion - I was researching the text box height, number of lines, wrapping, etc. I think the easiest solution is to look at all the POI strings for a video, if 1 or more contains newlines/line breaks then set the maxLines to 2, otherwise 1?

This also means the default for short title/location will always be one line like before, it looks like it's set to 2 lines at the moment due to maxLines=2 (both of those properties never work the way I expect them to!)

[theothernt]

One other small thing I noticed. Originally in styles.xml there is a style definition for the Clock and Location Text. It also includes an alpha value of 0.7 as I didn't want the text to be at full brightness.

Now that the Location Text can fade in/out from 0f to 1f, it breaks that. In future, I might allow that to be configured (0.5, 0.7, 1.0) so a variable might make sense. Although I can do this after the PR is merged - it's not too important, plus you've done a lot of work already! 😅

[Hofferic]

The alpha setting absolutely just slipped by me, I assumed the text itself was drawn with a transparent color. Either way, changing the opacity (and making it configurable) should be fairly straightforward using the new uitextsettings, and until then it is easily fixed by replacing the constant.

Your suggested workaround for deciding whether or not to make the text double height just comes with the issue that the clock will then follow the lead and jump up and down between videos. That might be more annoying than the small inconsistency in the bottom padding of the pointext.

[theothernt]

Good point about the Clock moving up or down a bit after each video change. I'll leave it alone for the moment, run the screensaver for a few days and see how it looks.

I'm ok to merge this PR if you are?

The plan over the next week or two is to release a v1.1 beta1 with this feature and a few others. If there are no issues after 2 weeks or so, I'll submit the update to the store.

Currently, I credit any features to the person via the Release page but there'll be a 'credits' part in the app and docs in future.

[Hofferic]

That is all fine by me 👍

[theothernt]

I wanted to get your opinion on something. I was originally planning to have the 'beta' version of Aerial Views as a seperate app (diff. Application ID) so you can have 1) stable and b) beta installed at the same time.

But I realised that's useful for me as a developer, but as a user it's probably better if you have the option to install the beta over the stable (and vice versa) so you don't have to copy settings, or change the screensaver each time - what do you think?

[Hofferic]

Honestly even as a developer I am mostly annoyed at having to constantly adjust the registered package for the active screensaver. As long as there is no usable system UI to select a screensaver (e.g. on firetv) I see no real end user benefit in declaring a host of different options for users to consume if there is a forced interaction with developer tools to use these options.

The basic differentiation of beta/release is relevant for stability purposes in the development context, but for sideloaded applications I see no real problems. At most there may be some inherent implied stability expectations, but at that point using console interactions for general installation steps might be used as a indicator of technical competence in my opinion.

Condensed into a single "opinion" I would say 'update the general binary and allow users to consume that' - allowing for people with very specific opinions to live those outside of watching videos of serene vacation spots - but that is just a slightly inebriated me ;)

[theothernt]

That all makes sense, and I agree. I was messing around with the idea of different versions that could be installed (flavors, build types with diff. App IDs) a couple of months ago but since the release of v1.0 it all seemed unnecessary for this app.

I think the only change to the beta will be the icon + version number!

And thanks again for the POI feature, there are zero issues so far. I just made a couple of small text changes to the Prefs and made all POI text fit on 1 line by removing the \n in some of the text.

Cheers!🍷

[theothernt]

v1.1 is finally out on GitHub and the Play Store - I mentioned your GitHub username in the Play Store's 'What's New' text as well 👍🏻

[Hofferic]

Oh nice, thanks for the heads-up 👍