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

Fix crash when opening an unknown room #6978

Merged
merged 7 commits into from
Aug 31, 2022
Merged

Conversation

bmarty
Copy link
Member

@bmarty bmarty commented Aug 31, 2022

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

Fix a crash when user opens a Timeline of a unknown room. This PR also contains some code cleanup, can be reviewed commit per commit if necessary.

This PR does not fix the root cause, which is quite unclear for now. This can be related to thread / space / etc. But it fixes the crash, and display a full screen error to the user.

If we got rageshake with the screen content in developer mode, we may have more info on the room which is tried to be open.

Note: I have seen #6543, but I think this is compatible with this change. I would prefer to first merge this PR, and then handle #6543.

Motivation and context

Less crash.
fixes #5611

Screenshots / GIFs

Before After
crash RoomNotFound

In developer mode, more info will be displayed:

image

Tests

Tested devices

  • Physical
  • Emulator
  • OS version(s):

Checklist

@bmarty bmarty requested review from a team and jmartinesp and removed request for a team August 31, 2022 13:24
) : VectorViewModel<RoomDetailViewState, RoomDetailAction, RoomDetailViewEvents>(initialState),
Timeline.Listener, ChatEffectManager.Delegate, CallProtocolsChecker.Listener, LocationSharingServiceConnection.Callback {

private val room = session.getRoom(initialState.roomId)!!
private val room = session.getRoom(initialState.roomId)
Copy link
Member Author

Choose a reason for hiding this comment

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

room can be null now.

@@ -266,6 +278,7 @@ class TimelineViewModel @AssistedInject constructor(
}

private fun prepareForEncryption() {
if (room == null) return
Copy link
Member Author

Choose a reason for hiding this comment

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

I could have used ? like room?. and timeline?. everywhere, but I think this is cleaner like this. When the room is null, there is nothing we can really do with this ViewModel.

Copy link
Member

@jmartinesp jmartinesp left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -538,11 +553,13 @@ class TimelineViewModel @AssistedInject constructor(
}

private fun handleJumpToReadReceipt(action: RoomDetailAction.JumpToReadReceipt) {
if (room == null) return
Copy link
Member

Choose a reason for hiding this comment

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

For this kind of single line 'if room not null, do X' I'd just use optional chaining (room?). It's a bit subjective, though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I have use the same pattern (nearly) everywhere for code consistency, but I admit that sometimes it's a bit too much.
Also doing like this has the advantage to not impact the git blame.

@ElementBot
Copy link

ElementBot commented Aug 31, 2022

Warnings
⚠️

vector/src/main/java/im/vector/app/features/home/room/detail/TimelineFragment.kt#L1127 - Prefer "SHOW_AS_ACTION_IF_ROOM" instead of "SHOW_AS_ACTION_ALWAYS"

⚠️

vector/src/main/java/im/vector/app/features/home/room/detail/TimelineFragment.kt#L1127 - Prefer "SHOW_AS_ACTION_IF_ROOM" instead of "SHOW_AS_ACTION_ALWAYS"

Generated by 🚫 dangerJS against fe42cdc

@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@bmarty
Copy link
Member Author

bmarty commented Aug 31, 2022

Buildkite will be happy, failure is due to matrix-org/pipelines#202

@bmarty bmarty merged commit 318352f into develop Aug 31, 2022
@bmarty bmarty deleted the feature/bma/null_room branch August 31, 2022 15:51
@bmarty bmarty mentioned this pull request Oct 5, 2022
68 tasks
jonnyandrew added a commit that referenced this pull request Dec 2, 2022
This error was seen before but has been reintroduced during refactoring.
- see #6978
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.

Crash - TimelineViewModel.<init> NullPointerException
3 participants