Skip to content
This repository has been archived by the owner on Jun 9, 2023. It is now read-only.

Selftext selection #178

Merged
merged 3 commits into from
May 2, 2020
Merged

Conversation

AbsurdlySuspicious
Copy link
Collaborator

@AbsurdlySuspicious AbsurdlySuspicious commented Apr 28, 2020

No description provided.

@AbsurdlySuspicious
Copy link
Collaborator Author

It's also somewhat problematic to provide affected thread links for scrolling bug since it seems to be pretty much poorly reproducible. I have some links saved, but they works ok now. I'm not really sure now is this the same bug as the one happening on selftext selection attempts

@AbsurdlySuspicious
Copy link
Collaborator Author

AbsurdlySuspicious commented Apr 28, 2020

Also selftext view scrolls to its beginning when clicked. I haven't yet checked where even this handler is, but it's probably conflicts with selection too

@Tunous Tunous added the feature New feature label Apr 29, 2020
@tunous-bot
Copy link

Test version of Dank has been automatically built from this pull request.
Click here to download it.

@saket
Copy link

saket commented Apr 29, 2020

vote count is rendered using spans to the same textview where the title is, which means that 'select all' selects the vote count too

You could identify the selectable title by reading spans:

val voteSpan = title.getSpans(0, title.length, ForegroundSpan::class.java).first()
val selectableTitle = title.substring(voteSpan.start, title.length)

Obviously this can fail in the future because it assumes there's only one ForegroundSpan and it's used for rendering the vote count, so maybe you should make use of a unique span in SubredditUiConstructor when constructing the title:

Truss titleBuilder = new Truss();
+titleBuilder.pushSpan(new VoteCountSpan())
titleBuilder.pushSpan(new ForegroundColorSpan(ContextCompat.getColor(c, Themes.voteColor(voteDirection))));
titleBuilder.append(Strings.abbreviateScore(submissionScore));
titleBuilder.popSpan();
+titleBuilder.popSpan();
titleBuilder.append("  ");
titleBuilder.append(Html.fromHtml(submission.getTitle()));
val voteSpan = title.getSpans(0, title.length, VoteCountSpan::class.java).first()
val selectableTitle = title.substring(voteSpan.start, title.length)

@AbsurdlySuspicious
Copy link
Collaborator Author

The problem is rather that selection within single TextView can't be limited to some range in an obvious way, so user can freely select anything in it:

Screenshot_20200430-003036_1

I think this is somewhat possible but probably will be extremely buggy. Like listening to touch events or override onSelectionChange and reset the selection or something like that. Splitting this textview in two would be easier and more smooth than this

@saket
Copy link

saket commented Apr 29, 2020 via email

@AbsurdlySuspicious
Copy link
Collaborator Author

I've got some nice results. Check this out: #182
I will keep this one as selftext selection PR

@AbsurdlySuspicious AbsurdlySuspicious changed the title Selftext + title selection Selftext selection Apr 30, 2020
@AbsurdlySuspicious AbsurdlySuspicious force-pushed the selection_selftext branch 2 times, most recently from 739cc86 to 5ee6422 Compare May 1, 2020 17:20
@AbsurdlySuspicious AbsurdlySuspicious marked this pull request as ready for review May 1, 2020 17:22
@AbsurdlySuspicious
Copy link
Collaborator Author

AbsurdlySuspicious commented May 1, 2020

I've got this to work normally with this workaround in second commit. Any selectable TextView that gains focus causes RecyclerView to scroll in it's direction. Automatic scroll on tap isn't really harmful but focus gained due to selection causes RV to scroll pretty much randomly and completely messes up selection if TextView doesn't fit on screen. You can see example of this on second gif in #177.

Anyway, long selftexts and comments can be selected without any problems now. Scrolls on reply and comment collapsing both still works and aren't affected by this fix.

@@ -278,6 +282,32 @@ public void smoothScrollToPosition(RecyclerView recyclerView, RecyclerView.State
linearSmoothScroller.setTargetPosition(position);
startSmoothScroll(linearSmoothScroller);
}

// Workaround to keep arrow keys navigation working
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't actually tested this since I've got no working OTG cable. Arrow navigation aside, two methods below can be effectively replaced by just return false

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think that this is something we should worry about? I've actually never used arrow navigation myself so have no idea how that would affect it.

Copy link
Collaborator Author

@AbsurdlySuspicious AbsurdlySuspicious May 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, keyboards and stuff. Dawn already uses some rather heavy UI mods so I wouldn't be surprised if it's already broken. Then I just cover these methods with false?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let’s keep it. Seems good enough as is for me.

@Tunous
Copy link
Owner

Tunous commented May 2, 2020

Please add change-log entry.

@AbsurdlySuspicious
Copy link
Collaborator Author

sorry, forgot that

@Tunous Tunous merged commit 1708a4f into Tunous:master May 2, 2020
@Tunous Tunous added this to the 0.9.0 milestone May 2, 2020
@AbsurdlySuspicious AbsurdlySuspicious deleted the selection_selftext branch May 2, 2020 16:09
msfjarvis added a commit to msfjarvis/Dawn that referenced this pull request Aug 5, 2020
…jarvis/swipe-gesture-icon-position' into features

* msfjarvis/multiple_accounts: (24 commits)
  Adds multiple accounts support
  Prepare next version
  Update changelog for 0.9.1 (Tunous#198)
  Force logout all users (Tunous#196)
  Handle exceptions while opening a browser (Tunous#108) (Tunous#193)
  Prevent selection of negative ranges in SelectionLimitingTextView (fixes Tunous#190) (Tunous#194)
  Prepare next version
  Release 0.9.0 (Tunous#184)
  Release workflow (Tunous#185)
  Fix image loading issues due to http urls (Tunous#183)
  Selftext selection (Tunous#178)
  Submission title selection (Tunous#182)
  Update changelog with missed changes
  Wrap comment byline when it is too long to fit in one line (Tunous#145)
  Revert "Run Debug workflow only on push"
  Upgrade to Gradle 6.3 and add checksum field for F-Droid (Tunous#180)
  Run Debug workflow only on push
  Upgrade Gradle to 6.2.1 (Tunous#152)
  Use GitHub Actions (#72)
  Switch to on-device link unfurler (Tunous#138)
  ...

* msfjarvis/swipe-gesture-icon-position: (26 commits)
  Return false to keep the touch flow going
  Make the gestures' icons follow touch event position on Y
  Swipe Actions' icons at same level as touch event
  Prepare next version
  Update changelog for 0.9.1 (Tunous#198)
  Force logout all users (Tunous#196)
  Handle exceptions while opening a browser (Tunous#108) (Tunous#193)
  Prevent selection of negative ranges in SelectionLimitingTextView (fixes Tunous#190) (Tunous#194)
  Prepare next version
  Release 0.9.0 (Tunous#184)
  Release workflow (Tunous#185)
  Fix image loading issues due to http urls (Tunous#183)
  Selftext selection (Tunous#178)
  Submission title selection (Tunous#182)
  Update changelog with missed changes
  Wrap comment byline when it is too long to fit in one line (Tunous#145)
  Revert "Run Debug workflow only on push"
  Upgrade to Gradle 6.3 and add checksum field for F-Droid (Tunous#180)
  Run Debug workflow only on push
  Upgrade Gradle to 6.2.1 (Tunous#152)
  ...
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants