Refactoring & Code Improvements
Most parts of NeoChat's codebase deserve some degree of refactoring. Here are various things that come to my mind at the moment. Feel free to add more things as you notice them:
-
Some classes (mostly NeoChatRoom) have bothname
anddisplayName
properties. I don't see the point in having both and it leads to weird code in qml (e.g. "if it has a name, use that, else if it has a displayname, use that, else use an empty string). Let's standardize on using thename
property. -
In some situations, QML complains that some property is null (mostly temporary, so the actual problems with that are minimal). QML complains about it, though. As much as possible, I'd prefer handling this sort of thing in the backend and exposing only valid objects to QML (or something like an empty QString, if required). Obviously, we shouldn't create "empty" NeoChatUser or NeoChatRoom objects.
-
RoomPage.qml containts various orthogonal screens: an empty page showing only a loading indicator, a page showing "accept" and "decline" buttons for invitations, the normal room page, potentially in the future a "space home" page, etc. We should split those components into different files to make this more maintainable than it currently is. I've tried this in the past and failed due to problems with the message list view, footers, headers, etc. in the ScrollablePage... It really is something that needs to be done though. -
Models:
- All models should have a
Roles
enum containing roles ending inRole
(e.g.,NameRole
) - The enum should be registered to Qt using
Q_ENUM(Roles)
- The first role of each model should be something that can be used as a name or identifier (e.g. userids, roomids, etc.) and should be assigned the
Qt::DisplayRole
value (e.g.,DisplayRole = Qt::DisplayRole
). This helps massively when exploring the model in gammaray. - Avoid using
beginResetModel
andendResetModel
anddataChanged
for all rows and roles as much as possible, as it causes the models to reload more data than is probably necessary - Models should check for valid row (and column and parent? not that those really matter...) before doing anything
- Let's standardize on either switch-cases or if cascades for handling the different roles
- Models should properly handle their data changing in real time (i.e. without having to restart the client before something like a name change shows up). We can handle this relatively well with connection and dataChanged() and it will result in amazing UX. This also applies to properties outside of models
- All models should have a
-
Let's improve our logging. In addition to #562, which mostly focuses on the backend of having useful logs without cluttering up the terminal, i want to focus here on providing useful and consistent log messages:
- Use qCDebug / qCWarning /qCCritical consistently
- always use a logging category; add one, if necessary
- Don't add "category" information in the message itself (i.e.
qCWarning(MessageEventModel) << "Failed to find event
instead ofqWarning() << "MessageEventModel: failed to find event
) - Let's use some consistent style for the messages themselves; i don't have any concrete ideas since we don't really do any logging at the moment
- Only use qCWarnings for things that are actually unexpected, at the moment i see a lot of messages like "failed to find event xy" where i'm assuming those events are just not loaded