Note to self feature addition #256

Merged
PapaTutuWawa merged 28 commits from :feature_note_to_self into master 2 months ago

What does this PR do?

This PR introduces a new "Note to self" feature for the moxxy user. Here, the user can select the "Note to Self" SpeedDialChild and then:

  1. If no such "Note to self" Conversation exists, it will create one and redirect the user to the conversation page.
  2. If such a conversation already exists, the user will just be redirected to that page without creating a new Conversation.

Currently, the JID for such a conversation is kept as an empty string - "". This is probably not the best solution, but in any other case - I noticed that this conversation will get synced to other clients(like Dino) as well. Since we want this to be an offline feature - the empty string is a possible solution for now.

What remains to be done?

  1. Right now, there is no separation between a normal conversation and a "note to self" conversation, except the empty string JID for a "note to self" conversation.
  2. Create a new entity in the Conversation model called type, which will differentiate between a:
    • 1:1 peer chat
    • MUC(upcoming feature)
    • Note to Self chat
  3. Isolate the XMPP services to be used only if the conversation is of the 1:1 peer chat. Keep all interactions of the Note type conversations limited to the local sqflite database.
  4. Write tests for the new feature.

A simple demo of the current implementation is attached below.

What does this PR do? This PR introduces a new "Note to self" feature for the `moxxy` user. Here, the user can select the "Note to Self" `SpeedDialChild` and then: 1. If no such "Note to self" `Conversation` exists, it will create one and redirect the user to the conversation page. 2. If such a conversation already exists, the user will just be redirected to that page without creating a new Conversation. Currently, the `JID` for such a conversation is kept as an empty string - `""`. This is probably not the best solution, but in any other case - I noticed that this conversation will get synced to other clients(like Dino) as well. Since we want this to be an offline feature - the empty string is a possible solution for now. What remains to be done? 1. Right now, there is no separation between a normal conversation and a "note to self" conversation, except the empty string `JID` for a "note to self" conversation. 2. Create a new entity in the `Conversation` model called `type`, which will differentiate between a: * 1:1 peer chat * MUC(upcoming feature) * Note to Self chat 3. Isolate the `XMPP` services to be used only if the conversation is of the 1:1 peer chat. Keep all interactions of the `Note` type conversations limited to the local `sqflite` database. 4. Write tests for the new feature. A simple demo of the current implementation is attached below.
ikjot-2605 added 2 commits 3 months ago
34971950ad feat(notes): Add notes to self SpeedDialChild
Signed-off-by: Ikjot Singh Dhody <ikjotsd@gmail.com>
2b6ed19847 feat(notes): Make JID "" for notes conversation.
Signed-off-by: Ikjot Singh Dhody <ikjotsd@gmail.com>
Owner

Thanks for the PR! Could you rebase your branch on master? I see that your editor introduced code formatting, which was done just yesterday on the entire repository.

Thanks for the PR! Could you rebase your branch on master? I see that your editor introduced code formatting, which was done just yesterday on the entire repository.
PapaTutuWawa requested changes 3 months ago
@ -2,2 +2,4 @@
android.useAndroidX=true
android.enableJetifier=true
org.gradle.java.home=/usr/lib/jvm/java-17-openjdk-amd64
Owner

This should not be part of the PR.

This should not be part of the PR.
Poster

Understood, will remove.

Understood, will remove.
ikjot-2605 marked this conversation as resolved
@ -283,0 +290,4 @@
child: const Icon(Icons.notes),
onTap: () {
context.read<NewConversationBloc>().add(
NewConversationAddedEvent(
Owner

I think we might be able to add a new event in the NewConversationBloc that creates a Conversation with the type of Notes. Or we adapt the event so that we can tell it what type of chat we want. The latter might be the cleaner or the two solutions.

I think we might be able to add a new event in the NewConversationBloc that creates a Conversation with the type of `Notes`. Or we adapt the event so that we can tell it what type of chat we want. The latter might be the cleaner or the two solutions.
Poster

Done.

Done.
ikjot-2605 marked this conversation as resolved
ikjot-2605 added 1 commit 3 months ago
09fc55d2c7 Merge branch 'master' into feature_note_to_self
Signed-off-by: Ikjot Singh Dhody <ikjotsd@gmail.com>
ikjot-2605 added 1 commit 3 months ago
2928602e8d feat(notes): Remove JAVA_HOME override from gradle
Signed-off-by: Ikjot Singh Dhody <ikjotsd@gmail.com>
ikjot-2605 added 2 commits 3 months ago
9e3700001d feat(notes): Add 'type' argument to Conversation.
Signed-off-by: Ikjot Singh Dhody <ikjotsd@gmail.com>
30f6ecd2f8 feat(notes): Fix enum decoding - ConversationType.
Signed-off-by: Ikjot Singh Dhody <ikjotsd@gmail.com>
ikjot-2605 added 1 commit 3 months ago
31ee7b919b feat(notes): Diasble Notification Service for Note
Format SpeedDialChild for Conversation.

Signed-off-by: Ikjot Singh Dhody <ikjotsd@gmail.com>
ikjot-2605 added 1 commit 3 months ago
8570997cb0 feat(notes): Remove moxxmpp implementation - Note.
Message stanza not sent for Notes to Self.

Signed-off-by: Ikjot Singh Dhody <ikjotsd@gmail.com>
ikjot-2605 added 1 commit 3 months ago
6650686d48 feat(notes): Update ConversationTopBar for Notes.
For notes, don't show block user, add to contacts row.

For notes, don't give encryption options, and don't allow to block user.

Signed-off-by: Ikjot Singh Dhody <ikjotsd@gmail.com>
Poster

@PapaTutuWawa, I have added the basic functionalities to this PR. As of now, the Note to Self conversation:

  1. Creates a new conversation with jid='' for notes.
  2. Doesn't send any message over the network, just adds it to the local sqflite database.
  3. Disables the block user/add to contacts option in the ConversationPage.
  4. Removes the Action Button for encryption/decryption on the ConversationTopBar.
  5. Removes the block user action from the second PopupMenuButton, leaving only the Close Chat option there.

Some questions I have regarding further requirements:

  1. Should a user be able to send voice notes in a Note type conversation?
  2. Should a user be able to see the Forward option in a Note message(even though it hasn't been implemented yet)?
  3. Should note retractions be handled any differently from a normal conversation?
  4. Should I put up a screen recording for the current implementation and show the difference between a "normal" chat and a "note to self" chat?

Thanks 😃

@PapaTutuWawa, I have added the basic functionalities to this PR. As of now, the Note to Self conversation: 1. Creates a new conversation with `jid=''` for notes. 2. Doesn't send any message over the network, just adds it to the local `sqflite` database. 3. Disables the block user/add to contacts option in the `ConversationPage`. 4. Removes the Action Button for encryption/decryption on the `ConversationTopBar`. 5. Removes the block user action from the second `PopupMenuButton`, leaving only the Close Chat option there. Some questions I have regarding further requirements: 1. Should a user be able to send voice notes in a `Note` type conversation? 2. Should a user be able to see the `Forward` option in a Note message(even though it hasn't been implemented yet)? 3. Should note retractions be handled any differently from a normal conversation? 4. Should I put up a screen recording for the current implementation and show the difference between a "normal" chat and a "note to self" chat? Thanks 😃
ikjot-2605 added 1 commit 3 months ago
8a33d88e31 feat(notes): German translation - SpeedDialChild.
Signed-off-by: Ikjot Singh Dhody <ikjotsd@gmail.com>
ikjot-2605 reviewed 3 months ago
@ -481,2 +481,3 @@
backgroundColor: const Color.fromRGBO(0, 0, 0, 0),
appBar: const ConversationTopbar(),
appBar: ConversationTopbar(
isNoteConversation: widget.conversationJid == '',
Poster

@PapaTutuWawa, would it be cleaner(for future conversation types, eg: group chat) if I add a conversationType argument instead of this isNoteConversation argument?

@PapaTutuWawa, would it be cleaner(for future conversation types, eg: group chat) if I add a `conversationType` argument instead of this `isNoteConversation` argument?
Owner

Yes, I think that would be cleaner.

Yes, I think that would be cleaner.
ikjot-2605 marked this conversation as resolved
PapaTutuWawa requested changes 3 months ago
PapaTutuWawa left a comment
Owner

Looks good to me. I do have some requests, however.

Also, there are some things that should also be done:

  • When we send a file to the self chat, it should not be uploaded to the server but still behave as if we did (if that makes sense 😄)
  • As you changed the database schema, you also have to write a database migration.
Looks good to me. I do have some requests, however. Also, there are some things that should also be done: - When we send a file to the self chat, it should not be uploaded to the server but still behave as if we did (if that makes sense 😄) - As you changed the database schema, you also have to write a database migration.
@ -1,3 +1,3 @@
org.gradle.jvmargs=-Xmx1536M
android.useAndroidX=true
android.enableJetifier=true
android.enableJetifier=true
Owner

I think you accidentally removed a newline. Not sure how this happened. However, it's not a big issue.

I think you accidentally removed a newline. Not sure how this happened. However, it's not a big issue.
Poster

Somehow, I am unable to see this change in the PR diff. 🤔

Somehow, I am unable to see this change in the PR diff. 🤔
ikjot-2605 marked this conversation as resolved
@ -135,6 +135,7 @@
"conversations": {
"speeddialNewChat": "New chat",
"speeddialJoinGroupchat": "Join groupchat",
"speeddialAddNoteToSelf": "Note to Self",
Owner

I'm not sure if the "S" in "Self" should be capitalized.

I'm not sure if the "S" in "Self" should be capitalized.
ikjot-2605 marked this conversation as resolved
@ -135,6 +135,7 @@
"conversations": {
"speeddialNewChat": "Neuer chat",
"speeddialJoinGroupchat": "Gruppenchat beitreten",
"speeddialAddNoteToSelf": "Notiz an mich selbst",
Owner

I would just say "Notiz an mich". It's a bit shorter and essentially says the same thing.

I would just say "Notiz an mich". It's a bit shorter and essentially says the same thing.
ikjot-2605 marked this conversation as resolved
@ -8,2 +10,4 @@
String intToString(int i) => '$i';
int stringToInt(String s) => int.parse(s);
String conversationTypeToString(ConversationType type) =>
Owner

I think these should be made a switch statement. That makes it a tiny bit easier when we add another chat type.

I think these should be made a switch statement. That makes it a tiny bit easier when we add another chat type.
ikjot-2605 marked this conversation as resolved
@ -490,2 +492,3 @@
builder: (context, state) {
if (state.conversation?.inRoster ?? false) {
if ((state.conversation?.inRoster ?? false) ||
state.conversation?.jid == '') {
Owner

I think that checking the state.conversation?.type attribute would be a cleaner choice.

I think that checking the `state.conversation?.type` attribute would be a cleaner choice.
ikjot-2605 marked this conversation as resolved
@ -44,3 +44,2 @@
implements PreferredSizeWidget {
const ConversationTopbar({super.key});
const ConversationTopbar({required this.isNoteConversation, super.key});
Owner

I don't think that this is required. In the BlocBuilder we already have access to the entire conversation that is displayed, if there is one.

I don't think that this is required. In the `BlocBuilder` we already have access to the entire conversation that is displayed, if there is one.
ikjot-2605 marked this conversation as resolved
@ -95,2 +92,2 @@
item.avatarUrl,
),
NewConversationAddedEvent(item.jid, item.title,
item.avatarUrl, ConversationType.chat),
Owner

I think there is a comma missing after the last argument.

I think there is a comma missing after the last argument.
ikjot-2605 marked this conversation as resolved
@ -114,6 +111,9 @@ class NewConversationPage extends StatelessWidget {
item.avatarUrl,
item.jid,
0,
item.jid == ''
Owner

I would just handle the "note to self" feature similarly to the "add contact" item. As we don't add an empty item to the roster, the item.jid case should never happen here.

I would just handle the "note to self" feature similarly to the "add contact" item. As we don't add an empty item to the roster, the `item.jid` case should never happen here.
ikjot-2605 marked this conversation as resolved
Owner

Should a user be able to send voice notes in a Note type conversation?

I don't see a reason why it should not be allowed.

Should a user be able to see the Forward option in a Note message(even though it hasn't been implemented yet)?

A share button would make more sense, but that's out of the scope of the PR. So hiding it is a good idea.

Should note retractions be handled any differently from a normal conversation?

I would say the same as with message sending: Retract in the database but only send it if the chat is not a self chat.

Should I put up a screen recording for the current implementation and show the difference between a "normal" chat and a "note to self" chat?

If it's not too much effort, I think that would be pretty cool!

> Should a user be able to send voice notes in a Note type conversation? I don't see a reason why it should not be allowed. > Should a user be able to see the Forward option in a Note message(even though it hasn't been implemented yet)? A share button would make more sense, but that's out of the scope of the PR. So hiding it is a good idea. > Should note retractions be handled any differently from a normal conversation? I would say the same as with message sending: Retract in the database but only send it if the chat is not a self chat. > Should I put up a screen recording for the current implementation and show the difference between a "normal" chat and a "note to self" chat? If it's not too much effort, I think that would be pretty cool!
ikjot-2605 added 2 commits 3 months ago
9d73fc3a94 feat(notes): Miscellaneous fixes - Review 1.
Signed-off-by: Ikjot Singh Dhody <ikjotsd@gmail.com>
a291d9ab07 feat(notes): Database migration-Conversation type.
Signed-off-by: Ikjot Singh Dhody <ikjotsd@gmail.com>
ikjot-2605 added 1 commit 3 months ago
4aacd36c59 feat(notes): Add switch case for helper.
Signed-off-by: Ikjot Singh Dhody <ikjotsd@gmail.com>
ikjot-2605 added 4 commits 3 months ago
e4523a2d33 feat(notes): Handle file upload for notes.
Signed-off-by: Ikjot Singh Dhody <ikjotsd@gmail.com>
f378c60bf5 feat(notes): Handle message/note retractions.
Signed-off-by: Ikjot Singh Dhody <ikjotsd@gmail.com>
d529974cd9 feat(notes): Hide forward option for notes.
Signed-off-by: Ikjot Singh Dhody <ikjotsd@gmail.com>
2a186377df feat(notes): Add edit note capability.
Signed-off-by: Ikjot Singh Dhody <ikjotsd@gmail.com>
Poster

@PapaTutuWawa, I've made all the changes that you suggested, including the database migration and the sending files(emulate) feature for notes. Please take a look when possible.

Attached is a screen recording of the functioning of the notes feature. Please take a look whenever convenient.

Thanks :)

@PapaTutuWawa, I've made all the changes that you suggested, including the database migration and the sending files(emulate) feature for notes. Please take a look when possible. Attached is a screen recording of the functioning of the notes feature. Please take a look whenever convenient. Thanks :)
ikjot-2605 reviewed 3 months ago
@ -156,3 +156,3 @@
return FileChatBaseWidget(
message,
message.isFileUploadNotification
message.conversationJid == ''
Poster

Hey @PapaTutuWawa, I'm concerned about the cleanliness of this change because I had to add a check in the UI instead of handling it within the XMPP service. The reason for this is that I couldn't differentiate between different file types like 'file', 'audio', and 'photo' in the sendFiles function of the XMPP service. Do you have any suggestions to improve this or is it acceptable?

Additionally, I had to make this change because the srcUrl is null for files in "Note" conversations, which causes a Null Object error. To address this issue, I simply used the filename as a substitute, since I can't set isFileUploadNotification as true for "Note" conversations without breaking Photo/Audio file notes.

Hey @PapaTutuWawa, I'm concerned about the cleanliness of this change because I had to add a check in the UI instead of handling it within the XMPP service. The reason for this is that I couldn't differentiate between different file types like 'file', 'audio', and 'photo' in the sendFiles function of the XMPP service. Do you have any suggestions to improve this or is it acceptable? Additionally, I had to make this change because the srcUrl is null for files in "Note" conversations, which causes a Null Object error. To address this issue, I simply used the filename as a substitute, since I can't set isFileUploadNotification as true for "Note" conversations without breaking Photo/Audio file notes.
Owner

I'm sorry, but I don't understand why you need to differentiate file types in the XMPP server.

The issue with srcUrl is something I know of and I plan to address it in #257.

I'm sorry, but I don't understand why you need to differentiate file types in the XMPP server. The issue with `srcUrl` is something I know of and I plan to address it in #257.
Owner

I just had a look at the code and I think the best course of action would be to just always set the message's filename attribute and use that. If we have the URL, extract the filename. If we have the filename (via SFS), use that. I'll do that in the coming days.

I just had a look at the code and I think the best course of action would be to just always set the message's `filename` attribute and use that. If we have the URL, extract the filename. If we have the filename (via SFS), use that. I'll do that in the coming days.
Poster

@PapaTutuWawa, just to make sure I understand the issue - the message.srcUrl is null, so the null check operator is failing. Assuming my understanding to be correct, should I make the changes assuming the filename attribute will always be set appropriately(for now)? That way, when those changes have been made, I can pull those and this issue won't come up anymore.

@PapaTutuWawa, just to make sure I understand the issue - the message.srcUrl is null, so the null check operator is failing. Assuming my understanding to be correct, should I make the changes assuming the `filename` attribute will always be set appropriately(for now)? That way, when those changes have been made, I can pull those and this issue won't come up anymore.
Owner

Yes. It turns out that we already set the filename attribute whenever we receive a file, it's just mostly the UI being angry for not having a source. I fixed that in 7f41ec2aac.

Yes. It turns out that we already set the `filename` attribute whenever we receive a file, it's just mostly the UI being angry for not having a source. I fixed that in 7f41ec2aac44698a0bca148ce30dcd337cbb0025.
ikjot-2605 marked this conversation as resolved
PapaTutuWawa requested changes 3 months ago
PapaTutuWawa left a comment
Owner

Looks good. Only minor issues left.

Looks good. Only minor issues left.
@ -0,0 +4,4 @@
Future<void> upgradeFromV30ToV31(Database db) async {
await db.execute(
'ALTER TABLE $conversationsTable ADD COLUMN type TEXT NOT NULL DEFAULT "chat";');
Owner

There's a trailing comma missing here.

There's a trailing comma missing here.
ikjot-2605 marked this conversation as resolved
@ -858,2 +855,2 @@
command.originId,
t.messages.retractedFallback,
if (command.conversationJid != '') {
(GetIt.I.get<XmppConnection>().getManagerById(messageManager)!
Owner

Could you change getManagerById(...) as MessageManager to getManagerById<MessageManager>(messageManager)!? My mistake.

Could you change `getManagerById(...) as MessageManager` to `getManagerById<MessageManager>(messageManager)!`? My mistake.
ikjot-2605 marked this conversation as resolved
ikjot-2605 added 1 commit 2 months ago
ac5fc38de6 feat(notes): Formatting changes.
Update file.dart to work when unified filename is used.

Signed-off-by: Ikjot Singh Dhody <ikjotsd@gmail.com>
ikjot-2605 changed title from WIP: Note to self feature addition to Note to self feature addition 2 months ago
ikjot-2605 force-pushed feature_note_to_self from 955a44fec3 to ac5fc38de6 2 months ago
ikjot-2605 added 1 commit 2 months ago
ikjot-2605 added 1 commit 2 months ago
1c1b598768 feat(notes): Fix quoted file widget.
Signed-off-by: Ikjot Singh Dhody <ikjotsd@gmail.com>
ikjot-2605 reviewed 2 months ago
@ -25,3 +25,3 @@
borderRadius: 8,
),
filenameFromUrl(message.srcUrl!),
message.filename!,
Poster

@PapaTutuWawa hope this change is fine, it was giving a null check issue due to the srcUrl, but I've fixed it. This change seemed like a consistent one based on your latest commit.

@PapaTutuWawa hope this change is fine, it was giving a null check issue due to the srcUrl, but I've fixed it. This change seemed like a consistent one based on your latest commit.
Owner

Oops, guess I missed that one. Thanks! I added that in 25caf3f4a6, so it's fine.

Oops, guess I missed that one. Thanks! I added that in 25caf3f4a64a5ca6fec3f5a2ef1177fbcf8b93e6, so it's fine.
PapaTutuWawa marked this conversation as resolved
Poster

Hello @PapaTutuWawa, I have retested the feature, and it seems to be functioning correctly now. However, I noticed an issue on the current master branch. When I send a file to my own JID, the received file appears to be continuously downloading, and the build method for the FileChatWidget keeps returning the _buildDownloading widget indefinitely.

Can you please confirm if this issue is specific to my system or if it's a bug in the code? If it is indeed a bug that needs to be fixed, maybe we could open a new issue for the same and handle it in a separate PR?

I apologize if this wasn't the right place to report a potential bug 😅.

Hello @PapaTutuWawa, I have retested the feature, and it seems to be functioning correctly now. However, I noticed an issue on the current `master` branch. When I send a file to my own JID, the received file appears to be continuously downloading, and the build method for the `FileChatWidget` keeps returning the `_buildDownloading` widget indefinitely. Can you please confirm if this issue is specific to my system or if it's a bug in the code? If it is indeed a bug that needs to be fixed, maybe we could open a new issue for the same and handle it in a separate PR? I apologize if this wasn't the right place to report a potential bug 😅.
Owner

I just tested your code locally and have some comments:

  • Chat states are sent when typing inside the self-chat
  • It seems like the file upload slots are requested for files sent in the self-chat
  • It would be nice if the messages were immediately marked as read and encrypted in the self-chat.
  • When going into the profile view, there is an issue with the shared media list. That's on me, though, so don't worry about it.

I also cannot reproduce the issue with files always downloading. To you mean that you send the message to your own bare JID or do you mean the self-chat?

I just tested your code locally and have some comments: - Chat states are sent when typing inside the self-chat - It seems like the file upload slots are requested for files sent in the self-chat - It would be nice if the messages were immediately marked as read and encrypted in the self-chat. - When going into the profile view, there is an issue with the shared media list. That's on me, though, so don't worry about it. I also cannot reproduce the issue with files always downloading. To you mean that you send the message to your own bare JID or do you mean the self-chat?
Poster

I also cannot reproduce the issue with files always downloading. To you mean that you send the message to your own bare JID or do you mean the self-chat?

@PapaTutuWawa, here I meant sending a file to my own bare JID. I don't see the issue anymore - it was probably an issue with my local config/network. Apologies for the inconvenience. :/

> I also cannot reproduce the issue with files always downloading. To you mean that you send the message to your own bare JID or do you mean the self-chat? @PapaTutuWawa, here I meant sending a file to my own bare JID. I don't see the issue anymore - it was probably an issue with my local config/network. Apologies for the inconvenience. :/
ikjot-2605 added 1 commit 2 months ago
ikjot-2605 added 2 commits 2 months ago
99257f4b28 feat(notes): Disable Chat State/File Upload-Notes
Signed-off-by: Ikjot Singh Dhody <ikjotsd@gmail.com>
8ef62e7ff1 feat(notes): Read/Encrypted Markers for Notes.
Signed-off-by: Ikjot Singh Dhody <ikjotsd@gmail.com>
Poster

I just tested your code locally and have some comments:

  • Chat states are sent when typing inside the self-chat
  • It seems like the file upload slots are requested for files sent in the self-chat
  • It would be nice if the messages were immediately marked as read and encrypted in the self-chat.
  • When going into the profile view, there is an issue with the shared media list. That's on me, though, so don't worry about it.

@PapaTutuWawa I've addressed the first 3 issues in my 2 latest commits. Please take a look whenever convenient. I'm unsure if this is the best way to update the "read" marker for notes. Please let me know if you have any suggestions on improvements.

Thanks 😄

> I just tested your code locally and have some comments: > - Chat states are sent when typing inside the self-chat > - It seems like the file upload slots are requested for files sent in the self-chat > - It would be nice if the messages were immediately marked as read and encrypted in the self-chat. > - When going into the profile view, there is an issue with the shared media list. That's on me, though, so don't worry about it. @PapaTutuWawa I've addressed the first 3 issues in my 2 latest commits. Please take a look whenever convenient. I'm unsure if [this](https://codeberg.org/moxxy/moxxy/commit/8ef62e7ff188989cfce7f2e94928ddd88f81ed3c) is the best way to update the "read" marker for notes. Please let me know if you have any suggestions on improvements. Thanks 😄
PapaTutuWawa requested changes 2 months ago
PapaTutuWawa left a comment
Owner

Looks good, only some minor issues left to fix.

Looks good, only some minor issues left to fix.
@ -225,3 +239,3 @@
sid,
false,
c.encrypted,
recipient == '' ? true : c.encrypted,
Owner

If we do that here, then we might as well set the read and displayed markers here if the conversation is of type note.

If we do that here, then we might as well set the read and displayed markers here if the conversation is of type `note`.
Poster

makes sense.

makes sense.
ikjot-2605 marked this conversation as resolved
@ -292,0 +305,4 @@
),
);
} else if (conversation!.type == ConversationType.note) {
await updateChatMarkersForNotes(message!);
Owner

See above.

See above.
ikjot-2605 marked this conversation as resolved
@ -538,3 +556,3 @@
conn.generateId(),
false,
encrypt[recipient]!,
conversation?.type == ConversationType.note
Owner

Same as above.

Same as above.
ikjot-2605 marked this conversation as resolved
@ -693,3 +710,1 @@
thumbnails[path] ?? [],
),
);
if (recipients.contains('')) {
Owner

I think we can just unconditionally remove the empty string. Dart does not throw an exception when you try to remove an element that does not exist.

I think we can just unconditionally remove the empty string. Dart does not throw an exception when you try to remove an element that does not exist.
ikjot-2605 marked this conversation as resolved
@ -242,1 +242,3 @@
MessageBubbleBottom(widget.message, widget.sent),
MessageBubbleBottom(
widget.message,
widget.message.conversationJid == '' ? true : widget.sent,
Owner

Is there a reason for this? The sent attribute should be accurate, even in the self-chat. Unless you found a bug. The same comment applies for all changes of the chat widgets.

Is there a reason for this? The `sent` attribute should be accurate, even in the self-chat. Unless you found a bug. The same comment applies for all changes of the chat widgets.
Poster

This was my bad, thought that this change was required, but I had misinterpreted it's requirement. Apologies.

This was my bad, thought that this change was required, but I had misinterpreted it's requirement. Apologies.
ikjot-2605 marked this conversation as resolved
@ -147,6 +150,7 @@ class ImageChatWidget extends StatelessWidget {
// TODO(PapaTutuWawa): Maybe use an async builder
if (message.mediaUrl != null && File(message.mediaUrl!).existsSync()) {
print("HAHAHHA");
Owner

This should not be part of your PR 😄

This should not be part of your PR 😄
Poster

Oops, forgot to remove this debug print statement. 😆

Oops, forgot to remove this debug print statement. 😆
ikjot-2605 marked this conversation as resolved
ikjot-2605 added 1 commit 2 months ago
398c23fccb feat(notes): Remove unneccessary changes.
Remove unneccessary "sent" changes in file widgets.

Signed-off-by: Ikjot Singh Dhody <ikjotsd@gmail.com>
ikjot-2605 added 1 commit 2 months ago
0cf237914b feat(notes): Update read marker cleaner for notes.
Signed-off-by: Ikjot Singh Dhody <ikjotsd@gmail.com>
Poster

Looks good, only some minor issues left to fix.

@PapaTutuWawa, thanks for the insightful review, I've made the suggested changes. Please review whenever convenient. :)

> Looks good, only some minor issues left to fix. @PapaTutuWawa, thanks for the insightful review, I've made the suggested changes. Please review whenever convenient. :)
PapaTutuWawa requested changes 2 months ago
PapaTutuWawa left a comment
Owner

Minor nitpicks. Otherwise, this looks ready to merge.

Minor nitpicks. Otherwise, this looks ready to merge.
@ -225,3 +228,3 @@
sid,
false,
c.encrypted,
recipient == '' ? true : c.encrypted,
Owner

See below.

See below.
ikjot-2605 marked this conversation as resolved
@ -233,6 +236,8 @@ class XmppService {
stickerHashKey: sticker?.hashKey,
srcUrl: sticker?.urlSources.first,
mediaType: sticker?.mediaType,
received: recipient == '' ? true : false,
Owner

As we already have the conversation, I think just checking if c.type == ConversationType.note is a bit cleaner.

As we already have the conversation, I think just checking if `c.type == ConversationType.note` is a bit cleaner.
ikjot-2605 marked this conversation as resolved
@ -234,2 +237,4 @@
srcUrl: sticker?.urlSources.first,
mediaType: sticker?.mediaType,
received: recipient == '' ? true : false,
displayed: recipient == '' ? true : false,
Owner

See above.

See above.
ikjot-2605 marked this conversation as resolved
@ -155,3 +158,3 @@
radius,
maxWidth,
sent,
message.conversationJid == '' ? true : sent,
Owner

Is there a reason for this check?

Is there a reason for this check?
ikjot-2605 marked this conversation as resolved
ikjot-2605 added 1 commit 2 months ago
02e73ade5e feat(notes): Utilize converation object for notes.
Signed-off-by: Ikjot Singh Dhody <ikjotsd@gmail.com>
PapaTutuWawa requested changes 2 months ago
PapaTutuWawa left a comment
Owner

Last minor nitpicks. Looks really good otherwise!

Last minor nitpicks. Looks really good otherwise!
@ -550,0 +558,4 @@
isUploading:
conversation?.type != ConversationType.note ? true : false,
received: recipient == '' ? true : false,
displayed: recipient == '' ? true : false,
Owner

I would change to two recipient == '' to conversation?.type == ConversationType.note to make it consistent with the isUploading assignment.

I would change to two `recipient == ''` to `conversation?.type == ConversationType.note` to make it consistent with the `isUploading` assignment.
ikjot-2605 marked this conversation as resolved
@ -696,3 +712,4 @@
}
}
_log.finest('File upload submitted');
Owner

Could we move this into the if-clause? It confused me while testing to see "File upload submitted" when I have not seen a upload slot request.

Could we move this into the if-clause? It confused me while testing to see "File upload submitted" when I have not seen a upload slot request.
ikjot-2605 marked this conversation as resolved
ikjot-2605 added 1 commit 2 months ago
a86d83eeba feat(notes): Fix upload logger.
Consistent method for checking note vs chat conversation.

Signed-off-by: Ikjot Singh Dhody <ikjotsd@gmail.com>
Owner

Looks good to me. I will test this tomorrow and then merge it. Thank you very much!

Looks good to me. I will test this tomorrow and then merge it. Thank you very much!
Poster

Looks good to me. I will test this tomorrow and then merge it. Thank you very much!

Awesome! It was a pleasure working on it. Please do let me know if any further changes would be suitable whenever convenient.

Thanks 😃

> Looks good to me. I will test this tomorrow and then merge it. Thank you very much! Awesome! It was a pleasure working on it. Please do let me know if any further changes would be suitable whenever convenient. Thanks 😃
Owner

I found a small issue while testing: When you react to a message in the self chat (or remove the reaction), then a message is sent.

I found a small issue while testing: When you react to a message in the self chat (or remove the reaction), then a message is sent.
ikjot-2605 added 1 commit 2 months ago
a78db354ab feat(notes): Handle reactions correctly for notes.
Signed-off-by: Ikjot Singh Dhody <ikjotsd@gmail.com>
Poster

I found a small issue while testing: When you react to a message in the self chat (or remove the reaction), then a message is sent.

@PapaTutuWawa
Apologies, missed this previously. I've fixed the same now.

> I found a small issue while testing: When you react to a message in the self chat (or remove the reaction), then a message is sent. @PapaTutuWawa Apologies, missed this previously. I've fixed the same now.
Owner

Looks good. Thank you very much for the contribution!

Looks good. Thank you very much for the contribution!
PapaTutuWawa merged commit fd1e14e4cd into master 2 months ago

Reviewers

PapaTutuWawa requested changes 2 months ago
The pull request has been merged as fd1e14e4cd.
Sign in to join this conversation.
No reviewers
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

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