Updates to InitialValues

[Nohus]

• Deleted the Defaults.kt file, defaults very defined in multiple places: nulls in InitialValues, default method parameters in methods in InitialValues, and initializations where the values are used. Instead, I moved all initial values to InitialValues. Instead of InitialValues being nullable, a default one is always created, and is the single source of truth for initial values.
• Removed applyStaticInitialValues() since now they are applied directly.
• Moved tileSize, workerCount and highFidelityColors to InitialValues. I think it makes sense to have all non-required parameters in the same place, without having to define some in one way and others in another.
• Removed the initialValues variable which was being set to null after use. Now it's a function that disables itself.
• Changed the return this in every InitialValues method to using apply, which is more idiomatic and cleaner.
• Renamed padding to preloadingPadding to be clearer about what it does (could be confused with Modifier.padding())
• Added rotationEnabled(), preloadingPadding(), and bitmapFilteringEnabled() to InitialValues. This should make the MapState initialization cleaner, because all settings can be set in InitialValues, rather than having to mutate it after creation to continue the setup.

[Nohus]

Of course feel free to disagree with anything here. : )

[p-lr]

What's the point of setting isRotationEnabled in InitialValues, since it only enables user gestures and we don't need to set an initial value for the rotation?
Also, the workers count has nothing to do in there.
However I see good ideas.

[Nohus]

What's the point of setting isRotationEnabled

So instead of:

MapState([..], initialValues = InitialValues()[..]).apply {
    enableRotation()
}

We can just do:

MapState([..], initialValues = InitialValues()[..].rotationEnabled(true))

Without the additional setup afterwards.

[p-lr]

Sure, but I find the first one more readable.

[Nohus]

we don't need to set an initial value for the rotation

Why not? In my case I have multiple types of maps of the same area, which have slightly different dimensions. So when the user changes the map type, I need to recreate the MapState, but I would like the transform to be preserved, so I need to set the initial rotation (to be whatever it was in the old Mapstate).

[p-lr]

Sorry, I forgot just one word. I meant "we don't need it to set an initial value for the rotation".
We don't need to have enableRotation() in InitValues to initiate the rotation.

[Nohus]

Sure, but I find the first one more readable.

Hmm. Here is my actual code before the changes:

MapState(
        levelCount = info.zoomLevels,
        fullWidth = info.fullWidth,
        fullHeight = info.fullHeight,
        initialValues = InitialValues()
            .scroll(initialPosition.first, initialPosition.second)
            .scale(initialScale)
            .minimumScaleMode(Fill)
            .maxScale(16f)
            .tileSize(info.tileSize)
            .workerCount(16)
    
    ).apply { 
        enableRotation()
        setPreloadingPadding(preloadingPadding)
        setBitmapFilteringEnabled { it.scale < 1 }
    }

And after:

MapState(
        levelCount = info.zoomLevels,
        fullWidth = info.fullWidth,
        fullHeight = info.fullHeight,
        initialValues = InitialValues()
            .scroll(initialPosition.first, initialPosition.second)
            .scale(initialScale)
            .minimumScaleMode(Fill)
            .maxScale(16f)
            .tileSize(info.tileSize)
            .workerCount(16)
            .rotationEnabled(true)
            .preloadingPadding(preloadingPadding)
            .bitmapFilteringEnabled { it.scale < 1 }
    )

The second version is more consistent, where in the first version you have to set some attributes in one way, and some change afterwards with an apply block.

[p-lr]

Also, the idea of InitValue is to initiate things which can change dynamically. So the tile size is definitely out.

[Nohus]

the workers count has nothing to do in there

the idea of InitValue is to initiate things which can change dynamically

I thought it was created to not pollute the MapState constructor with too many values. 🤔

[p-lr]

I get your point about readability. But the goal is not for InitValues to swallow everything that can be done in apply block.
In the MapState constructor, we have 3 things :

• mandatory parameters (dimensions, level count)
• fix parameters but optional (tile size, worker count)
• initial values (init values which can change dynamically, and only do that -- don't provide apis except when we have to)

[Nohus]

I can see why you don't consider many of these fitting as "initial values". I didn't think of the class as initial values, but as options for creating the MapState. That's why my original name for it was MapStateOptions.

[p-lr]

Yes the naming reflects the intention

[p-lr]

I'm not saying no to the idea of MapStateOptions. Just not now. Maybe in a v3 haha

[p-lr]

I'm going to keep your good ideas about more idiomatic builder pattern, and better initialization.

[p-lr]

Other than that, there's a good news: it's working! And we're getting closer this final v2.0.0

[Nohus]

it's working

Well I didn't just push the code without trying to run it first. 😄

[p-lr]

Please keep this PR open until I'm done (maybe tomorrow)

[Nohus]

I get your point about readability. But the goal is not for InitValues to swallow everything that can be done in apply block. In the MapState constructor, we have 3 things :

• mandatory parameters (dimensions, level count)
• fix parameters but optional (tile size, worker count)
• initial values (init values which can change dynamically, and only do that -- don't provide apis except when we have to)

Would you then say things like bitmapFilteringEnabled and rotationEnabled fit in the constructor as optional parameters? I agree with the division in these 3 categories, but the problem I see is that these things don't have a place, and you need yet another way to set them (the apply block).

[p-lr]

rotationEnabled can be changed dynamically.

[Nohus]

It's just that creating an object (MapState) and then having to mutate it directly afterwards feels to me like the object has lacking facilities to set it up properly in the first place.

[p-lr]

It's just that creating an object (MapState) and then having to mutate it directly afterwards feels to me like the object has lacking facilities to set it up properly in the first place.

When some APIs are used for dynamic behavior, and aren't actually useful at state construction (such as rotationEnabled), I prefer not to duplicate APIs and have two ways to do the same thing. That's a design choice which has pros and cons.
That's why there's a single API to add layers, in an apply block (and no possibility to add it in the constructor).

[Nohus]

Cherry-picked your commit on top.

[Nohus]

Or what you did. 😄

[p-lr]

That's a bit hacky but that works!

[Nohus]

One could argue adding a commit is less hacky than rewriting history. :)

[p-lr]

By "hacky" I meant why I did ;)