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

Support tagged events in Room Account Data (MSC2437) #4753

Merged
merged 6 commits into from
Jan 6, 2022

Conversation

Florian14
Copy link
Contributor

Introduce tagged_events model in the matrix SDK.

This event is stored in the Room Account Data. Clients can use it to identify some state_events (as favourites for example), see the related MSC to learn more about it.

@github-actions
Copy link

github-actions bot commented Dec 17, 2021

Unit Test Results

  66 files  ±0    66 suites  ±0   1m 3s ⏱️ -1s
135 tests ±0  135 ✔️ ±0  0 💤 ±0  0 ±0 
418 runs  ±0  418 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit 8d7b2ab. ± Comparison against base commit 3ec2a09.

♻️ This comment has been updated with latest results.

Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

Thanks, some small remarks and suggested change.

I guess the next step is to add some API to the SDK services to let the app play with tags?

Also m.hidden events should be hidden from the timeline, and it should be handled by default by the SDK (CC @ganfra).

}

companion object {
const val TAGS_KEY = "tags"
Copy link
Member

Choose a reason for hiding this comment

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

TAGS_KEY is useless. Better to define a string in the data class TaggedEventsContent (see my other remark there)

Copy link
Contributor Author

@Florian14 Florian14 Jan 6, 2022

Choose a reason for hiding this comment

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

I agree, removed the variable for now

}

data class TaggedEventInfo(
var keywords: List<String>? = null,
Copy link
Member

Choose a reason for hiding this comment

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

Please add @Json(name = "keywords") for code clarity.
Also it could be a val and not a var

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thx

val originServerTs: Long? = null,

@Json(name = "tagged_at")
var taggedAt: Long? = null
Copy link
Member

Choose a reason for hiding this comment

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

It could be a val and not a var

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

val updatedTags = HashMap<String, Map<String, TaggedEventInfo>>(tags)
updatedTags[tag] = tagMap
tags = updatedTags
}
Copy link
Member

Choose a reason for hiding this comment

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

I propose to rewrite to

    fun tagEvent(eventId: String, tag: String, info: TaggedEventInfo) {
        val tagMap = tags[tag].orEmpty().toMutableMap()
                .also { it[eventId] = info }

        tags = tags.toMutableMap().also { it[tag] = tagMap }
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found a better optimization :)

Copy link
Member

@bmarty bmarty Jan 6, 2022

Choose a reason for hiding this comment

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

TIL the plus() and minus() operator extensions on Map. Note that it can be written using + and - but for code clarity I prefer what you have done. Thanks!

val updatedTags = HashMap<String, Map<String, TaggedEventInfo>>(tags)
updatedTags[tag] = tagMap
tags = updatedTags
}
Copy link
Member

Choose a reason for hiding this comment

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

I propose to rewrite to:

    fun untagEvent(eventId: String, tag: String) {
        val tagMap = tags[tag]?.toMutableMap() ?: return
        tagMap.remove(eventId)

        tags = tags.toMutableMap().also { it[tag] = tagMap }
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same here

Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

Thanks for the update

@bmarty bmarty merged commit b46b76c into develop Jan 6, 2022
@bmarty bmarty deleted the feature/fre/tagged_events branch January 6, 2022 20:06
@bmarty
Copy link
Member

bmarty commented Jan 6, 2022

Small change after merge: 401479f

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.

3 participants