Anki-Android icon indicating copy to clipboard operation
Anki-Android copied to clipboard

fix(Whiteboard): broken color picker in RTL layout

Open thedroiddiv opened this issue 2 months ago • 23 comments

Purpose / Description

Replace the existing color picker library (inactive since last 4 years) with another, fairly up-to-date library.

Fixes

  • Fixes: #19237

Approach

  1. Create a utility function to create a color picker dialog with appropriate defaults
  2. Replace the usage of com.mrudultora.colorpicker.ColorPickerPopUp with the equivalent new colorPicker
before after

https://github.com/user-attachments/assets/3e86f0c0-d58e-4a49-82a8-091825d72099

Before
After

How Has This Been Tested?

Manually tested on Poco X3 NFC

Checklist

  • [x] You have a descriptive commit message with a short title (first line, max 50 chars).
  • [x] You have commented your code, particularly in hard-to-understand areas
  • [x] You have performed a self-review of your own code
  • [x] UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
  • [ ] UI Changes: You have tested your change using the Google Accessibility Scanner

thedroiddiv avatar Oct 27 '25 21:10 thedroiddiv

[!IMPORTANT] Maintainers: This PR contains https://github.com/ankidroid/Anki-Android/labels/Strings changes

  1. Sync Translations before merging this PR and wait for the action to complete
  2. Review and merge the auto-generated PR in order to sync all user-submitted translations
  3. Sync Translations again and merge the PR so the huge automated string changes caused by merging this PR are by themselves and easy to review

github-actions[bot] avatar Oct 27 '25 21:10 github-actions[bot]

Learning: NEVER change your phone's language to something you don't understand ☠️☠️

thedroiddiv avatar Oct 28 '25 14:10 thedroiddiv

The drawing screen is also updated right? (Note editor -> clip icon -> Drawing)

ZornHadNoChoice avatar Oct 28 '25 21:10 ZornHadNoChoice

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.

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

https://github.com/user-attachments/assets/01334489-003f-4dbb-8a6f-28b33d07b40c

thedroiddiv avatar Nov 03 '25 16:11 thedroiddiv

@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

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

Screen_recording_20251104_081917.webm

lukstbit avatar Nov 04 '25 06:11 lukstbit

Wow, weird! Will inspect it further, trying with different device. Difference I see here is light theme, will try once with light theme too.

thedroiddiv avatar Nov 04 '25 06:11 thedroiddiv

Reproduced with a light theme, but not with a dark one.

BrayanDSO avatar Nov 04 '25 09:11 BrayanDSO

This issue only happens when initially selected color is "Black". When the initially selected color is black, color picker's anchor is at center (White point) and brightness is at max.

thedroiddiv avatar Nov 04 '25 16:11 thedroiddiv

Stuck at resolving the invalid color value, looks like a bug upstream related to conversion between ARGB and HSV pallets.

thedroiddiv avatar Nov 10 '25 16:11 thedroiddiv

You also need to fill out the Licenses section under the PR description template

BrayanDSO avatar Nov 11 '25 09:11 BrayanDSO

Alpha now set to correct position after setInitialColor

thedroiddiv avatar Nov 22 '25 18:11 thedroiddiv

Picked bubble shows color with alpha

image image image image image

thedroiddiv avatar Nov 26 '25 09:11 thedroiddiv

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?

david-allison avatar Nov 26 '25 11:11 david-allison

Hmm... Lemme try!

thedroiddiv avatar Nov 26 '25 11:11 thedroiddiv

image image

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

thedroiddiv avatar Nov 26 '25 11:11 thedroiddiv

Android Vector XML isn't my strong suit.

I'd presume you can just place a circle of radius n+1 underneath it

david-allison avatar Nov 26 '25 11:11 david-allison

Yeah, that's what I did, in actual svg though.

thedroiddiv avatar Nov 26 '25 11:11 thedroiddiv

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

Screenshot 2025-11-26 at 23 51 27

david-allison avatar Nov 26 '25 23:11 david-allison

Latency-wise: there's noticeable lag between the position updates of the under-cursor icon and position marker.

Is it noticeable enough to bother a user? I tried on my device, physical and emulator. I did see some lag between my mouse movement and cursor but it din't bother me. I'd see this as a problem if I were drawing something.

I also managed to end up with this situation when I released my mouse button

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.

https://github.com/user-attachments/assets/4964b07b-0b03-4dd3-83a3-c2f1b86014b6

thedroiddiv avatar Nov 27 '25 09:11 thedroiddiv

Is it noticeable enough to bother a user? I tried on my device, physical and emulator.

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"

david-allison avatar Nov 27 '25 13:11 david-allison

latency is similar to before

https://github.com/user-attachments/assets/3f91a1a1-3506-4fad-bbfb-237f98508520

BrayanDSO avatar Nov 27 '25 13:11 BrayanDSO

Latency of the new color selection cursor updates- it's not aligned with the 'touch' cursor. Screenshot from a video I made:

It's expected that they're not perfectly in sync with the user's touch, but they should be aligned with each other

Screenshot 2025-11-27 at 14 52 20

david-allison avatar Nov 27 '25 14:11 david-allison

Thanks @david-allison Will apply the patch the push this week :) Time to learn more about Choreographer API, haven't yet looked at it!

thedroiddiv avatar Dec 08 '25 14:12 thedroiddiv