Skip to content

tvOS Support#500

Closed
lechium wants to merge 22 commits into
FLEXTool:masterfrom
lechium:tvos
Closed

tvOS Support#500
lechium wants to merge 22 commits into
FLEXTool:masterfrom
lechium:tvos

Conversation

@lechium
Copy link
Copy Markdown

@lechium lechium commented Jan 5, 2021

@NSExceptional i branched from your most recent commits to master and then re-applied my changes to the project, this make the commit history much smaller (the old one was largely useless and way to large anyway) and should satisfy your request for a linear history.

@lechium lechium mentioned this pull request Jan 5, 2021
@NSExceptional
Copy link
Copy Markdown
Collaborator

You should probably convert this to a draft PR until you're totally done, for what it's worth 😅 I see you're still working away, keep it up for as long as you need!

Also I haven't looked at the chances much, but make sure you're not throwing in changes unrelated to tvOS support 👍🏻

Also, could you please start capitalizing your commit messages? 🙂 Thanks!

@lechium
Copy link
Copy Markdown
Author

lechium commented Jan 5, 2021

@NSExceptional I think im actually done for now, i noticed some things i missed last night hence the additional commits. tvOS support is the only thing ive worked on, I know you like to keep MRs very specific to the goal at hand! ill try to do better in future re: commit message capitalization, grammar and usefulness in general.

@lechium
Copy link
Copy Markdown
Author

lechium commented Jan 7, 2021

@NSExceptional on second thought maybe you are right about converting this to a draft MR, I'm just not quite sure how to do that! edit just seems to let me change the title and nothing more. Feel free to convert it yourself if that is something you can do on your end. thanks!

@NSExceptional NSExceptional marked this pull request as draft January 7, 2021 08:01
@NSExceptional
Copy link
Copy Markdown
Collaborator

Oh, I guess only I can do that, whoops. It's a draft now, let me know when it's ready!

While I'm here, do you mind if I force-push to the branch to capitalize all of your commits? You will have to do a hard reset on your end to avoid a merge, or you can rename them yourself and force-push

@lechium
Copy link
Copy Markdown
Author

lechium commented Jan 7, 2021

@NSExceptional thanks! i dont think i know where id have to reset to or how. im not very good at amending git after commits have happened either. ill see if i can figure it out over here. would be a good thing to get better at.

… as applicable. Obscure a private API call, fixed runtime browser issues on tvOS.
…oller to FLEXAlert style, fixed an accessory view style I overlooked, updated FLEXInjected to be smarter.
…r date picker with UTC timezone. Added hybrid layout mode for picker to prevent it from setting the frame width. Fixed the 'clear' button frame to match the done button frame width and x origin. Fixed web view protocol.
@NSExceptional
Copy link
Copy Markdown
Collaborator

NSExceptional commented Jan 13, 2021

@lechium I have a few comments since I see you are still adding commits since I last checked:

  1. Make sure you're only making changes related to tvOS support. Do not fix any other bugs or anything in this PR; break them out into a separate branch starting at FLEXTool/master and make a new PR with them. (ie 4557bd0)
  2. Do not try to obfuscate private API usage as you have been doing. Very soon I will update the FLEX license to disallow it's use within app store apps, so obfuscation of private API usage is not necessary and should be avoided.
  3. May I ask what you need flex_findFirstSubviewWithClass for? Enumerating subviews is terrible practice, even when using private APIs. There is almost always a better way, unless the subview in mind is not referenced by anything else, which is rare.
  4. Can you tell me what UIFLEXSwitch is for? And any other classes you've been adding? I didn't think a PR like this would require new classes
  5. This is an anti-pattern:
    if ([self darkMode]){
         self.view.backgroundColor = [UIColor colorWithWhite:0.0 alpha:0.8];
    } else {
         self.view.backgroundColor = [UIColor colorWithWhite:1.0 alpha:0.8];
    }
    Check out FLEXColor. The background color should be set to a dynamic UIColor that will automatically change with light mode or dark mode. (I've never built a tvOS app so apologies in advance if this API is unavailable on tvOS 13 or something) Similarly, direct usage of UIColor like this should be rare. Try to use FLEXColor first, we have colors for most things. Missing colors should be added.
  6. I haven't looked at most of the code yet, but make sure you're following the coding practices set by the codebase. I.e, try to use properties instead of methods where it makes sense and always use dot syntax when accessing a property.
    ([self darkMode] seems like a perfect candidate for what I'm referring to, but also you shouldn't need that method at all as per no. 5 above)

…s focused, added inverse UIColor category to FLEXColor for this exact purpose. Added a variable back that was needed for iOS and fixed building there again.
@lechium
Copy link
Copy Markdown
Author

lechium commented Jan 13, 2021

@lechium I have a few comments since I see you are still adding commits since I last checked:

  1. Make sure you're only making changes related to tvOS support. Do not fix any other bugs or anything in this PR; break them out into a separate branch starting at FLEXTool/master and make a new PR with them. (ie 4557bd0)

All the changes I am making are solely related to making tvOS a workable experience, a LOT of code needed to be changed / updated / written, this was unavoidable unfortunately.

  1. Do not try to obfuscate private API usage as you have been doing. Very soon I will update the FLEX license to disallow it's use within app store apps, so obfuscation of private API usage is not necessary and should be avoided.

Noted, I will discontinue doing that, and undo it in the places where I already have.

  1. May I ask what you need flex_findFirstSubviewWithClass for? Enumerating subviews is terrible practice, even when using private APIs. There is almost always a better way, unless the subview in mind is not referenced by anything else, which is rare.

Its funny you should ask, try changing the background color of a UIButton on tvOS without it! haha. I spent hours poking around trying to get a reference to this particular layer (look at this crazy hierarchy), the class highlighted that I search for has been the backing class since tvOS 9 and the layers/classes in between have changed several times, this is used to change the background of our faux UISwitch class that you inquire about in the next question.

  1. Can you tell me what UIFLEXSwitch is for? And any other classes you've been adding? I didn't think a PR like this would require new classes

The vast majority of this effort was providing classes or usage of #if *TARGET_OS_TV for things marked forbidden/unavailable on tvOS. Among those verboten classes are UISwitch, UIDatePicker, UIPickerView, UISlider, UIWebView, (WebKit exists but I can't get it to work at all on tvOS, after months of trying for other projects) UITabBar, UIPasteboard, UIMenuController, UIDocumentController (the list goes on) I have recreated the most necessary of the list by either writing them from scratch (KBDatePickerView) or porting swift projects (KBSlider is a Obj-C port of https://github.com/zattoo/TvOSSlider) I have not added any new class that wasn't 100% necessary. I've tried to keep everything I've added confined to the same folder here. UITextView's don't work the same as they do on iOS either so i had to re-purpose and modify a class I wrote for nitoTV to creatively recreate what is needed on that end.

Here is what the faux cobbled together switch looks like.

UIFLEXSwitch

  1. This is an anti-pattern:

    if ([self darkMode]){
         self.view.backgroundColor = [UIColor colorWithWhite:0.0 alpha:0.8];
    } else {
         self.view.backgroundColor = [UIColor colorWithWhite:1.0 alpha:0.8];
    }

    Check out FLEXColor. The background color should be set to a dynamic UIColor that will automatically change with light mode or dark mode. (I've never built a tvOS app so apologies in advance if this API is unavailable on tvOS 13 or something) Similarly, direct usage of UIColor like this should be rare. Try to use FLEXColor first, we have colors for most things. Missing colors should be added.

It's funny I didn't realize sooner I could use primaryBackgroundColorWithAlpha from FLEXColor based on the nice little comment I left in there based on the massive changes I needed to make IN that class for it to build on tvOS without patching the SDK. The next commit will have darkMode category and usages completely excised, good note thank you!

  1. I haven't looked at most of the code yet, but make sure you're following the coding practices set by the codebase. I.e, try to use properties instead of methods where it makes sense and always use dot syntax when accessing a property.
    ([self darkMode] seems like a perfect candidate for what I'm referring to, but also you shouldn't need that method at all as per no. 5 above)

I'm trying to do my best of adhering to your stylistic choices / best practices. If there are any other glaring examples please let me know.

@NSExceptional
Copy link
Copy Markdown
Collaborator

Oh gosh. I didn't realize it was that extensive.

I'm now reconsidering whether this should be merged if it's going to require us tearing up so much of the codebase. I think the approach I would have taken is to just disable any screens that would present any of those controls. :/

What do you think? Do you really want to be able to change ivars and properties on tvOS?

@lechium
Copy link
Copy Markdown
Author

lechium commented Jan 13, 2021

Oh gosh. I didn't realize it was that extensive.

I'm now reconsidering whether this should be merged if it's going to require us tearing up so much of the codebase. I think the approach I would have taken is to just disable any screens that would present any of those controls. :/

What do you think? Do you really want to be able to change ivars and properties on tvOS?

Yes, without those abilities this is like 1/100th as useful, and all of this already works beautifully. If you don't want to merge it then it's not a huge deal. I've done my best to make it as non-invasive as possible. The iOS version still builds and functions normally with no bugs or breaks anywhere. I mean I did write a UIDatePicker from scratch encompassing 99.99% of the functionality with full API compliance with its iOS counterpart, i'd be a little hurt if you didn't consider figuring out a way to merge this in! haha (JK, honestly, no pressure. you can point ppl towards my fork for tvOS support if worst comes to worst)

EDIT: In addition if you removed EVERY class that used ANY forbidden view you'd literally be left with nothing at all. #if TARGET_OS_TV in some form hits almost every UIViewController subclass in the project I think.

Most of the things here would be left out if we avoided all missing controls:
flex_latest

if it makes your decision any easier I would have zero expectation moving forward of you making changes keeping in mind tvOS needs. I would take full responsibility for making sure any changes you add or merge in would still work okay on tvOS. On the flip side I also promise to thoroughly test any changes I make to make sure they work in iOS and tvOS.

@NSExceptional
Copy link
Copy Markdown
Collaborator

NSExceptional commented Jan 13, 2021

In addition if you removed EVERY class that used ANY forbidden view you'd literally be left with nothing at all. #if TARGET_OS_TV in some form hits almost every UIViewController subclass in the project I think.

Heh, well my initial vision was to just throw that around everywhere until the project compiles. I didn't really expect anyone to want to edit properties and ivars or call methods with a remote 😅 I did not foresee the desire to wrap all unavailable controls, I was just thinking it would be more geared towards looking at the view hierarchy or the network traffic, browsing container files, etc

Your work is impressive, and I don't want to ask you to throw away so much of it. Would you be okay with me pointing towards your fork in the readme?

Additionally, if you want, I am still open to merging my original expectation of this PR (basically #ifing out unavailable APIs and screens until it compiles and works) if that would make it easier for you to maintain your fork and merge in new changes?

What are your thoughts?

@lechium
Copy link
Copy Markdown
Author

lechium commented Jan 13, 2021

In addition if you removed EVERY class that used ANY forbidden view you'd literally be left with nothing at all. #if TARGET_OS_TV in some form hits almost every UIViewController subclass in the project I think.

Heh, well my initial vision was to just throw that around everywhere until the project compiles. I didn't really expect anyone to want to edit properties and ivars or call methods with a remote 😅 I did not foresee the desire to wrap all unavailable controls, I was just thinking it would be more geared towards looking at the view hierarchy or the network traffic, browsing container files, etc

Your work is impressive, and I don't want to ask you to throw away so much of it. Would you be okay with me pointing towards your fork in the readme?

Additionally, if you want, I am still open to merging my original expectation of this PR (basically #ifing out unavailable APIs and screens until it compiles and works) if that would make it easier for you to maintain your fork and merge in new changes?

What are your thoughts?

Those changes alone are staggering, at that point I don't see the issue with adding a few new classes, I don't override any of the built in IOS classes with my own, they are just provided when there isn't an equivalent. It just seems kind of silly to only go half way on this. Just getting it to build requires HUGE changes across the board, take away my special classes you only remove 16 out of 198 #ifdefs across 70 different files and completely handicap the functionality of the project, just to leave out a few new classes.

@NSExceptional
Copy link
Copy Markdown
Collaborator

NSExceptional commented Jan 13, 2021

Like I said, I really only expected tvOS support to include features that don't involve typing text or editing values. I'm just not comfortable wrapping or duping so many core UIKit classes just to support an extreme feature set on a platform with already very little demand, and including it alongside all the iOS specific code.

All that code becomes code we now have to maintain, and it adds a huge amount of additional complexity to the project.

Maybe what we should work on is modular using the internals of FLEX and breaking out the UI into separate modules, so that it is less intrusive to add support for new platforms? This would make it easier to add the long awaited macOS support too

@lechium
Copy link
Copy Markdown
Author

lechium commented Jan 13, 2021

Like I said, I really only expected tvOS support to include features that don't involve typing text or editing values. I'm just not comfortable wrapping or duping so many core UIKit classes just to support an extreme feature set on a platform with already very little demand, and including it alongside all the iOS specific code.

All that code becomes code we now have to maintain, and it adds a huge amount of additional complexity to the project.

Maybe what we should work on is modular using the internals of FLEX and breaking out the UI into separate modules, so that it is less intrusive to add support for new platforms? This would make it easier to add the long awaited macOS support too

It's your project, I wouldn't ever want to make you uncomfortable with how it's being managed. I was just trying to point out how little things would change if you went in that particular direction with it. Making things more modular is always a welcomed & logical approach to these kind of problems, and I'd obviously have to let you take the lead on that.

We can close this MR if you want and you can update the README to point people towards my fork in the meantime for tvOS stuff. I'll still work on it while I have time until im satisfied with its state, and I'll maintain releases as I have been that are in line with the ones i release on my repos for jailbroken devices. Hopefully I'll have time when the refactor occurs and I'll adopt the newer paradigm(s) as appropriate.

@NSExceptional
Copy link
Copy Markdown
Collaborator

I forgot to close this, but I did at some point update the readme to point others to your fork :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants