Note to self feature addition
#256
Merged
PapaTutuWawa
merged 28 commits from :feature_note_to_self
into master
2 months ago
Loading…
Reference in New Issue
There is no content yet.
Delete Branch ':feature_note_to_self'
Deleting a branch is permanent. It CANNOT be undone. Continue?
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:Conversation
exists, it will create one and redirect the user to the conversation page.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?
JID
for a "note to self" conversation.Conversation
model calledtype
, which will differentiate between a:XMPP
services to be used only if the conversation is of the 1:1 peer chat. Keep all interactions of theNote
type conversations limited to the localsqflite
database.A simple demo of the current implementation is attached below.
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.
@ -2,2 +2,4 @@
android.useAndroidX=true
android.enableJetifier=true
org.gradle.java.home=/usr/lib/jvm/java-17-openjdk-amd64
This should not be part of the PR.
Understood, will remove.
@ -283,0 +290,4 @@
child: const Icon(Icons.notes),
onTap: () {
context.read<NewConversationBloc>().add(
NewConversationAddedEvent(
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.Done.
@PapaTutuWawa, I have added the basic functionalities to this PR. As of now, the Note to Self conversation:
jid=''
for notes.sqflite
database.ConversationPage
.ConversationTopBar
.PopupMenuButton
, leaving only the Close Chat option there.Some questions I have regarding further requirements:
Note
type conversation?Forward
option in a Note message(even though it hasn't been implemented yet)?Thanks 😃
@ -481,2 +481,3 @@
backgroundColor: const Color.fromRGBO(0, 0, 0, 0),
appBar: const ConversationTopbar(),
appBar: ConversationTopbar(
isNoteConversation: widget.conversationJid == '',
@PapaTutuWawa, would it be cleaner(for future conversation types, eg: group chat) if I add a
conversationType
argument instead of thisisNoteConversation
argument?Yes, I think that would be cleaner.
Looks good to me. I do have some requests, however.
Also, there are some things that should also be done:
@ -1,3 +1,3 @@
org.gradle.jvmargs=-Xmx1536M
android.useAndroidX=true
android.enableJetifier=true
android.enableJetifier=true
I think you accidentally removed a newline. Not sure how this happened. However, it's not a big issue.
Somehow, I am unable to see this change in the PR diff. 🤔
@ -135,6 +135,7 @@
"conversations": {
"speeddialNewChat": "New chat",
"speeddialJoinGroupchat": "Join groupchat",
"speeddialAddNoteToSelf": "Note to Self",
I'm not sure if the "S" in "Self" should be capitalized.
@ -135,6 +135,7 @@
"conversations": {
"speeddialNewChat": "Neuer chat",
"speeddialJoinGroupchat": "Gruppenchat beitreten",
"speeddialAddNoteToSelf": "Notiz an mich selbst",
I would just say "Notiz an mich". It's a bit shorter and essentially says the same thing.
@ -8,2 +10,4 @@
String intToString(int i) => '$i';
int stringToInt(String s) => int.parse(s);
String conversationTypeToString(ConversationType type) =>
I think these should be made a switch statement. That makes it a tiny bit easier when we add another chat type.
@ -490,2 +492,3 @@
builder: (context, state) {
if (state.conversation?.inRoster ?? false) {
if ((state.conversation?.inRoster ?? false) ||
state.conversation?.jid == '') {
I think that checking the
state.conversation?.type
attribute would be a cleaner choice.@ -44,3 +44,2 @@
implements PreferredSizeWidget {
const ConversationTopbar({super.key});
const ConversationTopbar({required this.isNoteConversation, super.key});
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.@ -95,2 +92,2 @@
item.avatarUrl,
),
NewConversationAddedEvent(item.jid, item.title,
item.avatarUrl, ConversationType.chat),
I think there is a comma missing after the last argument.
@ -114,6 +111,9 @@ class NewConversationPage extends StatelessWidget {
item.avatarUrl,
item.jid,
0,
item.jid == ''
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 don't see a reason why it should not be allowed.
A share button would make more sense, but that's out of the scope of the PR. So hiding it is a good idea.
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.
If it's not too much effort, I think that would be pretty cool!
@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 :)
@ -156,3 +156,3 @@
return FileChatBaseWidget(
message,
message.isFileUploadNotification
message.conversationJid == ''
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.
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 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.@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.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 in7f41ec2aac
.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";');
There's a trailing comma missing here.
@ -858,2 +855,2 @@
command.originId,
t.messages.retractedFallback,
if (command.conversationJid != '') {
(GetIt.I.get<XmppConnection>().getManagerById(messageManager)!
Could you change
getManagerById(...) as MessageManager
togetManagerById<MessageManager>(messageManager)!
? My mistake.WIP: Note to self feature additionto Note to self feature addition 2 months ago955a44fec3
toac5fc38de6
2 months ago@ -25,3 +25,3 @@
borderRadius: 8,
),
filenameFromUrl(message.srcUrl!),
message.filename!,
@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.
Oops, guess I missed that one. Thanks! I added that in
25caf3f4a6
, so it's fine.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 theFileChatWidget
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 😅.
I just tested your code locally and have some comments:
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. :/
@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 😄
Looks good, only some minor issues left to fix.
@ -225,3 +239,3 @@
sid,
false,
c.encrypted,
recipient == '' ? true : c.encrypted,
If we do that here, then we might as well set the read and displayed markers here if the conversation is of type
note
.makes sense.
@ -292,0 +305,4 @@
),
);
} else if (conversation!.type == ConversationType.note) {
await updateChatMarkersForNotes(message!);
See above.
@ -538,3 +556,3 @@
conn.generateId(),
false,
encrypt[recipient]!,
conversation?.type == ConversationType.note
Same as above.
@ -693,3 +710,1 @@
thumbnails[path] ?? [],
),
);
if (recipients.contains('')) {
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.
@ -242,1 +242,3 @@
MessageBubbleBottom(widget.message, widget.sent),
MessageBubbleBottom(
widget.message,
widget.message.conversationJid == '' ? true : widget.sent,
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.This was my bad, thought that this change was required, but I had misinterpreted it's requirement. Apologies.
@ -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");
This should not be part of your PR 😄
Oops, forgot to remove this debug print statement. 😆
@PapaTutuWawa, thanks for the insightful review, I've made the suggested changes. Please review whenever convenient. :)
Minor nitpicks. Otherwise, this looks ready to merge.
@ -225,3 +228,3 @@
sid,
false,
c.encrypted,
recipient == '' ? true : c.encrypted,
See below.
@ -233,6 +236,8 @@ class XmppService {
stickerHashKey: sticker?.hashKey,
srcUrl: sticker?.urlSources.first,
mediaType: sticker?.mediaType,
received: recipient == '' ? true : false,
As we already have the conversation, I think just checking if
c.type == ConversationType.note
is a bit cleaner.@ -234,2 +237,4 @@
srcUrl: sticker?.urlSources.first,
mediaType: sticker?.mediaType,
received: recipient == '' ? true : false,
displayed: recipient == '' ? true : false,
See above.
@ -155,3 +158,3 @@
radius,
maxWidth,
sent,
message.conversationJid == '' ? true : sent,
Is there a reason for this check?
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,
I would change to two
recipient == ''
toconversation?.type == ConversationType.note
to make it consistent with theisUploading
assignment.@ -696,3 +712,4 @@
}
}
_log.finest('File upload submitted');
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.
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 😃
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.
Looks good. Thank you very much for the contribution!
fd1e14e4cd
into master 2 months agoReviewers
fd1e14e4cd
.