Edgewall Software
Modify

Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#12169 closed enhancement (fixed)

Replace suggest.js with jQuery autocomplete

Reported by: Ryan J Ollos Owned by: walty <walty8@…>
Priority: normal Milestone: 1.2
Component: version control/changeset view Version:
Severity: normal Keywords: suggest.js anydiff jquery-ui bitesized
Cc: walty8@…
Release Notes:

Replaced suggest.js with jQuery UI's autocomplete.

API Changes:

The deprecated suggest.js has been removed. jQuery UI's autocomplete widget should be used instead.

Description (last modified by Ryan J Ollos)

suggest.js was deprecated in [11081] / #7355, to be replaced with jQuery autocomplete. The only use of suggest.js is:

Concurrently we can look at resolving #8471 and #8429.

Attachments (2)

T12169_suggest_autocomplete.diff (2.4 KB ) - added by Peter Suter 2 years ago.
12169-suggest-autocomplete.v2.patch (2.3 KB ) - added by walty <walty8@…> 2 years ago.

Download all attachments as: .zip

Change History (18)

Changed 2 years ago by Peter Suter

comment:1 Changed 2 years ago by Peter Suter

Just in case this safes anyone some time:

comment:2 Changed 2 years ago by Ryan J Ollos

Description: modified (diff)

comment:3 in reply to:  1 Changed 2 years ago by Jun Omae

Replying to psuter:

At least, three things:

  • We should keep parameter naming for compatibilities (qterm)
  • Use to_json() instead of JSONEncoder.encode
  • It would be good to rename variable elem since the variable is a list of path, e.g. paths

comment:4 Changed 2 years ago by walty <walty8@…>

Cc: walty8@… added

Changed 2 years ago by walty <walty8@…>

comment:5 Changed 2 years ago by walty <walty8@…>

Basically the same patch as psuter, just modified the 3 places mentioned by jomae.

For the query string problem ( termq), there is actually no easy way to do that except providing a source function. The reference is here: SO:10791968.

Last edited 2 years ago by Ryan J Ollos (previous) (diff)

comment:6 Changed 2 years ago by Ryan J Ollos

Milestone: next-dev-1.1.x1.2
Owner: set to Ryan J Ollos
Status: newassigned

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.

comment:7 Changed 2 years ago by Ryan J Ollos

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 Changed 2 years ago by Jun Omae

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 Changed 2 years ago by Jun Omae

Another suggestions:

  1. It would be good to use window.location.pathname rather than hard-coded diff url.
    -        $("#anydiff input[name$='_path']").autocomplete({source: "diff"});
    +        $("#anydiff input[name$='_path']").autocomplete({source: window.location.pathname});
    
  2. .encode('utf-8') is redundant since to_json() always returns str instance. In Trac 1.0.x with Python 2.5, to_json() returns str or unicode instance.
    -            content = to_json(paths).encode('utf-8')
    +            content = to_json(paths)
    

comment:10 Changed 2 years ago by Ryan J Ollos

API Changes: modified (diff)
Release Notes: modified (diff)

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 in reply to:  7 Changed 2 years ago by Jun Omae

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 Changed 2 years ago by Ryan J Ollos

Resolution: fixed
Status: assignedclosed

After additional revisions the changes have been committed to the trunk in [14316].

comment:13 Changed 2 years ago by Ryan J Ollos

Owner: changed from Ryan J Ollos to walty <walty8@…>

comment:14 Changed 2 years ago by walty <walty8@…>

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 in reply to:  14 Changed 2 years ago by Jun Omae

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 Changed 2 years ago by walty <walty8@…>

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.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain walty <walty8@…>.
The resolution will be deleted.
to The owner will be changed from walty <walty8@…> to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .

 
Note: See TracTickets for help on using tickets.