Closed carlosfrodriguez closed 2 years ago
Qeole
commented
2 years ago Where in the message does this happen? I looked at a few diffs where I have links in the commit descriptions (from git), and they're still working fine. I'll test more later.
If they're on a line that was modified as part of the patch, we could maybe re-format the link, but then how would we detect if the line was changed in the patch, if that lines contains just URLs?
Thanks for the report!
carlosfrodriguez
commented
2 years ago The important links are outside the Diff, this is why it caught my attention.
We have an automated system that alerts when a commit does not pass our unit tests the email contains links to the test results
Email example as: (for sure links in this sample are broken, but still should be clickable)
Some tests failed for f8847c753d on Gentoo (OTRS 8) + MySQL + Chrome (15351=
6 OK / 18 Not OK):
https://test.otrs.com/otrs/index.pl?Action=3DSomeAction
All results for this commit: https://test.otrs.com/otrs/index.pl?Action=3DSomeAction1
All results for this branch: https://test.otrs.com/otrs/index.pl?Action=3DSomeAction2
Carlos Rodriguez committed f8847c753d on Wed, 29 Sep 2021 08:02:00 -0500:
Optimized code.
Kernel/System/ProcessManagement/Modules/Base/Ticket.pm | 15 +++------------
1 file changed, 3 insertions(+), 12 deletions(-)
https://test.otrs.com/otrs/index.pl?Action=3DASomeAction3
diff --git a/Kernel/System/ProcessManagement/Modules/Base/Ticket.pm b/Kerne=
l/System/ProcessManagement/Modules/Base/Ticket.pm
index a28e7c9d3e..7e2547d331 100644
--- a/Kernel/System/ProcessManagement/Modules/Base/Ticket.pm
+++ b/Kernel/System/ProcessManagement/Modules/Base/Ticket.pm
@@ -37,9 +37,6 @@ sub _TransformTicketSearchParams {
OrderBy SortBy)
);
=20
- # We need another storage to set the correct values, otherwise OrderBy=
Direction could overwrite OrderBy;
- my %SearchParams;
-
my %ComplexParams =3D map { $_ =3D> 1 } (qw(TicketFlag NotTicketFlag A=
rticleFlag));
=20If it helps...
In larger mails I can see briefly the links highlighted (as clickable) and then set bak to plain text in less than 1 sec.
Qeole
commented
2 years ago Alright, I see it in my emails too. Not sure if it happens all the time of not for diffs (I thought I had a counter-example yesterday, can't find it again).
In larger mails I can see briefly the links highlighted (as clickable) and then set back to plain text in less than 1 sec.
The delay is probably the time that the add-on takes to colour the diff?
Not sure what happens exactly, I guess that manipulating the DOM for adding the colours removes the link tags somehow. I'll try to have a look when I can.
laggarcia
commented
2 years ago So, it seems I know more or less where the problem lies, but I still don't know how to fix it. In coloring.js, at colorizedDiff, all <pre> elements are wrapped by <code> elements with class="hljs diff". Inside some of these <pre>elements, there are <a> elements which are being included inside the <code> section.
The same issue happens in the signature <div>, and this case is treated as a special case in the code. That's why links appearing in the signature are formatted correctly even after highlighting the diff in the patch.
I saw that hljs can be configured with a noHightlightRe attribute. I tried to change the call to:
hljs.configure({
ignoreUnescapedHTML: true,
noHighlightRe: /^moz-txt-link-freetext$/i,
});Which hopefully would avoid highlight.js to highlight anything with the moz-txt-link-freetext class. But that didn't work. Ideally, we would want to ignore all /^moz-txt-link.*$/ as this would include things like e-mail as well. This is not working for me, probably because of my very limited knowledge of highlight.js.
Another approach would be to do something similar to what has been done for signatures for all the links. But I think it would be difficult to store the exact position of each link in the XML and re-insert them accordingly after the highlight.
laggarcia
commented
2 years ago I dig a little bit further and it seems the issue is being caused by how highlight.js works. In highlightElement function, it will get the HTML element (in our case, the top level element with all the e-mail content minus the signature) and apply the highlight on the element.textContent which is the HTML content without any tags. That way we loose the information about the links.
Qeole
commented
2 years ago Saving/restoring the links may be tricky in that case. I wonder how Thunderbird identifies them in the first place, it probably applies some regex on the content of the email. It could be just as simple to find this and redo it in the add-on, once the highlighting is done?
Qeole
commented
2 years ago Haven't looked into the details, but it seems to happen around here.
laggarcia
commented
2 years ago Saving/restoring the links may be tricky in that case. I wonder how Thunderbird identifies them in the first place, it probably applies some regex on the content of the email. It could be just as simple to find this and redo it in the add-on, once the highlighting is done?
I worked on a different approach. Instead of including the links back after highlighting, I thought about avoid the highlighting on them at first. My code is not really working (and I don't know why :smile: ), but the idea is that I am going through all the nodes under <pre> and marking with <code> only the ones that are pure text.
I have two problems with this approach:
highlights.js is able to find all the "pre code" elements and iterate through them. For some reason, though, it is not highlighting anything.<code> tags. Even though there is no '\n' after the included tags, it is printing a space in the e-mail after each <code> tag.Even though this is not working, I thought that including the patch here may help others that eventually investigate this issue.
diff --git a/scripts/coloring.js b/scripts/coloring.js
index 08a9ac8..f00ae2e 100644
--- a/scripts/coloring.js
+++ b/scripts/coloring.js
@@ -12,43 +12,34 @@ const coloring = {
* nodes with .hljs class between these <pre> node and their children.
*/
const preNodes = document.querySelectorAll("body > div > pre");
+
for (const pre of preNodes) {
- const code = document.createElement("CODE");
- code.style.padding = "0";
- transformations.setTabStyle(code, options.tabsize);
- code.setAttribute("class", "hljs diff");
+ for (let i = 0; i < pre.childNodes.length; i++) {
- while (pre.childNodes.length > 0) {
- code.appendChild(pre.childNodes[0]);
- }
- pre.appendChild(code);
- }
+ if (pre.childNodes[i].nodeName == "#text") {
- /*
- * Signature handling:
- * Search for a div with "moz-txt-sig" class set somewhere inside the plain
- * text mail. This node holds all signatures. We will remove it from the
- * DOM to exclude it from the highlighting and add it back afterwards.
- */
- let sigParentNode;
- const signatureNodes = document.querySelectorAll("div.moz-text-plain code > div.moz-txt-sig");
+ const code = document.createElement("CODE");
+
+ code.style.padding = "0";
+ transformations.setTabStyle(code, options.tabsize);
+ code.setAttribute("class", "hljs diff");
+
+ code.appendChild(pre.childNodes[i]);
+ pre.insertBefore(code, pre.childNodes[i]);
+
+ }
+ }
- if (signatureNodes.length) {
- sigParentNode = signatureNodes[0].parentNode;
- signatureNodes[0].remove();
}
/* Call library function, trigger highlighting */
- hljs.configure({ ignoreUnescapedHTML: true });
+ hljs.configure({
+ ignoreUnescapedHTML: true,
+ languages: "diff"
+ });
hljs.highlightAll();
- /* Re-add signature, highlighted with "comment" color */
- if (signatureNodes.length) {
- signatureNodes[0].className += " hljs-comment";
- sigParentNode.appendChild(signatureNodes[0]);
- }
-
/* Replace spaces and tabs if required */
if (options.spaces) {
for (const pre of preNodes) {Actually, this sounds like a good idea. I'm not sure why your code is not working, I wonder if embedding the <pre><code> in a <span> might be the cause, I'd have thought the former were block nodes and the span inline, so not sure how they interact.
I tried a variation, using the cssSelector configuration option from highlight.js, so we can use just one span and explicitly mark it for highlighting with a custom class. I only gave it a brief try and it seems to work, but I haven't taken the time to compare it with the current version to check if everything is in good order.
node and their children.
*/
const preNodes = document.querySelectorAll("body > div > pre");
+
for (const pre of preNodes) {
- const code = document.createElement("CODE");
+ for (let i = 0; i < pre.childNodes.length; i++) {
+ if (pre.childNodes[i].nodeName === "#text") {
+ const span = document.createElement("SPAN");
- code.style.padding = "0";
- transformations.setTabStyle(code, options.tabsize);
- code.setAttribute("class", "hljs diff");
+ span.style.padding = "0";
+ transformations.setTabStyle(span, options.tabsize);
+ span.setAttribute("class", "hljs diff colorediffs");
- while (pre.childNodes.length > 0) {
- code.appendChild(pre.childNodes[0]);
+ span.appendChild(pre.childNodes[i]);
+ pre.insertBefore(span, pre.childNodes[i]);
+ }
}
- pre.appendChild(code);
- }
-
- /*
- * Signature handling:
- * Search for a div with "moz-txt-sig" class set somewhere inside the plain
- * text mail. This node holds all signatures. We will remove it from the
- * DOM to exclude it from the highlighting and add it back afterwards.
- */
- let sigParentNode;
- const signatureNodes = document.querySelectorAll("div.moz-text-plain code > div.moz-txt-sig");
-
- if (signatureNodes.length) {
- sigParentNode = signatureNodes[0].parentNode;
- signatureNodes[0].remove();
}
/* Call library function, trigger highlighting */
- hljs.configure({ ignoreUnescapedHTML: true });
+ hljs.configure({
+ ignoreUnescapedHTML: true,
+ cssSelector: ".colorediffs",
+ });
hljs.highlightAll();
- /* Re-add signature, highlighted with "comment" color */
- if (signatureNodes.length) {
- signatureNodes[0].className += " hljs-comment";
- sigParentNode.appendChild(signatureNodes[0]);
- }
-
/* Replace spaces and tabs if required */
if (options.spaces) {
for (const pre of preNodes) {
```
laggarcia
commented
2 years ago I'll test your patch. I tried something similar and it didn't work for me. But, well, I am far from a javascript expert. I'll get back here as soon as I test it.
laggarcia
commented
2 years ago Your patch looks good! Thx!
CendioOssman
commented
2 years ago It sounds like a solution was found here? Any estimate on getting it rolled out? :)
Qeole
commented
2 years ago @laggarcia Did you intend to submit a PR for this, or shall I go ahead?
If you have a plain text mail with an URL it is normally converted to a link where you can clic on it.
If the email has a code diff (e.g. from GIT) and you have colorediffs installed and enabled, the diffs are correctly highlighted as expected but the URLs are set back as plain text, so they are not highlighted and converted to clickable links.
if colorediffs is temporary disabled (to test) then the same mail gets the links back, and lost again as soon as colorediffs is re-enabled.
((enjoy)) cr