Fix account drawer #9073
Fix account drawer #9073
Conversation
|
Thanks @asoucar! This definitely makes the crash go away, so we can confirm that is the source of the issue. This will give the folders unique keys like It would be helpful to understand how a folder becomes a placeholder folder to make that determination. I would think normally each folder has a real id, even if it is the [Gmail] folder. Let's also file follow up issues to
|
|
@kewisch Thanks for the feedback! To answer your questions, the top level folders like [GMAIL] don't have an accountid the way other folders do. I agree matching the behavior to Desktop in the future makes sense. |
asoucar
requested review from
kewisch,
wmontwe and
rafaeltonholo
as code owners
|
Do you have more details on why the [Gmail] folders don't have an account id? |
| @@ -87,10 +88,11 @@ internal class GetDisplayTreeFolder : UseCase.GetDisplayTreeFolder { | |||
| } | |||
|
|
|||
| private fun createPlaceholderFolder(name: String): DisplayAccountFolder { | |||
| placeholderCounter += 1 | |||
| return DisplayAccountFolder( | |||
| accountId = "accountId", | |||
There was a problem hiding this comment.
Let's change this to placeholder if we can't include the real account id.
There was a problem hiding this comment.
To avoid over-engineering the issue, I suggest using kotlin.uuid.Uuid.random().toString() as a placeholder instead of a fixed format like placeholder_{num}. I would preferable this unless there is a specific reason to use DisplayAccountFolder().id, particularly since we are dealing with a placeholder folder. However, I'm also fine with the suggested solution
Over-engineering
I would consider having a better class definition for DisplayFolder, by using sealed classes. With that, could have something like:
// feature/navigation/drawer/dropdown/domain/entity/DisplayFolder.kt
internal sealed interface DisplayFolder {
val id: String
val unreadMessageCount: Int
val starredMessageCount: Int
val isInTopGroup: Boolean get() = false
val folder: Folder? get() = null
}
// feature/navigation/drawer/dropdown/domain/entity/DisplayAccountFolder.kt
internal data class DisplayAccountFolder(
val accountId: String,
override val folder: Folder,
override val isInTopGroup: Boolean,
override val unreadMessageCount: Int,
override val starredMessageCount: Int,
) : DisplayFolder {
override val id: String = createDisplayAccountFolderId(accountId, folder.id)
}
// feature/navigation/drawer/dropdown/domain/entity/DisplayUnifiedFolder.kt
internal data class DisplayUnifiedFolder(
override val id: String,
val unifiedType: DisplayUnifiedFolderType,
override val unreadMessageCount: Int,
override val starredMessageCount: Int,
) : DisplayFolder
// feature/navigation/drawer/dropdown/domain/entity/DisplayPlaceholderFolder.kt
internal data class DisplayPlaceholderFolder(
val name: String,
) : DisplayFolder {
override val id: String = kotlin.uuid.Uuid.random().toString()
override val folder: Folder = Folder(
id = 0L,
name = name,
type = FolderType.REGULAR,
isLocalOnly = false,
)
override val isInTopGroup: Boolean = true
override val unreadMessageCount: Int = 0
override val starredMessageCount: Int = 0
}With that, the creation of the placeholder could be easily identified and could help us to prevent issues like the presented here.
There was a problem hiding this comment.
@kewisch @rafaeltonholo So the reason I switched from the placeholder_randomuuid format back to accountId_counter was due to issues during updating the unit tests. Matching the random uuid made less sense than having an incrementing placeholder. I messed around with the testing some more and I think I found the correct balance for the short term. I used placeholder for the placeholder folders but kept the counter since it both fixes the issue and is easier to test. This way we can mark the placeholder folders clearly still.
There was a problem hiding this comment.
@kewisch to answer your question on the [GMAIL] folder not having an id, see these pictures:
This normal inbox shows the currentFolders, including an id. By contrast, below the [GMAIL] path has no current folders and so no id to draw from
There was a problem hiding this comment.
It appears to me that [Gmail] isn't a real folder either, but it does contribute to the structure because it shows up in the path. Hence it will not have an id and needs a placeholder. I don't have a preference on uuid or number, I just insist on using placeholder so that when we have an issue around this in the future we'll know better what to look for. Thanks for taking care!
asoucar
force-pushed
the
fix-account-drawer-id
branch
2 times, most recently
from
4df4a00 to
ca61649
Compare
asoucar
force-pushed
the
fix-account-drawer-id
branch
from
ca61649 to
eccbd21
Compare
Prevent duplicate account id for placeholder folder Fixes: 9056 Bring back GH actions read permission to fix daily builds workflow Add instructions to propose ADRs Refactor: move :legacy:common.feature to app-common.feature Refactor: Removed :legacy:preferences from settings.gradle as this module is no longer used & empty Add link to security hashes in README Bump actions/setup-java from 4.7.0 to 4.7.1 Bumps [actions/setup-java](https://github.com/actions/setup-java) from 4.7.0 to 4.7.1. - [Release notes](https://github.com/actions/setup-java/releases) - [Commits](actions/setup-java@3a4f6e1...c5195ef) --- updated-dependencies: - dependency-name: actions/setup-java dependency-version: 4.7.1 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Bump github/codeql-action from 3.28.14 to 3.28.15 Bumps [github/codeql-action](https://github.com/github/codeql-action) from 3.28.14 to 3.28.15. - [Release notes](https://github.com/github/codeql-action/releases) - [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md) - [Commits](github/codeql-action@fc7e4a0...45775bd) --- updated-dependencies: - dependency-name: github/codeql-action dependency-version: 3.28.15 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Bump softprops/action-gh-release from 2.2.1 to 2.2.2 Bumps [softprops/action-gh-release](https://github.com/softprops/action-gh-release) from 2.2.1 to 2.2.2. - [Release notes](https://github.com/softprops/action-gh-release/releases) - [Changelog](https://github.com/softprops/action-gh-release/blob/master/CHANGELOG.md) - [Commits](softprops/action-gh-release@c95fe14...da05d55) --- updated-dependencies: - dependency-name: softprops/action-gh-release dependency-version: 2.2.2 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Middle ground, placeholder and counter Fixes: thunderbird#9056 (+1 squashed commit) Squashed commits: [ade7fcb] Adjust fix for testing Fixes: thunderbird#9056 Formatting
asoucar
force-pushed
the
fix-account-drawer-id
branch
from
c7d578f to
34815b5
Compare
kewisch
closed this pull request by merging all changes
into
thunderbird:main
in
8ad362e
Fix account drawer
Prevent duplicate account id for placeholder folder
Fixes: #9056