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

Nested visible frame inset fix #119

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

casperzandbergenyaacomm
Copy link
Collaborator

From commit messages:

After this fix there is one more issue:

When a horizontal layout section with visible frame inset is inside a section provider with vertical layout (or the other way around) the visible frame inset is actually twice the set value. This is due to the optimised index search not checking vertical position. Right now I added a guard to not show the section when visible frame is empty instead of when there is no frames inside the visibleFrame. Not sure if this is better optimised or not.

We can't really fix this since we can't actually check if the insets are in the right direction because the VisibleFrameInsetLayout might get transposed after checking.

Casper Zandbergen and others added 7 commits November 28, 2018 11:04
…th the visible indexes and the frame they appear in. This frame can be different from the input frame if the layout changes it (like VisibleFrameInsetLayout does).
…d by taking the order of indexes returned by the layout into account. This does add two filters when `visible(for:)` is called on a SectionProvider.
…ns a rect for the section to add its visible frame inset to.
…t (or the other way around). After this fix there is one more issue:

When a horizontal layout section with visible frame inset is inside a section provider with vertical layout (or the other way around) the visible frame inset is actually twice the set value. This is due to the optimised index search not checking vertical position. Right now I added a guard to not show the section when visible frame is empty instead of when there is no frames inside the visibleFrame. Not sure if this is better optimised or not.

A way to fix it would be to check if rootLayout is a vertical or horizontal layout and adjusting the inset inside the VisibleFrameInset init, but this is not the root of the problem so I'm not sure how to do it instead.
…e insets are in the right direction because the VisibleFrameInsetLayout might get transposed later.
@codecov-io
Copy link

Codecov Report

Merging #119 into master will increase coverage by 0.38%.
The diff coverage is 73.43%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #119      +/-   ##
==========================================
+ Coverage   73.46%   73.84%   +0.38%     
==========================================
  Files          41       41              
  Lines        1123     1147      +24     
==========================================
+ Hits          825      847      +22     
- Misses        298      300       +2
Impacted Files Coverage Δ
Sources/Protocol/Provider.swift 100% <ø> (ø) ⬆️
Sources/Layout/WrapperLayout.swift 20% <0%> (ø) ⬆️
Sources/Layout/VisibleFrameInsetLayout.swift 0% <0%> (ø) ⬆️
Sources/Layout/Layout.swift 20% <0%> (ø) ⬆️
Sources/Provider/EmptyCollectionProvider.swift 0% <0%> (ø) ⬆️
Sources/Layout/InsetLayout.swift 0% <0%> (ø) ⬆️
Sources/Provider/FlattenedProvider.swift 93.47% <100%> (+1.07%) ⬆️
Sources/CollectionView.swift 96.93% <100%> (ø) ⬆️
Sources/Layout/TransposeLayout.swift 89.47% <100%> (+0.58%) ⬆️
Sources/Protocol/LayoutableProvider.swift 100% <100%> (ø) ⬆️
... and 2 more

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 b78e4b3...910f732. Read the comment docs.

@casperzandbergenyaacomm
Copy link
Collaborator Author

Ah there is an unused CGRect extension in there by accident

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

Successfully merging this pull request may close these issues.

2 participants