Skip to content
This repository has been archived by the owner on Feb 12, 2023. It is now read-only.

feat(chatlog): Add custom web handler as hyperlink #6645

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dvn0
Copy link
Contributor

@dvn0 dvn0 commented Jun 22, 2022

Adds support for custom web scheme handlers. See MDN docs for more info.


This change is Reviewable

Copy link

@Tha14 Tha14 left a comment

Choose a reason for hiding this comment

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

Looks fine

@codecov-commenter
Copy link

Codecov Report

Merging #6645 (2239ebb) into master (2197bce) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #6645   +/-   ##
=======================================
  Coverage   12.08%   12.09%           
=======================================
  Files         308      308           
  Lines       20898    20899    +1     
=======================================
+ Hits         2526     2527    +1     
  Misses      18372    18372           
Impacted Files Coverage Δ
src/chatlog/textformatter.cpp 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2197bce...2239ebb. Read the comment docs.

@sudden6
Copy link
Member

sudden6 commented Jun 24, 2022

@dvn0 please add at least one positive and possibly one negative test case to: https://github.com/qTox/qTox/blob/master/test/chatlog/textformatter_test.cpp

@dvn0
Copy link
Contributor Author

dvn0 commented Jun 29, 2022

Probably not going to get around to making tests anytime soon, unfortunately. Sorry.

Copy link
Member

@anthonybilinski anthonybilinski left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 LGTMs obtained

a discussion (no related file):
My understanding of web+ scheme is that a website asks a web browser to ask a user to register the mapping from a web+<> to a web URL. I don't really see where qTox comes in to this - with qTox having no way to register web+ URIs to http(s) URLs, won't a link clicked in qTox just fail to open without ever getting to the web browser to resolve the mapping, even if it exists there?

Have you seen this work with a website that registers a web+ URI? Testing myself by adding a registration in firefox's console makes it so that those links in the browser then prompt to open the link with the registered URL, but from qTox I just get "gio: The specified location is not supported" and similar from command line.

Additionally firefox' general settings under applications shows the new link registration, but gnome's "default applications" is unchanged.


@dvn0
Copy link
Contributor Author

dvn0 commented Jul 5, 2022

@anthonybilinski - sounds like it doesn't work in your environment, but for me, if I open firefox from the command line with a registered web+[...] address then it opens as expected. It will open the website which is registered for the given schema. This should work for me from qTox as my xdg-open has the web+[...] protocol schema assigned to open with firefox.

Copy link
Member

@anthonybilinski anthonybilinski left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: 0 of 1 LGTMs obtained

a discussion (no related file):

Previously, anthonybilinski (Anthony Bilinski) wrote…

My understanding of web+ scheme is that a website asks a web browser to ask a user to register the mapping from a web+<> to a web URL. I don't really see where qTox comes in to this - with qTox having no way to register web+ URIs to http(s) URLs, won't a link clicked in qTox just fail to open without ever getting to the web browser to resolve the mapping, even if it exists there?

Have you seen this work with a website that registers a web+ URI? Testing myself by adding a registration in firefox's console makes it so that those links in the browser then prompt to open the link with the registered URL, but from qTox I just get "gio: The specified location is not supported" and similar from command line.

Additionally firefox' general settings under applications shows the new link registration, but gnome's "default applications" is unchanged.

Ah I see, thanks for the explanation. Firefox opened with web+[...]: works for me as well, but the piece my environment is missing is the web+[...] -> Firefox link. With that, I can see how qTox would dispatch the link properly and have no issue with this conceptually. Creating that link seems outside of qTox's scope, so not really something we need to worry about. Creating the links in chat still seems reasonable for us to do.



src/chatlog/textformatter.cpp line 96 at r1 (raw file):

    QRegularExpression(QStringLiteral(R"((?<=^|\s)\S*(gemini://\S+))")),
    QRegularExpression(QStringLiteral(R"((?<=^|\s)\S*(ed2k://\|file\|\S+))")),
    QRegularExpression(QStringLiteral(R"((?<=^|\s)\S*(web+\S+:\S+))")),

I believe not including an argument in the link is valid in the spec, and is accepted by Firefox testing, i.e. both "web+burger:" as well as "web+burger:foo" are valid. If you agree, the regex is overly restrictive, and the last \S+ could be changed to \S*.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants