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

feat(android): add getScaledTouchSlop() to ListView #11680

Merged
merged 5 commits into from
Jul 6, 2020

Conversation

m1ga
Copy link
Contributor

@m1ga m1ga commented May 3, 2020

JIRA:
https://jira.appcelerator.org/browse/TIMOB-27879

Optional Description:

Prevents the scrolling direction change to quickly. The current implementation will trigger an up/down change when you stop scrolling and your finger moves one pixel.

var win = Ti.UI.createWindow({backgroundColor: 'gray'});
var listView = Ti.UI.createListView({});
var sections = [];

var fruitSection = Ti.UI.createListSection({ headerTitle: 'Fruits'});
var fruitDataSet = [
    {properties: { height: 100,title: 'Apple'}},
    {properties: { height: 100,title: 'Apple'}},
    {properties: { height: 100,title: 'Apple'}},
    {properties: { height: 100,title: 'Apple'}},
    {properties: { height: 100,title: 'Apple'}},
    {properties: { height: 100,title: 'Apple'}},
    {properties: { height: 100,title: 'Apple'}},
    {properties: { height: 100,title: 'Banana'}},
    {properties: { height: 100,title: 'Banana'}},
    {properties: { height: 100,title: 'Banana'}},
    {properties: { height: 100,title: 'Banana'}},
    {properties: { height: 100,title: 'Banana'}},
    {properties: { height: 100,title: 'Banana'}},
    {properties: { height: 100,title: 'Banana'}},
    {properties: { height: 100,title: 'Banana'}},
    {properties: { height: 100,title: 'Banana'}},
    {properties: { height: 100,title: 'Banana'}},
    {properties: { height: 100,title: 'Banana'}},
    {properties: { height: 100,title: 'Banana'}},
];
fruitSection.setItems(fruitDataSet);
sections.push(fruitSection);

listView.appendSection(sections);
listView.addEventListener("scrolling",function(e){
	console.log(e.direction);
})
win.add(listView);
win.open();

@build
Copy link
Contributor

build commented May 3, 2020

Fails
🚫

🔬 There are library changes, but no changes to the unit tests. That's OK as long as you're refactoring existing code, but will require an admin to merge this PR. Please see README.md#unit-tests for docs on unit testing.

🚫 Tests have failed, see below for more information.
Warnings
⚠️

Commit 1dc9c5297285f8cbcdccddf1f54856306262cff2 has a message "scrollDamping" giving 2 errors:

  • subject may not be empty
  • type may not be empty
⚠️

Commit 46953a8eddc8f17a435ba8d75071f4336bc99b88 has a message "use touch slop" giving 2 errors:

  • subject may not be empty
  • type may not be empty
Messages
📖

💾 Here's the generated SDK zipfile.

📖 🎉 Another contribution from our awesome community member, m1ga! Thanks again for helping us make Titanium SDK better. 👍
📖

🚨 This PR has one or more commits with warnings/errors for commit messages not matching our configuration. You may want to squash merge this PR and edit the message to match our conventions, or ask the original developer to modify their history.

📖 ❌ 1 tests have failed There are 1 tests failing and 698 skipped out of 7371 total tests.

Tests:

ClassnameNameTimeError
android.emulator.Titanium.Apppause/resume events (9)8.895
Error: timeout of 5000ms exceeded
at Titanium.<anonymous> (/ti-mocha.js:6535:53120)

Generated by 🚫 dangerJS against 2f89959

@jquick-axway
Copy link
Contributor

@m1ga, I don't know if I like the idea of this. This effectively dampens all "scroll" events where it'll only fire if it has moved 5 pixels away from its last position. I'm curious what your use-case is.

@m1ga
Copy link
Contributor Author

m1ga commented Jun 20, 2020

@jquick-axway check the JIRA description. The current implementation will fire up/down changes for every pixel and it will create some noise if you stop moving your finger but the the phone will recognize tiny changes and e.g. it will already show/hide your views because it thinks you've change direction. Just putting the finger on the screen will most of the time already fire a up/down event.
Default value can always be 1 so it behaves like before but with 5 the user scroll experience is better

@jquick-axway
Copy link
Contributor

@m1ga, oh I see. Sorry about misunderstanding this. What you're doing makes sense.

My only feedback then is:

  • We shouldn't hard-code it to 5 pixels. Instead, use Google's ViewConfiguration.getScaledTouchSlop() to get the pixel tolerance needed which is scaled based on device DPI.
  • I don't think we need a new scrollDamping property. We can just hard-code it to use the touch-slop value like how Google does it internally with scroll views and drag-and-drop.

FYI: We use the touch-slop API to handle nested scrolling for Titanium's TextFields and TextAreas (link below). It works well and I haven't heard of any complaints.
TiUIEditText.java#L90-L93

What do you think?

@m1ga
Copy link
Contributor Author

m1ga commented Jun 23, 2020

@jquick-axway didn't know about that method! Nice, updated the PR, remove the custom value. Works fine here

@m1ga m1ga changed the title feat(android): add scrollDamping to ListView feat(android): add getScaledTouchSlop() to ListView Jun 23, 2020
Copy link
Contributor

@jquick-axway jquick-axway left a comment

Choose a reason for hiding this comment

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

CR: Pass

Copy link
Contributor

@ssjsamir ssjsamir left a comment

Choose a reason for hiding this comment

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

FR Passed Tested using the test case mentioned above, able to see scrolling direction being more stable and not changing constantly (perviously just placing your finger on the screen was enough to trigger the scroll event)

Test Environment

MacOS Big Sur: 11.0 Beta
Xcode: 12.0 Beta 
Java Version: 1.8.0_242
Android NDK: 21.3.6528147
Node.js: 12.18.1
""NPM":"5.0.0","CLI":"8.0.0""
API29 Pixel XL emulator

@sgtcoolguy sgtcoolguy merged commit 901f991 into tidev:master Jul 6, 2020
@m1ga m1ga deleted the scrollOffset branch August 7, 2020 07:54
@ssaddique
Copy link
Contributor

Fix verified on build 9.1.0.v20200804082025.

Test environment

OS Ver:         10.15.3
Xcode Ver:      Xcode 11.6
Appc NPM:       5.0.0
Appc CLI:       8.0.0
Node Ver:       10.17.0
Java Ver:       1.8.0_162
Device:      Pixel 3a (API 30 - Android 11)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants