Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mam in progress #59

Merged
merged 20 commits into from
Sep 29, 2024
Merged

mam in progress #59

merged 20 commits into from
Sep 29, 2024

Conversation

liuch
Copy link
Contributor

@liuch liuch commented Apr 12, 2019

No description provided.

src/xmpp/xmpp-im/types.cpp Outdated Show resolved Hide resolved
src/xmpp/xmpp-im/xmpp_carbons.cpp Outdated Show resolved Hide resolved
src/xmpp/xmpp-im/xmpp_tasks.cpp Outdated Show resolved Hide resolved
src/xmpp/xmpp-im/xmpp_carbons.cpp Outdated Show resolved Hide resolved
src/xmpp/xmpp-im/xmpp_carbons.cpp Outdated Show resolved Hide resolved
src/xmpp/xmpp-im/xmpp_carbons.cpp Outdated Show resolved Hide resolved
src/xmpp/xmpp-im/xmpp_carbons.cpp Show resolved Hide resolved
src/xmpp/xmpp-im/xmpp_carbons.h Outdated Show resolved Hide resolved
src/xmpp/xmpp-im/xmpp_tasks.cpp Outdated Show resolved Hide resolved
src/xmpp/xmpp-im/xmpp_tasks.cpp Outdated Show resolved Hide resolved
src/xmpp/xmpp-im/xmpp_tasks.cpp Outdated Show resolved Hide resolved
src/xmpp/xmpp-im/xmpp_tasks.cpp Outdated Show resolved Hide resolved
src/xmpp/xmpp-im/xmpp_tasks.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@pasnox pasnox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nearly all QLatin1String usages are plain wrong here.
QLatin1String make sense only if the api you will use has an overload taking a QLatin1String in plus to a QString overload.
If there is only QString parameters and you know you only deal with latin 1 content then just use QString::fromLatin1() then.

src/xmpp/xmpp-im/types.cpp Outdated Show resolved Hide resolved
src/xmpp/xmpp-im/types.cpp Outdated Show resolved Hide resolved
src/xmpp/xmpp-im/types.cpp Outdated Show resolved Hide resolved
src/xmpp/xmpp-im/types.cpp Outdated Show resolved Hide resolved
QDomElement e = s.createElement("urn:xmpp:carbons:2","private");
s.appendChild(e);
if (d->carbonsPrivate || d->wasEncrypted) {
s.appendChild(CarbonsManager::privateElement(stream->doc()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to xep 280, a private stanza also need a <no-copy/> node.

disable.setAttribute(QLatin1String("xmlns"), xmlns_carbons);
iq.appendChild(disable);
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is clearly rooms to create an helper taking a bool enabled parameter to change the enable/disable element name.

src/xmpp/xmpp-im/xmpp_carbons.cpp Outdated Show resolved Hide resolved
bool JT_MessageCarbons::take(const QDomElement &e)
{
if (iqVerify(e, Jid(), id())) {
if (e.attribute(QLatin1String("type")) != QLatin1String("result"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no QLatin1String variant for QDN::attribute() so it's waonas you domore allocations.
Use QString::fromLatin1("...")

if (type_ == ForwardedNone || !msg_)
return QDomElement();

QDomElement e = stream->doc().createElement(QLatin1String("forwarded"));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no QDomNode::createElement() taking a QLatin1String, so it's useless as you make 2 allocations, one for QLatin1String and one for QString, just use QString::fromLatin1("...") then.

@Neustradamus
Copy link
Contributor

It will be merged one day?

@Neustradamus
Copy link
Contributor

I think it will be nice to add this in the Google Summer of Code 2023:

@Neustradamus
Copy link
Contributor

Ri0n and others added 3 commits July 14, 2024 01:25
* move here from the mam branch, fix filtering, semantics, logic, etc

* add id filter

* actually add the id filtering logic

* some code cleanup

* add better error handling

* add `xmpp_mamtask` to cmake and fix compilation errors

* add mam manager (untested, compiles)

* add ability to get last messages from archive, fix some naming

* add ability to get messages before id

* make the manager just return the activated MAM task instead of handling the result

* fix archive behavior and return value for mam manager

---------

Co-authored-by: mcneb10 <[email protected]>
@Neustradamus
Copy link
Contributor

@Ri0n: What is missing to have the feature now? After the good work from @mcneb10?

@Ri0n Ri0n merged commit 35cd90c into master Sep 29, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants