refactor(core-modules): implement createNativeView and initNativeView for all components by farfromrefug · Pull Request #6102 · NativeScript/NativeScript

farfromrefug · 2018-07-21T09:37:43Z

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

The PR title follows our guidelines: https://github.com/NativeScript/NativeScript/blob/master/CONTRIBUTING.md#commit-messages.
There is an issue for the bug/feature this PR is for. To avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it.
You have signed the CLA.
All existing tests are passing: https://github.com/NativeScript/NativeScript/blob/master/DevelopmentWorkflow.md#running-unit-tests.
Tests for the changes are included - https://github.com/NativeScript/NativeScript/blob/master/WritingUnitTests.md.

What is the current behavior?

normal

What is the new behavior?

unchanged

…ated operations. This is to separate layout operations from text operations

…dule_refactoring

SvetoslavTsenov · 2018-07-23T09:57:19Z

test

farfromrefug · 2018-07-23T12:19:48Z

@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 · 2018-07-23T12:50:08Z

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.

… IOS

…dule_refactoring

farfromrefug · 2018-07-24T08:20:14Z

@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 · 2018-07-24T19:43:11Z

@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 · 2018-07-27T11:49:43Z

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 · 2018-07-27T11:52:25Z

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

farfromrefug · 2018-07-27T11:59:38Z

@MartoYankov great comments.

I will look at all that !

Thanks

farfromrefug · 2018-07-30T12:27:02Z

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

…dule_refactoring

MartoYankov · 2018-08-08T10:05:05Z

@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 · 2018-08-08T10:26:07Z

@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 · 2018-08-09T10:49:30Z

@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 · 2018-08-09T11:32:57Z

@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 · 2018-08-09T13:39:06Z

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

…veView

MartoYankov · 2018-09-20T15:02:31Z

        this._ios = new iOSFrame(this);
        this.viewController = this._ios.controller;
-        this.nativeViewProtected = this._ios.controller.view;
+        this.nativeViewProtected = this.viewController.view;


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

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

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.

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

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().

MartoYankov · 2018-09-20T15:06:03Z

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


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

MartoYankov · 2018-09-21T07:04:10Z

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


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

MartoYankov · 2018-09-21T07:49:45Z

test

MartoYankov · 2018-09-25T08:43:02Z

test

MartoYankov · 2018-09-25T15:08:43Z

test

MartoYankov · 2018-09-25T15:36:29Z

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

farfromrefug · 2018-09-25T17:13:43Z

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

MartoYankov · 2018-09-26T08:52:26Z

test

MartoYankov · 2018-09-26T10:03:47Z

test --ignore branch_ns_ui_sidedrawer_demo ios11 api23

MartoYankov · 2018-09-26T11:00:05Z

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

farfromrefug · 2018-09-26T11:01:17Z

@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 · 2019-11-02T20:38:53Z

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.

farfromrefug added 2 commits July 20, 2018 19:11

define and use nativeTextViewProtected for all text configuration rel…

ac28aed

…ated operations. This is to separate layout operations from text operations

separate createNativeView from nativeview delegate creations

bb67da7

ghost added the ♥ community PR label Jul 21, 2018

Merge branch 'master' of github.com:NativeScript/NativeScript into mo…

194231e

…dule_refactoring

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

farfromrefug added 3 commits July 21, 2018 11:42

fixed lint error

5839cb5

button refactoring

5bad5f6

better that way?

db752b1

farfromrefug added 2 commits July 24, 2018 10:19

fixed ios view creation as createNativeView is not fully supported on…

24b8f38

… IOS

Merge branch 'master' of github.com:NativeScript/NativeScript into mo…

0fb47fa

…dule_refactoring

prevent undefined error

58fc92c

update based on comments

a2f914a

farfromrefug added 2 commits July 30, 2018 14:29

Merge branch 'master' of github.com:NativeScript/NativeScript into mo…

1dbc942

…dule_refactoring

fixed missing delegate call on ios

48f7b66

farfromrefug and others added 2 commits August 11, 2018 16:09

refactored all UI widgets for better use of createNativeView/initNati…

2ae1133

…veView

Merge branch 'master' into module_refactoring

e20a8c5

Merge branch 'master' into module_refactoring

4ff80b9

MartoYankov reviewed Sep 20, 2018

View reviewed changes

Martin Guillon added 2 commits September 20, 2018 17:29

better to call this in initNativeView

9dfafff

oops!

9048879

MartoYankov reviewed Sep 21, 2018

View reviewed changes

Martin Guillon added 2 commits September 21, 2018 09:16

refactoring

ce76f94

not needed as we have createNativeView

e549485

refactor: call setupUI on components created with constructor

b8113d8

call setupUI on dialog components for iOS only

af042ab

move setupAsRootView before the modal is shown

8d39e65

MartoYankov approved these changes Sep 26, 2018

View reviewed changes

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 merged commit 46705ee into NativeScript:master Sep 26, 2018

ghost removed the in progress label Sep 26, 2018

farfromrefug deleted the module_refactoring branch September 26, 2018 11:02

farfromrefug mentioned this pull request Oct 2, 2018

[FR] easier way to override native view creation #5456

Closed

MartoYankov mentioned this pull request Oct 10, 2018

fix-next: drawer plugin viewcontroller init #6379

Merged

radeva added the ♥ community PR label Nov 2, 2018

lock Bot locked and limited conversation to collaborators Nov 2, 2019

Uh oh!

Conversation

farfromrefug commented Jul 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

What is the current behavior?

What is the new behavior?

Uh oh!

SvetoslavTsenov commented Jul 23, 2018

Uh oh!

farfromrefug commented Jul 23, 2018

Uh oh!

SvetoslavTsenov commented Jul 23, 2018

Uh oh!

farfromrefug commented Jul 24, 2018

Uh oh!

farfromrefug commented Jul 24, 2018

Uh oh!

MartoYankov commented Jul 27, 2018

Uh oh!

farfromrefug commented Jul 27, 2018

Uh oh!

farfromrefug commented Jul 27, 2018

Uh oh!

farfromrefug commented Jul 30, 2018

Uh oh!

MartoYankov commented Aug 8, 2018

Uh oh!

farfromrefug commented Aug 8, 2018

Uh oh!

MartoYankov commented Aug 9, 2018

Uh oh!

farfromrefug commented Aug 9, 2018

Uh oh!

MartoYankov commented Aug 9, 2018

Uh oh!

Uh oh!

MartoYankov Sep 20, 2018

Choose a reason for hiding this comment

Uh oh!

farfromrefug Sep 20, 2018

Choose a reason for hiding this comment

Uh oh!

MartoYankov Sep 20, 2018

Choose a reason for hiding this comment

Uh oh!

farfromrefug Sep 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MartoYankov Sep 21, 2018

Choose a reason for hiding this comment

Uh oh!

MartoYankov Sep 20, 2018

Choose a reason for hiding this comment

Uh oh!

MartoYankov Sep 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MartoYankov commented Sep 21, 2018

Uh oh!

MartoYankov commented Sep 25, 2018

Uh oh!

MartoYankov commented Sep 25, 2018

Uh oh!

MartoYankov commented Sep 25, 2018

Uh oh!

farfromrefug commented Sep 25, 2018

Uh oh!

MartoYankov commented Sep 26, 2018

Uh oh!

MartoYankov commented Sep 26, 2018

Uh oh!

MartoYankov commented Sep 26, 2018

Uh oh!

farfromrefug commented Sep 26, 2018

Uh oh!

lock Bot commented Nov 2, 2019

Uh oh!

farfromrefug commented Jul 21, 2018 •

edited

Loading

farfromrefug Sep 20, 2018 •

edited

Loading

MartoYankov Sep 21, 2018 •

edited

Loading