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

Clickable links in event descriptions #656

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

igordsm
Copy link

@igordsm igordsm commented Mar 7, 2021

Fix #571 . Implementation was based on this mailing list message. Requests for improvements are welcome.

calendar-link-2

Comment on lines +290 to +311
var link_regex = new GLib.Regex ("https?://[\\w.-]*(\\/[\\w-\\d]*)*");
comment_link_tag = buffer.create_tag ("link");
comment_link_tag.event.connect (on_link_clicked);
comment_link_tag.set_property ("foreground", "#0000FF");
comment_link_tag.underline = Pango.Underline.SINGLE;
GLib.MatchInfo mi;

Gtk.TextIter search_iter;
buffer.get_start_iter (out search_iter);
if (link_regex.match (buffer.text, 0, out mi)) {
do {
int start, end;
mi.fetch_pos (0, out start, out end);
var link_text = buffer.text.substring (start, end - start);

Gtk.TextIter starti, endi;
search_iter.forward_search (link_text, Gtk.TextSearchFlags.TEXT_ONLY, out starti, out endi, null);

buffer.apply_tag (comment_link_tag, starti, endi);
search_iter.forward_chars (link_text.length);
} while (mi.next ());
}
Copy link
Member

@pongloongyeat pongloongyeat Mar 12, 2021

Choose a reason for hiding this comment

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

I'm not familiar with this so please don't take my comment too seriously. Would it be possible to do the regex search on TextBuffer.changed? I tried your PR and when I type links in, it only adds the tag after I've saved the event and reopened it. Note however that doing a regex search on each TextBuffer.changed would probably be incredibly inefficient but I am of the opinion that event descriptions are typically short and at most contain 100 words or so, so performance shouldn't be too big of a hit.

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 we should assume that descriptions are going to be short, especially the type that have links. Auto-generated Zoom or Webex events have very long descriptions. That said, I'm not sure what the performance hit of regex on change would be.

Two potential compromises I can think of:

  1. Run the regex when the description field loses focus (so the focus goes somewhere else, like the title). This still wouldn't parse immediately, but it's possible that people could change the description and then go somewhere else, and then they'd see the benefit while they're still editing.
  2. Use a timeout to wait a second or two after editing stops, then parse. I've seen some work over in elementary/code that worked with this.

@mcclurgm
Copy link
Collaborator

Thanks for the PR! It seems to work as advertised for me. I haven't taken a very close look at the code, but in the meantime there are a few features that could use some work here:

  1. The formatting of the link should follow the system theme. I'm not sure how to get this to work in a TextView, but it should use the current accent color and look good in both dark and light modes. @elementary/ux do we want to use the system accent color for links like this or do we want to use the green accent that we use elsewhere in Calendar?
  2. There should be some sort of tooltip to indicate that links are clickable. Include something like "Hold ctrl and click to follow link" (the copy could use some work, of course).

Copy link
Collaborator

@mcclurgm mcclurgm left a comment

Choose a reason for hiding this comment

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

The regex that you're using isn't quite complete and could use some updating. It misses some common cases. I also noticed when compiling on my machine that there are a few unhandled errors that should be taken care of.

@@ -264,6 +285,31 @@ public class Maya.View.EventEdition.InfoPanel : Gtk.Grid {
Gtk.TextBuffer buffer = new Gtk.TextBuffer (null);
buffer.text = property.get_comment ();
comment_textview.set_buffer (buffer);

// Identify links in the description and apply a tag to style them
var link_regex = new GLib.Regex ("https?://[\\w.-]*(\\/[\\w-\\d]*)*");
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't account for some types of URLs, for example ones that include ? parameters. This is common in things like Zoom links.

Here's a blog post that goes over some example patterns. I'm not sure if there's any licensing issues involved with copying these, however.

@@ -264,6 +285,31 @@ public class Maya.View.EventEdition.InfoPanel : Gtk.Grid {
Gtk.TextBuffer buffer = new Gtk.TextBuffer (null);
buffer.text = property.get_comment ();
comment_textview.set_buffer (buffer);

// Identify links in the description and apply a tag to style them
var link_regex = new GLib.Regex ("https?://[\\w.-]*(\\/[\\w-\\d]*)*");
Copy link
Collaborator

Choose a reason for hiding this comment

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

The compiler also gives a warning of an unhandled GLib.RegexError on this line.


buffer.apply_tag (comment_link_tag, starti, endi);
search_iter.forward_chars (link_text.length);
} while (mi.next ());
Copy link
Collaborator

Choose a reason for hiding this comment

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

The compiler gives a warning of an unhandled GLib.RegexError here.

iter.forward_to_tag_toggle (tag);

var link_dst = comment_textview.buffer.get_text (start, iter, false);
Gtk.show_uri_on_window (null, link_dst, Gdk.CURRENT_TIME);
Copy link
Collaborator

@mcclurgm mcclurgm Jun 2, 2021

Choose a reason for hiding this comment

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

This also has an unhandled error of some sort (the compiler isn't very specific).

@danirabbit
Copy link
Member

I'm gonna convert this back to draft since there hasn't been any movement in the last couple weeks. Feel free to mark ready for review once requested changes have been made :)

@danirabbit danirabbit marked this pull request as draft June 15, 2021 19:05
@igordsm
Copy link
Author

igordsm commented Aug 9, 2021

I'm waiting on the granite update to proceed. As soon as it lands I'll get back to this.

@teamcons
Copy link

teamcons commented Dec 3, 2024

This PR is long stale and has been superseded by another merged PR. I would like to request closing it, to leave more breathing room to actual, relevant PR ?

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.

Links in comments aren't clickable
6 participants