Skip to content
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

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

Merged
merged 42 commits into from Sep 26, 2018

Conversation

Projects
None yet
5 participants
@farfromrefug
Copy link
Contributor

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

farfromrefug added some commits Jul 20, 2018

define and use nativeTextViewProtected for all text configuration rel…
…ated operations. This is to separate layout operations from text operations

@ghost ghost added the ♥ community PR label Jul 21, 2018

@ns-bot ns-bot added the cla: yes label Jul 21, 2018

farfromrefug added some commits Jul 21, 2018

@SvetoslavTsenov

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2018

test

@farfromrefug

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2018

@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

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2018

@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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2018

@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

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2018

@farfromrefug

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2018

@MartoYankov great comments.

I will look at all that !

Thanks

@farfromrefug

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2018

@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

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2018

@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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2018

@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

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2018

@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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2018

@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

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2018

@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.

farfromrefug added some commits Aug 11, 2018

@farfromrefug

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2018

@MartoYankov I should now have fixed all the errors! Sorry for the delay

Show resolved Hide resolved tns-core-modules/ui/scroll-view/scroll-view.ios.ts Outdated
@@ -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;

This comment has been minimized.

Copy link
@MartoYankov

MartoYankov Sep 20, 2018

Contributor

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

This comment has been minimized.

Copy link
@farfromrefug

farfromrefug Sep 20, 2018

Author Contributor

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

This comment has been minimized.

Copy link
@MartoYankov

MartoYankov Sep 20, 2018

Contributor

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.

This comment has been minimized.

Copy link
@farfromrefug

farfromrefug Sep 20, 2018

Author Contributor

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

This comment has been minimized.

Copy link
@MartoYankov

MartoYankov Sep 21, 2018

Contributor

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") {

This comment has been minimized.

Copy link
@MartoYankov

MartoYankov Sep 20, 2018

Contributor

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

farfromrefug added some commits Sep 20, 2018

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

This comment has been minimized.

Copy link
@MartoYankov

MartoYankov Sep 21, 2018

Contributor

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

farfromrefug added some commits Sep 21, 2018

@MartoYankov

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2018

test

@MartoYankov

This comment has been minimized.

Copy link
Contributor

commented Sep 25, 2018

test

@MartoYankov

This comment has been minimized.

Copy link
Contributor

commented Sep 25, 2018

test

@MartoYankov

This comment has been minimized.

Copy link
Contributor

commented Sep 25, 2018

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

@farfromrefug

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2018

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

@MartoYankov

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2018

test

@MartoYankov

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2018

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

2 checks passed

ci/jenkins/core-modules-tests PASSed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ghost ghost removed the in progress label Sep 26, 2018

@MartoYankov

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2018

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

@farfromrefug

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2018

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.