-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix(Whiteboard): broken color picker in RTL layout #19375
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
base: main
Are you sure you want to change the base?
Conversation
|
Important Maintainers: This PR contains Strings changes
|
f8cb9fe to
6d83ae0
Compare
AnkiDroid/src/main/java/com/ichi2/anki/ui/windows/reviewer/whiteboard/ColorPicker.kt
Outdated
Show resolved
Hide resolved
6d83ae0 to
fc185c3
Compare
fc185c3 to
203b635
Compare
|
Learning: NEVER change your phone's language to something you don't understand ☠️☠️ |
lukstbit
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's an issue with the behavior which I think is important:
If I start the color selection dialog and don't change at all the brightness then I get black as the color no matter what I select.
If the brightness slider is modified even a bit(even if changing it and then coming back to the initial position) then I get the color I selected.
Users will complain about this.
|
The drawing screen is also updated right? (Note editor -> clip icon -> Drawing) |
@lukstbit I don't get what you mean? I tried doing so, I didn't get black as color. I opened color-selection dialog and only changed color (no change to alpha/brightness). Screen_recording_20251103_215049.mp4 |
c9d2bbf to
b38aece
Compare
I can see the behavior I mentioned on both reviewers. It would be helpful if someone else can confirm/infirm what I'm seeing: Screen_recording_20251104_081652.webmScreen_recording_20251104_081917.webm |
|
Wow, weird! Will inspect it further, trying with different device. Difference I see here is light theme, will try once with light theme too. |
|
Reproduced with a light theme, but not with a dark one. |
|
This issue only happens when initially selected color is "Black". |
b38aece to
5451daa
Compare
|
Stuck at resolving the invalid color value, looks like a bug upstream related to conversion between ARGB and HSV pallets. |
david-allison
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only nitpicks, feel free to dismiss all. From a code perspective, this looks great.
I have neither checked the dependency, nor have I run the screen to see it in action
AnkiDroid/src/main/java/com/ichi2/anki/ui/windows/reviewer/whiteboard/ColorPickerDialog.kt
Outdated
Show resolved
Hide resolved
38bfc7c to
2f1681b
Compare
david-allison
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is excellent, thank you so much! Treat all my patches as TOTALLY optional.
The documentation of the library source is requested, only to make maintenance a little easier
- Install AnkiDroid
- Open the color picker in 'Reviewer'
- You see this:
Intuitively, a user is going to want to make red. They select 'red' and they see this:
Then they press 'OK', and they select get black, because they didn't move the bottom ('brightness') slider to 'red'.
The simple solution to this would be to show a 'current' selection [GIMP below, for reference].
AnkiDroid/src/main/java/com/ichi2/anki/ui/windows/reviewer/whiteboard/ColorPickerDialog.kt
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/ui/windows/reviewer/whiteboard/ColorPickerDialog.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/ui/windows/reviewer/whiteboard/ColorPickerDialog.kt
Outdated
Show resolved
Hide resolved
2f1681b to
5354ae1
Compare
ec9cf14 to
e773664
Compare
|
Almost!! The first color picker in your example could be confused for being a selection marker, as the selected color and outer color are the same. Could you try a minimal circular border (1px white) between the current color, and the outer color of the selection marker? |
|
Hmm... |
@david-allison Looks good? I had to import the svg into figma and put a white circle, adjust and play around the dimensions to be concentric with color-tile. Do you think there's a easier way to do that in XML? In compose I see a very clear way just to add a modifier if I want to draw something, I can go crazy and draw anything with canvas behind any ui-node. |
e773664 to
ad7f259
Compare
|
Android Vector XML isn't my strong suit. I'd presume you can just place a circle of radius n+1 underneath it |
|
Yeah, that's what I did, in actual svg though. |
|
Checked it out. Design-wise: yes. Latency-wise: there's noticeable lag between the position updates of the under-cursor icon and position marker. The latency is noticeable when moving fast. Reminds me of the tests done by MS: https://www.youtube.com/watch?v=vOvQCPLkPt4 I also managed to end up with this situation when I released my mouse button
|
Is it noticeable enough to bother a user? I tried on my device, physical and emulator.
The bubble tries to stay within the bounds of color wheel, while showing the color under the pointer. In general scenario too, bubble's tails stays at circumference of the circle, but the color is one at the center of pointer. Screencast.From.2025-11-27.14-34-09.mp4 |
BrayanDSO
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- There's a noticeable glitch when opening the dialog
Screen_recording_20251127_063307.mp4
- The flipping behavior when dragging the picker to the top is unnecessary and annoying IMO
Screen_recording_20251127_063413.mp4
Brayan's video (mildly) bothers me due to the latency EDIT: if this is an involved fix, we need to make the judgment call: "is this better than the past functionality, and can we add appropriate TODOs" |
|
latency is similar to before Screen_Recording_20251127_103602_AnkiDroid.mp4 |
Fixes: ankidroid#19237 replace existing color picker from another library Replaced With: https://github.com/skydoves/ColorPickerView/releases # Conflicts: # AnkiDroid/src/main/java/com/ichi2/anki/ui/windows/reviewer/whiteboard/WhiteboardFragment.kt
ad7f259 to
f14d31b
Compare









Purpose / Description
Replace the existing color picker library (inactive since last 4 years) with another, fairly up-to-date library.
Fixes
Approach
com.mrudultora.colorpicker.ColorPickerPopUpwith the equivalent new colorPickerfixed_rtl_picker.mp4
Before
before.mp4
After
after.mp4
How Has This Been Tested?
Manually tested on Poco X3 NFC
Checklist