Skip to content

Comments

Hyperlink access to chords/lyrics/mp3... from the chat window (#591)#592

Merged
corrados merged 2 commits intomasterfrom
feature_jc_rosichini_chatlinks
Sep 20, 2020
Merged

Hyperlink access to chords/lyrics/mp3... from the chat window (#591)#592
corrados merged 2 commits intomasterfrom
feature_jc_rosichini_chatlinks

Conversation

@corrados
Copy link
Contributor

Pull request with code provided in Hyperlink access to chords/lyrics/mp3... from the chat window #591

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
Copy link
Collaborator

@pljones pljones Sep 15, 2020

Choose a reason for hiding this comment

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

Why? That trailing slash can cause different site behaviour (in "badly behaved" sites).

Choose a reason for hiding this comment

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

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 :-)

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

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);
Copy link
Collaborator

@pljones pljones Sep 15, 2020

Choose a reason for hiding this comment

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

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.)

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think relying on the browser to clean up is a safe option.

Choose a reason for hiding this comment

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

Hi Peter
IMHO The browser will merely not open the wrong url and issue a 404 page.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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>
Copy link
Collaborator

Choose a reason for hiding this comment

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

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);

Copy link
Collaborator

Choose a reason for hiding this comment

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

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).

Choose a reason for hiding this comment

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

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);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

@corrados corrados changed the title added code provided by jc-Rosichini in #591 Hyperlink access to chords/lyrics/mp3... from the chat window (#591) Sep 18, 2020
@corrados
Copy link
Contributor Author

I'll merge this now. Any open issue shall be resolved on the master branch.

@corrados corrados merged commit 58a2a3a into master Sep 20, 2020
@corrados corrados deleted the feature_jc_rosichini_chatlinks branch September 21, 2020 05:30
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.

3 participants