[messagingframework] Ignore Re: and Fwd: when sort by subject. Contributes to JB#6059 #1

bshmarin · 2021-06-21T17:07:24Z

Add sort key for sort by trimmed subject (without Re, Fwd, e.t.c prefixes).

okodron · 2021-06-21T20:10:29Z

Previous discussion is here: https://git.sailfishos.org/mer-core/messagingframework/merge_requests/58

llewelld

My SQL sorting skills are no great shakes, so I have a couple of queries. As I've also commented, it would be good to have a header added to the patch.

llewelld · 2021-06-28T14:14:03Z

rpm/0032-Ignore-Re-Fwd-when-sort-by-subject.patch

+                };
+                static const QString trimTemplate = QStringLiteral("replace(%1, '%2', '')");
+                for (const QLatin1String &prefix : prefixes)
+                    field = trimTemplate.arg(field, prefix);


I may be misreading this, but am I correct in thinking that this will remove all instances of "Re:" etc. from the string before sorting, independent of the position? So for example "HERE: a message" will be sorted as if it were "HE a message"?

Is there any way to have it only remove prefixed strings?

llewelld · 2021-06-28T14:14:36Z

rpm/0032-Ignore-Re-Fwd-when-sort-by-subject.patch

+                for (const QLatin1String &prefix : prefixes)
+                    field = trimTemplate.arg(field, prefix);
+            }
+            sortColumns.append(QLatin1String("ltrim(ltrim(") + field + QLatin1String(",'\\\"')) COLLATE NOCASE ") +


What's the purpose of having two nested ltrim statements? It looks like one may be redundant.

llewelld · 2021-06-28T14:17:09Z

rpm/0032-Ignore-Re-Fwd-when-sort-by-subject.patch

@@ -0,0 +1,77 @@
+diff --git a/src/libraries/qmfclient/qmailmessagesortkey.cpp b/src/libraries/qmfclient/qmailmessagesortkey.cpp


It would be helpful if you could include a header similar to the other patches here, so that they can be applied using git am. The easiest way to generate the appropriate patch file that I'm aware of is to commit your changes to a temporary branch and then use git format-patch -N HEAD~ to create the patch file.

chriadam · 2021-06-30T01:45:41Z

Also, can you please add a commit message in the patch file and in the commit itself, which clearly states that this patch should NOT be upstreamed to Qt QMF, due to OMP having no CLA with TQC? Just to prevent any possible mistakes, when we upstream patches in future.

pvuorela · 2021-11-05T15:35:35Z

rpm/0035-Add-TrimmedSubject-message-sort-key.patch

+ {
+-    QSqlQuery query(simpleQuery(QLatin1String("SELECT id FROM mailmessages"),
+    bool sortByTrimmedSubject = false;
+    for (const QMailMessageSortKey::ArgumentType &arg : sortKey.arguments())


Multiline for loop needs braces by qt coding style.

chriadam · 2021-11-17T03:41:33Z

rpm/0035-Add-TrimmedSubject-message-sort-key.patch

+     map.insert(QMailMessageSortKey::Sender, QMailMessageKey::Sender);
+     map.insert(QMailMessageSortKey::Recipients, QMailMessageKey::Recipients);
+     map.insert(QMailMessageSortKey::Subject, QMailMessageKey::Subject);
+    map.insert(QMailMessageSortKey::TrimmedSubject, QMailMessageKey::TimeStamp);


is Timestamp intentional? Can TrimmedSubject be added to QMailMessageKey?

is Timestamp intentional?

Yes - the order of the time is used at the SQL level when many equal subjects.

Can TrimmedSubject be added to QMailMessageKey?

For what?

Am I correct with the following: currently, your PR only supports using the trimmed subject for sorting; when performing a query from the database, the ORDER BY clause will use the Timestamp field in the query, and then the method which performs the query will then iterate over the (timestamp-sorted) results and then perform an additional sorting using the trimmed subject (via map insertion).

If so, there are a couple of potential issues:

specifying a DESC sort order will result in the timestamp ordering returned by the query being DESC order, but the additional sorting via map insertion will not be inverted.

sorting by the trimmed subject field will result in a potentially significant performance reduction, as the ordering cannot be done at the database query level, but is done in memory.

The existing QMF code basically uses that one mapping of sort key to mail message key to determine both the sort field and the filter field (at least, if I am reading the comment in messageSortMapInit() correctly), and for all other cases the mapping is defined such that the ORDER BY and SELECT fields are the same. Your PR breaks this assumption, because it defines that the ORDER BY field is Timestamp but handles SELECT in code rather than via expansion from the mapping.

Also, the word "trimmed" already has a well-defined meaning in Qt code (i.e. trimmed of whitespace). A better name might be simplifiedSubject or similar.

Conceptually, I would prefer if we can support this simplified subject more generally (that is, for both sorting and for filtering), without custom codepaths to handle this case specifically. Presumably, it would require modifying QMailMessageHeader, adding the appropriate keys (which would most likely map to the normal Subject field, with a flag for subject simplification, and then adding a method somewhere like QString simplifiedSubject(const QString &subj) const which is used every time we query the subject for these cases - there are probably only a couple of places, e.g. when querying headers, and when querying messages). Of course, this doesn't solve the performance hit, but that can be documented.

but the additional sorting via map insertion will not be inverted.

Agree.

potentially significant performance reduction

SQLite also works in memory, so the performance hit shouldn't be too noticeable, but by means of sqlite we will not be able to carry out the sorting we need without creating additional fields in the database.

...that is, for both sorting and for filtering...

I do not see any advantages of filtering by a simplified subject compared to a subject.

Also, the word "trimmed" already has a well-defined meaning in Qt code (i.e. trimmed of whitespace). A better name might be simplifiedSubject or similar.

May be originalSubject ?

Regarding originalSubject - I agree, that sounds good to me.

Regarding the advantages of supporting filtering as well as sorting - the only real advantage is that it keeps the code consistent for all keys. The current PR adds some cognitive load for future maintainers, because this specific key works differently to the rest.

Regarding the performance - ordinarily I would say: fair enough, better to avoid additional fields and complexity, so long as the performance overhead of current solution is very low or negligible. However, maybe this is related to the above point about "consistent codepaths for all keys" - it might be cognitively simpler to add them. I'm not sure.

@pvuorela @dcaliste I'd appreciate your thoughts here. I am not familiar enough with QMF to have a hard stance on what is best; that said, the unique semantics for this key type does seem suboptimal to me, from a maintenance perspective. A comprehensive commit message might help with that, I suppose.

Regarding originalSubject - I agree, that sounds good to me.

Changed.

chriadam · 2021-11-17T03:42:18Z

One comment, aside from that I would still like the commit message to include a warning that the patch should not be upstreamed due to lacking a CLA.

bshmarin · 2021-11-17T09:19:48Z

One comment, aside from that I would still like the commit message to include a warning that the patch should not be upstreamed due to lacking a CLA.

I'm waiting...

Added.

…subject without Re: and Fwd: prefixes. Contributes to JB#6059 !!! WARNING !!! Don't upstream this patch to the QTC due to lacking a CLA.

krnlyng · 2021-11-22T15:49:29Z

This doesn't account for languages it seems. In german "AW:" is a common prefix to reply emails.

I understand that localizing this might be hard or even impossible, but i wanted to mention it. In case it was already discussed in https://git.sailfishos.org/mer-core/messagingframework/merge_requests/58 then ignore this, but the link is not working anymore.

pvuorela · 2021-11-22T16:04:04Z

This doesn't account for languages it seems. In german "AW:" is a common prefix to reply emails.

What I gather the standard is "Re:" and if something else is used the client is broken. Broken clients unfortunately including Exchange.

Would be nice to work with all sorts of usage, but as you said that can get problematic if every language has its own strings.

dcaliste · 2021-11-22T16:05:46Z

I agree, reminding what was originally in git.sailfishos.org MR would be helpfull.

I'm wondering for instance what is the purpose of this MR. I mean, its use case. Is it because the "sort by object" entry in the pulldown menu is broken in the sense that mails with the same object, but having these prefix, are not sorted together ? In that case, the purpose is actually to group by thread. A better implementation may rely then on References header. But well, since people often reply to an email to start a new thread, this solution is doomed also.

chriadam · 2021-11-23T08:22:12Z

Well, FWIW, this PR now looks fine to me. I don't think it would be accepted upstream (after discussing with Matt, the concern about the differing semantics of this key would probably be a showstopper there) but given that this isn't being upstreamed, I think that isn't a big issue.

pvuorela · 2021-11-25T08:44:40Z

I'm wondering for instance what is the purpose of this MR. I mean, its use case. Is it because the "sort by object" entry in the pulldown menu is broken in the sense that mails with the same object, but having these prefix, are not sorted together ?

Yes, that's the case.

In that case, the purpose is actually to group by thread. A better implementation may rely then on References header. But well, since people often reply to an email to start a new thread, this solution is doomed also.

Also a good point and maybe not a bad alternative. Though because of that thread splitting into new conversation thinking if the UI should then be something else than "sort by subject".

pvuorela · 2021-12-16T13:41:45Z

rpm/0035-Add-OriginalSubject-message-sort-key.patch

+    if (sortByOriginalSubject) {
+        QMap<QString, quint64> map;
+        while (query.next())
+            map.insertMulti(extractValue<QString>(query.value(1)).replace(QRegExp(QStringLiteral("^(re:|fw:|fwd:|\\s*|\\\")*"), Qt::CaseInsensitive), QString()),


Suppose it would be better creating the regexp outside the loop, i.e. not separately for every result.

Also on new code it could be better using QRegularExpression instead of QRegexp.

llewelld reviewed Jun 28, 2021

View reviewed changes

bshmarin force-pushed the jb6059 branch from 38b6d84 to e473efd Compare November 3, 2021 16:30

pvuorela reviewed Nov 5, 2021

View reviewed changes

bshmarin force-pushed the jb6059 branch from e473efd to c1dc04f Compare November 8, 2021 09:11

pvuorela requested a review from chriadam November 16, 2021 12:03

chriadam reviewed Nov 17, 2021

View reviewed changes

[messagingframework] Add TrimmedSubject message sort key for sort by …

6ef4d9b

…subject without Re: and Fwd: prefixes. Contributes to JB#6059 !!! WARNING !!! Don't upstream this patch to the QTC due to lacking a CLA.

bshmarin force-pushed the jb6059 branch from c1dc04f to 6ef4d9b Compare November 22, 2021 15:38

bshmarin mentioned this pull request Nov 22, 2021

[nemo-qml-plugin-email] Add OriginalSubject sorting order. Contributes… sailfishos/nemo-qml-plugin-email#3

Open

pvuorela reviewed Dec 16, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[messagingframework] Ignore Re: and Fwd: when sort by subject. Contributes to JB#6059 #1

[messagingframework] Ignore Re: and Fwd: when sort by subject. Contributes to JB#6059 #1

bshmarin commented Jun 21, 2021

okodron commented Jun 21, 2021

llewelld left a comment

llewelld Jun 28, 2021

bshmarin Nov 22, 2021

llewelld Jun 28, 2021

bshmarin Nov 22, 2021

llewelld Jun 28, 2021

bshmarin Nov 22, 2021

chriadam commented Jun 30, 2021

pvuorela Nov 5, 2021

bshmarin Nov 8, 2021

chriadam Nov 17, 2021

bshmarin Nov 17, 2021 •

edited

Loading

chriadam Nov 18, 2021

bshmarin Nov 18, 2021

bshmarin Nov 19, 2021

chriadam Nov 22, 2021

bshmarin Nov 22, 2021

chriadam commented Nov 17, 2021

bshmarin commented Nov 17, 2021 •

edited

Loading

krnlyng commented Nov 22, 2021

pvuorela commented Nov 22, 2021

dcaliste commented Nov 22, 2021

chriadam commented Nov 23, 2021

pvuorela commented Nov 25, 2021

pvuorela Dec 16, 2021

		@@ -0,0 +1,77 @@
		diff --git a/src/libraries/qmfclient/qmailmessagesortkey.cpp b/src/libraries/qmfclient/qmailmessagesortkey.cpp

[messagingframework] Ignore Re: and Fwd: when sort by subject. Contributes to JB#6059 #1

Are you sure you want to change the base?

[messagingframework] Ignore Re: and Fwd: when sort by subject. Contributes to JB#6059 #1

Conversation

bshmarin commented Jun 21, 2021

okodron commented Jun 21, 2021

llewelld left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriadam commented Jun 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bshmarin Nov 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriadam commented Nov 17, 2021

bshmarin commented Nov 17, 2021 • edited Loading

krnlyng commented Nov 22, 2021

pvuorela commented Nov 22, 2021

dcaliste commented Nov 22, 2021

chriadam commented Nov 23, 2021

pvuorela commented Nov 25, 2021

Choose a reason for hiding this comment

bshmarin Nov 17, 2021 •

edited

Loading

bshmarin commented Nov 17, 2021 •

edited

Loading