Feature/kotlin

[markusressel]

This is a PR for #36

As of now I have tested the basic functionality of the library (as in using it on the emulator) and it seems to work just like the java version.

For this PR I tried to convert the code and leverage delegation and other features of Kotlin to minimize boilerplate. I did not try to change any logic to speed up performance or "improve" other things. This PR is only intended to use Kotlin as a way to write the same logic with way less and more readable code that has Null-Safety built in for free.

Some things I was unsure about when fleshing out the details:

• The documentation probably needs a bit of an overhaul, which I didn't tackle yet
• Do you really want to expose the underlying ZoomEngine with all it's features outside of implementations like ZoomLayout and ZoomImageView? Previously it was possible to access the ZoomEngine by asking f.ex. ZoomLayout. I think all methods exposed to the outside should be defined in the ZoomApi so I made that field private.
• I used property delegation to give ZoomLayout and ZoomImageView users the ability to use the ZoomApi without the need to redundantly specify the "delegation methods" that simply point to the inner engine field. To use delegation I had to change the ZoomEngine constructor so it doesn't need this as a parameter anymore. This was ok for the Listener, but not for the container. Since I didn't want to make the container type Nullable (View?) I initialized it with an empty inline View class. If you know a better solution to this let me know.

I know this is a pretty big change (could it be any bigger?), so I am open to any kind of discussion.

[markusressel]

Since javadoc doesn't support Kotlin (which breaks the build atm) I would recommend to use Dokka instead. It supports Java as well as Kotlin. I will look into this later.

[natario1]

Thanks!

I will take a look when I find some time. Meanwhile, the issue with both your bullet points is that they would be breaking changes. I agree that with ZoomApi the engine could be hidden, but someone might be using it, so we don't want to break their code.

Same applies to delegation, let's not change the constructors. You can use an internal constructor without "this" but still provide the old constructor.

I would also make the container a private lateinit var and create an internal function setContainer() to initialize.

Anyway, thanks, it's cool to see this in Kotlin!
About Dokka & publishing I have spent some hours on this recently, have a working lib here https://github.com/natario1/Elements/blob/master/library/build.gradle

[natario1]

Sorry for the delay! I've been through some stuff, moved to another country and so on.

[markusressel]

No problem I'm pretty busy right now too. Will take a look in approx. 2 weeks when exams are over.

[natario1]

Cool!

[markusressel]

Well it was a little over 2 weeks (shame on me) but I finally made it.
I hope you can still use this to some degree. I saw you already did some work on migrating this to AndroidX and a lot of other stuff too. I did not have the time yet to look into documentation but as this is a thing I will need for my own libraries I hope to get around this soon™.

[Tadaboody]

Any help needed to close this PR? I'd like this to get merge so I can help with some other issues, namely #2 without it disrupting this PR

[natario1]

I am super busy and I have not reviewed changes but I am glad you made it @markusressel .

I don't know how to manage this, I have an open PR at #59 which I am actively working at (there are more commits in my local machine + more coming) and I am in need of pushing that forward. It changes constructors, logic, adds stuff, removes stuff.

Either we wait for #59 and then rewrite this and merge this quickly, or merge this now in a kotlin branch. Later improve the kotlin branch until it matches java. But it will be hard without a single diff to look at. What do you guys think.

[markusressel]

androidx is pretty big and useful as are the other parts of your PR. If it lessens the pressure on you @natario1 I'm happy with reapplying the knowledge of this PR to the version after #59 is merged and apply it to this PR. We just have to find a time where the switch to kotlin is doable and can get merged before making huge architectural changes again.

[natario1]

Sorry for the delay. I will merge #59 now so @markusressel if you still want to go with this kotlin job, that would be cool.

When you start we could add a little header to README to block new contributions for a while.
If you would like to have write access here, take care about issues or whatever else, let me know. You have already set up some well thought PRs and seem to be interested in the project. I don't think the amount of work is going to increase, but my time decreases :-)

I think it makes sense (like you did here?) to leave the demo app in java, so we can ensure that the API surface did not change from there.

[markusressel]

Great! I hope I can find some extra time this week(end) and get this language port out of the way soon.

Well my time isn't exactly increasing either (hence the delay xD) but since I already know pretty much all of the codebase I may help where I can.

Yes, my intention also was to leave the demo app in java to ensure backwards compatibility is still there.

[markusressel]

There are some things that need to be addressed before this can get merged:

• there is still some bug with overpinching - fixed
• documentation is not yet generated for kotlin code.

[markusressel]

@natario1 Ok so from my point of view this is a state that works just like the original version does.
Would be great if you could spare an evening to check if anything looks suspicious.

[natario1]

Will do this weekend, thanks!

[natario1]

• ZoomApi.kt : do we need JvmStatic / JvmField on the companion object fields?
• ZoomApi.kt : annotated fields like @get:Zoom should also have the setter annotated, so I think you can simply use @Zoom?
• ZoomEngine.kt L 339: Wrong method name in replaceWith
• ZoomEngine.kt L 797: Should be newZoom, not zoom

Some Kotlinish nits,

• Engine: can mListeners be a mutableListOf instead of ArrayList?
• Engine: can you use forEach { } when iterating (dispatchOnIdle & dispatchOnMatrix)
• Logger: the string function could be one line with kotlin's joinToString() with a space separator, solves the trim problem also

Could you fix/check these? Should be easy. The rest seems cool (at a first glance, honestly I can't process 2k lines with my eyes on two browser tabs)

[natario1]

Last thing which is important, sorry.

For most files git took this change as a delete + create new file. This is wrong because it means that all git history is lost for these files. Can you fix this?

I would do this

• fix stuff from my comment above
• push to GitHub
• take a new clean branch from master
• rename file extensions from java to kt
• make a commit. Git should understand this is a rename
• Now edit these files, remove java content and paste kotlin content from GitHub
• Also paste changes to gradle files (or pick commits)
• Force push this new branch into feature/kotlin

Gave you access to the repo now

[markusressel]

• ZoomApi.kt : do we need JvmStatic / JvmField on the companion object fields?

Not on const fields.

• ZoomApi.kt : annotated fields like @get:Zoom should also have the setter annotated, so I think you can simply use @Zoom?

Seems plausible.

• ZoomEngine.kt L 339: Wrong method name in replaceWith

Right, missed that. I also removed the shorthand and replaced it with a default parameter and @JvmOverloads.

• ZoomEngine.kt L 797: Should be newZoom, not zoom

I don't see any zoom there, am I missing something?

Some Kotlinish nits,

• Engine: can mListeners be a mutableListOf instead of ArrayList?

Sure.

• Engine: can you use forEach { } when iterating (dispatchOnIdle & dispatchOnMatrix)

Yep. I removed the local variable val matrix = matrix in dispatchOnMatrix too, or is this of any importance to use a local variable there?

• Logger: the string function could be one line with kotlin's joinToString() with a space separator, solves the trim problem also

Well the empty string special case is an additional line but I replaced the other stuff with joinToString().

Could you fix/check these? Should be easy. The rest seems cool (at a first glance, honestly I can't process 2k lines with my eyes on two browser tabs)

No problem, we can always enhance it later :)

[natario1]

Sorry I was looking at lines in this commit: 499f9ac . The if (zoom > 0) condition should be on newZoom

[markusressel]

Sorry I was looking at lines in this commit: 499f9ac . The if (zoom > 0) condition should be on newZoom

Ah yes I fixed that already.
I don't have time this evening anymore but I will get around the git commit thingy asap in the coming days (christmas is kind of busy).

[markusressel]

I renamed the files using git mv because git was not able to pick up file renaming automatically.
When comparing the commits on GitHub files are shown as edited, but on the "Files Changed" Tab they are (partly) still shown as deleted... I think github is broken here, the commits should be fine.

Anything else to consider?

[natario1]

Yes, looks like it works. It's fine, will merge now! Thank you for the effort!

[natario1]

As notes for the future, to release a new version, you just bump version in build.gradle and README, merge, then create a GitHub release, add who did what + PR links in the release notes, and Travis should publish to Bintray. Version names should be vX.X.X or Travis ignores them.

I tend to squash commits in PRs when merging. I just used the rebase option now because there was the rename thing and we would probably have lost history again.