Query for label on tap in RepoIssuesVC

[BrianLitwin]

Connected to #2263

Summary:

In RepositoryIssuesViewController, when a user taps a label, the label is appended to the searchBar text and the query is fetched.

Test Notes:

Manually:

• tapping on a label adds the appropriate query to the search bar and fetches the appropriate result.
• tapping on a label that is already contained in the query doesn't append duplicate labels
• tapping a point beside the last label in the LabelListView is passed through and an IssuesVC is pushed. Ie, the LabelListView doesn't eat touches inside it's frame where there are no label cells.

How:

There's a lot of chaining delegates involved. Reviewers: it will help to look at this view hierarchy:

• RepositoryIssuesViewController ( BaseListViewController )
  • SearchBarSectionController ( ListSwiftSectionController )
    • SearchBarCell ( UICollectionViewCell )
  • RepositorySummarySectionController ( ListSwiftSectionController )
    • RepositorySummaryCell ( SelectableCell )
      • LabelListView ( UICollectionView )

The label is inside the LabelListView. It tells RepositorySummaryCell, which tells RepositorySummarySectionController, which tells RepositoryIssuesViewController that it has been tapped.
Then:
The RepoIssueViewController then tells SearchBarSectionController to tell SearchBarCell to set the searchBar text appropriately. Then the fetch proceeds as it currently would, as though the user entered the text in the searchBar. My hope is that this preserves a "one-way data flow".

[BrianLitwin]

Manual Test Screen Vid

It's kinda hard to see but this tests the items in Test Notes above:

[BrianLitwin]

@jdisho Thanks for your review! I'm not 100% sure why this is, but I don't see the pattern of conforming to protocols in extensions used elsewhere in the app. I'm just trying to stay consistent with the rest of the codebase; I don't have any strong opinion on the issue. See IssuesViewController for an example of conforming to a lot of delegates inside the class.

[rnystrom]

+1 on not using extension unless necessary. I hit compiler errors early on in GitHawk's development from over-using them and lost a ton of time debugging + refactoring.

[rnystrom]

This is a bit of a backwards pattern. Delegates should be communicating "back up" instead of sending info down.

Instead, can we just get the search VC from the adapter and set its text?

func didTap(label: String) {
  // probably move "header" to an actual property in the VC
  guard let controller = feed.swiftAdapter.sectionController(for: "header") as SearchBarSectionController
    else { return }
  controller.append(query: " label:\(label)")
}

Much more straightforward

[rnystrom]

Is this not triggered when setting the text in the bar?

[BrianLitwin]

Yeah the delegate method is not triggered here on it's own. The text in the search bar changes but no fetch/update happens.

[rnystrom]

I think we should follow the same pattern of fetching the SC explicitly and instead of backwards-wiring the delegate to the cell, just find the cell and set the text in the SC:

func append(query: String) {
  guard let cell = collectionContext?.cellForItemAt(index: 0) as? SearchBarCell { else return }
  cell.searchBar.text = query
}

[rnystrom]

Eep, mucking w/ hitTest makes me so nervous. Why is this necessary?

Also simply checking for x > maxX looks too limited, should there be some point-in-rect math?

[BrianLitwin]

I highlighted the LabelListView frame in orange. The UicollectionView's frame is set to its bounds.

Without the HitTest override, if you click inside the orange border on an area that's not a LabelCell, nothing happens. In the HitTest override, I'm trying to process taps that land on a label cell, and pass through taps that touchdown right of the right-most label cell.

Perhaps setting the CollectionView's frame is more appropriate, so that it's only as wide as it needs to be given the number of labels that it presents?

[BrianLitwin]

This seems to send the taps to the right places:

// this is copied/pasted from existing methods to find width 
    static func labelTextTotalWidth(labels: [RepositoryLabel]) -> CGFloat {
        let interitemSpacing = labels.count > 1 ? CGFloat(labels.count - 1) * Styles.Sizes.labelSpacing : 0.0
        return labels.reduce(0, { $0 + LabelListCell.size($1.name).width }) + interitemSpacing
    }


override func layoutSubviews() {
        super.layoutSubviews()
        collectionView.frame = CGRect(
            x: bounds.minX,
            y: bounds.minY,
            width: min(LabelListView.labelTextTotalWidth(labels: labels), bounds.width)
            height: bounds.height
        )
}

[BrianLitwin]

Okay with this name?

[BrianLitwin]

Reason for keeping a reference to SearchBarCell and using it here:

let cell = collectionContext?.cellForItemAt(index: 0) as? SearchBarCell 

Above code was returning nil if the searchBar was scrolled out of view. Eg scroll down a little, tap a label, and the collectionContext.cellForItem method would return nil.

[BrianLitwin]

Wanted to add this screenshot with the updated method used to calculate the CollectionView's frame. See this comment above. Here, the CollectionView's border is Orange. Seems like it's working correctly: