Hyperlink access to chords/lyrics/mp3... from the chat window (#591)#592
Hyperlink access to chords/lyrics/mp3... from the chat window (#591)#592
Conversation
| int indx_space = strChatText.indexOf(" ", indx_http); | ||
| if (indx_space==-1) indx_space=strChatText.length(); | ||
|
|
||
| int cutslash = (strChatText.at(indx_space-1)=='/') ? 1:0; // drop "/" if last character of url text |
There was a problem hiding this comment.
Why? That trailing slash can cause different site behaviour (in "badly behaved" sites).
There was a problem hiding this comment.
Hi, the trailing slash (if any) would prevent for instance Firefox to open the site
I noticed that when entering as an hyperlink https://www.google.com/ versus https://www.google.com, this is the reason of cutting this trailing slash.
This is not a real issue anyway since any invalid url will be rejected by the browser :-)
There was a problem hiding this comment.
Firefox opens (from your post above) both equally well. If it's a defect in the QTextBrowser widget's URL handling, that's a different matter. (And another reason to take care with using it for active links.)
There was a problem hiding this comment.
Really, just simplify it. If I enter something as simple as http://example.com/?key=value, your slash stripper fails. I'd dump all that code and just find the index of either "http://" or "https://" through to space and pass that string.
src/chatdlg.cpp
Outdated
| // analyze strChatText to check if hyperlink | ||
| if (strChatText.contains("https://", Qt::CaseInsensitive) | strChatText.contains("http://", Qt::CaseInsensitive)) | ||
| { | ||
| int indx_http = strChatText.indexOf("http", 0); |
There was a problem hiding this comment.
So if I say "the http protocol is indicated by a 'http://' or 'https://' prefix on a url", this will find the first "http" (between "the" and "protocol").
(Parsing plain text to active links is fraught with danger, too, as you noted on the main post.)
There was a problem hiding this comment.
That's correct.
A better way would be to find the index of strChatText.indexOf("http://", 0) or strChatText.indexOf("https://", 0) and include some more checks.
But in any case if the link appears to be invalid the browser will deal with it and depict it as non sense.
There was a problem hiding this comment.
I don't think relying on the browser to clean up is a safe option.
There was a problem hiding this comment.
Hi Peter
IMHO The browser will merely not open the wrong url and issue a 404 page.
There was a problem hiding this comment.
Take a look at Qt's QUrl. A simple check like QUrl(text).isValid() would go a long way.
| </property> | ||
| <property name="openExternalLinks"> | ||
| <bool>true</bool> | ||
| </property> |
There was a problem hiding this comment.
It would be useful, if we retain the licence popup as a separate dialog, for these properties to be set there, too.
src/chatdlg.cpp
Outdated
| int cutslash = (strChatText.at(indx_space-1)=='/') ? 1:0; // drop "/" if last character of url text | ||
|
|
||
| QString new_strChatText = strChatText.left(indx_http)+"<a href=\""+strChatText.mid(indx_http,indx_space-indx_http-cutslash)+"\">"+strChatText.mid(indx_http,indx_space-indx_http-cutslash)+"</a> "+strChatText.right(strChatText.length()-indx_space); | ||
|
|
There was a problem hiding this comment.
strChatText.mid(indx_http,indx_space-indx_http-cutslash is repeated. It would be easier to read if it were put into a variable (strUrl, perhaps).
I'd also suggest, to improve readability and reduce the likelihood of dubious URLs being misread, to use a fixed width font at 110% size (or 1.1rem).
There was a problem hiding this comment.
Here is a short code modif prior I leave for a few days trip
I'll try to get connected while I'm out.
Cheers
code at: https://we.tl/t-ZGz7CK0XqG
| int cutslash = (strChatText.at(indx_space-1)=='/') ? 1:0; // drop "/" if last character of url text | ||
|
|
||
| QString URL_name = strChatText.mid(indx_http,indx_space-indx_http-cutslash); // as entered by the user | ||
| QUrl URL = QUrl::fromUserInput(URL_name); |
There was a problem hiding this comment.
Nice. With the check for http/https, this should mean at least half-way sane values go in. If it comes back invalid, the check below then catches it. I'd still like the text more readable than "normal" so it catches the reader attention and is clearer.
|
I'll merge this now. Any open issue shall be resolved on the master branch. |
Pull request with code provided in Hyperlink access to chords/lyrics/mp3... from the chat window #591