Skip to content

Conversation

Terrance
Copy link
Contributor

The existing behaviour toggles all accounts from a single provider when one of them is selected in the filter dialog.

This change means selections are managed by account name rather than type, in order to allow filtering on individual accounts.

For context, I use DAVx5 with a set of three address books, one current and two for old/archived contacts -- day-to-day I'm only interested in the current contacts.

The existing behaviour toggles all accounts from a single provider when
one of them is selected in the filter dialog.

This change means selections are managed by account name rather than
type, in order to allow filtering on individual accounts.
Copy link
Member

@Bnyro Bnyro left a comment

Choose a reason for hiding this comment

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

Reasonable, thanks!

Copy link
Member

@Bnyro Bnyro left a comment

Choose a reason for hiding this comment

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

After a quick thought, I think it might be better to use both, the account name and account type for filtering, so that there won't be issues when two accounts from different providers are named the same.

@Terrance Terrance requested a review from Bnyro August 28, 2023 18:26
@Terrance
Copy link
Contributor Author

I've rebased this locally against main, though it looks like filters aren't currently working there (with or without my changes) -- filterOptions.hiddenAccountNames is an empty list when evaluating whether to hide each contact?

@Bnyro
Copy link
Member

Bnyro commented Aug 28, 2023

Yes, what we should do is to add the following to ContactData

data class ContactData(...) {
val accountIdentifier = "$accountType|$accountName"
}

and use that instead when filtering the contacts or saving them to hiddenAccountNames (probably should be renamed to hiddenAccountIdentifiers).

@Bnyro
Copy link
Member

Bnyro commented Sep 7, 2023

#257

@Bnyro Bnyro closed this Sep 7, 2023
@Terrance
Copy link
Contributor Author

Terrance commented Sep 7, 2023

I had to open a new PR because #238 blocked write access.

Apologies, I don't seem to have the option here to allow write access for maintainers (maybe down to my compartmentalisation of forks into a GitHub organisation of my own).

I had added the refactor you suggested locally, but that didn't help with the group filters not working in the first place, and I hadn't yet had chance to dig into what changed on the main branch.

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