Adds an inner fill to the pointer and the ability to stroke inside the pointer

[jonathan-livly]

Adds an inner fill and the ability to stroke inside the pointer as seen here

This PR is slightly more opinionated with the name changes since it was written as a drop in replacement into our code base since I was unsure if you were still accepting PRs at the time. Deprecated some of the names to make it easier for our team to reason about the code after the updates.

Let me know if you disagree with any of the changes.

[tankery]

Hi, @jonathan-livly , Thanks for the PR. I understand that the changed name may sounds more intuitive. But unless there is a strong reason, we'd prefer to keep the existing APIs.

The original design of this name for the "pointerStrokeWidth", is actually based on the entire circle bar perspective. And the name is also matched with other properties such as circleStrokeWidth and pointerHaloWidth.

Regarding the new property you added, the pointerFillInnerStrokeWidth, it's a good design to have something like this. We already already have a similar concept, the pointerHaloBorderWidth. Given this, do you think we could naming it as something like pointerBorderWidth and pointerBorderColor?

Also if we are adding a new width to the pointer, please do consider the calculation of "pointerHaloPaint", which should display outside the boarder when userIsMovingPointer.

[jonathan-livly]

Hi, @jonathan-livly , Thanks for the PR. I understand that the changed name may sounds more intuitive. But unless there is a strong reason, we'd prefer to keep the existing APIs.

Understandable, I reverted those changes

The original design of this name for the "pointerStrokeWidth", is actually based on the entire circle bar perspective. And the name is also matched with other properties such as circleStrokeWidth and pointerHaloWidth.

Regarding the new property you added, the pointerFillInnerStrokeWidth, it's a good design to have something like this. We already already have a similar concept, the pointerHaloBorderWidth. Given this, do you think we could naming it as something like pointerBorderWidth and pointerBorderColor?

Also if we are adding a new width to the pointer, please do consider the calculation of "pointerHaloPaint", which should display outside the boarder when userIsMovingPointer.

So the way that I designed the effect was that there is a another circle drawn over the pointer created by pointerPaint with pointerFillPaint. It will always be at most as big as pointerFillPaint and pointerFillInnerStrokeWidthcan only be used to reduce the size. I added a check to make sure negative numbers can't be used after your comment. As it shrinks the pointerPaint is effectively stroking pointerFillPaint by the size of pointerFillInnerStrokeWidth.

I'm not super happy with the names I came up with either, pointerBorderWidth kind of makes sense to me and I'd be happy to make that change, but pointerBorderColor I think would be confusing based on the way that it works. Maybe pointerInnerBorderWidth or pointerInnerFillBorderWidth and pointerInnerFillColor

[tankery]

Thanks, I see what you mean, added some inline comments for your changes, please check if it make sense.

[tankery]

If we use a concept like "pointerInnerStrokeWidth" or "pointerOverlayStrokeWidth", we could simply apply the pointerInnerStrokeWidth here, and let the developer to do the math, and give developer the flexibility.

[jonathan-livly]

So this change I'd like to push back on.

I think that making the developer do the math adds more complexity without any added benefit. The user of the api would only be using the overlay to add a border inside of the pointer. When presented with a design you would be looking at the border width and doing the subtraction isn't difficult but really isn't needed. The use case was actually motivated directly by looking at an XD and knowing the size of the border.

By also giving them the ability to size the pointer overlay directly you introduce the ability to mistakenly use this property instead of the halo if it grows outside the pointer, which isn't possible when defining the border width directly. Currently if the border is increased too much the only downside is that it is no longer visible which will be immediately obvious as a mistake to the developer.

[jonathan-livly]

Updated per code review and added a comment

[tankery]

Thanks for the update, looks good overall, just some small questions.

[tankery]

If we use the concept of "overlay", than the border width of this overlay looks more like it's own width (circle diameter).

So at here we may directly set the pointerOverlayPaint.strokeWidth = pointerOverlayBorderWidth

[AlvaroZapp]

Hi!
I need this feature, when will it be launched?
Thanks!

[jonathan-livly]

Updated. Sorry for being a little slow, our repository is quite a bit ahead so haven't had much time to go back, but we would like to not have to use our fork.

Updated per the other code comments. I think we are just thinking about the pointerOverlayBorderWidth in different mindsets. The technical implementation of it is as an overlay which is why I believe you think it should be sized directly, but from a usage stand point I don't think it really makes to get the goal of its implementation, which is a border for the pointer. In that case it would also need to be changed to pointerOverlayWidth since the usage would be different

Ideally the pointer would simply be a circle of a certain width with a border/stroke of a certain width, which is how I believe most designers would be implementing the design specs they are passing off. With that in mind I think its an unnecessary step to make them do the math to determine the size of overlay to get the border they want.

With that in mind I think its a minor difference in how we are thinking so if you feel very strongly, its a change I can make but I think it makes the API harder to use, although only very slightly.

[tankery]

@jonathan-livly I do appreciate the effort you put to improve this library.

I understand your current approach would be more easy to apply from a design specs. And I agree that the current naming pattern for this library is not good enough for many cases.

But for this specific PR, I would think that naming consistency has a higher priority.

As you would see, we currently already have many properties with names like "**Width", such as pointerStrokeWidth, circleStrokeWidth, etc. And they all represent the Paint.strokeWidth when drawing that object.

So if we are adding a new property with the same naming pattern, but the Width is not the same meaning as before, this may require more explanation and context switching for developers.

Also if we use the concept of "overlay", we are talking about the white circle in your screenshot right? Then the "width" of this object should be something for the object itself, instead of the "gap width" between the while overlay and the blue pointer.

So I would still recommend just setting the pointerOverlayBorderWidth as the Paint.strokeWidth for the overlay object. (And maybe name it as pointerOverlayStrokeWidth?)

And after this PR. Feel free to rename properties to a better understanding pattern. But please do consider that this circular seek bar is built not only for "circle/point" pointers, but also for "arc/sector" pointers.