Mam improvements #134

ron282 · 2022-05-27T08:03:13Z

No description provided.

codecov · 2022-05-27T08:38:22Z

Codecov Report

Merging #134 (edd533b) into master (862d56d) will decrease coverage by 2.05%.
The diff coverage is 35.11%.

@@            Coverage Diff             @@
##           master     #134      +/-   ##
==========================================
- Coverage   27.96%   25.91%   -2.06%     
==========================================
  Files          71       73       +2     
  Lines        4051     4063      +12     
==========================================
- Hits         1133     1053      -80     
- Misses       2918     3010      +92

Impacted Files	Coverage Δ
source/base/MessageHandler.h	`0.00% <ø> (ø)`
source/base/Settings.cpp	`28.43% <0.00%> (-0.84%)`	⬇️
source/base/Settings.h	`100.00% <ø> (ø)`
source/contacts/RosterController.cpp	`44.39% <0.00%> (-6.67%)`	⬇️
source/room/MucCollection.cpp	`0.00% <0.00%> (-75.00%)`	⬇️
source/xep/mam/MamManager.cpp	`10.93% <0.00%> (-9.24%)`	⬇️
source/xep/mam/MamRequest.h	`0.00% <0.00%> (ø)`
source/xep/omemo/lurch/lurch.c	`0.00% <0.00%> (ø)`
source/room/MucManager.cpp	`27.35% <34.61%> (-36.78%)`	⬇️
source/base/MessageHandler.cpp	`52.97% <56.14%> (+2.24%)`	⬆️
... and 9 more

lgtm-com · 2022-05-27T10:41:09Z

This pull request introduces 1 alert and fixes 2 when merging 31ec272 into 50ef274 - view on LGTM.com

new alerts:

1 for FIXME comment

fixed alerts:

2 for FIXME comment

ron282 · 2022-06-11T10:15:05Z

Hi @geobra . Did you have the opportunity to look at my pull requests? I have another one after these ones related to ui improvements and ability to choose attachments to download

geobra · 2022-06-22T19:56:48Z

Hi @ron282. Sorry, was in Holiday. Will try to do the review at the weekend.

geobra · 2022-06-25T19:31:30Z

I have to dig more into your code.

From a functional point of view, the 'hard' stuff of requesting the MaM messages and to catch them inside the MessageHandler seems to work good. But I only get the MaM messages in the debug xml window. It did not get passed to the database.

On my side, if I send a message to a group and go online 2 minutes later with shmoose, this line always discards the message:
if(timestamp > 0 && timestamp <= start)

timestamp is always bigger then start. And start is set to -14 days.
I have to recalculate this values to get a clue what went wrong here.

Does it work on your side?

geobra · 2022-06-26T09:45:29Z

Okay, 'start' is always 'now' on my system, instead of -14 days.
'delay' is a diff, but should be an absolute time value. I use this code to get the 'timestamp':

    auto stamp = delay->getStamp();
    std::tm time_tm = to_tm(stamp);
    timestamp = mktime(&time_tm);

With this changes
persistence_->addMessage(...)
is called.

But the message still did not show up in the gui. Need to digg more...

ron282 · 2022-06-26T14:04:09Z

Thanks for your feedback. I had a look. I don't have the issue. I'm testing directly on a phone (Xperia 10)

geobra · 2022-06-27T19:16:07Z

source/base/Settings.cpp

+    QDateTime returnValue = QDateTime::currentDateTimeUtc().addDays(-15);
+    QSettings settings;
+    QString jid=getJid();


This must be the jid of the other message partner or the group jid. As a result, there will be one entry in the settings file for each contact.

setLatestMamSyncDate is called after a Mam IQ Fin message is received. On the next request of the next group to catch MaM messages, the start-time is 'now'. If there are multiple groups, only the first group is requested with the correct last date. The next groups are requested with a start date of 'now'.

geobra · 2022-06-27T19:43:51Z

source/base/MessageHandler.cpp

+    Swift::Forwarded::ref forwarded = nullptr;
+    Swift::Message::ref forwardedMessage = nullptr;
+    qint64 timestamp = 0;
+    QString partyJid = QString::fromStdString(message->getFrom().toBare().toString());


partyJid is now set several times. And on my MaM messages, it get reset in different locations. Finally, the message is pushed into the db as a 1to1 messages with the group member, instead of a group messages. I will have a look at it and will make a proposal for a handleMessageReceived method. Please give me some time.

geobra · 2022-06-30T19:54:39Z

Have something working with some cheating. Had to clean it up now again... ;-).

ron282 · 2022-07-02T14:49:12Z

The muc archived msg must have a stanza-id with a by attribute which gives the bare JID of the account. Could it be a solution?

ron282 · 2022-07-03T16:13:07Z

I don't see the include-groupchat parameter in the archive request. This could be also an explanation why you don't see them.

geobra · 2022-07-03T19:02:11Z

There is an include-groupchat parameter in the MaM request? Interesting. The current implementation works without that. Do you have a link to read about that?

ron282 · 2022-07-03T21:45:13Z

https://xmpp.org/extensions/xep-0313.html. Example 12

geobra · 2022-07-05T20:20:50Z

Ok, I see. Thank you for the link.

The implementation get's to complex to test all cases manually. I started to setup a unit-test for the handleMessageReceived method. I will share all the code as soon as the first test succeeds.

geobra · 2022-07-05T20:44:08Z

First simple test is running. Have a look at mam_improvments branch at my git repo. Basic stuff and mam handling is working here. Test must be filled with all different kinds of msg'es which gets handled. Needs some time ;-).

geobra · 2022-07-05T20:44:52Z

You are welcome to add more tests or cherry-pick some changes and add to your branch for testing.

ron282 · 2022-07-05T20:48:00Z

I don't think the inlcude-groupchat has anything to do with your issue. I will have a look at your implementation. Are you testing with private groups or public ones?

ron282 · 2022-07-05T20:53:03Z

I have also a couple of additionnal improvements waiting for pull requests: omemo and muc, ui improvements for contact view and for message view.

geobra · 2022-07-06T19:34:43Z

Currently the test is just set up and test plain 1to1 msg. Group message test will be implemented next. And it should not matter if it is a private or a public group chat.

Are your other improvements independent from MaM? You are welcome to make another PR for other stuff.

lgtm-com · 2022-07-10T10:51:50Z

This pull request introduces 1 alert and fixes 2 when merging 9fd8b71 into 50ef274 - view on LGTM.com

new alerts:

1 for FIXME comment

fixed alerts:

2 for FIXME comment

lgtm-com · 2022-07-10T15:00:30Z

This pull request introduces 1 alert and fixes 2 when merging 0b9eaaa into 50ef274 - view on LGTM.com

new alerts:

1 for FIXME comment

fixed alerts:

2 for FIXME comment

lgtm-com · 2022-07-15T12:11:08Z

This pull request introduces 1 alert and fixes 2 when merging 0d67de0 into ebfc82c - view on LGTM.com

new alerts:

1 for FIXME comment

fixed alerts:

2 for FIXME comment

ron282 · 2022-07-15T13:26:32Z

This is a proposal to only sync mam with messages inserted since the last sync based on msgId and not based on date. There is one entry in the conf file per mam storage.

lgtm-com · 2022-07-15T14:56:54Z

This pull request introduces 1 alert and fixes 2 when merging cf67e61 into ebfc82c - view on LGTM.com

new alerts:

1 for FIXME comment

fixed alerts:

2 for FIXME comment

ron282 · 2022-07-15T21:09:35Z

I have some problems with the change in MessageHandler.cpp line 68 to convert delay into timestamp. mktime is using local time instead of utc time leading to incorrect time.

lgtm-com · 2022-07-15T23:35:00Z

This pull request introduces 1 alert and fixes 2 when merging 002c69e into ebfc82c - view on LGTM.com

new alerts:

1 for FIXME comment

fixed alerts:

2 for FIXME comment

lgtm-com · 2022-07-16T09:34:48Z

This pull request introduces 1 alert and fixes 2 when merging b2ae907 into ebfc82c - view on LGTM.com

new alerts:

1 for FIXME comment

fixed alerts:

2 for FIXME comment

geobra · 2022-07-16T19:40:00Z

You are right. I did no had the UTC issue in mind. Let's revert it to your implementation.

lgtm-com · 2022-07-17T10:04:05Z

This pull request introduces 1 alert and fixes 2 when merging 01710e3 into ebfc82c - view on LGTM.com

new alerts:

1 for FIXME comment

fixed alerts:

2 for FIXME comment

lgtm-com · 2022-07-18T17:46:15Z

This pull request introduces 1 alert and fixes 2 when merging bef0a71 into ebfc82c - view on LGTM.com

new alerts:

1 for FIXME comment

fixed alerts:

2 for FIXME comment

lgtm-com · 2022-07-19T12:29:59Z

This pull request introduces 2 alerts and fixes 2 when merging 92b0716 into ebfc82c - view on LGTM.com

new alerts:

1 for Resource not released in destructor
1 for FIXME comment

fixed alerts:

2 for FIXME comment

This reverts commit 7d88109.

lgtm-com · 2022-07-20T11:30:41Z

This pull request introduces 2 alerts and fixes 2 when merging a924fcf into ebfc82c - view on LGTM.com

new alerts:

1 for Resource not released in destructor
1 for FIXME comment

fixed alerts:

2 for FIXME comment

lgtm-com · 2022-08-07T20:57:34Z

This pull request introduces 2 alerts and fixes 2 when merging edd533b into 862d56d - view on LGTM.com

new alerts:

1 for Resource not released in destructor
1 for FIXME comment

fixed alerts:

2 for FIXME comment

geobra · 2022-09-03T21:12:31Z

Short update. I wrote more tests. Only a few tests are missing now and one critical FIXME. If these are resolved, we can resume discussions about a merge :-).

ron282 · 2022-09-06T17:54:59Z

Thanks. I will have a look at your tests. I'm ok to write some. I just need to get some examples.

carlosgonz0 · 2022-09-11T14:52:54Z

Hi friends, any nighly version to testing the new improvements?? i still can not using Shmoose because not compat with Conversations when using omemo.

Thank of advance.

geobra · 2022-09-12T13:14:48Z

Really, you have issues with omemo and Conversations? That works without any issues for me.
What is the problem at your side?

geobra · 2022-09-12T13:16:08Z

BTW, we did not make many changes on the omemo implementation. I do not expect that a nightly build will solve your issues.

geobra · 2022-09-22T18:09:02Z

@ron282 is it ok for you to postpone this until we have the qxmpp prove of concept?

ron282 · 2022-09-22T20:03:25Z

@geobra yes no pb to postpone

process mam msgs and std msgs in the same function

31ec272

ron282 force-pushed the mam_improvements branch from 0c39487 to 31ec272 Compare May 27, 2022 09:19

ron282 marked this pull request as ready for review May 27, 2022 17:25

geobra reviewed Jun 27, 2022

View reviewed changes

test mam improvments on ron's input

ee73613

more tests

9fd8b71

avoid core dump when attempting to decrypt group chat msgs

0b9eaaa

schorsch and others added 2 commits July 11, 2022 22:29

test mam msges

042aebd

avoid core dump when attempting to decrypt group chat msgs (geobra#137)

bd19b1b

ron282 added 9 commits July 20, 2022 11:57

fix issue when requesting incomplete mam request

41b8829

only request mam msgs from the last sync

55e23f5

fix issue on remove contact

6bf6dd5

dont send delivery receipt for mam msg

f43a27f

rollback on timestamp computation

788c965

get room name from caps

1965d1d

Revert "get room name from caps"

a68a226

This reverts commit 7d88109.

get room name from caps

5fc0342

public and private groups rework

a924fcf

ron282 force-pushed the mam_improvements branch from 92b0716 to a924fcf Compare July 20, 2022 09:59

merge with latest master devs

edd533b

Mam improvements #134

Are you sure you want to change the base?

Mam improvements #134

Conversation

ron282 commented May 27, 2022

codecov bot commented May 27, 2022 • edited Loading

Codecov Report

lgtm-com bot commented May 27, 2022

ron282 commented Jun 11, 2022

geobra commented Jun 22, 2022

geobra commented Jun 25, 2022

geobra commented Jun 26, 2022

ron282 commented Jun 26, 2022 • edited Loading

geobra Jun 27, 2022

Choose a reason for hiding this comment

geobra Jun 27, 2022

Choose a reason for hiding this comment

geobra commented Jun 30, 2022

ron282 commented Jul 2, 2022 • edited Loading

ron282 commented Jul 3, 2022

geobra commented Jul 3, 2022

ron282 commented Jul 3, 2022 • edited Loading

geobra commented Jul 5, 2022

geobra commented Jul 5, 2022

geobra commented Jul 5, 2022

ron282 commented Jul 5, 2022

ron282 commented Jul 5, 2022

geobra commented Jul 6, 2022

lgtm-com bot commented Jul 10, 2022

lgtm-com bot commented Jul 10, 2022

lgtm-com bot commented Jul 15, 2022

ron282 commented Jul 15, 2022

lgtm-com bot commented Jul 15, 2022

ron282 commented Jul 15, 2022

lgtm-com bot commented Jul 15, 2022

lgtm-com bot commented Jul 16, 2022

geobra commented Jul 16, 2022

lgtm-com bot commented Jul 17, 2022

lgtm-com bot commented Jul 18, 2022

lgtm-com bot commented Jul 19, 2022

lgtm-com bot commented Jul 20, 2022

lgtm-com bot commented Aug 7, 2022

geobra commented Sep 3, 2022

ron282 commented Sep 6, 2022

carlosgonz0 commented Sep 11, 2022

geobra commented Sep 12, 2022

geobra commented Sep 12, 2022

geobra commented Sep 22, 2022

ron282 commented Sep 22, 2022

codecov bot commented May 27, 2022 •

edited

Loading

ron282 commented Jun 26, 2022 •

edited

Loading

ron282 commented Jul 2, 2022 •

edited

Loading

ron282 commented Jul 3, 2022 •

edited

Loading