Sort the address book by name and mail address

hpi-swa-teaching / IMAPClient

This project provides a library and tool set to access email through IMAP in the Squeak/Smalltalk environment. (SWT22-13)

MIT License

22 stars 3 forks source link

Sort the address book by name and mail address #403

Closed laugengebaeck closed 3 years ago

laugengebaeck commented 3 years ago

This closes #402.

UI of the ICAdressBookDialog now looks like this:

addressbook_sort

coveralls commented 3 years ago

Pull Request Test Coverage Report for Build 924628831

0 of 0 changed or added relevant lines in 0 files are covered.
10 unchanged lines in 2 files lost coverage.
Overall coverage decreased (-0.2%) to 75.453%

Files with Coverage Reduction	New Missed Lines	%
packages/IMAPClient-Core.package/ICEndPoint.class/instance/testAccountWith..st	1	0%
packages/IMAPClient-Core.package/ICEmail.class/instance/retrieveBody.st	9	0%
<!--	Total:	10	-->

Totals
Change from base Build 893267833:	-0.2%
Covered Lines:	1042
Relevant Lines:	1381

💛 - Coveralls

laugengebaeck commented 3 years ago

We should maybe also invalidate the sorting when a new contact is added by the user, because otherwise this new contact might not be sorted into the right order.

Edit: This is done now.

laugengebaeck commented 3 years ago

Another thing we should think about before merging this pull request (we talked about that last week): Should we change the address book to use ICContacts internally? The current sorting method is quite ugly (and hard to understand) because it works by splitting strings.

elenagensch commented 3 years ago

Yeah, this is indeed ugly. We decided to pass Contacts as Strings in order to use addAllFirstUnlessAlreadyPresent: (https://github.com/hpi-swa-teaching/IMAPClient/blob/develop/packages/IMAPClient-Core.package/ICAddressBook.class/instance/addEmail..st#L4). When they are passed as ICContacts every time you open the address book you attach all contacts to the existing address book (because their id is unique). But that decision was made before sorting, so dissolving that makes sense.

sm1lla commented 3 years ago

If that makes it simpler we should definitly change it. But then this should be done this week, as we are planning to merge on Monday at the lastest.

laugengebaeck commented 3 years ago

The tests requested by @sm1lla should be added now.

I decided not to change the ICAddressBook to use ICContacts since the PluggableTreeSpec we use for displaying can only display the collection it is watching directly (without asString). We could use a PluggableMultiColumnListSpec or something like that (which would resolve that issue), but this seems to require a lot of complicated changes.

laugengebaeck commented 3 years ago

Can I merge this? (I would list Christian as the first author)