Implement #86 include call history in dialpad#332
Conversation
|
Android development and Kotlin are not my forte, so I'm sure there's much to be desired with my solution. I think ideally there would be an entirely new adapter that could, for example, have a horizontal line between contacts and history to better differentiate them. Other ideas I have for improvement:
|
|
Thanks for your contribution! I presume you are still working on this PR since it's marked as draft? If not:
|
|
I submitted it as a draft in anticipation of these changes being necessary. |
I'm sure you can do it, don't give up so soon ;) The |
|
I was able to get a stripped down adapter almost working. The header and recent call items work but I can't seem to set the contact items to populate data. I think it's because the contact binding works differently. Can you take a look at what I'm missing? |
|
I was able to fix all the issues I found except one: the fast scroller no longer works. It was getting an index out of bounds error and when I added a length check that fixed the error but caused it to no longer display. |
|
Hey, I'll check this soon. What's up with the commit history? :) |
Rebase + additional changes. I'm not a git expert; is there a better way to reduce commit clutter? |
|
The following are your new commits, am I right?
You might be able to fix it by cherry-picking but I'll do it (if you can't). |
|
You can also squash commits into one. It's easy in GUI clients - just select commits, right-click on them and select Squash. Then you only have to force push and history gets cleaner. |
|
Yeah, that also works. |
Looks right.
Git desktop won't let me squash anything without complaining about merge commits. |
naveensingh
force-pushed
the
implement-history-in-dialpad
branch
from
5871ca1 to
df90376
Compare
|
Fixed the commits. I have the original history backed up locally just in case. Will review the code later. |
naveensingh
left a comment
naveensingh
left a comment
There was a problem hiding this comment.
While this works, there is a lot of duplication now. Any change in original adapters will have to be copy-pasted to the new adapter and vice versa. It'll be hard to maintain.
At the moment, I don't have any ideas on how to handle this cleanly. Maybe we can use two RecyclerViews with static text views as headers? That way, we would be able to reuse the existing adapters for contacts and recents, but it's unclear how action mode will work with two adapters in play.
| } | ||
| } | ||
|
|
||
| allCallItems = newItems |
There was a problem hiding this comment.
Since getContacts and getRecents are async, allCallItems = newItems will execute before the code finishes fetching the items. The current code will work, but it relies on an implicit behavior.
Also, the callback is never being called.
|
|
||
| val privateContacts = MyContactsContentProvider.getContacts(this, privateCursor) | ||
| if (privateContacts.isNotEmpty()) { | ||
| allCallItems.addAll(privateContacts.map { DialpadItem(it) }) | ||
| } |
There was a problem hiding this comment.
Not sure what's happening here.
If the goal is to add private contacts to allCallItems, this should be handled by getContacts, not getRecents.
allCallItems should only be updated in one place. Otherwise, subtle bugs and crashes are possible.
| class DialpadItem { | ||
| val id: Int | ||
| val header: String? | ||
| val isHeaderForContacts: Boolean | ||
| val contact: Contact? | ||
| val recentCall: RecentCall? | ||
| val itemType: DialpadItemType | ||
|
|
||
| constructor(header: String, isHeaderForContacts: Boolean) { | ||
| this.id = increment() | ||
| this.header = header | ||
| this.isHeaderForContacts = isHeaderForContacts | ||
| this.contact = null | ||
| this.recentCall = null | ||
| this.itemType = DialpadItemType.HEADER | ||
| } | ||
|
|
||
| constructor(contact: Contact) { | ||
| this.id = increment() | ||
| this.header = null | ||
| isHeaderForContacts = false | ||
| this.contact = contact | ||
| this.recentCall = null | ||
| this.itemType = DialpadItemType.CONTACT | ||
| } | ||
|
|
||
| constructor(recentCall: RecentCall) { | ||
| this.id = increment() | ||
| this.header = null | ||
| isHeaderForContacts = false | ||
| this.contact = null | ||
| this.recentCall = recentCall | ||
| this.itemType = DialpadItemType.RECENTCALL | ||
| } | ||
|
|
||
| fun isHeader(): Boolean = header != null | ||
|
|
||
| fun isContact(): Boolean = contact != null | ||
|
|
||
| fun isRecentCall(): Boolean = recentCall != null | ||
|
|
||
| fun getItemId(): Int = this.id | ||
|
|
||
| enum class DialpadItemType { | ||
| HEADER, | ||
| CONTACT, | ||
| RECENTCALL | ||
| } | ||
|
|
||
| companion object { | ||
| private var idCount = 0 | ||
|
|
||
| fun increment(): Int { | ||
| return ++idCount | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
How about a sealed class instead? Example:
- https://github.com/FossifyOrg/Messages/blob/master/app/src/main/kotlin/org/fossify/messages/models/ThreadItems.kt
- https://github.com/FossifyOrg/Messages/blob/master/app/src/main/kotlin/org/fossify/messages/models/Message.kt (Message is a ThreadItem)
fun increment(): Int
It's better to use a stable ID than a counter. hashCode() has been used throughout the project for this.
Note: RecentCall already has a getItemId() function.
| newItems.add(DialpadItem("Contacts", true)) | ||
| newItems.addAll(contacts.map { DialpadItem(it) }) | ||
| } | ||
|
|
||
| getRecents(contacts, loadAllRecents) { recentCalls -> | ||
| if (recentCalls.isNotEmpty()) { | ||
| newItems.add(DialpadItem("Call History", false)) |
There was a problem hiding this comment.
Hardcoded strings here. There's already string resources available for both of these.
naveensingh
added
the
waiting for author
|
This pull request was automatically closed due to inactivity and/or lack of response from the author. If you have further updates or additional information, please comment or reopen the PR to continue. |
fossifybot
Bot
removed
the
waiting for author
3 participants
What is it?
Description of the changes in your PR
Before/After Screenshots/Screen Record
Fixes the following issue(s)