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

Plugin utils #214

Merged
merged 13 commits into from
Sep 2, 2020
Merged

Plugin utils #214

merged 13 commits into from
Sep 2, 2020

Conversation

zsoltk
Copy link
Contributor

@zsoltk zsoltk commented Sep 1, 2020

No description provided.

@zsoltk zsoltk requested a review from lukaville September 2, 2020 11:40
@@ -110,6 +110,8 @@ object RIBs {

override fun handleNonFatalWarning(warningMessage: String, throwable: Throwable?) {}

override fun handleDebugMessage(format: String, vararg args: Any) {}
override fun handleDebugMessage(format: String, vararg args: Any) {
Log.d(TAG, format.format(args))
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe let's add default implementation for handleNonFatalWarning as well?

import com.badoo.ribs.core.Rib

abstract class DebugControls<T : Rib>(
override val label: String,
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 DebugControls class? maybe we can create a label parameter in AbstractDebugControls?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, to disallow overriding the other parameters in the parent, which can only be done by the host class.

import com.badoo.ribs.core.plugin.ViewLifecycleAware

@SuppressWarnings("LongParameterList")
abstract class AbstractDebugControls<T : Rib> internal constructor(
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe it would be better to define an interface for debug controls and use composition to build it instead of inheriting from AbstractDebugControls?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Different params are needed for the two use-cases (represented as child classes), but under the hood it's common functionality, and they need to work together. I don't see how an interface would help here, since they're plugins, we don't interact with them from the outside.

@@ -63,8 +63,10 @@ dependencies {
implementation deps.external.mviCoreAndroid
implementation deps.external.rxrelay2
implementation deps.external.rxandroid2
implementation deps.external.leakCanary // use debugImplementation in real life apps
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe let's use debugImplemenatation in sandbox as well to showcase how to configure it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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


implementation project(":libraries:rib-base")
implementation project(":libraries:rib-debug-utils")
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we should include debug utils only for debug build? I think we can create a class that will provide debug plugins in debug source set and no plugins in release source set

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zsoltk zsoltk merged commit 57901e2 into badoo:master Sep 2, 2020
private var viewGroupForChildren: (() -> ViewGroup)? = null,
private var growthDirection: GrowthDirection? = null,
private val nodeAware: NodeAware = NodeAwareImpl(),
private val ribAware: RibAware<T> = RibAwareImpl()
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like it is unused? do we need Rib generic type for this class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's there for children to trigger workflow operations.

See SwitcherDebugControls which uses it.

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.

2 participants