hint for no goal when filtering and ptr on empty list #506

krugerk · 2024-10-28T23:37:27Z

uses the common technique of setting the backgroundView of the CollectionView when no items would be displayed

fixes #226
fixes #604

new notice screen:

and now with pull-to-refresh:

and the new hint:

theospears · 2024-11-10T02:53:07Z

BeeSwift/Gallery/GalleryViewController.swift

@@ -431,9 +414,19 @@ class GalleryViewController: UIViewController, UICollectionViewDelegateFlowLayou
    }

    func numberOfSections(in collectionView: UICollectionView) -> Int {
-        return 1
+        switch (filteredGoals.isEmpty, goals.isEmpty) {


numberOfSections seems like it should be a pure function which just returns a number and doesn't have side effects like updating the background. Maybe updateGoals() is a better place for this to live?

I am not sure I understand. With this change the background is set according to the number of sections and is being set in the numberOfSections. So yeah, that method has the side effect of setting the background.
As is, updateGoals() does not seem to be an ideal spot for the change either.
I made a change; take a look

BeeSwift/Gallery/GalleryViewController.swift

krugerk · 2024-12-07T01:07:13Z

BeeSwift/Gallery/GalleryViewController.swift

-        }
-        let searchItem = UIBarButtonItem(barButtonSystemItem: .search, target: self, action: #selector(self.searchButtonPressed))
-        self.navigationItem.leftBarButtonItem = searchItem
+        self.navigationItem.leftBarButtonItem = UIBarButtonItem(barButtonSystemItem: .search, target: self, action: #selector(self.searchButtonPressed))
    }

    func collectionView(_ collectionView: UICollectionView, numberOfItemsInSection section: Int) -> Int {
        return ServiceLocator.versionManager.lastChckedUpdateState() == .UpdateRequired ? 0 : self.filteredGoals.count


include 'update required' as one of the reasons for an empty list (show need upgrade in the background view)

theospears

I agree using BackgroundView on CollectionView is a reasonable way to handle this.

This PR will conflict with #542. Given you opened it first I don't want to put the work on fixing all the merge conflicts on you, so added some feedback about what updates are necessary for this to be merged. If you're happy to follow up on them in the next week I'll hold off on #542 and fix it up to be compatible after this is merged.

theospears · 2024-12-27T04:12:13Z

BeeSwift/Gallery/GalleryViewController.swift

    }

    func collectionView(_ collectionView: UICollectionView, numberOfItemsInSection section: Int) -> Int {
        return ServiceLocator.versionManager.lastChckedUpdateState() == .UpdateRequired ? 0 : self.filteredGoals.count
    }

    func numberOfSections(in collectionView: UICollectionView) -> Int {
-        return 1
+        [filteredGoals.isEmpty, goals.isEmpty].contains(true) ? 0 : 1


Suggested change

[filteredGoals.isEmpty, goals.isEmpty].contains(true) ? 0 : 1

filteredGoals.isEmpty ? 0 : 1

If goals is empty then filtered goals will always be, right?

Yes, if goals is empty, then any subset of it would also be empty, including the one known as filteredGoals.

I see what you mean: we are always using the combination of the filter applied to the goals and thus the set in filteredGoals. When the searchBar.text the filtered is considered off and all goals are in filteredGoals.

Perhaps these names are unclear. Something like allGoals and visibleGoals might have been a better choice?

BeeSwift/Gallery/GalleryViewController.swift

theospears · 2024-12-27T04:16:52Z

BeeSwift/Gallery/GalleryViewController.swift

-    var goals : [Goal] = []
-    var filteredGoals : [Goal] = []
+    var goals : [Goal] = [] {
+        didSet {


[Do] Move this logic (and filteredGoals.didSet) to didUpdateGoals which contains all the other logic/view invalidation which is run when the set of goals update.

theospears · 2024-12-27T04:20:16Z

BeeSwift/Gallery/GalleryViewController.swift

+        var view: UIView? {
+            switch (filteredGoals.isEmpty, goals.isEmpty) {
+            case (true, false):
+                return self.makeViewForEmptyCollection(when: .noGoalsMatchingFilter)


[Do] Remove the enum and the multiple views, just pass in a string here.

I've marked this as a Do, but I'm open to being convinced otherwise. At the moment I don't see the benefit to justify the additional complexity.

Currently collectionView?.backgroundView is being set to a UIView which is just a container, containing a BSLabel with the reason no goals are shown, and, below it, another UIView acting as a spacer.

The intent was to show the text/label "above the fold" for better visibility and to not have it sit in the middle, which would be covered by the MBProgressHUD activity indicator.

Your proposal is just to set the backgroundView to the label directly?

I'm happy either to set it to the view, or the label directly. I just don't think we get much from having the NoGoalReason enum - makeViewForEmptyCollection could just take the string to show.

krugerk · 2024-12-29T22:27:12Z

I agree using BackgroundView on CollectionView is a reasonable way to handle this.

This PR will conflict with #542. Given you opened it first I don't want to put the work on fixing all the merge conflicts on you, so added some feedback about what updates are necessary for this to be merged. If you're happy to follow up on them in the next week I'll hold off on #542 and fix it up to be compatible after this is merged.

Thanks for the consideration. (some amount of) Updating the MR so that it is mergable with its target is to be expected. And I would update it accordingly.

BeeSwift/Gallery/GalleryViewController.swift

krugerk · 2024-12-30T00:59:26Z

BeeSwift/Gallery/GalleryViewController.swift


        cell.goal = goal

        return cell
    }

    func collectionView(_ collectionView: UICollectionView, layout collectionViewLayout: UICollectionViewLayout, referenceSizeForFooterInSection section: Int) -> CGSize {
-        return CGSize(width: 320, height: section == 0 && self.goals.count > 0 ? 5 : 0)
+        return CGSize(width: 320, height: section == 0 && self.allGoals.count > 0 ? 5 : 0)
    }

    func collectionView(_ collectionView: UICollectionView, didSelectItemAt indexPath: IndexPath) {
        let row = (indexPath as NSIndexPath).row


Suggested change

let row = (indexPath as NSIndexPath).row

let row = indexPath.row

krugerk · 2024-12-30T00:59:50Z

BeeSwift/Gallery/GalleryViewController.swift

    }

    func collectionView(_ collectionView: UICollectionView, didSelectItemAt indexPath: IndexPath) {
        let row = (indexPath as NSIndexPath).row
-        if row < self.filteredGoals.count { self.openGoal(self.filteredGoals[row]) }
+        if row < self.visibleGoals.count { self.openGoal(self.visibleGoals[row]) }


Suggested change

if row < self.visibleGoals.count { self.openGoal(self.visibleGoals[row]) }

if row < self.visibleGoals.count {

self.openGoal(self.visibleGoals[row])

}

krugerk · 2024-12-30T11:05:37Z

BeeSwift/Gallery/GalleryViewController.swift

+            matchingGoal = self.allGoals.filter({ (goal) -> Bool in
                return goal.slug == slug
            }).last


Suggested change

matchingGoal = self.allGoals.filter({ (goal) -> Bool in

return goal.slug == slug

}).last

matchingGoal = self.allGoals.last(where: { $0.slug == slug })

unrelated to this PR though

@objc func openGoalFromNotification(_ notification: Notification) { var goalFromIdentifier: Goal? { guard let identifier = notification.userInfo?["identifier"] as? String, let url = URL(string: identifier), let objectID = viewContext.persistentStoreCoordinator?.managedObjectID(forURIRepresentation: url) else { return nil } return viewContext.object(with: objectID) as? Goal } var goalFromSlug: Goal? { guard let slug = notification.userInfo?["slug"] as? String else { return nil } return self.allGoals.last(where: { $0.slug == slug }) } if let matchingGoal = goalFromIdentifier ?? goalFromSlug { self.navigationController?.popToRootViewController(animated: false) self.openGoal(matchingGoal) } }

krugerk · 2024-12-30T11:07:59Z

BeeSwift/Gallery/GalleryViewController.swift

+        case (true, false):
+            return self.makeViewForEmptyCollection(when: .noGoalsMatchingFilter)
+        case (true, true):
+            return self.makeViewForEmptyCollection(when: .noActiveGoals)
+        default:
+            return nil


Suggested change

case (true, false):

return self.makeViewForEmptyCollection(when: .noGoalsMatchingFilter)

case (true, true):

return self.makeViewForEmptyCollection(when: .noActiveGoals)

default:

return nil

case (true, false):

self.makeViewForEmptyCollection(when: .noGoalsMatchingFilter)

case (true, true):

self.makeViewForEmptyCollection(when: .noActiveGoals)

default:

nil

krugerk · 2024-12-30T11:09:14Z

BeeSwift/Gallery/GalleryViewController.swift

@@ -429,20 +416,20 @@ class GalleryViewController: UIViewController, UICollectionViewDelegateFlowLayou
    func collectionView(_ collectionView: UICollectionView, cellForItemAt indexPath: IndexPath) -> UICollectionViewCell {
        let cell:GoalCollectionViewCell = self.collectionView!.dequeueReusableCell(withReuseIdentifier: self.cellReuseIdentifier, for: indexPath) as! GoalCollectionViewCell

-        let goal:Goal = self.filteredGoals[(indexPath as NSIndexPath).row]
+        let goal:Goal = self.visibleGoals[(indexPath as NSIndexPath).row]


Suggested change

let goal:Goal = self.visibleGoals[(indexPath as NSIndexPath).row]

let goal:Goal = self.visibleGoals[indexPath.row]

krugerk · 2024-12-30T11:09:53Z

BeeSwift/Gallery/GalleryViewController.swift

@@ -429,20 +416,20 @@ class GalleryViewController: UIViewController, UICollectionViewDelegateFlowLayou
    func collectionView(_ collectionView: UICollectionView, cellForItemAt indexPath: IndexPath) -> UICollectionViewCell {
        let cell:GoalCollectionViewCell = self.collectionView!.dequeueReusableCell(withReuseIdentifier: self.cellReuseIdentifier, for: indexPath) as! GoalCollectionViewCell


why self.collectionView with ! and not merely the collectionView passed into this method?

BeeSwift/Gallery/GalleryViewController.swift

krugerk · 2024-12-30T11:21:33Z

BeeSwift/Gallery/GalleryViewController.swift

+    var allGoals : [Goal] = []
+    var visibleGoals : [Goal] = []


Suggested change

var allGoals : [Goal] = []

var visibleGoals : [Goal] = []

var allGoals = [Goal]()

var visibleGoals = [Goal]()

krugerk · 2024-12-30T11:21:49Z

BeeSwift/Gallery/GalleryViewController.swift

    var deadbeatView = UIView()
    var outofdateView = UIView()


Suggested change

var deadbeatView = UIView()

var outofdateView = UIView()

let deadbeatView = UIView()

let outOfDateView = UIView()

uses the common technique of setting the backgroundView of the CollectionView when no items would be displayed fixes beeminder#226

now it is in a small container and the visible text can be placed elsewhere easily

... or the search could placed in viewDidLoad with the rest of the setup

krugerk force-pushed the bugfix/226-no-pull-to-refresh-on-empty-gallery branch from 3da3a0d to 6781c47 Compare October 29, 2024 09:19

theospears self-assigned this Nov 6, 2024

theospears reviewed Nov 10, 2024

View reviewed changes

theospears assigned krugerk and unassigned theospears Nov 10, 2024

krugerk force-pushed the bugfix/226-no-pull-to-refresh-on-empty-gallery branch 2 times, most recently from ddf0553 to 5a582e2 Compare November 15, 2024 22:38

dreeves mentioned this pull request Nov 22, 2024

No pull-to-refresh on an empty gallery #226

Open

krugerk force-pushed the bugfix/226-no-pull-to-refresh-on-empty-gallery branch from 5a582e2 to d99fc1d Compare November 27, 2024 10:04

krugerk mentioned this pull request Dec 6, 2024

Switch the gallery view to use a StackView instead of manual layout #542

Merged

krugerk commented Dec 7, 2024

View reviewed changes

theospears requested changes Dec 27, 2024

View reviewed changes

krugerk force-pushed the bugfix/226-no-pull-to-refresh-on-empty-gallery branch from bfee8d4 to 04a0479 Compare December 30, 2024 00:57

krugerk requested a review from a team as a code owner December 30, 2024 00:57

krugerk requested review from theospears and removed request for a team December 30, 2024 00:57

krugerk commented Dec 30, 2024

View reviewed changes

BeeSwift/Gallery/GalleryViewController.swift Show resolved Hide resolved

krugerk commented Dec 30, 2024

View reviewed changes

krugerk force-pushed the bugfix/226-no-pull-to-refresh-on-empty-gallery branch from 9aabc04 to 3f5dad5 Compare December 30, 2024 11:04

krugerk commented Dec 30, 2024

View reviewed changes

BeeSwift/Gallery/GalleryViewController.swift Outdated Show resolved Hide resolved

krugerk commented Dec 30, 2024

View reviewed changes

krugerk added 2 commits January 12, 2025 16:04

hint for no goal when filtering and ptr on empty list

11cab4a

uses the common technique of setting the backgroundView of the CollectionView when no items would be displayed fixes beeminder#226

label was placed in the center and overlapped by hud

f212dec

now it is in a small container and the visible text can be placed elsewhere easily

krugerk added 10 commits January 12, 2025 16:13

clean code

0448b87

clean code

e9ed43b

allGoals and visibleGoals

75cae8a

didUpdateGoals

774a301

no search needed when no goals

8194bcb

... or the search could placed in viewDidLoad with the rest of the setup

remix, refactor around no goals reasons

c9a10ce

last(where:) over filter(where:).last

dfb5ad4

clean code

d9528fb

clean code

08d6a74

Notification as NSNotification 🤷

c86c79e

krugerk force-pushed the bugfix/226-no-pull-to-refresh-on-empty-gallery branch from af1943c to c86c79e Compare January 12, 2025 15:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

hint for no goal when filtering and ptr on empty list #506

hint for no goal when filtering and ptr on empty list #506

krugerk commented Oct 28, 2024 •

edited

Loading

theospears Nov 10, 2024

krugerk Nov 12, 2024

krugerk Dec 7, 2024

theospears left a comment

theospears Dec 27, 2024

krugerk Dec 29, 2024

theospears Dec 29, 2024

theospears Dec 27, 2024

theospears Dec 27, 2024

krugerk Dec 30, 2024

theospears Dec 30, 2024

krugerk commented Dec 29, 2024

krugerk Dec 30, 2024

krugerk Dec 30, 2024

krugerk Dec 30, 2024

krugerk Dec 30, 2024

krugerk Dec 30, 2024

krugerk Dec 30, 2024

krugerk Dec 30, 2024

krugerk Dec 30, 2024

krugerk Dec 30, 2024

	[filteredGoals.isEmpty, goals.isEmpty].contains(true) ? 0 : 1
	filteredGoals.isEmpty ? 0 : 1

	let row = (indexPath as NSIndexPath).row
	let row = indexPath.row

	let goal:Goal = self.visibleGoals[(indexPath as NSIndexPath).row]
	let goal:Goal = self.visibleGoals[indexPath.row]

		@@ -429,20 +416,20 @@ class GalleryViewController: UIViewController, UICollectionViewDelegateFlowLayou
		func collectionView(_ collectionView: UICollectionView, cellForItemAt indexPath: IndexPath) -> UICollectionViewCell {
		let cell:GoalCollectionViewCell = self.collectionView!.dequeueReusableCell(withReuseIdentifier: self.cellReuseIdentifier, for: indexPath) as! GoalCollectionViewCell

hint for no goal when filtering and ptr on empty list #506

Are you sure you want to change the base?

hint for no goal when filtering and ptr on empty list #506

Conversation

krugerk commented Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theospears left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krugerk commented Dec 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krugerk commented Oct 28, 2024 •

edited

Loading