-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Conversation
…ated operations. This is to separate layout operations from text operations
test |
@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 |
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: Let me know if you need some further information. |
@SvetoslavTsenov I realised the issue now. On iOS DO you want me to expand that PR to do that? I am willing to but it would be more changes for iOS. |
@SvetoslavTsenov still can't see what tests are failing with As for the tests app. It succeeds now on iOS but I keep getting So not sure how to know what makes the cli fail |
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.
That being said, here are some comments on how to improve the PR:
|
@MartoYankov Thanks. |
@MartoYankov great comments. I will look at all that ! Thanks |
@MartoYankov Made changes based on your comments. Should I update all iOS classes or do we do that in another work package? |
@farfromrefug I think the two weak references to Now, seeing that implemented I have some doubts about the new I'm thinking about making a PR to your fork if you are okay with the change. Let me know what you think. |
@MartoYankov a PR is fine ! |
@farfromrefug That way you would avoid repeating the listeners, but there is still some custom code in |
@MartoYankov sorry I think I misplaced Also isn't BTW I am thinking about upgrading all ui components from that PR. Any issue with that? |
@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 Regarding your second question - the 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; |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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") { |
There was a problem hiding this comment.
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") { |
There was a problem hiding this comment.
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.
test |
test |
test |
@farfromrefug Some dialogs and modals UI tests are failing. I know you can't see them, so I'll take care of them. |
@MartoYankov ok let me know If I can help in any way |
test |
test --ignore branch_ns_ui_sidedrawer_demo ios11 api23 |
@farfromrefug Thanks for the awesome PR. I hope the material components will come out nicely now. |
@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. |
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. |
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:
createNativeView
. So I subclassedcreateNativeView
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)nativeViewProtected
getter to either return the layout or the text widget. That PR actually defines anativeTextViewProtected
getter that is used for all text related operations. By default it returnsnativeViewProtected
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 increateNativeView
could be called ininitNativeView
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