Closed Bug 386600 Opened 17 years ago Closed 14 years ago

JavaScript autocomplete for review request field

Categories

(Bugzilla :: Attachments & Requests, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 4.0

People

(Reporter: ted, Assigned: guy.pyrzak)

References

Details

(Whiteboard: [wanted-bmo])

Attachments

(1 file, 6 obsolete files)

I searched but didn't find a dupe of this, so I'm filing new.  I'm attaching a patch that implements dropdown autocomplete on the review request field.  the JS magic is done using jQuery and a jQuery autocomplete implementation that I found online.  I'm sure it could be swapped out for pretty much any JS toolkit implementation.  I didn't include the jQuery source or the JS autocomplete source in my patch, since they'd just make it harder to read, and I didn't write them anyway.  Presumably this could also be hooked up to the CC field, etc, but I didn't get around to that.
Comment on attachment 270598 [details] [diff] [review]
add JS autocomplete to review request fields

>                     # Certain versions are broken, development versions are
>                     # always disallowed.
>-                    blacklist => ['^3\.000[3-6]', '_'],
>+                    blacklist => ['^3\.000[3-5]', '_'],
>                 },
>                 name => 'MySQL'},

There are reasons why these MySQL DBI versions are blacklisted. This should not be included in your patch.
(In reply to comment #1)
> There are reasons why these MySQL DBI versions are blacklisted. This should not
> be included in your patch.

Yeah, oops.  That was a hack to get things working on Ubuntu.  Didn't mean to include that.
 
Removed that file from the diff.
Attachment #270598 - Attachment is obsolete: true
Ted, without the missing JS files, what do you expect us to do with your patch?
Got my bugzilla install back up and running, you can see this in action here:
http://bugzilla.rawr.homeip.net/attachment.cgi?id=1&action=edit

I loaded a bunch of random usernames into my db, so just try setting r? and typing some characters into the requestee field.
Attached patch complete patch (obsolete) — Splinter Review
Well, I hacked it together in a few hours, and I just figured bugzilla was a better home for it than sitting in my tree.  Here's a complete patch, for reference.
Comment on attachment 270604 [details] [diff] [review]
complete patch

>Index: js/jquery-latest.pack.js

>+eval(function(p,a,c,k,e,d){e=function(c){return(c<a?"":e(parseInt(c/a)))+((c=c%a)>35

Huh? What's that??
I don't know, I didn't write it, and the JS is obviously compressed, which is why I left it out of my original patch.  It's just the jQuery library, downloaded from http://jquery.com/ .
Fwiw you'd be better off using the minified+gzipped jQuery, not the packed one. It's smaller, and faster on the users' machines as it doesn't have to be unpacked before use.
I've been thinking of doing a few things with the autocomplete. The requestee should be easy along with most of the other person fields. I'd also like to see something that integrates quicksearch into the duplicates field. I'll see if I can revive this along with the other autocomplete ideas I've had.
The patch here is pretty simple, I just took an off-the-shelf jQuery autocomplete plugin and attached it to the review request field. The other interesting part of the patch is queryuser.cgi, which is a CGI I added that takes a q=foo param, and returns one line of "login_name|realname" per user whose login name or real name matches "%foo%".
I know that this bug has a patch on it and lots of discussion, but it's actually a little too narrow. We want autocomplete for all user-based fields. Also, we don't want to use jQuery, but YUI, which is what is already in Bugzilla and which supports autocomplete.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → DUPLICATE
That's fine. I just hacked it into the review field because that was what annoyed me the most. Also, I don't think any of the YUI stuff existed when I wrote this patch.
Reopened, as bug 490923 finally didn't implement it.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Status: REOPENED → NEW
Whiteboard: [wanted-bmo]
It would take 1 line of code to implement this on 4.0 or trunk. But the problem is the field will not know if the users that show up are allowed to + the flag, so users will run into that after the save.
Attached patch 4.0 Version (obsolete) — Splinter Review
Assignee: attach-and-request → guy.pyrzak
Attachment #270599 - Attachment is obsolete: true
Attachment #270604 - Attachment is obsolete: true
Attachment #487037 - Flags: review?(mkanat)
Attachment #487037 - Flags: review?(LpSolit)
Flags: blocking4.0?
I don't see why it should block 4.0.
Severity: normal → enhancement
Status: NEW → ASSIGNED
well mostly because adding autocomplete is a big feature of 4.0 and not having it for flags is almost a bug than an enhancement.
Comment on attachment 487037 [details] [diff] [review]
4.0 Version

>=== modified file 'template/en/default/flag/list.html.tmpl'

>                 [% ELSE %]
>+                  [% INCLUDE global/userselect.html.tmpl

What's the point to duplicate the code in the ELSE block when it's already in the IF block? Looks like both should be combined.
Attachment #487037 - Flags: review?(mkanat)
Attachment #487037 - Flags: review?(LpSolit)
Attachment #487037 - Flags: review-
Attached patch v2 (obsolete) — Splinter Review
you were correct. I was trying to change as little code as possible b/c we're so close to rc1. Here is the simplified version.
Attachment #487037 - Attachment is obsolete: true
Attachment #487056 - Flags: review?(mkanat)
Attachment #487056 - Flags: review?(LpSolit)
Flags: blocking4.0? → blocking4.0+
Target Milestone: --- → Bugzilla 4.0
Comment on attachment 487056 [details] [diff] [review]
v2

The autocomplete parts look fine, but the rest of this is in code that LpSolit owns, so I'll let him review it.
Attachment #487056 - Flags: review?(mkanat) → review+
Comment on attachment 487056 [details] [diff] [review]
v2

>=== modified file 'template/en/default/flag/list.html.tmpl'

>+                [% END %]                  
>                   [% INCLUDE global/userselect.html.tmpl

Please fix the indentation. [% INCLUDE %] should now be vertically aligned with [% END %].


>-                             emptyok  => 1

This removal is incorrect. I can no longer remove the requestee.


>+                             classes => ["bz_userfield", "requestee"]

Is the bz_userfield class needed? Why not adding the new CSS rule to the .requestee class instead? (and why wasn't this class needed till today, when usemenuforusers was turned on?)


>+                             size => 30

30 is too large, see the current tip UI. This regresses bug 455559, see the patch there.


>           <span style="white-space: nowrap;">
>-            [% IF Param('usemenuforusers') %]
>               [% INCLUDE global/userselect.html.tmpl

Also fix the indentation here, now that [% IF %] is gone.


>+                          custom_userlist => type.grant_list

Don't call type->grant_list when usemenuforusers is turned off. This method is slow, see bug 528230, and is never used when this parameter is off.


Note also that this patch only applies cleanly to the 4.0 branch, not to 4.1.
Attachment #487056 - Flags: review?(LpSolit) → review-
Attached patch v3 (obsolete) — Splinter Review
did what LpSolit wanted. I will provide a 4.1 patch once i get an R+
Attachment #487056 - Attachment is obsolete: true
Attachment #489363 - Flags: review?(LpSolit)
(In reply to comment #22)
> >+                             classes => ["bz_userfield", "requestee"]
> 
> Is the bz_userfield class needed? Why not adding the new CSS rule to the
> .requestee class instead? (and why wasn't this class needed till today, when
> usemenuforusers was turned on?)

Could I have an answer to this question, please?


> >+                             size => 30
> 
> 30 is too large, see the current tip UI. This regresses bug 455559, see the
> patch there.

This comment hasn't been addressed. Consider this a r-.
the answer to both of your questions are "they are not needed". They should have beed deleted. My apologies. Assuming those strings are deleted, anything else?
Attached patch V4Splinter Review
i don't know what happened with that previous patch. i meant to delete both of the references.
Attachment #489363 - Attachment is obsolete: true
Attachment #491593 - Flags: review?(LpSolit)
Attachment #489363 - Flags: review?(LpSolit)
Comment on attachment 491593 [details] [diff] [review]
V4

r=LpSolit
Attachment #491593 - Flags: review?(LpSolit) → review+
Flags: approval4.0+
Flags: approval+
Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/trunk/
modified template/en/default/attachment/create.html.tmpl
modified template/en/default/attachment/edit.html.tmpl
modified template/en/default/flag/list.html.tmpl
Committed revision 7607.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/4.0/
modified template/en/default/attachment/create.html.tmpl
modified template/en/default/attachment/edit.html.tmpl
modified template/en/default/flag/list.html.tmpl
Committed revision 7486.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago14 years ago
Resolution: --- → FIXED
Blocks: 182396
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: