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 1 commit
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,7 @@ abstract class ViewRouter<V : View, I : Interactor<*, *>> : Router<I> {
) : super(interactor, component) {
this.view = view
if (XRay.isEnabled()) {
XRay.apply(this, view)
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 +44,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,70 +23,76 @@ 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.

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)
private var config = XRayConfig()
private val textPaint: Paint by lazy {
Paint().apply {
textSize = TEXT_SIZE.toFloat()
color = TEXT_COLOR
}
}

private fun getTextPaint(): Paint {
if (textPaint == null) {
textPaint =
Paint().apply {
textSize = TEXT_SIZE.toFloat()
color = TEXT_COLOR
}
private fun writeOnBitmap(bitmap: Bitmap, text: String) {
Canvas(bitmap).run {
val xStartPoint = (bitmap.width - textPaint.measureText(text)) / 2f
val yStartPoint = bitmap.height / 2f
drawText(text, xStartPoint, yStartPoint, textPaint)
}
return textPaint!!
}

companion object {
private val INSTANCE = XRay()
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider changing the class to an object, if it's supposed to be a singleton after all. Then we can get rid of companion object

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense! I will do that.

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

/** Toggles state of XRay. */
/** Setup XRay using a [XRayConfig] */
@JvmStatic
fun toggle() {
INSTANCE.isEnabled = !INSTANCE.isEnabled
public fun setup(config: XRayConfig) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need thread-safety? If so, we must instead provide a way to atomically "read and write" -- an .update { } function, as below:

INSTANCE = AtomicReference<XRayConfig>(XRayConfig())

inline fun update(function: (current: T) -> T) {
  INSTANCE.getAndUpdate(function)
}

Copy link
Author

Choose a reason for hiding this comment

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

I think we don´t need thread-safety. At least I never had thread issues with XRay.
Is it ok for you if we let it as is?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes it's ok, was just wondering.

INSTANCE.config = config
}

/** Toggles state of XRay. */
public fun toggle() {
Copy link
Author

Choose a reason for hiding this comment

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

toggle is still here to keep compatibility.

Copy link
Contributor

Choose a reason for hiding this comment

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

If so, I suggest we deprecate it.
Is there a substituting method?

Copy link
Author

Choose a reason for hiding this comment

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

Done.

val config = INSTANCE.config
setup(config.copy(enabled = !config.enabled))
}

/** @return `true` if XRay is enabled, `false` otherwise. */
@JvmStatic
fun isEnabled(): Boolean {
return INSTANCE.isEnabled
public fun isEnabled(): Boolean {
return INSTANCE.config.enabled
}

/**
* Puts [ViewBuilder]s riblet name in the background of the [View]
*
* @param viewRouter a [ViewRouter] which riblets name should be written.
* @param routerName the riblets name to be written.
* @param view a [View] to put the name behind.
*/
@JvmStatic
fun apply(viewRouter: ViewRouter<*, *>, view: View) {
internal fun apply(routerName: String, view: View) {
Copy link
Author

Choose a reason for hiding this comment

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

viewRouter was used only to get its name.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good change

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
INSTANCE.writeOnBitmap(bitmap, getShortRibletName(routerName))
view.background =
BitmapDrawable(view.context.resources, bitmap).apply {
gravity = Gravity.CENTER
}

if (INSTANCE.config.alphaEnabled) {
view.alpha = XRAY_ALPHA
}
}

private fun drawableToBitmap(drawable: Drawable): Bitmap {
Expand All @@ -110,8 +116,13 @@ class XRay private constructor() {
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
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package com.uber.rib.core

/**
* Configuration for XRay.
* @property enabled `true` to enable XRay. By default it is disabled.
* @property alphaEnabled `true` to enable alpha changes when XRay is enabled.
*/
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 test_initial_value() {
XRay.setup(XRayConfig())
assertFalse("XRay must be disabled by default", XRay.isEnabled())
}

@Test
fun apply_changesViewBackground() {
XRay.toggle()
val viewRouter: ViewRouter<*, *> = mock()
val view: View = mock { on { context } doReturn (RuntimeEnvironment.application.baseContext) }
apply(viewRouter, view)
XRay.apply("Test", view)
verify(view).background = any()
verify(view).alpha = 0.9f
}

@Test
fun apply_changesViewBackground_alphaDisabled() {
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_mustShortRouterName() {
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,13 @@ 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 +132,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 +160,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