Conversation
jancborchardt
left a comment
There was a problem hiding this comment.
It looks different from the message pinning style as far as I can see? Or does it work well in conjunction with that?
#5807
2 floating boxes would be fine. Ideal could be one floating box with a horizontal divider line, to save on space and visual complexity.
jancborchardt
left a comment
There was a problem hiding this comment.
Nevermind – saw #5867 just now :)
ab010e7 to
017c91a
Compare
mahibi
left a comment
There was a problem hiding this comment.
Would be nice to already follow the suggestion from #5867 (comment) to make it collapsible?
Otherwise fine for me to do this in followup PRs.
app/src/main/java/com/nextcloud/talk/models/json/upcomingEvents/UpcomingEvent.kt
Outdated
Show resolved
Hide resolved
@mahibi Yes, I will simply change it according to the discussion on the other item 👍 Yet I am unsure if that means, hide it forever? hide until you open the chat again or hide it based on the date of the event you display? WDYT @jancborchardt @nimishavijay ? |
There was a problem hiding this comment.
Yet I am unsure if that means, hide it forever? hide until you open the chat again or hide it based on the date of the event you display?
Probably best would be to:
- ✅ Always anyway show the "Upcoming call" box in the "Conversation info" view. Directly between conversation description and "Notification settings".
- ✅ If you x away the "Upcoming call" box that shows directly in the conversation view, show a notice saying "Upcoming call can be viewed in conversation info".
e3984d0 to
505b8dd
Compare
|
@mahibi implemented database layer change for dismissing and updated the design as discussed in the general design issue |
fixed review comments
jancborchardt
left a comment
There was a problem hiding this comment.
Looks good!
This should be added as follow-up :) #5865 (review)
Resolves #5661 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
62ce0d6 to
e528bf2
Compare
| override fun onNext(model: ConversationModel) { | ||
| runBlocking { | ||
| val existingEntity = dao.getConversationForUser(user.id!!, model.token).first() | ||
| model.hiddenUpcomingEvent = existingEntity?.hiddenUpcomingEvent |
There was a problem hiding this comment.
So this is the point where we begin to store local-only things in the same entity that is fed by the api.
That's okay of course. We would have to make sure this is handled everywhere where we use the endpoint which can easily be forgotten.
I'm just thinking if we should have a rule how we should do this in general.
What about to have this handled by mappers?
For example the mapper from ConversationModel to ConversationEntity could look like:
fun ConversationModel.asEntity(existing: ConversationEntity?): ConversationEntity {
return ConversationEntity(
internalId = internalId,
accountId = accountId,
....
// here, we could handle all "local-only" fields
hiddenUpcomingEvent = existing?.hiddenUpcomingEvent ?: false
)
}
With this, we won't accidentally forget to rescue specific columns from being overwritten.
What do you think @AndyScherzinger ?
There was a problem hiding this comment.
In general it makes sense to discuss that, yes.
If I am not mistaken than messageDraft is local-only as well, no?
There was a problem hiding this comment.
There was a problem hiding this comment.
Okay i see, the idea wasn't quite fully developed yet :D
So fine for me to merge then
There was a problem hiding this comment.
Happy to discuss how to improve it 👍
The mapper does respect the fields, there just isn't a mapper with an entitled parameter input.
app/src/main/java/com/nextcloud/talk/data/database/model/ConversationEntity.kt
Show resolved
Hide resolved
Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
|
APK file: https://www.kaminsky.me/nc-dev/android-artifacts/5865.apk |
|
/backport to stable-23.0 |

🖼️ Screenshots
🚧 TODO
🏁 Checklist
/backport to stable-xx.x