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

Migrate from QRegExp to QRegularExpression for Qt6 compatibility #2124

Merged
merged 4 commits into from
Dec 7, 2021

Conversation

softins
Copy link
Member

@softins softins commented Nov 16, 2021

Short description of changes

Migrate QRegExp to QRegularExpression for compatibility with both Qt5 and Qt6

Context: Fixes an issue?

Prerequisite for supporting Qt6. The code for src/util.cpp was taken from the comment by @ngocdh at
#1999 (comment)

Should ease further work towards Qt6, particularly the draft PR in #1999

Does this change need documentation? What needs to be documented and how?

No

Status of this Pull Request

Tested on Qt5 to ensure backward compatibility. Not tested on Qt6, but should be identical by design.

What is missing until this pull request can be merged?

Ready to merge.

Checklist

  • I've verified that this Pull Request follows the general code principles
  • I tested my code and it does what I want
  • My code follows the style guide
  • I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
  • I've filled all the content above

@softins softins requested a review from pljones November 16, 2021 22:51
@softins
Copy link
Member Author

softins commented Nov 17, 2021

Also took the opportunity to improve the URL regex so that it didn't swallow up terminating punctuation characters (such as full stop) into the hyperlink. It was based on the tested behaviour of clients such as Outlook which automatically convert plain text URLs into hyperlinks, and exclude certain terminating characters from the link when doing so (not just based on white space).

@pljones
Copy link
Collaborator

pljones commented Nov 20, 2021

@jamulussoftware/maindevelopers anyone object to this being merged?

@atsampson
Copy link
Contributor

@softins, in the URL regexp change, why is the negative lookahead assertion (the (<?...) bit) duplicated? The regexp seems to work fine with just one instance of it...

@softins
Copy link
Member Author

softins commented Nov 21, 2021

@softins, in the URL regexp change, why is the negative lookahead assertion (the (<?...) bit) duplicated? The regexp seems to work fine with just one instance of it...

It's not duplicated. One of them matches a single character class, and the other matches a ? followed by the character class. Negative look-behind assertions must be of fixed length, so I couldn't make the matching of the ? variable.

The behaviour with ? was what I observed in outlook's URL highlighting when I sent myself a plaintext email containing different kinds of URL with punctuation chars immediately following.

Maybe I should add an explanatory comment...

@pljones
Copy link
Collaborator

pljones commented Nov 21, 2021

Maybe I should add an explanatory comment...

You'd have to step through the full regexp if you do. That would help in future, though.

@softins
Copy link
Member Author

softins commented Nov 21, 2021

The regex could probably be improved; it did cause me a bit of head scratching! I would be open to suggestions. The intent is to exclude from the end of the URL characters that may well be legal within the URL.

I searched without success for a standard, official regex to do it. There were many different home-grown suggestions for validating a standalone URL, but not for identifying the extent of a URL within a surrounding string.

src/chatdlg.cpp Outdated Show resolved Hide resolved
Co-authored-by: Noam Postavsky <npostavs@gmail.com>
@softins
Copy link
Member Author

softins commented Dec 6, 2021

Anyone else want to review or comment? Otherwise I'll admin-merge it.

@atsampson
Copy link
Contributor

Looks good to me!

@softins softins merged commit f995159 into jamulussoftware:master Dec 7, 2021
@softins softins deleted the qt-regex-update branch December 7, 2021 12:51
@gilgongo gilgongo added this to the 3.8.2 milestone Jan 17, 2022
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