-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
#457: Converted java to Kotlin #835
base: dev
Are you sure you want to change the base?
#457: Converted java to Kotlin #835
Conversation
} | ||
} | ||
|
||
|
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.
Extra newlines to be removed.
constructor(filePath: String) : this(GifInfoHandle(filePath)) | ||
|
||
/** | ||
* Equivalent to `` GifMetadata(file.getPath())} |
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.
* Equivalent to `` GifMetadata(file.getPath())} | |
* Equivalent to `GifMetadata(file.getPath())}` |
* Current frame can be copied to 2D texture when needed. See [.glTexImage2D] and [.glTexSubImage2D]. | ||
*/ | ||
class GifTexImage2D(inputSource: InputSource, options: GifOptions?) { | ||
private val mGifInfoHandle: GifInfoHandle? |
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.
this should not be nullable
*/ | ||
get() = mGifInfoHandle!!.duration | ||
|
||
@Throws(Throwable::class) |
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.
this annotation is useless, please remove
try { | ||
return GifDrawable(resources, resId) | ||
} catch (ignored: IOException) { | ||
// ignored |
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.
this comment is not needed
private var conditionVariable: ConditionVariable? = null | ||
private var waiter: Waiter? = null |
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.
Can be lateinit instead of nullable
android-gif-drawable/src/main/java/pl/droidsonroids/gif/GifTexImage2D.kt
Show resolved
Hide resolved
val isRecycled: Boolean | ||
get() = gifInfoPtr == 0L | ||
|
||
@Throws(Throwable::class) |
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.
to be removed
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.
Another set of comments. This is still not the last one.
@@ -9,8 +10,8 @@ android { | |||
compileSdkVersion versions.compileSdk | |||
|
|||
compileOptions { | |||
sourceCompatibility JavaVersion.VERSION_1_8 | |||
targetCompatibility JavaVersion.VERSION_1_8 | |||
sourceCompatibility JavaVersion.VERSION_17 |
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.
There should be still java 1.8
See https://github.com/koral--/android-gif-drawable/issues/826
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.
The project does not compile for me when I move it back to 1.8
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.
OK, I'll check this further ASAP.
val resources = getInstrumentation().context.resources | ||
val drawable = GifDrawable(resources, pl.droidsonroids.gif.test.R.raw.test) | ||
val metaData = GifAnimationMetaData(resources, pl.droidsonroids.gif.test.R.raw.test) | ||
Assertions.assertThat(drawable.frameByteCount + metaData.allocationByteCount) |
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.
import Assertions.assertThat
here and in other similar files
@Before | ||
fun setUp() { | ||
val context = InstrumentationRegistry.getInstrumentation().context | ||
rootView = LayoutInflater.from(context).inflate(pl.droidsonroids.gif.test.R.layout.attrributes, null, false) |
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.
import pl.droidsonroids.gif.test.R
here and in other tests.
val responseStream: InputStream = ByteArrayInputStream(buffer) | ||
val gifDrawable = GifDrawable(responseStream) | ||
Assertions.assertThat(gifDrawable.error).isEqualTo(GifError.NO_ERROR) |
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.
val responseStream: InputStream = ByteArrayInputStream(buffer) | |
val gifDrawable = GifDrawable(responseStream) | |
Assertions.assertThat(gifDrawable.error).isEqualTo(GifError.NO_ERROR) | |
val responseStream: InputStream = ByteArrayInputStream(buffer) | |
val gifDrawable = GifDrawable(responseStream) | |
Assertions.assertThat(gifDrawable.error).isEqualTo(GifError.NO_ERROR) |
Separate arrange, act and assert sections with a newlines. Here and in other tests.
/** | ||
* See [GifDrawable.getDuration] | ||
* | ||
* @return duration of of one loop the animation in milliseconds. Result is always multiple of 10. |
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.
* @return duration of of one loop the animation in milliseconds. Result is always multiple of 10. | |
* @return duration of one loop the animation in milliseconds. Result is always multiple of 10. |
abstract fun open(): GifInfoHandle | ||
@Throws(IOException::class) |
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.
abstract fun open(): GifInfoHandle | |
@Throws(IOException::class) | |
abstract fun open(): GifInfoHandle | |
@Throws(IOException::class) |
fun testNotEqualReferences() { | ||
val reference = CallbackWeakReference(callback) | ||
val anotherReference = CallbackWeakReference(anotherCallback) | ||
Java6Assertions.assertThat(reference).isNotEqualTo(anotherReference) |
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.
Replace L4 with import org.assertj.core.api.Java6Assertions.assertThat
Remove Java6Assertions.
prefixes from the test methods.
@RunWith( | ||
MockitoJUnitRunner::class | ||
) |
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.
@RunWith( | |
MockitoJUnitRunner::class | |
) | |
@RunWith(MockitoJUnitRunner::class) |
Change also similar places, if any.
lateinit var resources: Resources | ||
|
||
private fun getMockedResources(resourceDensity: Int, displayDensity: Int): Resources { | ||
Mockito.doAnswer { invocation -> |
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.
import Mockito
just like Java6Assertions from one of the previous comments.
/** | ||
* Frame buffer, holds current frame. | ||
*/ | ||
var mBuffer: Bitmap |
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.
It should be internal. This also applies to most (if not all) other members which were previously package private.
But, it may break the backwards compatibility.
Please hold on any changes in this matter for now. I need to think about that.
android-gif-drawable/src/main/java/pl/droidsonroids/gif/GifIOException.kt
Outdated
Show resolved
Hide resolved
@koral-- Let me know if more comments on the review. I have resolved the comments couple of days back. |
Changes are looking very good. Great work! I plan to add some binary compatibility checker https://plugins.gradle.org/search?term=binary-compatibility I'll try to do that in a few days from now. |
Hi @rahul13agrawal I've just added https://github.com/Kotlin/binary-compatibility-validator. You can rebase on the latest dev and execute the gradle tasks:
It is perfectly OK that some method or class gets added. It will happen for e.g. all companion objects.
|
@koral-- I will go through this by this weekend. |
Hi @koral-- Sorry for such late response. I have rebase with the latest changes but whenever I run |
Indeed, it seems that disabling the check in the sample module only causes disabling it entirely. I need to investigate this. In the meantime you can comment out or delete this line: https://github.com/koral--/android-gif-drawable/blob/dev/sample/build.gradle#L65 and ignore the generate dump of the sample module. |
Fixes: https://github.com/koral--/android-gif-drawable/issues/457