Fix crash with unknown notification type #1123
Conversation
|
I'm still grumpy about Mastodon just breaking clients. If we're sure that it will stay the same in stable (very likely) I propose to merge this before the stable release. |
| @@ -136,6 +136,7 @@ public void onBindViewHolder(@NonNull RecyclerView.ViewHolder viewHolder, int po | |||
| (NotificationViewData.Concrete) notification; | |||
| Notification.Type type = concreteNotificaton.getType(); | |||
| switch (type) { | |||
| default: | |||
There was a problem hiding this comment.
Huh? I don't get it, default is a mention?
There was a problem hiding this comment.
When type is null, switch statement returns NullPointerException.
This seems to be a Java's specification.
There was a problem hiding this comment.
Indeed it is and indeed it does throw. I just don't understand how "default" changes that:
https://ideone.com/I82Z5B
| @@ -59,7 +59,7 @@ public Concrete(Notification.Type type, String id, Account account, | |||
| } | |||
|
|
|||
| public Notification.Type getType() { | |||
| return type; | |||
| return type == null ? Notification.Type.UNKNOWN : type; | |||
There was a problem hiding this comment.
New notification does not have a type at all? Why it's null?
There was a problem hiding this comment.
New notification type poll is not defined in Notification.Type so type will be null.
There was a problem hiding this comment.
Ah, I see. Maybe more correct would be to alter parser code so that it returns UNKNOWN if it doesn't match any.
Right now it makes a really confusing API in the data layer (we can have both null and UNKNOWN).
We could mark type field as @JsonAdapter and write our adapter (I can help with that). It could take 10 more minutes but it will be future-proof IMO.
There was a problem hiding this comment.
I think its even possible without a JsonAdapter, have a look how we handle Visibility in Status.kt
There was a problem hiding this comment.
When I passed instance original visibility unleakable, app crashed with same error.
So I think we need JsonAdapter...
2019-03-15 16:50:24.068 2655-2655/com.keylesspalace.tusky E/AndroidRuntime: FATAL EXCEPTION: main
Process: com.keylesspalace.tusky, PID: 2655
java.lang.NullPointerException: Attempt to invoke virtual method 'int com.keylesspalace.tusky.entity.Status$Visibility.ordinal()' on a null object reference
There was a problem hiding this comment.
Added NotificationTypeAdapter and anotated Notification.Type by JsonAdapter.
Please check whether it works correctly.
|
Looks fine. We don't need |
|
@charlag It's true. Removed |
|
Poll notifications now look like I mentioned myself. But better than crashing I guess. And it hopefully wont stay this way for long. |
Fix part of #1102 .
If there's "Your poll has ended." notification in user's notification list, app crashes.
This is a fatal problem because once app started to crash, we can't even switch account.
So I fixed not to crash app for now.