zotero / zotero-android

Zotero for Android
Other
377 stars 36 forks source link

Incorrect toggling of full-screen mode when using annotation tools #178

Closed dstillman closed 1 day ago

dstillman commented 3 weeks ago

And then tapping on the page toggles it again. I'm actually having trouble even reliably creating an annotation. For example, in non-full-screen mode:

  1. Tap the toolbar button to show the annotation toolbar.
  2. Tap T. The reader incorrectly switches into full-screen mode.
  3. Tap the page background. The reader incorrectly switches out of full-screen mode and doesn't create an annotation, and the T is deactivated.
  4. Tap the page background again. The reader switches back into full-screen mode. The T doesn't show as selected.
  5. Tap the page background again. A text annotation is finally created, despite T not having been showing as selected.
dstillman commented 3 weeks ago

And then a bonus bug: you have to tap again on the text annotation field to select it and show the keyboard.

It should be three taps: once to show the annotation toolbar, once on T, and once on the page. You should then be able to type.

Dima-Android commented 3 weeks ago

I’ve completely reworked the logic of showing/hiding top and bottom bars. We used to rely on PSPDFKIT to tell us when to hide our top bar. We used to always sync top bar visibility with the integrated into the PSPDFKIT’s bottom bar (thumbnail grid) and for some reason whenever annotation tools were selected/deselected it always triggered UI visibility callback. Now we control visibility of top/bottom bars in manual mode. When I looked closer at how iOS is doing it I realized there are cases when the top bar is visible while the bottom bar is hidden. For example this happens during scrolling. Also iOS does not do anything to top/bottom bars when interacting with any of the annotation tools. Accounting for all of this should help with the jumpy UI you described.

T button getting deselected after you place the TextBox and text field inside of it getting auto-focused is also what happens on iOS. I think it's the behavior of PSPDFKIT to deselect the currently selected tool right after the empty TextBox is placed and we are notified of this after the fact. Not completely sure whether the TextBox field not getting focused and keyboard not appearing would be fixed by these changes, but there is a chance that it will since some of the jumpiness was eliminated. At least testing on my devices shows that the field is getting focused now.

Please test it and see if it works better and whether other adjustments are needed.

dstillman commented 3 weeks ago

Well, the good news is that worked once — I tapped T, I tapped the page, the textbox appeared, and the keyboard popped up.

The bad news is that now I'm tapping T and tapping the page and the T gets selected and nothing shows up, and then if I tap the page again…things get weird. Sometimes a textbox shows up, sometimes full-screen mode toggles, sometimes a textbox shows up in a previous location, sometimes it maybe shows up where I tapped? Oh, and looking at the sidebar, it seems like it's just creating empty annotations all around the page. It's definitely not usable.

If I force-quit the app, it works properly again once, and then stops working properly after that.

Dima-Android commented 3 weeks ago

Record a video, I can't reproduce it.

dstillman commented 3 weeks ago

This doesn't show taps, but you can see when the T gets deselected and nothing immediately shows up that I was tapping on the page. Other taps are selecting empty text annotations from previous attempts. (I can't get rid of them due to https://github.com/zotero/zotero-android/issues/181.)

Among other things, those empty ones shouldn't exist at all — if there's no text and you tap off an annotation, it shouldn't be created or it should be deleted if it already exists. But the initial problem is that tapping is only working properly, with visible textbox and keyboard, on the first try.

https://github.com/user-attachments/assets/9a47cfd0-6b93-458f-b392-6cf417b16a57

Dima-Android commented 2 weeks ago

On my phone and emulators it’s a very intermittent bug, I had to super-quickly create about 30 text annotations to reproduce this. I think I’ve found the problem - a race condition that happens when two things happen: onAnnotationAdded PSPDFKIT callback is triggered and we add Annotation to a database Another PSPDFKIT callback OnAnnotationSelected triggers our code related to processing the selection of annotation and as part of this we need to call selectedAnnotation function which is supposed to find this annotation in the database by key. Sometimes N2 is called before the N1 is finished, hence the selectedAnnotation returns null and this makes our code wrongly assume that the user is doing a deselection and so our code calls PSPDFKIT’s clearSelectedAnnotations() which leads to what you see - empty annotation is created, then immediately deselected and hence keyboard is not appearing.. The solution is to make sure those two processes run on the same thread hence avoiding the race condition.

But I am not sure if I got it right, so please test again.

dstillman commented 1 day ago

This seems OK now.