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

Conversation

lucasnlm
Copy link

@lucasnlm lucasnlm commented Sep 6, 2023

Description:
This PR adds setup(), XRayConfig to RIBs XRay to allow setup it without rely on the existing toggle() function.

Problem it fixes
The current toggle() function may be problematic because it changes a static boolean. Example: depending on where XRay is enabled or if it's as debug in a RIB used twice, it will enabled and disable immediately.

XRayConfig adds the ability to enable XRay without change the Views alpha. The alpha change may be problematic when we have many RIBs attached due to the overlapping.

@CLAassistant
Copy link

CLAassistant commented Sep 6, 2023

CLA assistant check
All committers have signed the CLA.

@@ -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.


/** 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.

}

/** 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.

* @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

@lucasnlm lucasnlm marked this pull request as ready for review September 6, 2023 22:50

/** Utility class that shows riblets name in its background. */
class XRay private constructor() {
private var isEnabled = false
private var textPaint: Paint? = null
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?

* @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
Contributor

Choose a reason for hiding this comment

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

Good change

Comment on lines 46 to 47
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.

}

/** Toggles state of XRay. */
public fun toggle() {
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?

Comment on lines 18 to 27
/**
* 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,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is so simple IMO it should live on XRay.kt file at the top level. No need for a new file

Copy link
Author

Choose a reason for hiding this comment

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

Done

@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.

Copy link
Contributor

@psteiger psteiger left a comment

Choose a reason for hiding this comment

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

I'd probably not make Paint lazy, as the XRay object itself will only be initialized first time it is called anyway, so by lazy in its property for those purposes feels redundant

Stamped though

@psteiger
Copy link
Contributor

@lucasnlm please build a version of RIBs with your change and make sure it works as expected using our apps, if so we can go ahead with landing

@lucasnlm
Copy link
Author

@psteiger How can I do that?

@psteiger
Copy link
Contributor

psteiger commented Oct 6, 2023

@lucasnlm I tagged on in internal comms with smoke test instructions

@lucasnlm
Copy link
Author

@lucasnlm I tagged on in internal comms with smoke test instructions

I hope to test it this week. Thanks for the reminder.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants