Skip to content
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

Add ability to select posts/comments #1272

Closed
wants to merge 5 commits into from

Conversation

micahmo
Copy link
Member

@micahmo micahmo commented Apr 3, 2024

Pull Request Description

This PR adds support for free selection mode of posts and comments. This works whether in markdown (source) mode or not. Note that for comments, when in selectable mode, the long-press menu is only accessible from the metadata area. Since this may not be obvious to users, I've added an explicit button to exit selectable mode.

P.S. Can you make sure cupertinoTextSelectionControls looks good on iOS?

Review without whitespace.

Issue Being Fixed

Issue Number: #1127

Screenshots / Recordings

qemu-system-x86_64_BiVazhB9Ua.mp4

Checklist

  • Did you update CHANGELOG.md?
  • Did you use localized strings where applicable?
  • Did you add semanticLabels where applicable for accessibility?

@micahmo
Copy link
Member Author

micahmo commented Apr 4, 2024

Just a quick note on this. The original bug implied that "Share"/"Web search" might be useful things things to have in the selection menu. For example, here's what those look like in the Android system webview.

image

Share has already been added to the menu for Android in flutter/flutter#141447, but it doesn't appear to be in a stable release yet (I tried the latest, 3.19.5). It does appear to be in the latest pre-release, 3.21.0-1.0.pre.2, so I think we'll get it when we upgrade. I did make some code changes so that it should work automatically at that point. (While we could add custom buttons to the menu now, it's probably best to wait for the official ones.) Screenshot below.

image

Regarding search, it seems like this PR (flutter/flutter#139361) added support for querying custom text actions (i.e., allowing other apps to insert themselves into the text actions menu), but it's unclear to me whether this would support a "Web search" action like we see in system apps. For context, here is the umbrella issue: flutter/flutter#107578.

Anyway, all that to say, there's more to do here, but we need to wait for some Flutter upgrades.

@hjiangsu
Copy link
Member

Just wanted to say thanks for doing this! This is something that was very much lacking and would be super useful to have. Just food for thought: do you think its better to have in-line selection of text, or bring up a modal which then allows free-selection of text?

I've been looking at other apps, and it seems like most, if not all of them decided to use the modal route for selection of text. It does seem like there are some advantages to the modal approach. For example,

  • It allows us to have action buttons when selecting text (e.g., the most common one being a "select all" action). We somewhat have this for comments, but it's two separate actions (one for selecting text, another for copying all text)

    • In our case, we could add more action buttons. One I can think of right now is the ability to toggle between markdown/preview modes more easily. Right now, if we want to copy the raw markdown, we have to perform several taps (tap to open overflow menu, tap to toggle source, tap to open overflow menu, tap to select text, perform text selection, and perform everything in reverse to get back the default view)
    • If we have a modal, then it would very much simplify this use-case of selecting raw markdown since the actual post body is unaffected
  • It makes the action of selecting text very deliberate. We can somewhat see it in the demo above with the comments selection. If I have text selection on for a comment, it's not very obvious that I'm in that "mode" without the need for an extra button to exit out of that mode. Furthermore, selecting text potentially conflicts with other actions on that page

    • For example, I tried to select some text in a comment, but since I have swipe gestures enabled, it was also trying to perform the swipe action in some cases where I wasn't very precise
    • This potentially extends to other swipe gestures, like the full screen swipe to go back
RPReplay_Final1713459867.mp4

@hjiangsu
Copy link
Member

One drawback of using a modal though that I just thought of, is that we would want to be able to perform text selection when replying to a post/comment.

I believe I made a regression in #1165 where the content of the post/comment was no longer selectable (because it's now using PostSubview and CommentContent widgets) rather than pure selectable markdown.

@micahmo
Copy link
Member Author

micahmo commented Apr 18, 2024

I believe I made a regression in #1165 where the content of the post/comment was no longer selectable (because it's now using PostSubview and CommentContent widgets) rather than pure selectable markdown.

Ah, I think we always want that to be selectable. I also had an idea that if you selected text in the reply comment and pressed the markdown quote button, it would immediatley put that selected text in the quote (without having to copy/paste). That could also go along with the ability to view raw in the reply area, so you can quote something exactly how it was originally formatted. But that can come later. 😊

@hjiangsu
Copy link
Member

Ah, I think we always want that to be selectable.

Just made a PR to fix this! #1315

@micahmo micahmo mentioned this pull request Apr 25, 2024
3 tasks
@hjiangsu
Copy link
Member

hjiangsu commented May 1, 2024

If its okay with you, I'll close this PR in favour of #1327?

@micahmo micahmo closed this May 1, 2024
@micahmo micahmo deleted the feature/selectable-text branch June 2, 2024 01:45
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.

2 participants