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

Better power level checking, part 1 #799

Merged
merged 5 commits into from
Sep 18, 2024

Conversation

KitsuneRal
Copy link
Member

This PR is the first, API-breaking, part of additional facilities to simplify answering a question "am I allowed to do X?". While porting Quaternion to libQuotient 0.9, I thought that it would be great to have something like Room::canRedact() - turned out though that actually figuring out whether you can redact a certain event is not that easy to answer (and the spec doesn't seem to be very clear in that part). So I will propose can*() methods as an API/ABI-compatible addition after 0.9.0 is out (once I make them right) but here's some groundwork that needs to come in 0.9.0.

@KitsuneRal KitsuneRal added the enhancement A feature or change request for the library label Sep 15, 2024
Copy link

sonarcloud bot commented Sep 15, 2024

@KitsuneRal KitsuneRal mentioned this pull request Sep 15, 2024
@TobiasFella
Copy link
Member

is it expected that this breaks compilation of code like

m_room->setState<Quotient::RoomPowerLevelsEvent>(QJsonObject());

@KitsuneRal
Copy link
Member Author

Rather not; I'll check what's going on.

Checking whether an event is a state event only makes sense in room
context, hence the moving. As for Quotient::isStateEvent(), this checks
an event type id against known event types that are derived from
StateEvent; the function will be used to simplify the API to check power
levels in a later commit.
Allows to use designated initialisers, see the next commit.
This commit makes RoomPowerLevelsEvent _always_ available via
currentState() - no need to query()/queryOr(), just get() and safely
access all the members. This is possible because the spec defines
default power levels for all kinds of cases; we can just inject
a synthetic RoomPowerLevelsEvent object with those defaults into
currentState() and once the real event comes (either from the homeserver
or from the cache) it replaces the synthetic one by normal means of
state processing.

Thanks to that, Room::memberEffectivePowerLevel() implementation becomes
much simpler; and to match it on the other side of power level checks,
Room::powerLevelFor() is introduced as a unified call returning the
power level necessary to send any specific event type, state or not.
@KitsuneRal KitsuneRal force-pushed the kitsune/better-power-level-checking-1 branch from c57e97a to 0a2543f Compare September 17, 2024 13:17
@KitsuneRal
Copy link
Member Author

m_room->setState<Quotient::RoomPowerLevelsEvent>(QJsonObject());

Ok, I didn't intend to break the legitimate things; that being said, this one is not quite legitimate. Below is the write-up for documentation's sake; TL;DR is that you should avoid using empty JSON to initialise events, since pre-0.8 you should be able to "default-construct" (most of) them with an empty list of arguments instead.

So, unimportant historical details: what used to be happening before is the empty QJsonObject was (effectively implicitly) converted to PowerLevelEventContent because unlike most other event types PowerLevelEventContent had a constructor accepting QJsonObject, and the StateEvent<> constructor used that constructor on the arguments passed to setState() in line with the emplace-idiom (creating an owned object in-place from its arguments). The problem is that, unless you go ~5 levels deep into the call hiearchy in the library code, it is not clear whether that QJsonObject is used to create an event object or merely the content part of it. This is why constructors in Event/RoomEvent etc. that accept QJsonObject have been made private since libQuotient 0.7, in particular; and around the same time, most event content structures were made default-constructible. RoomPowerLevelsEvent and PowerLevelsEventContent were initially made based on EventContent:: classes, that is - with a QJsonObject constructor and fillJson() method. However, the only reason EventContent:: classes were made this way has been because they are polymorphic. PowerLevelsEventContent doesn't need polymorphism; and in any case it had to have the defaults defined within the structure, rather than rely on the event class correctly filling them (which has been the way it all worked before the big event types refactoring in 0.7).

Back to important stuff. To provide cross-compatible code, I can suggest explicitly creating a PowerLevelEventContent object by writing m_room->setState<RoomPowerLevelEvent>(fromJson<PowerLevelEventContent>(QJsonObject{}));. If there are too many such places then I can restore (and immediately deprecate) PowerLevelEventContent::PowerLevelEventContent(const QJsonObject&) - but I'd prefer to avoid that.

TobiasFella
TobiasFella previously approved these changes Sep 17, 2024
@KitsuneRal KitsuneRal merged commit 798d632 into dev Sep 18, 2024
6 checks passed
@KitsuneRal KitsuneRal deleted the kitsune/better-power-level-checking-1 branch September 18, 2024 06:58
KitsuneRal added a commit that referenced this pull request Sep 19, 2024
This was introduced in #799, only accidentally passing through tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A feature or change request for the library
Projects
Status: 0.9 - Done
Development

Successfully merging this pull request may close these issues.

2 participants