RoomMessageEvent refactoring, take 2 #20
Conversation
…sageEvent According to the spec, this key has the same status as msgtype: both should exist in any message. Besides, it's always supposed to be a plain text so there's no polymorphism allowed here.
TextContent is a class to deal with formatted (HTML, RTF, Markdown) text messages. Right now it only supports Vector's non-standard "formatted_body".
| } | ||
|
|
||
| template <class ContentT> | ||
| MessageEventContent::Base* make2(const QJsonObject& json) |
There was a problem hiding this comment.
A minor nitpicking issue: I don't really like the name "make2", as we might end up with make1-make10. Unfortunately, I don't really have a good name myself... Maybe just use make for VideoContent and AudioContent? VideoContent might use ThumbnailContent in the future, too, as it is currently an inconsistency
There was a problem hiding this comment.
Well, yeah, didn't like it either. The "proper" modern C++ way would be to make a couple of specializations for the two types but it's code duplication again, and rather dumb one. And yes, VideoContent is supposed to be like ImageContent and FileContent (see below).
|
Everything else looks good :) Thanks! |
|
Matthew commented that further on any content will have an optional thumbnail, defined under "content". Therefore I'm going to do to things:
With the two of these, make2<> will be unneeded. |
…nt only The spec is told to allow any event to have a thumbnail in future, and the thumbnail will reside under "content" JSON key rather than "info".
I rehashed my original attempt to refactor RoomMessageEvent and MessageEventContent family. The primary goals are the same: remove repetitive code that parses JSON and add HTML text messages support. An additional by-product is overall tightening of the code - in particular, messages without msgtype are no more even tried (the spec forbids those); and I chose to actually parse incoming MIME types using QMimeDatabase (it's a stock fd.o database on Linux and a Qt-bundled one on Windows).
This time it's made in several commits for easier reviewing and merging. I also abstained from renaming/moving MessageEventType. The change still breaks compatibility, as content classes have moved to a dedicated namespace. However, I only found a single place and only in Quaternion that actually names those classes, so the impact is very small. Besides, support of rich text in Quaternion needs an update to the same code in Quaternion, anyway.