Merge pull request #1512 from omnivore-app/fix/android-popover-menu-position

PDF Highlight menu - Android

android/Omnivore/app/src/main/java/app/omnivore/omnivore/networking/HighlightMutations.kt

@@ -4,8 +4,10 @@ import android.util.Log
 import app.omnivore.omnivore.graphql.generated.CreateHighlightMutation
 import app.omnivore.omnivore.graphql.generated.DeleteHighlightMutation
 import app.omnivore.omnivore.graphql.generated.MergeHighlightMutation
+import app.omnivore.omnivore.graphql.generated.UpdateHighlightMutation
 import app.omnivore.omnivore.graphql.generated.type.CreateHighlightInput
 import app.omnivore.omnivore.graphql.generated.type.MergeHighlightInput
+import app.omnivore.omnivore.graphql.generated.type.UpdateHighlightInput
 import app.omnivore.omnivore.models.Highlight
 import com.apollographql.apollo3.api.Optional
 import com.google.gson.Gson
@@ -29,6 +31,18 @@ data class CreateHighlightParams(
   )
 }
 
+data class UpdateHighlightParams(
+  val highlightId: String?,
+  val `annotation`: String?,
+  val sharedAt: String?,
+) {
+  fun asUpdateHighlightInput() = UpdateHighlightInput(
+    annotation = Optional.presentIfNotNull(`annotation`),
+    highlightId = highlightId ?: "",
+    sharedAt = Optional.presentIfNotNull(sharedAt)
+  )
+}
+
 data class MergeHighlightsParams(
   val shortId: String?,
   val id: String?,
@@ -74,6 +88,17 @@ suspend fun Networker.deleteHighlights(highlightIDs: List<String>): Boolean {
   return !hasFailure
 }
 
+suspend fun Networker.updateWebHighlight(jsonString: String): Boolean {
+  val input = Gson().fromJson(jsonString, UpdateHighlightParams::class.java).asUpdateHighlightInput()
+  return updateHighlight(input)
+}
+
+suspend fun Networker.updateHighlight(input: UpdateHighlightInput): Boolean {
+  val result = authenticatedApolloClient().mutation(UpdateHighlightMutation(input)).execute()
+  Log.d("Network", "update highlight result: $result")
+  return result.data?.updateHighlight?.onUpdateHighlightSuccess?.highlight != null
+}
+
 suspend fun Networker.mergeWebHighlights(jsonString: String): Boolean {
   val input = Gson().fromJson(jsonString, MergeHighlightsParams::class.java).asMergeHighlightInput()
   return mergeHighlights(input)

android/Omnivore/app/src/main/java/app/omnivore/omnivore/ui/reader/AnnotationEditView.kt

@@ -1,5 +1,6 @@
 package app.omnivore.omnivore.ui.reader
 
+import android.content.DialogInterface
 import android.os.Bundle
 import android.view.LayoutInflater
 import android.view.View
@@ -20,10 +21,24 @@ import androidx.compose.ui.focus.focusRequester
 import androidx.compose.ui.platform.ComposeView
 import androidx.compose.ui.platform.ViewCompositionStrategy
 import androidx.compose.ui.unit.dp
-import androidx.fragment.app.Fragment
+import androidx.fragment.app.DialogFragment
 import app.omnivore.omnivore.ui.theme.OmnivoreTheme
 
-class AnnotationEditFragment : Fragment() {
+class AnnotationEditFragment : DialogFragment() {
+  private var onSave: (String) -> Unit = {}
+  private var onCancel: () -> Unit = {}
+  private var initialAnnotation: String = ""
+
+  fun configure(
+    initialAnnotation: String,
+    onSave: (String) -> Unit,
+    onCancel: () -> Unit,
+  ) {
+    this.initialAnnotation = initialAnnotation
+    this.onSave = onSave
+    this.onCancel = onCancel
+  }
+
   override fun onCreateView(
     inflater: LayoutInflater,
     container: ViewGroup?,
@@ -36,23 +51,29 @@ class AnnotationEditFragment : Fragment() {
       setContent {
         OmnivoreTheme {
           AnnotationEditView(
-            initialAnnotation = "Initial Annotation",
-            onSave = {},
-            onCancel = {}
+            initialAnnotation,
+            onSave,
+            onCancel,
+            dismissAction = { dismiss() }
           )
         }
       }
     }
   }
+
+  override fun onDismiss(dialog: DialogInterface) {
+    onCancel()
+    super.onDismiss(dialog)
+  }
 }
 
-// TODO: better layout and styling for this view
 @OptIn(ExperimentalMaterial3Api::class)
 @Composable
 fun AnnotationEditView(
   initialAnnotation: String,
   onSave: (String) -> Unit,
   onCancel: () -> Unit,
+  dismissAction: () -> Unit = {}
 ) {
   val annotation = remember { mutableStateOf(initialAnnotation) }
   val focusRequester = FocusRequester()
@@ -71,6 +92,7 @@ fun AnnotationEditView(
         TextButton(
           onClick = {
             onCancel()
+            dismissAction()
           }
         ) {
           Text("Cancel")
@@ -85,6 +107,7 @@ fun AnnotationEditView(
         TextButton(
           onClick = {
             onSave(annotation.value)
+            dismissAction()
           }
         ) {
           Text("Save")

android/Omnivore/app/src/main/java/app/omnivore/omnivore/ui/reader/PDFReader.kt

@@ -6,17 +6,16 @@ import android.content.ClipData
 import android.content.ClipboardManager
 import android.content.Context
 import android.graphics.PointF
+import android.graphics.Rect
 import android.graphics.RectF
 import android.graphics.drawable.Drawable
 import android.net.Uri
 import android.os.Bundle
-import android.view.Gravity
-import android.view.MotionEvent
-import android.view.View
+import android.util.Log
+import android.view.*
 import android.widget.Button
 import android.widget.EditText
 import android.widget.ImageView
-import android.widget.PopupMenu
 import androidx.activity.viewModels
 import androidx.appcompat.app.AppCompatActivity
 import androidx.core.content.ContextCompat
@@ -53,6 +52,8 @@ class PDFReaderActivity: AppCompatActivity(), DocumentListener, TextSelectionMan
   private var hasLoadedHighlights = false
   private var pendingHighlightAnnotation: HighlightAnnotation? = null
   private var textSelectionController: TextSelectionController? = null
+  private var clickedHighlight: Annotation? = null
+  private var clickedHighlightPosition: PointF? = null
 
   private lateinit var fragment: PdfFragment
   private lateinit var thumbnailBar: PdfThumbnailBar
@@ -84,7 +85,12 @@ class PDFReaderActivity: AppCompatActivity(), DocumentListener, TextSelectionMan
     viewModel.loadItem(slug, this)
   }
 
-  // TODO: implement onDestroy to remove listeners?
+  override fun onDestroy() {
+    actionMode?.finish()
+    resetHighlightTap()
+    // TODO: remove listeners?
+    super.onDestroy()
+  }
 
   private fun load(params: PDFReaderParams) {
     // First, try to restore a previously created fragment.
@@ -254,7 +260,9 @@ class PDFReaderActivity: AppCompatActivity(), DocumentListener, TextSelectionMan
     clickedAnnotation: Annotation?
   ): Boolean {
     if (clickedAnnotation != null) {
-      showHighlightSelectionPopover(clickedAnnotation)
+      clickedHighlight = clickedAnnotation
+      clickedHighlightPosition = pagePosition
+      startActionMode(null, ActionMode.TYPE_FLOATING)
     }
 
     return super.onPageClick(document, pageIndex, event, pagePosition, clickedAnnotation)
@@ -265,38 +273,6 @@ class PDFReaderActivity: AppCompatActivity(), DocumentListener, TextSelectionMan
     super.onPageChanged(document, pageIndex)
   }
 
-  private fun showHighlightSelectionPopover(clickedAnnotation: Annotation) {
-    // TODO: anchor popover at exact position of tap (maybe add an empty view at tap loc and anchor to that?)
-    val popupMenu = PopupMenu(this, fragment.view, Gravity.CENTER, androidx.appcompat.R.attr.actionOverflowMenuStyle, 0)
-
-    popupMenu.menuInflater.inflate(R.menu.pdf_highlight_selection_menu, popupMenu.menu)
-
-    popupMenu.setOnMenuItemClickListener { item ->
-      when(item.itemId) {
-        R.id.annotate -> {
-          viewModel.annotationUnderNoteEdit = clickedAnnotation
-          // Disabled notes for now since we didn't implement on ios
-//          showAnnotationView()
-        }
-        R.id.delete -> {
-          viewModel.deleteHighlight(clickedAnnotation)
-          fragment.document?.annotationProvider?.removeAnnotationFromPage(clickedAnnotation)
-        }
-        R.id.copyPdfHighlight -> {
-          val omnivoreHighlight = clickedAnnotation.customData?.get("omnivoreHighlight") as? JSONObject
-          val quote = omnivoreHighlight?.get("quote") as? String
-          quote?.let {
-            val clipboard = getSystemService(Context.CLIPBOARD_SERVICE) as ClipboardManager
-            val clip = ClipData.newPlainText(it, it)
-            clipboard.setPrimaryClip(clip)
-          }
-        }
-      }
-      true
-    }
-    popupMenu.show()
-  }
-
   private fun tintDrawable(drawable: Drawable, tint: Int): Drawable {
     val tintedDrawable = DrawableCompat.wrap(drawable)
     DrawableCompat.setTint(tintedDrawable, tint)
@@ -381,24 +357,105 @@ class PDFReaderActivity: AppCompatActivity(), DocumentListener, TextSelectionMan
     )
   }
 
+  var actionMode: ActionMode? = null
+
+  private val actionModeCallback = object : ActionMode.Callback2() {
+    // Called when the action mode is created; startActionMode() was called
+    override fun onCreateActionMode(mode: ActionMode, menu: Menu): Boolean {
+      actionMode = mode
+      mode.menuInflater.inflate(R.menu.pdf_highlight_selection_menu, menu)
+      return true
+    }
+
+    // Called each time the action mode is shown. Always called after onCreateActionMode, but
+    // may be called multiple times if the mode is invalidated.
+    override fun onPrepareActionMode(mode: ActionMode, menu: Menu): Boolean {
+      return false // Return false if nothing is done
+    }
+
+    // Called when the user selects a contextual menu item
+    override fun onActionItemClicked(mode: ActionMode, item: MenuItem): Boolean {
+      return when (item.itemId) {
+        R.id.annotate -> {
+          Log.d("pdf", "annotate button tapped")
+          clickedHighlight?.let {
+            viewModel.annotationUnderNoteEdit = it
+            showAnnotationView(viewModel.pluckExistingNote(it) ?: "")
+          }
+          true
+        }
+        R.id.delete -> {
+          Log.d("pdf", "remove button tapped")
+          clickedHighlight?.let {
+            viewModel.deleteHighlight(it)
+            fragment.document?.annotationProvider?.removeAnnotationFromPage(it)
+          }
+          resetHighlightTap()
+          true
+        }
+        R.id.copyPdfHighlight -> {
+          Log.d("pdf", "copy button tapped")
+
+          val omnivoreHighlight = clickedHighlight?.customData?.get("omnivoreHighlight") as? JSONObject
+          val quote = omnivoreHighlight?.get("quote") as? String
+          quote?.let {
+            val clipboard = getSystemService(Context.CLIPBOARD_SERVICE) as ClipboardManager
+            val clip = ClipData.newPlainText(it, it)
+            clipboard.setPrimaryClip(clip)
+          }
+          resetHighlightTap()
+          true
+        }
+        else -> {
+          Log.d("pdf", "unrecognized action")
+          resetHighlightTap()
+          false
+        }
+      }
+    }
+
+    // Called when the user exits the action mode
+    override fun onDestroyActionMode(mode: ActionMode) {
+      clickedHighlight = null
+      clickedHighlightPosition = null
+    }
+
+    override fun onGetContentRect(mode: ActionMode?, view: View?, outRect: Rect?) {
+      clickedHighlightPosition?.let {
+        val xValue = it.x.toInt()
+        val yValue = it.y.toInt()
+        val rect = Rect(xValue, yValue, xValue, yValue)
+        outRect?.set(rect)
+      }
+    }
+  }
+
+  private fun resetHighlightTap() {
+    actionMode?.finish()
+    actionMode = null
+    clickedHighlight = null
+    clickedHighlightPosition = null
+    viewModel.annotationUnderNoteEdit = null
+  }
+
+  override fun startActionMode(callback: ActionMode.Callback?, type: Int): ActionMode? {
+    return super.startActionMode(actionModeCallback, type)
+  }
+
   private fun showAnnotationView(initialText: String) {
-    val annotationDialog = Dialog(this)
-    annotationDialog.setContentView(R.layout.annotation_edit)
-
-    val textField = annotationDialog.findViewById(R.id.highlightNoteTextField) as EditText
-    textField.setText(initialText)
-    val confirmButton = annotationDialog.findViewById(R.id.confirmAnnotation) as Button
-
-    confirmButton.setOnClickListener {
-      val newNoteText =
-      annotationDialog.dismiss()
-    }
-
-    val cancelBtn = annotationDialog.findViewById(R.id.cancel) as Button
-    cancelBtn.setOnClickListener {
-      annotationDialog.dismiss()
-    }
-
-    annotationDialog.show()
+    val annotationEditFragment = AnnotationEditFragment()
+    annotationEditFragment.configure(
+      onSave = { newNote ->
+        clickedHighlight?.let { highlight ->
+          viewModel.updateHighlightNote(highlight, newNote)
+        }
+        resetHighlightTap()
+      },
+      onCancel = {
+        resetHighlightTap()
+      },
+      initialAnnotation = initialText
+    )
+    annotationEditFragment.show(fragment.childFragmentManager, null)
   }
 }

android/Omnivore/app/src/main/java/app/omnivore/omnivore/ui/reader/PDFReaderViewModel.kt

@@ -9,6 +9,7 @@ import androidx.lifecycle.viewModelScope
 import app.omnivore.omnivore.DatastoreRepository
 import app.omnivore.omnivore.graphql.generated.type.CreateHighlightInput
 import app.omnivore.omnivore.graphql.generated.type.MergeHighlightInput
+import app.omnivore.omnivore.graphql.generated.type.UpdateHighlightInput
 import app.omnivore.omnivore.models.LinkedItem
 import app.omnivore.omnivore.networking.*
 import com.apollographql.apollo3.api.Optional
@@ -115,7 +116,7 @@ class PDFReaderViewModel @Inject constructor(
 
     if (overlapIds.isNotEmpty()) {
       val input = MergeHighlightInput(
-        annotation = Optional.presentIfNotNull(newAnnotation.contents),
+        annotation = Optional.Absent, // TODO: make sure we preserve note locally
         articleId = itemID,
         id = highlightID,
         overlapHighlightIdList = overlapIds,
@@ -143,6 +144,27 @@ class PDFReaderViewModel @Inject constructor(
     }
   }
 
+  fun updateHighlightNote(annotation: Annotation, note: String) {
+    // Save the updated note locally
+    val omnivoreHighlight = annotation.customData?.get("omnivoreHighlight") as? JSONObject
+    omnivoreHighlight?.put("editedNote", note)
+    omnivoreHighlight?.let {
+      Log.d("pdf", "setting custom data: $omnivoreHighlight")
+      annotation.customData = JSONObject().put("omnivoreHighlight", it)
+    }
+
+    // Sync update with data service
+    viewModelScope.launch {
+      val input = UpdateHighlightInput(
+        annotation = Optional.presentIfNotNull(note),
+        highlightId = pluckHighlightID(annotation) ?: "",
+        sharedAt = Optional.Absent
+      )
+      networker.updateHighlight(input)
+      Log.d("network", "updated $annotation")
+    }
+  }
+
   fun deleteHighlight(annotation: Annotation) {
     val highlightID = pluckHighlightID(annotation) ?: return
     viewModelScope.launch {
@@ -168,6 +190,22 @@ class PDFReaderViewModel @Inject constructor(
     return omnivoreHighlight?.get("id") as? String
   }
 
+  fun pluckExistingNote(annotation: Annotation): String? {
+    val omnivoreHighlight = annotation.customData?.opt("omnivoreHighlight") as? JSONObject ?: return null
+
+    val editedNote = omnivoreHighlight.opt("editedNote") as? String
+    if (editedNote != null) { return editedNote }
+
+    val shortID = omnivoreHighlight.get("shortId") as? String ?: return null
+
+    pdfReaderParamsLiveData.value?.articleContent?.highlights?.let {
+      val matchingHighlight = it.firstOrNull { highlight -> highlight.shortId == shortID }
+      return matchingHighlight?.annotation
+    }
+
+    return null
+  }
+
   private fun hasOverlaps(leftAnnotation: Annotation, rightAnnotation: Annotation): Boolean {
     for (leftRect in (leftAnnotation as? HighlightAnnotation)?.rects ?: listOf()) {
       for (rightRect in (rightAnnotation as? HighlightAnnotation)?.rects ?: listOf()) {

android/Omnivore/app/src/main/java/app/omnivore/omnivore/ui/reader/WebReader.kt

@@ -88,6 +88,7 @@ fun WebReaderLoadingContainer(slug: String, webReaderViewModel: WebReaderViewMod
       TopAppBar(
         modifier = Modifier
           .height(height = with(LocalDensity.current) {
+            webReaderViewModel.currentToolbarHeight = toolbarHeightPx.value.toInt()
             toolbarHeightPx.value.roundToInt().toDp()
           } ),
         backgroundColor = MaterialTheme.colorScheme.surfaceVariant,
@@ -316,12 +317,12 @@ class OmnivoreWebView(context: Context) : WebView(context) {
       if (viewModel?.lastTapCoordinates != null) {
         val scrollYOffset = viewModel?.scrollState?.value ?: 0
         val xValue = viewModel!!.lastTapCoordinates!!.tapX.toInt()
-        val yValue = viewModel!!.lastTapCoordinates!!.tapY.toInt() + scrollYOffset
+        val yValue = viewModel!!.lastTapCoordinates!!.tapY.toInt() + scrollYOffset + (viewModel?.currentToolbarHeight ?: 0)
         val rect = Rect(xValue, yValue, xValue, yValue)
 
-        Log.d("wv", "scrollState: $scrollYOffset")
-        Log.d("wv", "setting rect based on last tapped rect: ${viewModel?.lastTapCoordinates.toString()}")
-        Log.d("wv", "rect: $rect")
+        Log.d("wvt", "scrollState: ${viewModel?.scrollState?.value}, bar height: ${viewModel?.currentToolbarHeight}")
+        Log.d("wvt", "setting rect based on last tapped rect: ${viewModel?.lastTapCoordinates.toString()}")
+        Log.d("wvt", "rect: $rect")
 
         outRect?.set(rect)
       } else {

android/Omnivore/app/src/main/java/app/omnivore/omnivore/ui/reader/WebReaderViewModel.kt

@@ -2,6 +2,7 @@ package app.omnivore.omnivore.ui.reader
 
 import android.util.Log
 import androidx.compose.foundation.ScrollState
+import androidx.compose.ui.unit.Density
 import androidx.lifecycle.MutableLiveData
 import androidx.lifecycle.ViewModel
 import androidx.lifecycle.viewModelScope
@@ -33,6 +34,7 @@ class WebReaderViewModel @Inject constructor(
   var lastJavascriptActionLoopUUID: UUID = UUID.randomUUID()
   var javascriptDispatchQueue: MutableList<String> = mutableListOf()
   var scrollState = ScrollState(0)
+  var currentToolbarHeight = 0
 
   val webReaderParamsLiveData = MutableLiveData<WebReaderParams?>(null)
   val annotationLiveData = MutableLiveData<String?>(null)
@@ -77,6 +79,10 @@ class WebReaderViewModel @Inject constructor(
       }
       "updateHighlight" -> {
         Log.d("Loggo", "receive update highlight action: $jsonString")
+        viewModelScope.launch {
+          val isHighlightUpdateSynced = networker.updateWebHighlight(jsonString)
+          Log.d("Network", "isHighlightUpdateSynced = $isHighlightUpdateSynced")
+        }
       }
       "articleReadingProgress" -> {
         viewModelScope.launch {

android/Omnivore/app/src/main/res/layout/annotation_edit.xml

@@ -1,24 +0,0 @@
-<?xml version="1.0" encoding="utf-8"?>
-<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
-    android:orientation="vertical"
-    android:layout_width="fill_parent" android:layout_height="fill_parent"
-    android:background="#ffffff">
-
-    <EditText
-        android:layout_width="fill_parent"
-        android:layout_gravity="center"
-        android:layout_height="wrap_content"
-        android:hint="Add note."
-        android:id="@+id/highlightNoteTextField"
-        android:textColor="#000000"
-        android:lines="3" />
-
-    <LinearLayout android:layout_width="fill_parent"
-        android:layout_height="fill_parent">
-        <Button android:id="@+id/confirmAnnotation" android:layout_height="wrap_content"
-            android:layout_width="wrap_content" android:text="Confirm"  android:layout_weight="1" />
-        <Button android:id="@+id/cancel" android:layout_height="wrap_content"
-            android:layout_width="wrap_content"  android:layout_weight="1"
-            android:text="Cancel" />
-    </LinearLayout>
-</LinearLayout>

android/Omnivore/app/src/main/res/menu/pdf_highlight_selection_menu.xml

@@ -1,15 +1,21 @@
 <?xml version="1.0" encoding="utf-8"?>
 <menu xmlns:app="http://schemas.android.com/apk/res-auto"
     xmlns:android="http://schemas.android.com/apk/res/android">
-    <item
-        android:id="@+id/delete"
-        android:title="@string/pdf_remove_highlight"
-        app:showAsAction="always">
-    </item>
-
     <item
         android:id="@+id/copyPdfHighlight"
         android:title="@string/pdf_highlight_copy"
         app:showAsAction="always">
     </item>
+
+    <item
+        android:id="@+id/annotate"
+        android:title="@string/pdf_highlight_menu_note"
+        app:showAsAction="always">
+    </item>
+
+    <item
+        android:id="@+id/delete"
+        android:title="@string/pdf_remove_highlight"
+        app:showAsAction="always">
+    </item>
 </menu>

android/Omnivore/app/src/main/res/values/strings.xml

@@ -11,4 +11,5 @@
     <string name="pdf_highlight_copy">Copy</string>
     <string name="highlight_note">Note</string>
     <string name="copyTextSelection">Copy</string>
+    <string name="pdf_highlight_menu_note">Note</string>
 </resources>

packages/web/components/templates/article/HighlightsLayer.tsx

@@ -275,8 +275,8 @@ export function HighlightsLayer(props: HighlightsLayerProps): JSX.Element {
       }
 
       const tapAttributes = {
-        tapX: event.clientX,
-        tapY: event.clientY,
+        tapX: event.screenX,
+        tapY: event.screenY,
       }
 
       window?.AndroidWebKitMessenger?.handleIdentifiableMessage(

packages/web/lib/highlights/useSelection.tsx

@@ -17,8 +17,8 @@ export function useSelection(
   const handleFinishTouch = useCallback(
     async (mouseEvent) => {
       const tapAttributes = {
-        tapX: mouseEvent.clientX,
-        tapY: mouseEvent.clientY,
+        tapX: mouseEvent.screenX,
+        tapY: mouseEvent.screenY,
       }
 
       window?.AndroidWebKitMessenger?.handleIdentifiableMessage(