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.

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

Bugfix: Chats not loading on tablet layout #280

Merged
merged 15 commits into from
Jan 10, 2025

Conversation

FelberMartin
Copy link
Collaborator

Problem Description

This PR addresses multiple bugs / issues:

  1. Chats not loading on tablet? #274
  2. Back Navigation in ThreadUI not working #267
  3. When loading a chat that has not been opened before, the ChatList shortly displayed "No posts found" before showing the loaded posts.

Changes

  • To fix 1: Removed the viewModelStoreOwner introduced by Tim Ortel in Feature: Reload conversation message on websocket reconnect #30. I talked to him, but he could not remember why exactly this approach with the store was introduced in the first place. I could not find any bugs related to my removal of this code, I tested both smartphone and tablet layouts, navigation between chats and threads, disconnecting from the internet, etc.
  • To fix 2: Update showThread properly
  • To fix 3: Check for loadStates.isIdle
  • Misc: Reset scroll position when entering new chat (needed when switching chats in tablet layout)

Steps for testing

On both tablet and smartphone test:

  • Go to a chat
  • Go to another chat (on tablet via the ConversationOverview on the left) -> chat is loaded properly
  • Open a thread
  • Close a thread both via the Android back button and the arrow icon in the TopBar -> both work
  • On tablet: Open a thread and select a different chat on the left -> new chat is shown in the right column

Please also thoroughly test the changes to fix bug 1., and test any scenario that could be affected by removing the viewModelStoreOwner.

@FelberMartin FelberMartin added the ready for review This PR can be reviewed label Jan 9, 2025
@FelberMartin FelberMartin self-assigned this Jan 9, 2025
@FelberMartin
Copy link
Collaborator Author

For future reference

In case we discover any problem related to the removal of the viewModelStoreOwner, here is a code snippet that also fixes Bug 1:

    val lifecycle = LocalLifecycleOwner.current.lifecycle
    val lifecycleObserver = rememberUpdatedState(
        LifecycleEventObserver { _, event ->
            if (event == Lifecycle.Event.ON_DESTROY) {
                store.clear()
            }
        }
    )

    DisposableEffect(lifecycle) {
        val observer = lifecycleObserver.value
        lifecycle.addObserver(observer)
        onDispose {
            lifecycle.removeObserver(observer)
        }
    }

I decided to not insert this code right away, because we currently do not know why the code is even needed. If we discover the reason, please also insert a detailed comment explaining why this code is needed.

Copy link
Contributor

@eylulnc eylulnc left a comment

Choose a reason for hiding this comment

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

I think it looks good.
Tested in both phone, tablet.
Chats are visible. ✅
Navigating back from chat leads to conversation list. ✅
No chat found is not display if a content exist. ✅

Base automatically changed from chore/two-column-conversation-layout to develop January 10, 2025 16:39
# Conflicts:
#	feature/metis/conversation/src/main/kotlin/de/tum/informatics/www1/artemis/native_app/feature/metis/conversation/ui/ConversationScreen.kt
Copy link
Contributor

@julian-wls julian-wls left a comment

Choose a reason for hiding this comment

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

Great work!
I test all scenarios on both, tablet and phone.
The Android back button is also working now leading the user back to the conversation list.
Your changes seem appropriate and can be merged imo.

Regarding the removal of viewModelStoreOwner I couldn't find any issues or bugs. I don't find any reason why viewModelStoreOwner should be used here either.

@FelberMartin FelberMartin added ready to merge This PR can be merged and removed ready for review This PR can be reviewed labels Jan 10, 2025
@FelberMartin FelberMartin merged commit 5a58bb7 into develop Jan 10, 2025
5 checks passed
@FelberMartin FelberMartin deleted the bugfix/chats-not-loading-on-tablet-layout branch January 10, 2025 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge This PR can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Chats not loading on tablet? Back Navigation in ThreadUI not working
3 participants