#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 , 9 years ago
Attachment: | T12169_suggest_autocomplete.diff added |
---|
follow-up: 3 comment:1 by , 9 years ago
comment:2 by , 9 years ago
Description: | modified (diff) |
---|
comment:3 by , 9 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
elem
since the variable is a list of path, e.g.paths
comment:4 by , 9 years ago
Cc: | added |
---|
by , 9 years ago
Attachment: | 12169-suggest-autocomplete.v2.patch added |
---|
comment:5 by , 9 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 , 9 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 , 9 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 , 9 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 , 9 years ago
Another suggestions:
- It would be good to use
window.location.pathname
rather than hard-codeddiff
url.- $("#anydiff input[name$='_path']").autocomplete({source: "diff"}); + $("#anydiff input[name$='_path']").autocomplete({source: window.location.pathname});
.encode('utf-8')
is redundant sinceto_json()
always returnsstr
instance. In Trac 1.0.x with Python 2.5,to_json()
returnsstr
orunicode
instance.- content = to_json(paths).encode('utf-8') + content = to_json(paths)
comment:10 by , 9 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 , 9 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.
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 , 9 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
After additional revisions the changes have been committed to the trunk in [14316].
comment:13 by , 9 years ago
Owner: | changed from | to
---|
follow-up: 15 comment:14 by , 9 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 , 9 years ago
Just found a minor issue, might be it's better add a
return
statement 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 , 9 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.js
on diff. (Available via Browse Source → View changes… button)