Skip to content

refactor(core-modules): implement createNativeView and initNativeView for all components #6102

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 42 commits into from
Sep 26, 2018

Conversation

farfromrefug
Copy link
Collaborator

@farfromrefug farfromrefug commented Jul 21, 2018

This PR propose a simple refactoring to make life easier for plugin developers.
I am writing a plugin which brings material components for iOS and Android.

in material-components, Google simply inherited native components to create material ones (UIButton, UITextField, ...)
So creating a nativescript component was pretty straight forward as I simply had to subclass the corresponding tns components and replace the createNativeView

Except that It was not working for some reasons:

  • some native views were created in the tns constructor component. So if I was subclassing some native components were unnecessarily created (iOS label is an example)
  • delegate/listener were created in createNativeView. So I subclassed createNativeView I was loosing the delegate/listener setup, I then had do it myself and also re-define those delegate/listener classes exactly the same way it was done in Nativescript (unnecessary)
  • in the case of the android textfield the material component was actually made of 2 components, the layout and text widget. Which means that, in nativescript, any layout operation is to be applied to the native layout widget. All text operations should be applied to the text widget. For now I had to use a dirty, not fully working working, overriding of the nativeViewProtected getter to either return the layout or the text widget. That PR actually defines a nativeTextViewProtected getter that is used for all text related operations. By default it returns nativeViewProtected That way a plugin can simply override it to use it's own widget. This has no performance consequences.

All those are really just refactoring which does not impact Nativescript performances. However they do make plugins more performant because the prevent redefining already defined Nativescript delegate/listener classes. But also because they prevent unused native widget creations for plugins subclassing some Nativescript widget classes like Label.

The only thing I am not sure about if is if createDelegate which I call in createNativeView could be called in initNativeView instead. I am waiting for some feedback from you on this.

PR Checklist

What is the current behavior?

normal

What is the new behavior?

unchanged

@ghost ghost added the ♥ community PR label Jul 21, 2018
@SvetoslavTsenov
Copy link
Contributor

test

@farfromrefug
Copy link
Collaborator Author

@SvetoslavTsenov what does that mean? Also I can't see the details log of ci/jenkins/core-modules-tests — FAILed. So I can't see what to fix

@SvetoslavTsenov
Copy link
Contributor

Hey @farfromrefug, I have executed some integration tests on our CI (which is internal). The result so far is that unit tests are failing for both platform and uitests app crashes on start up(you can find it in app folder). Could you please rebase your branch again and run unit tests on you side. To do that you can simple do:
npm run setup npm run tsc cd tests tns run ios/ android

Let me know if you need some further information.

@farfromrefug
Copy link
Collaborator Author

@SvetoslavTsenov I realised the issue now. On iOS createNativeView is not used.
I also realise that there is a comment about this in the code saying it should be done the same way as in android and use it.

DO you want me to expand that PR to do that? I am willing to but it would be more changes for iOS.
Could make that PR harder to merge.
I
I still defined createNativeView for iOS Label and Button but call It in the constructor. Also I only create if not already created.
This does not follow the same rule as Android. But otherwise tests will fail.

@farfromrefug
Copy link
Collaborator Author

@SvetoslavTsenov still can't see what tests are failing with ci/jenkins/core-modules-tests — FAILed.. Details page brings fail to open page

As for the tests app. It succeeds now on iOS but I keep getting waitUntilReady Timeout., Stack: Error: waitUntilReady Timeout. errors on android. Any idea?
Finally I have a lot of tests failing on android not related to what I have done. Like :
Test: --- [ACTION-BAR.test_ActionBar_set_title_as_number_doesnt_thrown] FAILED: Expected: false, Actual: true, Stack: Error: Expected: false, Actual: true

So not sure how to know what makes the cli fail

@MartoYankov
Copy link
Contributor

Hi @farfromrefug, thanks for the PR and thumbs up for the material components plugin. Just a heads up - the repo you linked to the plugin appears to be private, so we can't take a look.

I reviewed the PR, discussed it with the team and we agree with the changes.

  • Just for context and reference, I think you should take a look at ViewBase's setupUI method. This is the place where createNativeView() and initNativeView() are called and where the backwards compatibility for native view create in constructor is handled.
  • Also, if you haven't already, take a look at this article in the docs - https://docs.nativescript.org/plugins/ui-plugin-custom#building-ui-plugins-using-custom-components - largely explaining what you are trying to do.
  • We consider the iOS components native view create in constructor as technical debt and we should move to createNativeView() on all components eventually. The goal is to have a unified api with the Android components.

That being said, here are some comments on how to improve the PR:

  • On all the iOS components where you added createNativeView() remove the constructor altogether. The createNativeView() method will be called from the setupUI() method.
  • There is no need to couple the new createDelegate(nativeView) method with createNativeView(). I think you even forgot to call it in editable-text-base.android.ts. My suggestion is to call this method in setupUI() right after createNativeView(). Of course this would mean to add it in view-base.d.ts and view-base.ts with an empty comment for backwards compatibility.
  • Also, please add a comment to the nativeTextViewProtected getter on why we are doing this and why you would want to override it.

@farfromrefug
Copy link
Collaborator Author

@MartoYankov Thanks.
Sorry about the link. I fixed it:
https://github.com/Akylas/nativescript-material-components

@farfromrefug
Copy link
Collaborator Author

@MartoYankov great comments.

I will look at all that !

Thanks

@farfromrefug
Copy link
Collaborator Author

@MartoYankov Made changes based on your comments.
There is one thing bugging still. In text-field.ios.ts we now create to weak references to this because createNativeView and initNativeViewDelegates are not called in the same place anymore. Is that a big deal?

Should I update all iOS classes or do we do that in another work package?

@MartoYankov
Copy link
Contributor

@farfromrefug I think the two weak references to this are fine and also, updating the other iOS and Android classes can happen in separate PRs, so that they are more manageable.

Now, seeing that implemented I have some doubts about the new initNativeViewDelegates method that has a parameter any. It's a bit confusing having both initNativeViewDelegates and initNativeView. What do you think about moving the contents of the initNativeViewDelegates to initNativeView? Will it work for your case? It will have the added benefit that during initNativeView we already have nativeViewProtected, so you won't have to pass parameters.

I'm thinking about making a PR to your fork if you are okay with the change. Let me know what you think.

@farfromrefug
Copy link
Collaborator Author

@MartoYankov a PR is fine !
Now about moving the initNativeViewDelegates inside initNativeView That s actually what I try to get around.
The issue is that if you component uses a subclass of the nativeView Nativescript is using you want to have a custom initNativeView but keep the delegate from Nativescript core.
The real issue is that otherwise you have to declare again all the "listener" classes like the ones define here

@MartoYankov
Copy link
Contributor

@farfromrefug That way you would avoid repeating the listeners, but there is still some custom code in initNativeView like assigning the listener owner. Wouldn't it be better if in your inheritor class you call the super.initNativeView() which would initialize the listeners of the base NativeScript core class and then put your custom initNativeView logic there?

@farfromrefug
Copy link
Collaborator Author

@MartoYankov sorry I think I misplaced initNativeView and createNativeView ...
So I am ok with initNativeView being the place where all listener/delegates are created.
One thing: it seems initNativeView is never used on iOS. No issue there?
Cause I would move all delegate creation there even on iOS.

Also isn't initNativeView called multiple times? I mean it's called from setNativeView which is called from nativeView setter which could be called often? And there is no check to see if new nativeView is equal to current one. So if called again with same view, initNativeView will be called again and thus all listener created again.

BTW I am thinking about upgrading all ui components from that PR. Any issue with that?

@MartoYankov
Copy link
Contributor

@farfromrefug Regarding your first question - yes, the current state of affairs is that iOS components are using their constructors to create and init, while Android components are using createNativeView and initNativeView. As I previously commented, we consider this technical debt and should move the code from all iOS constructors to createNativeView and initNativeView.

Regarding your second question - the nativeView setter shouldn't be called often. I'm not sure about the scenario where it was needed, but there is a check if the new value is equal to the current one. The first check in setNativeView should account for that.

I'm okay with making the changes to all components in this PR. We have to change the title though.

@@ -30,7 +30,11 @@ export class Frame extends FrameBase {
super();
this._ios = new iOSFrame(this);
this.viewController = this._ios.controller;
this.nativeViewProtected = this._ios.controller.view;
this.nativeViewProtected = this.viewController.view;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should remove this line. If it stays, there is no need for createNativeView.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I am not sure. The reason I did that is for nativeViewProtected to be available as soon as the constructor is called. I think it's important from the frame.
Will test this

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran the tests and they worked. I'm also conflicted. I think for non UI components (Page, Frame), it might be better to keep the nativeViewProtected in constructor and omit createNativeView.

Copy link
Collaborator Author

@farfromrefug farfromrefug Sep 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I have both because if _tearDownUI is called you loose it without being able to get it back.Let me know what you prefer

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the idea of _tearDownUI is to destroy the components, so that the garbage collection can release them. We are currently not recycling native views. I think for now you can remove this line and keep the one in createNativeView().


public createNativeView() {
const view = UIScrollView.new();
if (this.orientation === "horizontal") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be called in initNativeView(). The bonus is we can call updateScrollBarVisibility() method there.

this.nativeViewProtected = UIScrollView.new();
initNativeView() {
super.initNativeView();
if (this.orientation === "horizontal") {
Copy link
Contributor

@MartoYankov MartoYankov Sep 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can now remove this if and call this.updateScrollBarVisibility(this.scrollBarIndicatorVisibile) for better code reuse.

@MartoYankov
Copy link
Contributor

test

@MartoYankov
Copy link
Contributor

test

@MartoYankov
Copy link
Contributor

test

@MartoYankov
Copy link
Contributor

@farfromrefug Some dialogs and modals UI tests are failing. I know you can't see them, so I'll take care of them.

@farfromrefug
Copy link
Collaborator Author

@MartoYankov ok let me know If I can help in any way

@MartoYankov
Copy link
Contributor

test

@MartoYankov
Copy link
Contributor

test --ignore branch_ns_ui_sidedrawer_demo ios11 api23

@MartoYankov MartoYankov changed the title refactor(text widgets and button): refactoring for easy plugin development refactor(core-modules): implement createNativeView and initNativeView for all components Sep 26, 2018
@MartoYankov MartoYankov merged commit 46705ee into NativeScript:master Sep 26, 2018
@ghost ghost removed the in progress label Sep 26, 2018
@MartoYankov
Copy link
Contributor

@farfromrefug Thanks for the awesome PR. I hope the material components will come out nicely now.

@farfromrefug
Copy link
Collaborator Author

@MartoYankov Thanks! Yes will update my plugin right now with this. Need some changes on the runtime though (might already be in right now) for that plugin to be published.

@lock
Copy link

lock bot commented Nov 2, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked and limited conversation to collaborators Nov 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants