Skip to content

Conversation

@PoloNX
Copy link

@PoloNX PoloNX commented Feb 27, 2024

Write the function proposed here. Works fine

@XITRIX
Copy link
Collaborator

XITRIX commented Feb 27, 2024

@xfangfang, sorry, if that was your proposal, I would not agree with it at all. This should be 2 completely different UI components, at least @PoloNX can inherit Slider to not duplicate code for now (and later rewrite it as separate component), but in current solutions having irrevertible public function is completely bad practice

I'd better say that previous PR was good enough, only namings and a bit of code clean up had to be done there. It was much better than this solution

@xfangfang
Copy link
Owner

@XITRIX It’s okay for me to accept the original pr, but I feel that there is a lot of reusable code between the new and current components, so there is no need to reimplement the basic parts.
For me, perhaps inheriting the ProgressView from the previous PR and reimplementing the Slider would be a better idea, because as I mentioned before, the current slider component will experience jitter when progress changes occur.

@XITRIX
Copy link
Collaborator

XITRIX commented Feb 28, 2024

Ok, I checked how HOS uses such UI element, and I kinda changed my mind
IMG_6463
IMG_6464
It could be just a progress and draggable slider at the same time, so maybe this PR approach is a nice idea, btw it should include showPointer() as well, so this operation could be reverted back after hiding it

Speaking about jittering I also noticed it, not sure what is the source of it, probably floating point in width calculations causes it ... I could check if you want (I've made this slider several years ago btw :D)

@PoloNX
Copy link
Author

PoloNX commented Feb 29, 2024

Hi, just added the showPointer() method. Can someone review the code ?

@XITRIX
Copy link
Collaborator

XITRIX commented Mar 1, 2024

Check, pls, that showPointer() is exact mirror of hidePointer(), i.e. I cannot see reverse for pointer->setTranslationX(-3.5); in showPointer() function

Also, I'm not sure that this functions should call setFocusable, hide\show Pointer is more like "styling" functions and I as developer do not expect that it will change behaviour as well, I think it's better to explicitly call setFocusable(false) on place, if you really need to disable it's interaction behaviours. I.e. like I've mentioned above, in HOS it hides pointer, but do not disable interactions with it

@PoloNX
Copy link
Author

PoloNX commented Mar 1, 2024

When you focus the pointer on Hos, is there a small blue selection circle or nothing at all? Perhaps it should be hidden if it's not there on Hos.

@XITRIX
Copy link
Collaborator

XITRIX commented Mar 1, 2024

It appears when you start to drag it, you can check it's behaviour on your own in Album app

@PoloNX
Copy link
Author

PoloNX commented Mar 3, 2024

is it good ?

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.

3 participants