290244 - Consider autolinkification for "keyword"

Status: RESOLVED WONTFIX

(Bugzilla :: Creating/Changing Bugs, enhancement, P3)

Description

[Chris Raplee]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; SV1; .NET CLR 1.1.4322; .NET CLR 2.0.40607)
Build Identifier: 

I think it would be nifty if "keyword somekeyword" could be autolinkified the 
way "bug bugnum" is. It looks like this would be a smallish change to quoteUrls
() in global.pl, and some modification to describekeywords.cgi and the 
associated template (to provide a landing point for the link). 

I can see a lot of comments on the order of "Added keyword SetsDogsOnFire after 
the unfortunate incident in the test lab".

Also, if the change is made for this, it would probably be worthwhile to have 
links next to the Keywords field the way we have links next to the "Depends on" 
and "Blocks" fields. 


Reproducible: Always

Steps to Reproduce:

Comment 1

[Paul]

not a bad idea but where does the link go?

Comment 2

[Chris Raplee]

The autolink would go to describekeywords.cgi#keywordname 

Comment 3

[Paul]

It'd be sweet if the definition showed up when hovering the mouse over the word too.

Comment 4

[Paul]

Attachment: patch for tip

Comment 5

[Max Kanat-Alexander]

Comment on attachment 207907 [details] [diff] [review]
patch for tip

Bradley is the person who originally re-wrote all this foo, and I don't really deeply understand it. Let's see if he reviews it! :-)

Comment 6

[Max Kanat-Alexander]

Oh, also note that we just moved quoteUrls into Bugzilla::Template; it's not in globals.pl anymore.

Comment 7

[Vlad Dascalu]

Comment on attachment 207907 [details] [diff] [review]
patch for tip

This doesn't apply anymore to CVS code, we moved the code in Template.pm.

+                         "select name, description from keyworddefs", "name");

SELECT, FROM need to be written in uppercase.

+            s/(\.|\*|\+|\?|\^|\$|\(|\)|\[|\]|\{|\}|\||\\)/\\$1/g;

Couldn't we use \Q$keywords\E further down, instead of $keywords, in order to avoid this error-prune escaping thing?

Looks good otherwise.

Comment 8

[Paul]

Attachment: \Q \E version

Comment 9

[Frédéric Buclin]

Comment on attachment 233468 [details] [diff] [review]
\Q \E version

>+    my $keywordhashref = Bugzilla->dbh->selectall_hashref(
>+                         "select name, description from keyworddefs", "name");

You should use Bugzilla::Keyword->get_all() instead.


>+    my $keyword_regex;
>+    # Special characters might be in the keywords so join them with '|'s and surround with \Q \E
>+    foreach (keys(%$keywordhashref))
>+    {
>+            $keyword_regex .= "\E|" if ( length($keyword_regex) > 0 );
>+            $keyword_regex        .= "\Q$_";
>+    }
>+    $keyword_regex .= "\E" if ( length($keyword_regex) > 0 );

You are going to fill your web server error log, because $keyword_regex is first undefined. Moreover, I'm pretty sure this code could be cleaner using map() and join().


>+    my $startanchor = '<a href="describekeywords.cgi#';
>+    $text =~ s/(^|\s+|,|\()($keyword_regex)(\s+|$|,|\.|\))/$1$startanchor$2"
>+              title="$keywordhashref->{$2}->{"description"}">$2<\/a>$3/g;

I strongly disagree with this way of linkifying keywords. First of all, it should detect the string 'keyword foo' instead of 'foo' alone, especially if 'foo' is e.g. 'bug', 'fixed', 'ue'. Moreover, this regexp looks as broken as the one used in 009bugwords.t (see a bug about incorrect bugwords detection). Now, I also disagree with this bug. Much better (IMO) is to have the list of selected keywords right below the textfield when keywords are entered and linkified in the same way as dependent bugs. This way there is no ambiguity about what keywords are.

This partially summaries some feeling we had on IRC.

Comment 10

[Paul]

Attachment: using the get_all() and map as suggested

Comment 11

[Max Kanat-Alexander]

Comment on attachment 233519 [details] [diff] [review]
using the get_all() and map as suggested

Hey Paul. I actually didn't understand what this code does, last time I looked at it.

Now, as far as I understand, every time a keyword appeared in a comment, it would be linkified, but only to its own description.

Although the code is good, the functionality itself doesn't seem very useful. I imagine just seeing comments lit up like Christmas trees, with links that nobody would ever really need to click on.

Now, if we made the keywords field itself into links to the descriptions, that might be more useful.

But as this bug is, I don't think it's a feature we want.