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

Adding recursive find on firstResponder UIView extension. #447

Merged
merged 13 commits into from Apr 19, 2018

Conversation

Projects
None yet
5 participants
@LucianoPAlmeida
Copy link
Member

LucianoPAlmeida commented Apr 13, 2018

Checklist

  • I checked the Contributing Guidelines before creating this request.
  • New extensions are written in Swift 4.
  • New extensions support iOS 8.0+ / tvOS 9.0+ / macOS 10.10+ / watchOS 2.0+.
  • I have added tests for new extensions, and they passed.
  • All extensions have a clear comments explaining their functionality, all parameters and return type in English.
  • All extensions are declared as public.
  • I have added a changelog entry describing my changes.
@SwifterSwiftBot

This comment has been minimized.

Copy link

SwifterSwiftBot commented Apr 13, 2018

1 Message
📖 Thank you for submitting a pull request to SwifterSwift. The team will review your submission as soon as possible.

Generated by 🚫 Danger

@@ -86,11 +86,20 @@ public extension UIView {

/// SwifterSwift: First responder.
public var firstResponder: UIView? {

This comment has been minimized.

Copy link
@SD10

SD10 Apr 13, 2018

Member

We may want to consider making this a function now and mention it's recursive behavior in the comment.
/// SwifterSwift: Recursively find the first responder.

}
return nil
guard
!isFirstResponder else { return self }

This comment has been minimized.

Copy link
@SD10

SD10 Apr 13, 2018

Member

Not gonna lie, this style really irks me 😂 Nit though

This comment has been minimized.

Copy link
@LucianoPAlmeida

LucianoPAlmeida Apr 13, 2018

Author Member

😂

@codecov

This comment has been minimized.

Copy link

codecov bot commented Apr 13, 2018

Codecov Report

Merging #447 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #447      +/-   ##
==========================================
+ Coverage   91.64%   91.69%   +0.05%     
==========================================
  Files          58       58              
  Lines        2680     2686       +6     
==========================================
+ Hits         2456     2463       +7     
+ Misses        224      223       -1
Flag Coverage Δ
#ios 91.69% <100%> (+0.05%) ⬆️
#osx 91.69% <100%> (+0.05%) ⬆️
#tvos 91.69% <100%> (+0.05%) ⬆️
Impacted Files Coverage Δ
Sources/Extensions/UIKit/UIViewExtensions.swift 66.96% <100%> (+1.38%) ⬆️

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 0bb4f03...e222a27. Read the comment docs.

@LucianoPAlmeida

This comment has been minimized.

Copy link
Member Author

LucianoPAlmeida commented Apr 13, 2018

Related to #415 :))

if let responder = subviews.first(where: { $0.isFirstResponder }) {
return responder
}
for subview in subviews {

This comment has been minimized.

Copy link
@omaralbeik

omaralbeik Apr 13, 2018

Member

This can be replaced with for subview in subviews where let responder = subview.firstResponder { ... }

This comment has been minimized.

Copy link
@LucianoPAlmeida

LucianoPAlmeida Apr 13, 2018

Author Member

@omaralbeik I've tried that, but was givin compilation error ¯_(ツ)_/¯
screen shot 2018-04-13 at 07 22 38

This comment has been minimized.

Copy link
@omaralbeik

omaralbeik Apr 13, 2018

Member

hmm, how about this

for subview in subviews where subview.firstResponder() != nil {
    return subview.firstResponder()!
}

This comment has been minimized.

Copy link
@LucianoPAlmeida

LucianoPAlmeida Apr 13, 2018

Author Member

@omaralbeik I think this is no good because it calls the recursive function twice, also see the "!" really irks me 😂

This comment has been minimized.

Copy link
@SD10

SD10 Apr 13, 2018

Member

Yeah, I hate seeing force unwrapping in my code. The only time I do an explicit nil check and then force unwrap is if I don't care about the unwrapped value. If I'm going to use the value then I always use an if let.

@SD10

This comment has been minimized.

Copy link
Member

SD10 commented Apr 13, 2018

I still think we should make this one a function. A property is pretty deceptive in terms of complexity. Iterating all the subviews and then iterating all the subview's subviews 🤔

/// SwifterSwift: Set some or all corners radiuses of view.
/// SwifterSwift: Recursively find the first responder.
public func firstResponder() -> UIView? {
guard

This comment has been minimized.

Copy link
@guykogus

guykogus Apr 13, 2018

Contributor

Wouldn't this work?

public var firstResponder: UIView? {
    guard !isFirstResponder else { return self }

    return subviews.first(where: { $0.firstResponder != nil })
}

It would do a recursive, depth-first search for the first responder.

This comment has been minimized.

Copy link
@guykogus

guykogus Apr 13, 2018

Contributor

Also, doing it this way you wouldn't need to turn it into a function and we can avoid a breaking change.

This comment has been minimized.

Copy link
@LucianoPAlmeida

LucianoPAlmeida Apr 13, 2018

Author Member

@guykogus Thank's for the feedback :))
But actually, the way is implemented is because it's better for performance, in the way you mentioned if you have a first responder as the last subview, for example, it will navigate to the whole view hierarchy tree for the other subviews before gets there

This comment has been minimized.

Copy link
@guykogus

guykogus Apr 13, 2018

Contributor

So my way is a depth-first search. Logically it's the exact same as what you've done.
Alternately we could do it as a breadth-first search, but that's a bit uglier.

public var firstResponder: UIView? {
    var firstResponder: UIView?

    var views = [self]
    var i = 0
    while i < views.count, firstResponder == nil {
        let view = views[i]
        if view.isFirstResponder {
            firstResponder = view
        } else {
            views.append(contentsOf: view.subviews)
            i += 1
        }
    }
    return firstResponder
}

The question is, why would one method be better than the other? Is there any reason why BF search would be faster than DF?

This comment has been minimized.

Copy link
@SD10

SD10 Apr 14, 2018

Member

I think it's about the likelihood of which view is going to be the first responder. I would rather search horizontally before going vertical.

This comment has been minimized.

Copy link
@guykogus

guykogus Apr 14, 2018

Contributor

But it's still not a breadth-first search, it's a BFS/DFS hybrid. It's a poorly written algorithm. It double-checks all the subviews, which isn't the right way to do it.

This comment has been minimized.

Copy link
@LucianoPAlmeida

LucianoPAlmeida Apr 15, 2018

Author Member

Hey @guykogus, thank you for your feedback, I agree that BFS is a good solution to this case 👍

@SD10

SD10 approved these changes Apr 14, 2018

Copy link
Member

SD10 left a comment

LGTM

/// SwifterSwift: Set some or all corners radiuses of view.
/// SwifterSwift: Recursively find the first responder.
public func firstResponder() -> UIView? {
guard

This comment has been minimized.

Copy link
@guykogus

guykogus Apr 14, 2018

Contributor

But it's still not a breadth-first search, it's a BFS/DFS hybrid. It's a poorly written algorithm. It double-checks all the subviews, which isn't the right way to do it.

@SD10
Copy link
Member

SD10 left a comment

@LucianoPAlmeida Let's wait a bit longer before merging since there is still discussion if you don't mind. I would dig deeper but its 4am and I want to rip out my eyes 😂

/// SwifterSwift: Set some or all corners radiuses of view.
/// SwifterSwift: Recursively find the first responder.
public func firstResponder() -> UIView? {
var views = [UIView](arrayLiteral: self)

This comment has been minimized.

Copy link
@guykogus

guykogus Apr 15, 2018

Contributor

Why not var views = [self]? Seems simpler.

This comment has been minimized.

Copy link
@LucianoPAlmeida

LucianoPAlmeida Apr 15, 2018

Author Member

Just for clarity and readability on the code :))

@@ -224,7 +215,22 @@ public extension UIView {
// MARK: - Methods
public extension UIView {

/// SwifterSwift: Set some or all corners radiuses of view.
/// SwifterSwift: Recursively find the first responder.
public func firstResponder() -> UIView? {

This comment has been minimized.

Copy link
@guykogus

guykogus Apr 15, 2018

Contributor

Why change this from a computed variable to a function? It means that we have a breaking change.

This comment has been minimized.

Copy link
@omaralbeik

omaralbeik Apr 15, 2018

Member

We use functions when Big-O is not constant to give a sense that this operation depends on the size of the object. Example: Swift's uppercased() method in String vs cgColor property in UIColor

This comment has been minimized.

Copy link
@LucianoPAlmeida

This comment has been minimized.

Copy link
@guykogus

guykogus Apr 16, 2018

Contributor

Great idea, I never thought about this. You can also use the Complexity delimiter to the documentation if you ever feel the need.

This comment has been minimized.

Copy link
@omaralbeik

omaralbeik Apr 16, 2018

Member

@guykogus That's a great idea, feel free to open an issue or submit a PR 🙂

@omaralbeik
Copy link
Member

omaralbeik left a comment

LGTM, thank you @LucianoPAlmeida 🙂

@SD10

SD10 approved these changes Apr 19, 2018

@SD10 SD10 merged commit 71161cf into master Apr 19, 2018

5 checks passed

codecov/changes No unexpected coverage changes found.
Details
codecov/patch 100% of diff hit (target 91.64%)
Details
codecov/project 91.69% (+0.05%) compared to 0bb4f03
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@SD10 SD10 deleted the recursive-first-responder branch Apr 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.