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.

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

Improve XRay implementation #614

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@ abstract class ViewRouter<V : View, I : Interactor<*, *>> : Router<I> {
) : super(interactor, component) {
this.view = view
if (XRay.isEnabled()) {
XRay.apply(this, view)
XRay.toggle()
XRay.apply(getName(), view)
Copy link
Author

Choose a reason for hiding this comment

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

Change this because it was leaking a this reference to a static function.

}
}

Expand All @@ -44,7 +45,7 @@ abstract class ViewRouter<V : View, I : Interactor<*, *>> : Router<I> {
) : super(null, interactor, RibRefWatcher.getInstance(), getMainThread()) {
this.view = view
if (XRay.isEnabled()) {
XRay.apply(this, view)
XRay.apply(getName(), view)
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,95 +23,109 @@ import android.graphics.drawable.BitmapDrawable
import android.graphics.drawable.Drawable
import android.view.Gravity
import android.view.View
import androidx.annotation.VisibleForTesting

/** Utility class that shows riblets name in its background. */
class XRay private constructor() {
private var isEnabled = false
private var textPaint: Paint? = null
Copy link
Author

Choose a reason for hiding this comment

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

Changed textPaint to a lazy val.

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's all static info, why not immediately initialize it?

Copy link
Author

Choose a reason for hiding this comment

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

Paint is a big object. I rather initialize it only if needed. WDYT?

Copy link
Author

Choose a reason for hiding this comment

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

BTW, XRay is called in every ViewRouter constructor. So, I will initialize Paint every time. Even if we don't want to use XRay.

Copy link
Contributor

Choose a reason for hiding this comment

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

@lucasnlm but this is a singleton object, right? So it will really only initialize once. There's only one instance of the XRay class, or am I misunderstanding it?

Copy link
Contributor

Choose a reason for hiding this comment

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

"I rather initialize it only if needed. WDYT?"
Ok this makes sense.

public object XRay {
private var config = XRayConfig()
private val textPaint =
Paint().apply {
textSize = TEXT_SIZE.toFloat()
color = TEXT_COLOR
}

private fun writeOnBitmap(bitmap: Bitmap, text: String) {
val canvas = Canvas(bitmap)
val textPaint = getTextPaint()
val xStartPoint = (bitmap.width - textPaint.measureText(text)) / 2f
val yStartPoint = bitmap.height / 2f
canvas.drawText(text, xStartPoint, yStartPoint, textPaint)
Canvas(bitmap).run {
val xStartPoint = (bitmap.width - textPaint.measureText(text)) / 2f
val yStartPoint = bitmap.height / 2f
drawText(text, xStartPoint, yStartPoint, textPaint)
}
}

private fun getTextPaint(): Paint {
if (textPaint == null) {
textPaint =
Paint().apply {
textSize = TEXT_SIZE.toFloat()
color = TEXT_COLOR
}
}
return textPaint!!
/** Setup XRay using a [XRayConfig] */
@JvmStatic
public fun setup(config: XRayConfig) {
this.config = config
}

companion object {
private val INSTANCE = XRay()
private const val FRAME_WIDTH = 500
private const val FRAME_HEIGHT = 150
private const val XRAY_ALFA = 0.9f
private const val TEXT_SIZE = 30
private const val TEXT_COLOR = Color.RED
/** Toggles state of XRay. */
@Deprecated(
message = "toggle() may lead to switch-on-switch-off behavior. Use setup() instead.",
replaceWith = ReplaceWith("setup(XRayConfig(enabled = !config.enabled))"),
)
@JvmStatic
public fun toggle() {
setup(config.copy(enabled = !config.enabled))
}

/** Toggles state of XRay. */
@JvmStatic
fun toggle() {
INSTANCE.isEnabled = !INSTANCE.isEnabled
}
/** @return `true` if XRay is enabled, `false` otherwise. */
@JvmStatic
public fun isEnabled(): Boolean {
return this.config.enabled
}

/** @return `true` if XRay is enabled, `false` otherwise. */
@JvmStatic
fun isEnabled(): Boolean {
return INSTANCE.isEnabled
}
/**
* Puts [ViewBuilder]s riblet name in the background of the [View]
*
* @param routerName the riblets name to be written.
* @param view a [View] to put the name behind.
*/
@JvmStatic
internal fun apply(routerName: String, view: View) {
val oldBackground = view.background
val bitmap: Bitmap =
if (oldBackground != null) {
drawableToBitmap(oldBackground)
} else {
Bitmap.createBitmap(FRAME_WIDTH, FRAME_HEIGHT, Bitmap.Config.ARGB_8888)
}
writeOnBitmap(bitmap, getShortRibletName(routerName))
view.background =
BitmapDrawable(view.context.resources, bitmap).apply { gravity = Gravity.CENTER }

/**
* Puts [ViewBuilder]s riblet name in the background of the [View]
*
* @param viewRouter a [ViewRouter] which riblets name should be written.
* @param view a [View] to put the name behind.
*/
@JvmStatic
fun apply(viewRouter: ViewRouter<*, *>, view: View) {
val oldBackground = view.background
val bitmap: Bitmap =
if (oldBackground != null) {
drawableToBitmap(oldBackground)
} else {
Bitmap.createBitmap(FRAME_WIDTH, FRAME_HEIGHT, Bitmap.Config.ARGB_8888)
}
INSTANCE.writeOnBitmap(bitmap, getRibletName(viewRouter))
val newBackground = BitmapDrawable(view.context.resources, bitmap)
newBackground.gravity = Gravity.CENTER
view.background = newBackground
view.alpha = XRAY_ALFA
if (config.alphaEnabled) {
view.alpha = XRAY_ALPHA
}
}

private fun drawableToBitmap(drawable: Drawable): Bitmap {
if (drawable is BitmapDrawable) {
if (drawable.bitmap != null) {
return drawable.bitmap
}
private fun drawableToBitmap(drawable: Drawable): Bitmap {
if (drawable is BitmapDrawable) {
if (drawable.bitmap != null) {
return drawable.bitmap
}
val bitmap: Bitmap =
if (drawable.intrinsicWidth <= 0 || drawable.intrinsicHeight <= 0) {
Bitmap.createBitmap(FRAME_WIDTH, FRAME_HEIGHT, Bitmap.Config.ARGB_8888)
} else {
Bitmap.createBitmap(
drawable.intrinsicWidth,
drawable.intrinsicHeight,
Bitmap.Config.ARGB_8888,
)
}
val canvas = Canvas(bitmap)
drawable.draw(canvas)
return bitmap
}
val bitmap: Bitmap =
if (drawable.intrinsicWidth <= 0 || drawable.intrinsicHeight <= 0) {
Bitmap.createBitmap(FRAME_WIDTH, FRAME_HEIGHT, Bitmap.Config.ARGB_8888)
} else {
Bitmap.createBitmap(
drawable.intrinsicWidth,
drawable.intrinsicHeight,
Bitmap.Config.ARGB_8888,
)
}
val canvas = Canvas(bitmap)
drawable.draw(canvas)
return bitmap
}

private fun getRibletName(viewRouter: ViewRouter<*, *>): String {
return viewRouter.javaClass.simpleName.replace("Router", "")
@VisibleForTesting
internal fun getShortRibletName(originalName: String): String {
return if (originalName != "Router") {
originalName.replace("Router", "")
} else {
originalName
}
}

private const val FRAME_WIDTH = 500
private const val FRAME_HEIGHT = 150
private const val XRAY_ALPHA = 0.9f
private const val TEXT_SIZE = 30
private const val TEXT_COLOR = Color.RED
}

public data class XRayConfig(
val enabled: Boolean = false,
val alphaEnabled: Boolean = true,
)
Original file line number Diff line number Diff line change
Expand Up @@ -16,27 +16,62 @@
package com.uber.rib.core

import android.view.View
import com.uber.rib.core.XRay.Companion.apply
import org.junit.Assert.assertEquals
import org.junit.Assert.assertFalse
import org.junit.Assert.assertTrue
import org.junit.Before
import org.junit.Test
import org.junit.runner.RunWith
import org.mockito.kotlin.any
import org.mockito.kotlin.doReturn
import org.mockito.kotlin.mock
import org.mockito.kotlin.never
import org.mockito.kotlin.verify
import org.mockito.kotlin.verifyNoMoreInteractions
import org.robolectric.RobolectricTestRunner
import org.robolectric.RuntimeEnvironment

@RunWith(RobolectricTestRunner::class)
class XRayTest {
@Before
fun setup() {
XRay.setup(XRayConfig(enabled = true))
}

@Test
fun apply_changesViewBackground() {
XRay.toggle()
val viewRouter: ViewRouter<*, *> = mock()
val view: View = mock { on { context } doReturn (RuntimeEnvironment.application.baseContext) }
apply(viewRouter, view)
fun `test initial value`() {
XRay.setup(XRayConfig())
assertFalse("XRay must be disabled by default", XRay.isEnabled())
}

@Test
fun `apply function changes view background`() {
val view: View = mock { on { context } doReturn RuntimeEnvironment.application.baseContext }
XRay.apply("Test", view)
verify(view).background = any()
verify(view).alpha = 0.9f
}

@Test
fun `apply function changes view background but with alpha disabled`() {
XRay.setup(XRayConfig(enabled = true, alphaEnabled = false))
val view: View = mock { on { context } doReturn RuntimeEnvironment.application.baseContext }
XRay.apply("Test", view)
verify(view).background = any()
verifyNoMoreInteractions(viewRouter)
verify(view, never()).alpha = any()
}

@Test
fun `getShortRibletName must short router name`() {
assertEquals("Test", XRay.getShortRibletName("TestRouter"))
assertEquals("Router", XRay.getShortRibletName("Router"))
}

@Test
fun `xray toggle`() {
XRay.toggle()
assertFalse(XRay.isEnabled())

XRay.toggle()
assertTrue(XRay.isEnabled())
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,11 @@ protected constructor(
interactorGeneric.setRouterInternal(this)
}

/** @return the name of the router. This is used for logging and debugging purposes. */
protected fun getName(): String {
return javaClass.simpleName
}

/**
* Dispatch back press to the associated interactor. Do not override this.
*
Expand Down Expand Up @@ -125,8 +130,8 @@ protected constructor(

ribRefWatcher.logBreadcrumb(
"ATTACHED",
childRouter.javaClass.simpleName,
this.javaClass.simpleName,
childRouter.getName(),
this.getName(),
)
RibEvents.emitRouterEvent(RibEventType.ATTACHED, childRouter, this)
var childBundle: Bundle? = null
Expand All @@ -153,8 +158,8 @@ protected constructor(
ribRefWatcher.watchDeletedObject(interactor)
ribRefWatcher.logBreadcrumb(
"DETACHED",
childRouter.javaClass.simpleName,
this.javaClass.simpleName,
childRouter.getName(),
this.getName(),
)
if (savedInstanceState != null) {
val childrenBundles = savedInstanceState?.getBundleExtra(KEY_CHILD_ROUTERS)
Expand Down
Loading