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.

Sign up for GitHub

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

Jump to bottom

Add Kotlin to SharedCode #67

Merged
merged 3 commits into from
Jun 3, 2022
Merged

Add Kotlin to SharedCode #67

merged 3 commits into from
Jun 3, 2022

Conversation

nachtien
Copy link
Contributor

@nachtien nachtien commented Jan 20, 2022 •
edited
Loading

Adds Kotlin to the SharedCode

@nachtien nachtien changed the title Introduce Kotlin to SharedCode Add Kotlin to SharedCode Jan 20, 2022
@nachtien nachtien force-pushed the nick/sharedcode-kotlin branch 5 times, most recently from e7b52bc to a0261f0 Compare January 20, 2022 13:43
@robertjcolley robertjcolley mentioned this pull request Jan 20, 2022
Closed
adelarsq
adelarsq approved these changes Jan 21, 2022
View reviewed changes
Copy link
Contributor

@adelarsq adelarsq left a comment

Choose a reason for hiding this comment

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

Good job!

@nachtien nachtien marked this pull request as draft February 1, 2022 16:47
@nachtien
Copy link
Contributor Author

nachtien commented Feb 1, 2022

I'm going to verify whether the clickState is nullable

@nachtien nachtien force-pushed the nick/sharedcode-kotlin branch from a0261f0 to 291bc2e Compare March 30, 2022 13:25
@nachtien
Copy link
Contributor Author

Looks like clickState is not null. Updated. Sorry this took so long!

@nachtien nachtien marked this pull request as ready for review March 30, 2022 13:27
nachtien
nachtien commented Mar 30, 2022
View reviewed changes
android/sharedCode/src/main/java/com/viro/core/ClickListener.kt Outdated
* @param clickState The status of the click event.
* @param location The location of the event in world coordinates.
*/
void onClickState(int source, Node node, ClickState clickState, Vector location);
fun onClickState(source: Int, node: Node?, clickState: ClickState, location: Vector?)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Node can be null here

location can be null here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But node should never be null, so maybe i shouldn't mark it as nullable?

Copy link
Contributor

@adelarsq adelarsq Apr 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@nachtien I think that node itself can't be null.

Node can be null

This two makes sense since not every id identify a node and a node can be already removed.

location can be null

This also make sense since we can pass an "wrong" location.

Would be possible some logging on this cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm confused, so do you think node is nullable or not?

Copy link
Contributor

Choose a reason for hiding this comment

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

@nachtien Node isn't nullable. But location is nullable I think.

Copy link
Contributor Author

@nachtien nachtien Apr 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Let's just add Kotlin to Gradle for now, I'll convert the other classes later to be sure :)

@nachtien nachtien force-pushed the nick/sharedcode-kotlin branch from 3ffee71 to b1ba4e9 Compare April 22, 2022 01:10
@nachtien nachtien force-pushed the nick/sharedcode-kotlin branch from b1ba4e9 to eafaf6a Compare April 22, 2022 01:12
@nachtien nachtien requested a review from adelarsq April 22, 2022 01:12
@nachtien
Copy link
Contributor Author

Note that I bumped Kotlin to 1.6.21

@nachtien
Copy link
Contributor Author

@adelarsq can you approve (if you think its fine) and merge this?

@adelarsq
Copy link
Contributor

adelarsq commented May 20, 2022 •
edited
Loading

@adelarsq can you approve (if you think its fine) and merge this?

I will take a look.

@doranteseduardo @robertjcolley Anything blocked by these changes?

@robertjcolley
Copy link
Collaborator

I can take a look this weekend. I don't believe anything is blocked by or is blocking this

@nachtien
Copy link
Contributor Author

nachtien commented Jun 3, 2022

Let's get this @robertjcolley !

@hannesa2
Copy link
Collaborator

hannesa2 commented Jun 3, 2022

In my point of view, a community project needs more maintainer with the power to merge.
This micro changes are not merged since January !

@robertjcolley robertjcolley merged commit 6f7e911 into ReactVision:main Jun 3, 2022
@adelarsq
Copy link
Contributor

adelarsq commented Jun 6, 2022 •
edited
Loading

@hannesa2 New contributors are always welcome. Just ping me if interested and we can talk about.

@robertjcolley Thanks for the revision.

@nachtien Thanks for the contribution!

@nachtien nachtien deleted the nick/sharedcode-kotlin branch June 6, 2022 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@doranteseduardo doranteseduardo Awaiting requested review from doranteseduardo

@adelarsq adelarsq Awaiting requested review from adelarsq

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nachtien @adelarsq @robertjcolley @hannesa2