Skip to content

Consider value classes for relative and global adapter positions #2827

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

Open
nikclayton opened this issue Feb 5, 2025 · 1 comment
Open

Comments

@nikclayton
Copy link
Contributor

There are a number of functions in the library that either accept or return adapter positions, and these are either global positions or relative positions.

In these cases the type of the value is Int. But I don't think it's safe to add a position returned from a relative position function to a position returned from a global position function, or vice versa. And sending a relative position as a value to a function expecting a global position (or vice versa) is also an error. But since these are Ints it's an easy mistake to make.

It might be worth looking at value classes for these. That would prevent these classes of errors.

For example:

// First, define types for GlobalPosition and RelativePosition
@JvmInline value class GlobalPosition(private val v: Int): Comparable<GlobalPosition> {
    operator fun plus(inc: Int) = GlobalPosition(v + inc)
    operator fun minus(dec: Int) = GlobalPosition(v - dec)
    operator fun plus(inc: GlobalPosition) = inc + v
    operator fun minus(dec: GlobalPosition) = dec - v
    
    override operator fun compareTo(other: GlobalPosition): Int = v.compareTo(other.v)
}

@JvmInline value class RelativePosition(private val v: Int): Comparable<RelativePosition> {
    operator fun plus(inc: Int) = RelativePosition(v + inc)
    operator fun minus(dec: Int) = RelativePosition(v - dec)
    operator fun plus(inc: RelativePosition) = inc + v
    operator fun minus(dec: RelativePosition) = dec - v
    
    override operator fun compareTo(other: RelativePosition): Int = v.compareTo(other.v)
}

// Second, some functions that return GlobalPosition and RelativePosition.
// Even though these functions return the same Int they are different values.
fun getGlobalPosition() = GlobalPosition(1)
fun getRelativePosition() = RelativePosition(1)

// Third, functions that add data to lists using either a GlobalPosition or
// a RelativePosition as a parameter.
fun addAtGlobalPosition(pos: GlobalPosition) = println("Added at $pos")
fun addAtRelativePosition(pos: RelativePosition) = println("Added at $pos")

// Finally, demo using these functions.
fun main() {
    val g = getGlobalPosition()
    val r = getRelativePosition()
    
    println("global: $g")
    println("relative: $r")
    
    addAtGlobalPosition(g)
    addAtRelativePosition(r)
    
    // This fails to compile, as it should, because it's trying to send a
    // RelativePosition to a function expecting a GlobalPosition.
    //
    // Argument type mismatch: actual type is 'RelativePosition', but 'GlobalPosition' was expected.
    //
    // Uncomment the next line to see the failure
    //addAtGlobalPosition(r)
    
    // Adding or subtracting from a position works as expected.
    println("GlobalPosition +/- Int")
    addAtGlobalPosition(g + 1)
    addAtGlobalPosition(g - 1)
    
    println("RelativePosition +/- Int")
    addAtRelativePosition(r + 1)
    addAtRelativePosition(r - 1)
    
    // Adding two GlobalPositions, or two RelativePositions works as expected
    println("Add/subtract GlobalPosition +/- GlobalPosition")
    addAtGlobalPosition(g + g)
    addAtGlobalPosition(g - g)
    
    println("RelativePosition +/- RelativePosition")
    addAtRelativePosition(r + r)
    addAtRelativePosition(r - r)    
    
    // Adding a GlobalPosition to a RelativePosition does not work, since the result
    // doesn't make sense.
    //
    // This fails to compile:
    // None of the following candidates is applicable: ...
    //
    // Uncomment the next two lines to see the failure.
    // addAtGlobalPosition(g + r)
    // addAtRelativePosition(r + g)
    
    // GlobalPosition should compare to one another, but not Int, or RelativePosition
    assert(GlobalPosition(1) < GlobalPosition(2))
    assert(GlobalPosition(2) > GlobalPosition(1))
    
    // This fails to compile:
    // Argument type mismatch: actual type is 'Kotlin.Int', but 'GlobalPosition' was expected.
    //
    // Uncomment the next line to see the failure.
    // assert(GlobalPosition(1) < 2)

    // This fails to compile:
    // Argument type mismatch: actual type is 'RelativePosition', but 'GlobalPosition' was expected.
    //
    // Uncomment the next line to see the failure.
    // assert(GlobalPosition(1) < RelativePosition(2))
}
@mikepenz
Copy link
Owner

mikepenz commented Feb 6, 2025

Thank you @nikclayton for the feedback. I really appreciate it.

That said. Given the future is Compose for Android UI there are no plans to significantly change the MaterialDrawer anymore. Even more so, the relative vs. global positioning comes from the underlaying FastAdapter which also does not plan to have any significant updates anymore.

At this stage I am mostly focusing on bugfixes to make sure the library still serves it's purpose and does not have major issues.

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

No branches or pull requests

2 participants