tankery / CircularSeekBar

Custom circular SeekBar (Circle, Semi-circle, and Ellipse) for Android
Apache License 2.0
376 stars 60 forks source link

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

Open jonathan-livly opened 2 years ago

jonathan-livly commented 2 years ago

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

image

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 commented 2 years ago

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 commented 2 years ago

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

jonathan-livly commented 2 years ago

Updated per code review and added a comment

AlvaroZapp commented 2 years ago

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

jonathan-livly commented 2 years ago

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 commented 2 years ago

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