Scroll indicators update - positioning correction, support for both indicators at the same time

[janek]

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

Type of change: Bug fix + feature

Todos

• [x] corrected positioning
• [x] added support for both indicators at the same time
• [x] implemented default/base apple inset values
• [x] added support for configurable scrollIndicatorInsets (as per apple docs)
• [x] 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

• [x] Self-review: I am confident this is the simplest and clearest way to achieve the expected behaviour
• [x] 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)

[rikner]

@ephemer @janekszynal is there anything preventing us from merging this? do we need any tests?

[janek]

hey @ephemer, I can't reply directly to your last comment, not sure why. It might be because I changed the code it was referencing (and it got auto-marked as outdated) or because it was a comment to the PR in general and not a line in code (and therefore doesn't have a reply thread).

Anyway, I changed the add/insert subview code. Couldn't avoid writing a few extra lines, but I think it's all pretty understandable and therefore no big harm done. Let me know what you think.

[ephemer]

Hi @janek, I'd really like to get this PR merged because it's been open for a long time. My main concern is still this ordering of the indicators. I have the feeling that part could be buggy still. Let's take some time when you're back in the office to go through this and get it merged

[janek]

yeah, I would love that too, it's been very long. Let's try tomorrow!

[codecov[bot]]

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%)	:arrow_up:
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.

[janek]

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]

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

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.

[janek]

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 :)