Scroll indicators update - positioning correction, support for both indicators at the same time by janek · Pull Request #229 · flowkey/UIKit-cross-platform

janek · 2018-06-14T13:43:29Z

Corrects positioning, adds configurable indicator insets, adds conditional positioning when one/both indicators are visible.

Type of change: Bug fix + feature

Todos

corrected positioning
added support for both indicators at the same time
implemented default/base apple inset values
added support for configurable scrollIndicatorInsets (as per apple docs)
fixed other subviews of UIScrollView being added in front of indicators

Expected behavior

Scroll indicators should now position and style themselves in a way indistinguishable from Apple's.

Testing Details

Tested visually on NativePlayer with some additional marker views for better confidence.
Tested with a smaller dedicated app, both visually and numerically.

Screenshots

Current (desired) positioning of indicators:

Please check if the PR fulfills these requirements

Self-review: I am confident this is the simplest and clearest way to achieve the expected behaviour
There are no dependencies on other PRs or I have linked dependencies through Zenhub
The commit messages are clean and understandable
Tests for the changes have been added (for bug fixes / features)
Code runs on all relevant platforms (if major change: screenshots attached)

…set from code

…sent

…set from code

…sent

…a commented-out stub that could help fix that later

codecov · 2020-02-17T15:37:10Z

Codecov Report

Merging #229 into master will increase coverage by 0.71%.
The diff coverage is 97.59%.

@@            Coverage Diff             @@
##           master     #229      +/-   ##
==========================================
+ Coverage   52.15%   52.86%   +0.71%     
==========================================
  Files          87       87              
  Lines        3160     3210      +50     
==========================================
+ Hits         1648     1697      +49     
- Misses       1512     1513       +1

Impacted Files	Coverage Δ
Sources/UIScrollView.swift	`90.90% <92.85%> (+2.02%)`	⬆️
Sources/UIScrollView+indicatorsInternal.swift	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8f10d29...6a899ae. Read the comment docs.

…nd not only didSet)

janek · 2020-02-18T16:50:59Z

hey @ephemer :) I improved a lot here, addressing all the smaller comments you had.

Unfortunately, I still have a concern:

if you check scrollView.subviews on iOS, it will not include scroll indicators. In our version, in the state from this PR, it will include them. We can address this here or with a new issue, depending on how demanding/important it is. I gave it a moment's thought and decided it's best to ask you for opinion before trying, and a tip on how to go about it.

ephemer · 2020-02-18T17:27:19Z

@janek we could override var subviews to return a filtered version of private var _subviews = [UIView]. It's tricky though because AFAIK we're using subviews for some "internal" UIKit logic, which may or may not be correct.

But to me that's an issue that's totally separate from this PR. If you would like to update that let's do it for a practical (rather than idealistic) reason: right now I don't think it matters for us

janek · 2020-02-18T17:45:59Z

cool, that's what I was thinking, too, for the implementation. Glad I had the right idea (and the right idea to ask).

In that case, this is ready for re-review.

ephemer

Hi @janek, code-wise the library code seems fine but the tests are very shonky as is.

The tests contain false assumptions and hardcoded outdated values which don't mean anything. Sometimes we're calling an identical assertion twice (what's the point in calling XCTAssertEqual on two static values twice?), even though the second time the reality is that the underlying values have changed. It's not up to scratch as written.

Please try to rewrite the new tests in more finer-grained ones that test one thing, that are clearly named so it's obvious what to fix if they break, that don't rely on hardcoded values if possible, and that test the absolute minimal amount possible while still ensuring behaviour. And please be careful to get the new indexes after changing the order of an array.

ephemer · 2020-02-18T18:06:46Z

UIKitTests/UIScrollViewTests.swift

+        let indexOfInsertedView2 = scrollView.subviews.index(of: viewToBeInserted1)!
+
+        // If we try to insert at a position occupied by or above indicators,
+        // the inserted view should 'slide down' and assume the highest position below indicators


nice, these comments are helpful thanks!

but this should just be its own standalone test. it tests one single behaviour that obviously must be true. if we insert a subview it should always be behind the indicators. end of test. if that breaks we know what to fix. does that make sense?

ephemer · 2020-02-18T18:08:23Z

UIKitTests/UIScrollViewTests.swift

+        let indexOfInsertedView1 = scrollView.subviews.index(of: viewToBeInserted1)!
+
+        // This might seem weird, but that's what happens in iOS:
+        // the inserted view and the one that was in its place before will have the same index


is it possible this is only weird because of how the test is written?

It doesn't seem that weird to me that the indexes will overlap once we insert a new subview if we're not getting the new indexes after inserting the subview?

What am I missing here? It seems like we should get the new indexes here which would be less confusing?

You're not missing anything, I can't believe I missed this! 🤦‍♂ thanks!

samples/getting-started/DemoApp/ViewController.swift

ephemer · 2020-02-18T18:13:06Z

UIKitTests/UIScrollViewTests.swift

+
+
+        let viewToBeInserted2 = UIView()
+        scrollView.insertSubview(viewToBeInserted2, at: 5) // position currently occupied by scroll indicator


this test is too long. this could be two or three separate tests which would be much clearer if one of them fails

Thanks for this and all the other very valid concerns. I rewrote the test(s) in a way that hopefully takes into account all of them.

ephemer · 2020-02-18T18:14:56Z

UIKitTests/UIScrollViewTests.swift

+        XCTAssertEqual(indexOfInsertedView1, 1)
+        XCTAssertEqual(label2Index, 1)
+        XCTAssertEqual(label3Index, 2)
+        XCTAssertEqual(horizontalIndicatorIndex, 3)


This is misleading at best: all of these indexes will be incorrect now, so what's the point in asserting what their old value was? I'm really not happy with this, as written

ephemer · 2020-02-18T18:17:42Z

UIKitTests/UIScrollViewTests.swift

+
+        XCTAssert(label2Index > label1Index)
+
+        XCTAssertEqual(label1Index, 0)


what's the point in adding three subviews? we already have other tests that test the behaviour of addSubview. we're just writing more test code that we have to maintain.

how is this different to the case at the bottom?

ephemer · 2020-02-18T18:17:43Z

UIKitTests/UIScrollViewTests.swift

+        let label2Index = scrollView.subviews.index(of: view2)!
+        let label3Index = scrollView.subviews.index(of: view3)!
+
+        XCTAssert(horizontalIndicatorIndex > label1Index)


how is this different to the case at the bottom?

ephemer · 2020-02-18T18:20:39Z

UIKitTests/UIScrollViewTests.swift

+
+        let horizontalIndicatorIndex = scrollView.subviews.index(of: scrollView.horizontalScrollIndicator)!
+        let verticalIndicatorIndex = scrollView.subviews.index(of: scrollView.verticalScrollIndicator)!
+        let label1Index = scrollView.subviews.index(of: view1)!


I don't think it makes sense to put this into a variable, because (as mentioned below) it will be invalid as soon as we insert another view.

XCTAssertGreaterThan( scrollView.subviews.index(of: scrollView.horizontalScrollIndicator), scrollView.subviews.index(of: view1) )

samples/getting-started/DemoApp/ViewController.swift

…ndicators-update

janek · 2020-02-24T14:54:42Z

thanks @ephemer! I didn't reply to all of your comments, but I took them into account when rewriting tests. It's definitely a big improvement, and I'm quite happy. Also very happy to perfect them if there is anything else that you see could be improved :)

ephemer

Awesome! I am stoked with how great these new tests you wrote are looking now! Thanks for your work on this @janek

ephemer · 2020-03-13T09:51:36Z

UIKitTests/UIScrollViewTests.swift

+        scrollView.addSubview(mockView)
+
+        XCTAssertGreaterThan(
+            scrollView.subviews.index(of: scrollView.horizontalScrollIndicator)!,


These tests look seriously amazing now. Great work @janek!

that's too nice! thanks a lot!!

janek added the WIP label Jun 14, 2018

janek force-pushed the scroll-indicators-update branch from be4e9de to 38a274b Compare June 29, 2018 13:45

janek added 6 commits June 29, 2018 15:54

corrected positioning code

a699f9c

added a (hopefully temporary) decay term to offset calculation

61cf77b

included additional insets in the positioning, eliminated decayTerm

7c048e8

ensured default insets load appropriately, added small fixes

c1330a0

lay out indicators out to their initial position when new insets are …

8324590

…set from code

Corrected indicators positioning

9f1538c

janek force-pushed the scroll-indicators-update branch from 38a274b to 9f1538c Compare June 29, 2018 13:55

janek added 2 commits June 29, 2018 16:09

Added initial distance from scrollview frame

d4d833e

support for both indicators at once

bb7abba

janek changed the title ~~Scroll indicators update - adding scrollIndicatorsInsets~~ Scroll indicators update - positioning correction, support for both indicators at the same time Jun 29, 2018

janek added 4 commits June 29, 2018 17:46

additional spacing that prevents overlap when both indicators are pre…

6ce8af6

…sent

connected confiugrable insets code to placement code

15a32c5

abstracted out 1 additional step for clarity, corrected var privacy

ebb41cd

correction for overlap-avoidance, style improvements

19611b4

janek removed the WIP label Jul 1, 2018

corrected syntax bug

5f3149e

janek requested a review from ephemer July 7, 2018 13:47

janek added 11 commits July 13, 2018 14:44

corrected positioning code

e11665c

added a (hopefully temporary) decay term to offset calculation

696b5ec

included additional insets in the positioning, eliminated decayTerm

4a0d458

ensured default insets load appropriately, added small fixes

9df9578

lay out indicators out to their initial position when new insets are …

4b165b7

…set from code

Corrected indicators positioning

54afeee

Added initial distance from scrollview frame

4b2ed66

support for both indicators at once

a09c179

additional spacing that prevents overlap when both indicators are pre…

f26136c

…sent

connected confiugrable insets code to placement code

9042ce9

abstracted out 1 additional step for clarity, corrected var privacy

f3acd87

janek added 5 commits February 17, 2020 14:12

Add a test for scroll veiw with scroll indicators subview hierarchy

65c5f0c

Added information about indicators being accesible in subviews and …

0e44547

…a commented-out stub that could help fix that later

Fix mockTouch

fd14007

Add a guard to 'layout indicators if necessary'

2039d6d

FIx whitespace

b2e2c46

janek added 8 commits February 17, 2020 16:42

simplified earliestIndicatorInSubviewHierarchy

6379d21

Improve subview count test

fa4b095

Remove the stub for overriding subviews, as default is correct

abc824c

Update DemoApp

b3e59f0

Improve style in addSubview

17e0f0a

Add more hierarchy positions tests

658a4a2

Improve tests, refactor insertSubview

7298827

Add a comment stating that applyScrollIndStyle is called on init (a…

7d39faa

…nd not only didSet)

janek mentioned this pull request Feb 18, 2020

insertSubview:at: seems to be off by one #308

Closed

Update the DemoApp to change black to blue

8c57422

janek requested a review from ephemer February 18, 2020 16:55

ephemer requested changes Feb 18, 2020

View reviewed changes

ephemer reviewed Feb 18, 2020

View reviewed changes

samples/getting-started/DemoApp/ViewController.swift Outdated Show resolved Hide resolved

janek added 5 commits February 18, 2020 19:52

Restore ViewController for master

cc0452b

Divide test into two, clarify purpose and remove bug

8a81fe4

Improve comment and test names

ac1f2ec

Merge commit '8f10d29ea93f93ca65633c6bd1f1b863d2461b5a' into scroll-i…

36e63e4

…ndicators-update

Update insertSubview test, fix a bug in insertSubview

6a899ae

janek requested a review from ephemer February 19, 2020 17:12

ephemer approved these changes Mar 13, 2020

View reviewed changes



		let viewToBeInserted2 = UIView()
		scrollView.insertSubview(viewToBeInserted2, at: 5) // position currently occupied by scroll indicator


		XCTAssert(label2Index > label1Index)

		XCTAssertEqual(label1Index, 0)

Conversation

janek commented Jun 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Todos

Expected behavior

Testing Details

Screenshots

Please check if the PR fulfills these requirements

Uh oh!

codecov bot commented Feb 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

janek commented Feb 18, 2020

Uh oh!

ephemer commented Feb 18, 2020

Uh oh!

janek commented Feb 18, 2020

Uh oh!

ephemer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

janek commented Feb 24, 2020

Uh oh!

ephemer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

janek commented Jun 14, 2018 •

edited

Loading

codecov bot commented Feb 17, 2020 •

edited

Loading