Add fallback body to quotes #134

Merged
PapaTutuWawa merged 3 commits from millesimus/moxxyv2:fix_quote_fallback into master 4 weeks ago

With this PR I want to fix #128 by adding an emoji, the file size (dummy for now), and the original message body (usually the http url) as fallback quote.

For this, I added a general "getEmoji" method to the message model that returns a representative emoji for a message (for now?: only according to media type) and a dummy getFileSize method. As long as that returns 0, no file sizes will be printed in the fallback message.

In the screenshot, message "1" is an example for how it will look like after the merge. Message "2" is an example for how it will look like when the file size will be stored for each message.

With this PR I want to fix #128 by adding an emoji, the file size (dummy for now), and the original message body (usually the http url) as fallback quote. For this, I added a general "getEmoji" method to the message model that returns a representative emoji for a message (for now?: only according to media type) and a dummy getFileSize method. As long as that returns 0, no file sizes will be printed in the fallback message. In the screenshot, message "1" is an example for how it will look like after the merge. Message "2" is an example for how it will look like when the file size will be stored for each message.

I had a quick look over the changes and left some comments. Otherwise, it looks good to me.

I had a quick look over the changes and left some comments. Otherwise, it looks good to me.
PapaTutuWawa requested changes 2 months ago
PapaTutuWawa left a comment
Owner

I added some comments on the PR.

I added some comments on the PR.
/// Return an emoji for a MIME type. Primarily intended for fallback bodies.
// TODO(Unknown): Merge with mimeTypeToConversationBody?
String mimeTypeToEmoji(String? mime) {

I would just replace mimeTypeToConversationBody with this method. Also, I would remove the case for mime types text/*. They should just fallback to the generic file emoji.

I would just replace `mimeTypeToConversationBody` with this method. Also, I would remove the case for mime types `text/*`. They should just fallback to the generic file emoji.
millesimus marked this conversation as resolved
/// for media messages.
// TODO(Unknown): get actual file size,
// https://codeberg.org/moxxy/moxxyv2/issues/129
int get messageSize {

As mentioned, this needs to be stored as message metadata in the database. Also, a post-processing step is required if we cannot get the filesize from Stateless File Sharing metadata.

As mentioned, this needs to be stored as message metadata in the database. Also, a post-processing step is required if we cannot get the filesize from Stateless File Sharing metadata.
Poster

Hi, I added the media size to the database, but I circumvented the sent metadata (following the intuition that in the end we would want the real file size anyways).

Hi, I added the media size to the database, but I circumvented the sent metadata (following the intuition that in the end we would want the real file size anyways).
millesimus marked this conversation as resolved
/// potential privacy issue).
/// The body of the quotes message is always printed, in case other clients
/// provided useful fallback information.
String createFallbackBodyForQuotedMessage(MessageDetails details) {

This method should not be part of the MessageManager. The XMPP service can just compute it and hand it over using the quoteBody attribute. The reason is that this feature is pretty Moxxy specific and everything in lib/xmpp/ is planned to turn into its own XMPP library, which is why I try to keep everything there very generic.

This method should not be part of the MessageManager. The XMPP service can just compute it and hand it over using the `quoteBody` attribute. The reason is that this feature is pretty Moxxy specific and everything in lib/xmpp/ is planned to turn into its own XMPP library, which is why I try to keep everything there very generic.
Poster

Thanks for this comment, I refactored it and I think it really feels cleaner this way. :)

Thanks for this comment, I refactored it and I think it really feels cleaner this way. :)
millesimus marked this conversation as resolved
millesimus changed title from Add fallback body to quotes to WIP: Add fallback body to quotes 2 months ago
PapaTutuWawa added this to the Alpha Release 0.3.0 project 2 months ago
Poster

I also added a "size to string" method to calculate "KiB" / "MiB" outputs. I first used a more sophisticated algorithm, but I figured it would me more useful if we used the same as Conversations (and Gajim, iirc).

I also added a "size to string" method to calculate "KiB" / "MiB" outputs. I first used a more sophisticated algorithm, but I figured it would me more useful if we used the same as Conversations (and Gajim, iirc).
millesimus changed title from WIP: Add fallback body to quotes to Add fallback body to quotes 2 months ago
Poster

And last but not least, thank you so much for taking the time to review. I appreciate it :)

And last but not least, thank you so much for taking the time to review. I appreciate it :)
PapaTutuWawa requested changes 2 months ago
PapaTutuWawa left a comment
Owner

Except for the comments, it looks good. If you fix those, we could pretty much immediately merge this PR. I'll then just have to rebase my OMEMO branch on master. I have a conflict anyway, so it makes little sense to hold this one back.

Except for the comments, it looks good. If you fix those, we could pretty much immediately merge this PR. I'll then just have to rebase my OMEMO branch on master. I have a conflict anyway, so it makes little sense to hold this one back.
mediaWidth: mediaWidth,
mediaHeight: mediaHeight,
isFileUploadNotification: false,
mediaSize: File(downloadedPath).lengthSync(),

Could you please also add this for files we sent? I know that they (most likely) exist locally but without it we would be forced to do another stat when loading the message.

Could you please also add this for files we sent? I know that they (most likely) exist locally but without it we would be forced to do another stat when loading the message.
millesimus marked this conversation as resolved
} else {
quoteMessageSize = '';
}
return '${quotedMessage.messageEmoji} $quoteMessageSize${quotedMessage.body}';

I just noticed that in case of quotedMessage.mediaSize == null, the fallback body would look like, for example, 🖼️ • https://... (Notice the two spaces before and after the ). Could you please ensure that the double spaces are removed?

Also: the fact that quotedMessage.body contains the file URL is not always given. For example, when sending files encrypted using OMEMO, I am unsure if I should set the message body to the URL or not since it makes no sense without the metadata in XEP-0448 and thus cannot be used with OOB data. A better solution would be to use quotedMessage.srcUrl.

I just noticed that in case of `quotedMessage.mediaSize == null`, the fallback body would look like, for example, `🖼️ • https://...` (Notice the two spaces before and after the `•`). Could you please ensure that the double spaces are removed? Also: the fact that `quotedMessage.body` contains the file URL is not always given. For example, when sending files encrypted using OMEMO, I am unsure if I should set the message body to the URL or not since it makes no sense without the metadata in XEP-0448 and thus cannot be used with OOB data. A better solution would be to use `quotedMessage.srcUrl`.
Poster

Could you please ensure that the double spaces are removed?

Sorry, that was kinda stupid of a workaround. Double spaces are removed. ^^

> Could you please ensure that the double spaces are removed? Sorry, that was kinda stupid of a workaround. Double spaces are removed. ^^
Poster

Also: the fact that quotedMessage.body contains the file URL is not always given. For example, when sending files encrypted using OMEMO, I am unsure if I should set the message body to the URL or not since it makes no sense without the metadata in XEP-0448 and thus cannot be used with OOB data. A better solution would be to use quotedMessage.srcUrl.

Agreed, sounds better.

> Also: the fact that quotedMessage.body contains the file URL is not always given. For example, when sending files encrypted using OMEMO, I am unsure if I should set the message body to the URL or not since it makes no sense without the metadata in XEP-0448 and thus cannot be used with OOB data. A better solution would be to use quotedMessage.srcUrl. Agreed, sounds better.
millesimus marked this conversation as resolved
/// Show a combinatio of an emoji and its file type
String mimeTypeToConversationBody(String? mime) {
/// Return an emoji for a MIME type.
String mimeTypeToEmoji(String? mime) {

This looks good but the issue looks like the last conversation body will now turn into just "🖼️" instead of "🖼️ Image". I am not sure how that can be solved the cleanest but my first idea is to just add something like a bool addTypeName parameter that then optionally appends " Audio" or " Video". If you have a better solution, let me know.

This looks good but the issue looks like the last conversation body will now turn into just "🖼️" instead of "🖼️ Image". I am not sure how that can be solved the cleanest but my first idea is to just add something like a `bool addTypeName` parameter that then optionally appends " Audio" or " Video". If you have a better solution, let me know.
Poster

Oh, I see. I misunderstood you there.

Oh, I see. I misunderstood you there.
millesimus marked this conversation as resolved
/// Logic from:
/// @author iNPUTmice
/// https://github.com/iNPUTmice/Conversations/blob/d435c1f2aef1454141d4f5099224b5a03d579dba/src/main/java/eu/siacs/conversations/utils/UIHelper.java#L605
String fileSizeToString(int size) {

To me it feels a bit arbitrary to switch to MiB at 1.5 * 1024 KiB instead of just 1024 KiB. Also, I feel like most people would be more familiar with MB instead of MiB.

To me it feels a bit arbitrary to switch to MiB at 1.5 * 1024 KiB instead of just 1024 KiB. Also, I feel like most people would be more familiar with MB instead of MiB.
pubspec.yaml Outdated
hex: 0.2.0
image: 3.2.0
image_size_getter: 2.1.2
intl: 0.17.0

Is there any reason you pulled in intl and not used it anywhere?

Is there any reason you pulled in `intl` and not used it anywhere?
Poster

No, but I'm surprised that flutter analyze wouldn't point this out. ^^

No, but I'm surprised that `flutter analyze` wouldn't point this out. ^^
millesimus marked this conversation as resolved
millesimus changed title from Add fallback body to quotes to WIP: Add fallback body to quotes 2 months ago
PapaTutuWawa requested changes 2 months ago
PapaTutuWawa left a comment
Owner

I see only two really small issues. Otherwise, it looks good to me. Will test and merge once these little things are fixed.

I see only two really small issues. Otherwise, it looks good to me. Will test and merge once these little things are fixed.
if (quotedMessage.srcUrl != null && quotedMessage.srcUrl!.isNotEmpty) {
quotedMediaUrl = quotedMessage.srcUrl!;
} else {
quotedMediaUrl = quotedMessage.body;

The indentation is wrong here.

The indentation is wrong here.
millesimus marked this conversation as resolved
} else {
quotedMediaUrl = quotedMessage.body;
}
return '${quotedMessage.messageEmoji} $quoteMessageSize${quotedMediaUrl.isNotEmpty ? "$quotedMediaUrl" : ""}';

Small nitpick: Could you maybe create the string for the interpolation outside of the string? It would make the line a bit shorter and the code more readable. This is okay for the emoji function since it's really short and simple but this one is more complex.

Also, could you insert a blank line before the return?

Small nitpick: Could you maybe create the string for the interpolation outside of the string? It would make the line a bit shorter and the code more readable. This is okay for the emoji function since it's really short and simple but this one is more complex. Also, could you insert a blank line before the return?
Poster

Yes, of course. :)

Yes, of course. :)
millesimus marked this conversation as resolved
millesimus changed title from WIP: Add fallback body to quotes to Add fallback body to quotes 2 months ago
PapaTutuWawa requested changes 2 months ago
PapaTutuWawa left a comment
Owner

Looks good. Will test this when I get the change to.

Looks good. Will test this when I get the change to.
quoteMessageSize = '';
}
// create media url string, or use body if no srcUrl is stored

Minor nitpick: Comments should start with an uppercase letter.

Minor nitpick: Comments should start with an uppercase letter.
Poster

For THIS one I'll test squashing and force-reloading on Codeberg. :D

For THIS one I'll test squashing and force-reloading on Codeberg. :D
millesimus marked this conversation as resolved
quotedMediaUrl = '';
}
// concatenate emoji, size string, and media url and return

See above.

See above.
millesimus marked this conversation as resolved
millesimus force-pushed fix_quote_fallback from 6f8a209f3a to dab3d6bec0 2 months ago

Looks good to me. Will test this later and merge.

Looks good to me. Will test this later and merge.
PapaTutuWawa added the
backend/service
Improvement
labels 2 months ago

@millesimus I just merged my OMEMO implementation. Could you maybe rebase onto my changes?

@millesimus I just merged my OMEMO implementation. Could you maybe rebase onto my changes?
millesimus force-pushed fix_quote_fallback from dab3d6bec0 to 5dc4912ac9 1 month ago
Poster

Done! I also took the liberty to cleanup the commit history. :)

It's working with unencrypted messages, but OMEMO doesn't work for me yet so I couldn't test it.

Done! I also took the liberty to cleanup the commit history. :) It's working with unencrypted messages, but OMEMO doesn't work for me yet so I couldn't test it.
PapaTutuWawa requested changes 1 month ago
PapaTutuWawa left a comment
Owner

I found some issues. Could you please have a look at them?

I found some issues. Could you please have a look at them?
/// for media messages.
// TODO(Unknown): get actual file size,
// https://codeberg.org/moxxy/moxxyv2/issues/129
int get messageSize {

I'm not sure if I have forgotten anything, but isn't this getter redundant since we have mediaSize. A quick CTRL+F in the browser also shows no usage of .messageSize.

If this is not used, please remove it.

I'm not sure if I have forgotten anything, but isn't this getter redundant since we have `mediaSize`. A quick CTRL+F in the browser also shows no usage of `.messageSize`. If this is not used, please remove it.
millesimus marked this conversation as resolved
this.shouldEncrypt = false,
this.isMediaQuote = false,
this.quoteMessageEmoji,
this.quoteMessageSize = 0,

Same as above for isMediaQuote, quoteMessageEmoji and quoteMessageSize.

Same as above for `isMediaQuote`, `quoteMessageEmoji` and `quoteMessageSize`.
millesimus marked this conversation as resolved
millesimus force-pushed fix_quote_fallback from 5dc4912ac9 to 943a03bd2c 1 month ago
Poster

Thanks, I fixed them. :)

Thanks, I fixed them. :)

Looks good to me. Will test this tomorrow.

Looks good to me. Will test this tomorrow.
PapaTutuWawa merged commit 8d8c4d2da3 into master 4 weeks ago

Thank you very much for the contribution!

Thank you very much for the contribution!
Poster

Thanks to you for your invaluable help in making it! :)

Thanks to you for your invaluable help in making it! :)

Reviewers

PapaTutuWawa requested changes 1 month ago
The pull request has been merged as 8d8c4d2da3.
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: moxxy/moxxyv2#134
Loading…
There is no content yet.