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

[TIMOB-16878] Android: Refactored ScrollableView #9696

Merged
merged 14 commits into from May 9, 2018

Conversation

jquick-axway
Copy link
Contributor

@jquick-axway jquick-axway commented Dec 23, 2017

JIRA:

Summary:

  • TIMOB-5996 Modified ScrollableView.removeView() to support integer indexes like iOS.
  • TIMOB-15780 Fixed ScrollableView.setCurrentPage() to not fire a "scrollend" event.
    • Event should only be fired when dragged or for methods that trigger scrolling animations.
    • Added missing "dragstart" event to be paired with existing "dragend" event. Now matches docs.
  • TIMOB-16878 Fixed bug where ScrollableView ignores top-most child view's layout properties.
    • Prevented properties width, height, top, bottom, left, and right from working.
    • Fixed by wrapping view with a TiCompositeLayout, which is needed to handle these layout properties.
  • TIMOB-25539 Fixed crash caused by adding a border to the ScrollableView.
    • Bug was introduced in Titanium 6.1.0.
  • TIMOB-25625 Modified left/right page arrows to be density scaled.
    • Was hard-code to 80 pixels for all screens.
    • Now uses base mdpi (1x) resolution of 24 pixels and scales based on DPI from there.
  • TIMOB-25626 Fixed ScrollableView to support Ti.UI.SIZE for width.
    • Android's ViewPager Java class does not natively support WRAP_CONTENT.
    • Refactored onMeasure() code to re-size width/height of container based on largest child view in this case.
  • TIMOB-25634 Modified ScrollableView so that cacheSize property can be set dynamically like iOS.
    • Added methods setCacheSize() and getCacheSize().
    • Can no longer be set less than 3, matching iOS' min cache size.
    • Now better converts iOS style cache size value to Android offscreen page count value.
  • TIMOB-25636 Fixed bug where calling ScrollableView setters cause a crash if parent window is closed.
    • Bug was introduced as of Titanium 7.0.0.
  • Removed API Level 9 guards from code.
    • Outdated code. Min API Level is currently 16.

Test 1:
This test ensures border applied to ScrollableView does not cause a crash.

  1. Build and run the code attached to TIMOB-25539 on Android.
  2. Verify that the app does not crash on startup.
  3. Verify that a blue border is displayed around the ScrollableView.

Test 2:
Test that left/right page arrows are now density scaled. Note that the overlaid arrows used to be too tiny on high DPI devices.

  1. Build and run the code attached to TIMOB-25625 on an "hdpi" and "xxhdpi" Android devices.
  2. Scroll to the right in the ScrollableView on both devices.
  3. Verify that the overlaid left/right page arrows are about the same size. (Arrows should no longer be tiny on a xxxhdpi device.)

Test 3:
Tests ScrollableView Ti.UI.SIZE support for TIMOB-25626. Also tests layout fix for TIMOB-16878.

  1. Build and run the code attached to TIMOB-25626 on Android.
  2. Note that the light gray container is the ScrollableView and the dark gray area is the Window.
  3. Verify that view looks like Android-good1.png on startup. Light gray container should not fill the window. (That's the old bad behavior.)
  4. Scroll the light gray view to the right (by swiping left).
  5. Verify view looks like Android-good2.png. Container should grow a bit bigger.
  6. Scroll the light gray view to the right (by swiping left).
  7. Verify view looks like Android-good3.png. container should grow a bit bigger.
  8. Scroll the light gray view to the right (by swiping left).
  9. Verify view looks like Android-good4.png. container should grow a bit bigger.
  10. Scroll the light gray view to the right (by swiping left).
  11. Purple view should now completely fill light gray container like Android-good5.png.

Test 4:
Test dynamically changing cacheSize property.

  1. Build and run file ScrollableViewCacheSizeTest.js attached to TIMOB-25634 on Android.
  2. Verify that the following gets logged...
[INFO] :   @@@ Initialized cacheSize: 5
[INFO] :   @@@ Property assigned cacheSize: 4
[INFO] :   @@@ Method assigned cacheSize: 3

Test 5:

  1. Build and run file ScrollableViewAddRemoveTest.js attached to TIMOB-15780 on Android.
  2. Note that this app automatically cycles through the pages programmatically via a timer.
  3. Watch the Android log.
  4. Verify that @@@ [scrollend] does not get logged when it programmatically changes pages. This is the fix for TIMOB-15780.
  5. Scroll to another page with your finger. (Swipe left or right.)
  6. Verify that a [dragstart], [scroll], [dragend], and [scrollend] get logged when you scroll via the touchscreen yourself.
  7. Tap the "Remove" button in the bottom-right corner.
  8. An alert dialog is displayed. Tap the "Current" button to remove current page. This removes page by index and tests TIMOB-5996.
  9. Repeat steps 6 and 7 until all 4 pages have been removed.

- [TIMOB-16878] Fixed bug where ScrollableView ignores top-most child view's layout properties.
  * Prevented properties width, height, top, bottom, left, and right from working.
  * Fixed by wrapping view with a TiCompositeLayout, which is needed to handle these layout properties.
- [TIMOB-25539] Fixed crash caused by adding non-Titanium view to ScrollableView.
- [TIMOB-25625] Modified left/right page arrows to be density scaled.
- [TIMOB-25626] Fixed ScrollableView to support Ti.UI.SIZE for width.
  * Android's "ViewPager" Java class does not natively support WRAP_CONTENT.
  * Refactored onMeasure() code to re-size width/height of container based on largest child view in this case.
- Removed API Level 9 guards from code. (Outdated code. Min API Level is currently 16.)
… Replaces "moveNext" and "movePrev".

- Successfully tested on Android and iOS. (Have not tested on Windows.)
…16878]

- [TIMOB-5996] Modified ScrollalbeView.removeView() to support integer indexes like iOS.
- [TIMOB-15780] Fixed ScrollableView.setCurrentPage() to not fire a "scrollend" event.
  * Event should only be fired when dragged or for methods that trigger scrolling animations.
- [TIMOB-25634] Modified ScrollableView so that "cacheSize" property can be set dynamically like iOS.
- [TIMOB-25636] Fixed bug where calling ScrollableView setters cause a crash if parent window is closed.
  * Bug was introduced in Titanium 7.0.0.
Copy link
Contributor

@garymathews garymathews 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

Although, a minor change could be made.

{
// Do not allow given size to be less than min. (iOS min size is 3.)
if (value < ScrollableViewProxy.MIN_CACHE_SIZE) {
StringBuilder stringBuilder = new StringBuilder();
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure the reason behind StringBuilder here? We could just do this:

Log.w(TAG, "ScrollableView 'cacheSize' cannot be set less than " +
            ScrollableViewProxy.MIN_CACHE_SIZE + ". Given value: " + value);

@garymathews
Copy link
Contributor

@jquick-axway Looks like there's lint issues that need resolving

@jquick-axway jquick-axway modified the milestones: 7.1.0, 7.2.0 Feb 16, 2018
@build
Copy link
Contributor

build commented May 9, 2018

Messages
📖

💾 Here's the generated SDK zipfile.

Generated by 🚫 dangerJS

@lokeshchdhry lokeshchdhry merged commit dfc1cc0 into tidev:master May 9, 2018
@sgtcoolguy sgtcoolguy modified the milestones: 7.2.0, 7.3.0 May 16, 2018
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.

None yet

5 participants