#12169 closed enhancement (fixed)
Replace suggest.js with jQuery autocomplete
| Reported by: | Ryan J Ollos | Owned by: | |
|---|---|---|---|
| Priority: | normal | Milestone: | 1.2 |
| Component: | version control/changeset view | Version: | |
| Severity: | normal | Keywords: | suggest.js anydiff jquery-ui bitesized |
| Cc: | walty8@… | Branch: | |
| Release Notes: |
Replaced |
||
| API Changes: |
The deprecated |
||
| Internal Changes: | |||
Description (last modified by )
suggest.js was deprecated in [11081] / #7355, to be replaced with jQuery autocomplete. The only use of suggest.js is:
Attachments (2)
Change History (18)
by , 10 years ago
| Attachment: | T12169_suggest_autocomplete.diff added |
|---|
follow-up: 3 comment:1 by , 10 years ago
comment:2 by , 10 years ago
| Description: | modified (diff) |
|---|
comment:3 by , 10 years ago
Replying to psuter:
At least, three things:
- We should keep parameter naming for compatibilities (
q→term) - Use
to_json()instead ofJSONEncoder.encode - It would be good to rename variable
elemsince the variable is a list of path, e.g.paths
comment:4 by , 10 years ago
| Cc: | added |
|---|
by , 10 years ago
| Attachment: | 12169-suggest-autocomplete.v2.patch added |
|---|
comment:5 by , 10 years ago
Basically the same patch as psuter, just modified the 3 places mentioned by jomae.
For the query string problem ( term → q), there is actually no easy way to do that except providing a source function. The reference is here: SO:10791968.
comment:6 by , 10 years ago
| Milestone: | next-dev-1.1.x → 1.2 |
|---|---|
| Owner: | set to |
| Status: | new → assigned |
The changes look good to me. I added some minor modification in log:rjollos.git:t12169_suggest_js_replacement. I'll test in additional browsers tomorrow.
follow-up: 11 comment:7 by , 10 years ago
What are the use cases for keeping compatibility in parameter naming? [5097dc7b/rjollos.git] will keep the server-side API backward compatible. It wouldn't cover the use-case of implementing an IRequestFilter for modifying the xhr request. I'm always in favor of backwards compatibility, but it doesn't seem worthwhile to add a more complex solution unless we have a concrete use case. This change is being made in a major release, so API changes are acceptable even though we want to minimize them.
comment:8 by , 10 years ago
No use-cases for me. However, no way to make sure of that all plugins and users except Trac core don't use q parameter in the AnyDiffModule.
The server-side API backward compatibilities in [5097dc7b/rjollos.git] looks good to me.
comment:9 by , 10 years ago
Another suggestions:
- It would be good to use
window.location.pathnamerather than hard-codeddiffurl.- $("#anydiff input[name$='_path']").autocomplete({source: "diff"}); + $("#anydiff input[name$='_path']").autocomplete({source: window.location.pathname});
.encode('utf-8')is redundant sinceto_json()always returnsstrinstance. In Trac 1.0.x with Python 2.5,to_json()returnsstrorunicodeinstance.- content = to_json(paths).encode('utf-8') + content = to_json(paths)
comment:10 by , 10 years ago
suggest.js displays directory entries in bold. I've added similar behavior in rjollos.git:t12169_suggest_js_replacement.1. Overriding _renderItem required that jQuery UI be upgraded (#11019). The changes will require much more extensive testing than I've done so far.
comment:11 by , 10 years ago
What are the use cases for keeping compatibility in parameter naming? [5097dc7b/rjollos.git] will keep the server-side API backward compatible. It wouldn't cover the use-case of implementing an
IRequestFilterfor modifying the xhr request.
Sorry. I didn't think straight. I understand no need to keep parameter naming for compatibilities since response for the xhr request is modified (api change) in the proposed changes. It is enough by handling only term parameter.
- term = req.args.get('q') or req.args.get('term') + term = req.args.get('term')
comment:12 by , 10 years ago
| Resolution: | → fixed |
|---|---|
| Status: | assigned → closed |
After additional revisions the changes have been committed to the trunk in [14316].
comment:13 by , 10 years ago
| Owner: | changed from to |
|---|
follow-up: 15 comment:14 by , 10 years ago
Just found a minor issue, might be it's better add a return statement after the ajax response?
trunk/trac/versioncontrol/web_ui/changeset.py@14316:1202#L1200
Otherwise for each ajax request, all the logic below that would still be executed (though no effect).
comment:15 by , 10 years ago
Just found a minor issue, might be it's better add a
returnstatement after the ajax response?
Not needed. The req.send raises RequestDone exception that indicates whether request processing has completed and a response was sent.
comment:16 by , 10 years ago
actually I tried to add some debug messages on line 1204 and line 1230 last night, and found them did get executed. however when I tried again this morning, the debug message seems to be correct now.
I guess I mixed up the message of http request and xhr request by yesterday.
sorry about that.



Just in case this safes anyone some time:
suggest.json diff. (Available via Browse Source → View changes… button)