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.

Sign up for GitHub

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

Jump to bottom

Add badge in spaces button (PSG-944) #7088

Merged
merged 8 commits into from
Nov 30, 2022
Merged

Add badge in spaces button (PSG-944) #7088

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a83d54d
Add badge in spaces button
alfogrillo Nov 18, 2022
d1e434b
Address PR comments
alfogrillo Nov 18, 2022
4d3ca8e
Add changelog.d file
alfogrillo Nov 18, 2022
f1ce202
Remove duplicate code
alfogrillo Nov 18, 2022
f6c57e8
Optimise badge computation
alfogrillo Nov 18, 2022
4f1bf69
Merge branch 'develop' into alfogrillo/badges_spaces
alfogrillo Nov 18, 2022
9e2e2d0
Merge branch 'develop' into alfogrillo/badges_spaces
alfogrillo Nov 25, 2022
9be5f59
Refactor badge logic
alfogrillo Nov 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 52 additions & 7 deletions Riot/Modules/Home/AllChats/AllChatsViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,14 @@ class AllChatsViewController: HomeViewController {
private var isOnboardingCoordinatorPreparing: Bool = false

private var allChatsOnboardingCoordinatorBridgePresenter: AllChatsOnboardingCoordinatorBridgePresenter?

private var session: MXSession? {
UserSessionsService.shared.mainUserSession?.matrixSession
}
alfogrillo marked this conversation as resolved.
Show resolved Hide resolved

private var theme: Theme {
ThemeService.shared().theme
}

@IBOutlet private var toolbar: UIToolbar!
private var isToolbarHidden: Bool = false {
Expand Down Expand Up @@ -137,12 +145,13 @@ class AllChatsViewController: HomeViewController {
searchController.delegate = self

NotificationCenter.default.addObserver(self, selector: #selector(self.setupEditOptions), name: AllChatsLayoutSettingsManager.didUpdateSettings, object: nil)
NotificationCenter.default.addObserver(self, selector: #selector(self.updateBadgeButton), name: MXSpaceNotificationCounter.didUpdateNotificationCount, object: nil)
}

override func viewWillAppear(_ animated: Bool) {
super.viewWillAppear(animated)

self.toolbar.tintColor = ThemeService.shared().theme.colors.accent
self.toolbar.tintColor = theme.colors.accent
if self.navigationItem.searchController == nil {
self.navigationItem.searchController = searchController
}
Expand Down Expand Up @@ -460,10 +469,10 @@ class AllChatsViewController: HomeViewController {
return
}

self.update(with: ThemeService.shared().theme)
self.update()
}

private func update(with theme: Theme) {
private func update() {
alfogrillo marked this conversation as resolved.
Show resolved Hide resolved
self.navigationController?.toolbar?.tintColor = theme.colors.accent
}

Expand Down Expand Up @@ -500,22 +509,40 @@ class AllChatsViewController: HomeViewController {
self?.updateToolbar(with: menu)
}))
updateEmptyView()
updateBadgeButton()
}

private func updateRightNavigationItem(with menu: UIMenu) {
self.navigationItem.rightBarButtonItem = UIBarButtonItem(image: UIImage(systemName: "ellipsis.circle"), menu: menu)
}

private lazy var spacesButton: BadgedBarButtonItem = {
let innerButton = UIButton(type: .system)
innerButton.accessibilityLabel = VectorL10n.spaceSelectorTitle
innerButton.addTarget(self, action: #selector(self.showSpaceSelectorAction(sender:)), for: .touchUpInside)
innerButton.setImage(Asset.Images.allChatsSpacesIcon.image, for: .normal)
return BadgedBarButtonItem(withBaseButton: innerButton, theme: theme)
}()

@objc private func updateBadgeButton() {
guard isViewLoaded else {
return
}
spacesButton.badgeText = session.map {
"\($0.spaceService.rootSpacesNotificationCount)"
}
spacesButton.badgeBackgroundColor = session.map {
$0.spaceService.rootSpacesHaveHighlightNotification ? theme.noticeColor : theme.noticeSecondaryColor
} ?? .clear
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this implementation be shared with the old one on the MasterTabBarController?

Seems like using mainSession.spaceService.notificationCounter.notificationState(forAllSpacesExcept:) might be helpful rather than the private extension on the space service.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmmh, the logic looks different.
Here we show the count of all unread messages including the ones in current space.
Invites to spaces are ignored unfortunately! 😅
@Johennes do you think it makes sense to cover also invites under this PR (changing the ACs)?
Not having them covered looks like a UX bug to me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we show the count of all unread messages including the ones in current space.

My bad I hadn't noticed that 🙈

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thought looking at f1ce202. Moving the notificationState(forAllSpacesExcept: nil) up to this method would mean you could read allCount and allHighlightCount without having to iterate through the spaces twice.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Including space invites in the highlight count seems sensible to me as I assume you otherwise might miss them just like normal messages in another space than the one you're in?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh interesting. Maybe it still makes sense to sum that up? It's still an unread message of sorts so I suppose it should count as a 1.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or alternatively we could only use it to drive the color of the spaces icon badge?

Copy link
Contributor Author

@alfogrillo alfogrillo Nov 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we go this way probably we would need to implement both.
If you rely just on the color you could find yourself in a situation where you don't have unread messages but just invites and the badge wouldn't show. :-/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side note to do with as you wish: I was referring to invites as "an invite to a DM which only show up in the All Chats screen".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you rely just on the color you could find yourself in a situation where you don't have unread messages but just invites and the badge wouldn't show. :-/

Could we just use the red exclamation mark on the spaces icon in that case?

Side note to do with as you wish: I was referring to invites as "an invite to a DM which only show up in the All Chats screen".

Oh, I see! That's this case then:

simulator_screenshot_F08BEE4B-2C68-47EB-A56D-8695A6C6A65B

Well, I suppose we should be covering both. This one seems more straightforward though because the invite can simply count as a red badge of 1 in the all spaces space?

}

private func updateToolbar(with menu: UIMenu) {
guard isViewLoaded else {
return
}

self.isToolbarHidden = false
self.update(with: ThemeService.shared().theme)

let spacesButton = UIBarButtonItem(image: Asset.Images.allChatsSpacesIcon.image, style: .done, target: self, action: #selector(self.showSpaceSelectorAction(sender: )))
spacesButton.accessibilityLabel = VectorL10n.spaceSelectorTitle
self.update()

self.toolbar.items = [
spacesButton,
Expand Down Expand Up @@ -1076,3 +1103,21 @@ extension AllChatsViewController: SplitViewMasterViewControllerProtocol {
self.refreshCurrentSelectedCell(false)
}
}

private extension MXSpaceService {
var rootSpacesNotificationCount: UInt {
rootSpaces.reduce(0) { partialResult, space in
let count = notificationCounter.notificationState(forSpaceWithId: space.spaceId)?.allCount ?? 0
return partialResult + count
}
}

var rootSpacesHaveHighlightNotification: Bool {
rootSpaces.contains { space in
guard let notificationState = notificationCounter.notificationState(forSpaceWithId: space.spaceId) else {
return false
}
return notificationState.allHighlightCount > 0
}
}
}