Modify

Opened 4 years ago

Last modified 18 months ago

#8141 new defect

KeywordSuggestPlugin does not work well with Ffx4.0 betas

Reported by: dustin@… Owned by: rjollos
Priority: high Component: KeywordSuggestPlugin
Severity: normal Keywords:
Cc: hasienda, itamarost, falkb Trac Release: 0.11

Description

Once you type the first letter of a keyword, it immediately completes to a keyword beginning with that letter. I assume that this is a bug in jQuery's autocompletion in competition with the new HTML5 autocompletion. Perhaps a simple upgrade of jQuery would fix it?

Attachments (1)

keywordsuggestplugin-8141-itamaro-v1.patch (3.1 KB) - added by itamarost 3 years ago.
patch against keywordsuggestplugin trunk - fix several regressions with multisep-char

Download all attachments as: .zip

Change History (46)

comment:1 Changed 3 years ago by rjollos

I'll come back to this in a bit. For reference: http://bassistance.de/jquery-plugins/jquery-plugin-autocomplete.

comment:2 Changed 3 years ago by rjollos

  • Owner changed from scratcher to rjollos
  • Status changed from new to assigned

comment:3 Changed 3 years ago by rjollos

  • Resolution set to worksforme
  • Status changed from assigned to closed

This is working well for me with FF 7. Please test the 0.4 version of the plugin and reopen this ticket if you continue to have issues. I'd lean away from supporting browsers much older than FF 4, if even that.

comment:4 Changed 3 years ago by rjollos

Okay, I think I'm seeing the behavior you are talking about.

comment:5 Changed 3 years ago by rjollos

It looks like there are some updates for bgiframe and jQueryAutocompletePlugin, but I don't know anything about JavaScript and when I tried the upgrade I started seeing a bunch of errors:

Error: g is undefined
Source File: http://localhost:8000/tracdev/chrome/common/js/jquery.js
Line: 11

This will have to wait until someone else can provide a patch, or find some time to learn something about jQuery and JavaScript.

comment:6 Changed 3 years ago by rjollos

  • Cc hasienda added; anonymous removed

comment:7 Changed 3 years ago by rjollos

  • Resolution worksforme deleted
  • Status changed from closed to reopened

comment:8 Changed 3 years ago by rjollos

  • Cc itamarost added

comment:9 Changed 3 years ago by rjollos

  • Cc falkb added

I've upgraded the plugin from the jQuery plugin to jQuery UI 1.8.16. This is my first attempt at doing anything significant with jQuery, so please bear with me. It looks like there is a jQuery Autocomplete plugin and a jQuery UI autocomplete function. Are they different? If so, is there a reason to use one over the other? I haven't made an effort to research this yet, but plan to.

Here is the reference I used for writing (copying) the callback function.

I've followed the migration guide during this process. I'd appreciate any testing, code review and other feedback. I'm bumping the version to 0.5dev and will leave this ticket open for a little while to get feedback[. Please take a look at KeywordSuggestPlugin#News for more information on what is planned for this plugin.

The jQuery UI has a nice little download page for getting the jQuery components. For a theme, I grabbed UI Darkness, but I imagine it would be nice to be allow users to change the theme without modifying the source. Is there a standard way this is done for Trac plugin? If I was to make up some way to implement it, I'd add a theme option to trac.ini and instruct the user to prepare their theme from the jQuery UI download page, drop it in htdocs/css, and set the configuration option in trac.ini. The default would be theme = ui-darkness. Another way I could do this would be to grab all the themes, and provide a list that the user could pick from using the theme configuration option. I tend to like the latter option if it doesn't make the size of the source on checkout too large. Is there a Trac theme we could be using? UI-Darkness looks nice in 0.12, but current selection highlighting should probably be red rather than blue.

Another interesting example is the combobox. Would others like to have the configurable behavior of a drop-down for the list of suggestions? Another way a feature like this can be implemented is change the minLength parameter from 1 to 0 (1 is the default). I went ahead and did that in the latest change. Should this be a configurable option?

comment:10 Changed 3 years ago by rjollos

Another possible TODO: add an option for case-insensitive matching. Does anyone want this?

Another consideration: when the TagsPlugin is enabled and keywords are defined in the keywords configuration section of trac.ini, when on the wiki edit page, the list of suggestions will draw from both when the user is entering values in the Tag under field. Does it make good sense to do it this way? Maybe the keywords should only be used to provide suggestions for the ticket keywords field.

The working I'm about to commit won't support the mustmatch configuration option initially, but I'll get around to reimplementing that. Here is a reference.

comment:11 Changed 3 years ago by rjollos

(In [11005]) Refs #8141: (0.5dev)

comment:12 Changed 3 years ago by rjollos

(In [11006]) Refs #8141: Bumped revision. It should have been bumped in [11005].

comment:13 Changed 3 years ago by rjollos

(In [11007]) Refs #8141: FIX: matchcontains logic was inverted.

comment:14 Changed 3 years ago by rjollos

(In [11008]) Refs #8141:

  • Improved handling of input so that every entry is separated by the multipleseparator followed by a whitespace.

NOTE: This is not working well with the AutocompleteUsersPlugin. NOTE: Duplicate entries are not being removed.

comment:15 Changed 3 years ago by rjollos

Much of the code in [11008] was based on this demo.

comment:16 Changed 3 years ago by rjollos

Here is another issue: Trac 0.11.7 uses jQuery 1.2.6. Trac 0.12.x uses jQuery 1.4.4. It looks like the jQuery UI needs jQuery 1.3.2 or later. I might need to move the recent changes over to a 0.12 branch and use that for development going forward, leaving the 0.11 branch at r11004. Any suggestions on what to do here?

In addition to the noted conflict with the AutocompleteUsersPlugin, I'm seeing conflicts with the MenusPlugin. It looks like there might be some far-reaching jQuery compatibility issues to deal with here.

comment:17 Changed 3 years ago by rjollos

(In [11009]) Refs #8141: Added jQuery library 1.6.2, which was distributed with the jQuery UI. Initially, I was thinking this wasn't required because Trac has a version of the jQuery library, but it might be necessary so that the plugin works with older versions of Trac that have older versions of the jQuery library that don't support jQuery UI 1.8.16. The DateFieldPlugin comes with both jQuery UI 1.8.16 and jQuery 1.6.2, so I'm following the lead of that plugin.

comment:18 Changed 3 years ago by rjollos

As a next step, I'm going to try get AutocompleteUsersPlugin working with jQuery UI 1.8.16, and then maybe take a look at the MenusPlugin.

comment:19 Changed 3 years ago by rjollos

(In [11010]) Refs #8141: FIX: the appended comma was being enclosed in quotes.

comment:20 Changed 3 years ago by rjollos

(In [11011]) Refs #8141: FIX: Strip trailing whitespace and commas from the list of keywords.

comment:21 Changed 3 years ago by rjollos

(In [11012]) Refs #8141: Sort the keyword entries so that the list is alphabetical when pressing the down arrow to see all entries.

comment:22 Changed 3 years ago by rjollos

(In [11025]) Refs #8141:

  • Switch to using Blitzer theme, like DateFieldPlugin is using.
  • It looks like we don't want to include the jQuery library. It is provided by Trac, and loading two of them will result in conflicts.

comment:23 Changed 3 years ago by rjollos

(In [11027]) Refs #8141: Added missing part of example code that prevents overwrite of existing entries on selection.

From: http://jqueryui.com/demos/autocomplete/#multiple

comment:24 follow-up: Changed 3 years ago by itamarost

I really like the use of the new library and theme, but it seems some regression is introduced following these changes.

I prefer using a single whitespace as the character for multipleseparator, so the list of tags or keywords could look something like "tag1 tag2 etc", but using the current trunk, if I try setting multipleseparator to a single whitespace I encounter severe issues, and it looks like the plugin is trying to use the empty string as multipleseparator. For example:

  1. When submitting a page / ticket with tags / keywords, all values are concatenated (e.g. "tag1 tag2 etc" turns into "tag1tag2etc")
  2. The tags are split on every character instead on whole words

I tried replacing the two places in code that use self.multipleseparator to self.multipleseparator or ' ', but it didn't solve the first issue, and it doubled the spaces between words.

Is there a cleaner way to get the desired behavior?

comment:25 Changed 3 years ago by itamarost

Oh, another unrelated thing I forgot in the previous comment - it would be nice if the suggested completions list will not display values that already appear.

comment:26 in reply to: ↑ 24 ; follow-up: Changed 3 years ago by rjollos

Replying to itamarost:

Is there a cleaner way to get the desired behavior?

I'm not sure at the moment. I should have been far more careful in implementing the changes I did, and will be more careful in the future. There are a least two other know issues caused by the changes I made. I'd gladly take a patch, and will try to find some time to fix up these issues in the near future.

Perhaps it would be best to just go back to having quotes separate the delimiter. I probably did not even consider the case that a user would want whitespace as the delimiter when I made that change.

Changed 3 years ago by itamarost

patch against keywordsuggestplugin trunk - fix several regressions with multisep-char

comment:27 in reply to: ↑ 26 Changed 3 years ago by itamarost

Replying to rjollos:

Replying to itamarost:

Is there a cleaner way to get the desired behavior?

I'd gladly take a patch, and will try to find some time to fix up these issues in the near future.

Here's one :-)

Perhaps it would be best to just go back to having quotes separate the delimiter.

No need to bring back the quotes.

The patch I have attached does several things:

  • Remove the on-submit action - it seemed too complex to parse the list on client-side, and it seemed somewhat redundant to me given that the server already does that, and doesn't really care about whether you use comma or whitespace.
  • Assume the separator is a single whitespace if empty
  • In order to avoid doubling the inserted spaces - use trim() before appending the extra space
  • Handle splitting tags with mixed non-space and space separators (added |\s+ to the split regexp)

This patch should be used together with the patch to TagsPlugin in #9692, that uses the multisep-char from KeywordSuggestPlugin when present to populate the tags input field.

comment:28 Changed 3 years ago by rjollos

Thanks for the patch! I finally had some time to do some work on t-h.o tonight and now this ticket is very close to the top of my list.

comment:29 Changed 3 years ago by rjollos

  • Priority changed from normal to high

Sorry for the delay on this. I'll get the patch committed soon.

comment:30 Changed 3 years ago by rjollos

(In [11494]) Refs #8141: Created trunk, tags directory structure.

comment:31 Changed 3 years ago by rjollos

(In [11495]) Refs #8141: Created tag for v0.2.

comment:32 follow-up: Changed 2 years ago by rjollos

I couldn't see a reason for this statement, since the multiseparator option has a default.

'multipleseparator': self.multipleseparator or ' ', 

Thoughts?

comment:33 Changed 2 years ago by rjollos

  • Status changed from reopened to new

Sad how long this has take me to commit. My apologies.

In the future, I'd really like to get some functional tests working for this plugin.

comment:34 Changed 2 years ago by rjollos

(In [12094]) Refs #8141:

  • Allow whitespace as a separator. The default separator, if not otherwise specified, is whitespace.
  • Trim extra whitespace when selecting a keyword.
  • Removed fix-up of keyword list on the client-side. This is done on the server side (although there appears to be room for improvement there).

comment:35 Changed 2 years ago by rjollos

  • Status changed from new to assigned

Testing and feedback is appreciated. More patches are welcome. I promise rapid application of any patches in the future. No more 9 month delays ;)

comment:36 Changed 2 years ago by rjollos

I meant to credit itamarost in the commit message for [12094]. Sorry about that!

comment:37 in reply to: ↑ 32 ; follow-up: Changed 2 years ago by hasienda

Replying to rjollos:

I couldn't see a reason for this statement, since the multiseparator option has a default.

'multipleseparator': self.multipleseparator or ' ', 

Thoughts?

I would have reacted like you, but may it be to prevent invalid configuration, that is setting it to the empty string ('') more or less knowingly? This is non-sens, but I can't estimate behavior with such a setting - might play havoc on the code?

comment:38 in reply to: ↑ 37 Changed 2 years ago by rjollos

Replying to hasienda:

I would have reacted like you, but may it be to prevent invalid configuration, that is setting it to the empty string ('') more or less knowingly? This is non-sens, but I can't estimate behavior with such a setting - might play havoc on the code?

I did some brief testing last evening, and the empty string case seemed to work okay if I remember correctly, but let me not convince you with my words, rather with some unit tests ;)

comment:39 Changed 2 years ago by rjollos

I was having some trouble setting up the unit test. I kept getting this error when running them:

  File "/home/user/Workspace/trac-hacks/keywordsuggestplugin/trunk/keywordsuggest/tests/__init__.py", line 15, in test_suite
    import keywordsuggest.tests.keywordsuggest
  File "/home/user/Workspace/trac-hacks/keywordsuggestplugin/trunk/keywordsuggest/tests/keywordsuggest.py", line 13, in <module>
    from keywordsuggest.keywordsuggest import KeywordSuggestModule
ImportError: No module named keywordsuggest

I think it has something to do with the directory and py file having the same name. Renaming keywordsuggest.py -> web_ui.py resolved the issue. I reverted and reapplied this change twice and saw the same result each time. I don't particularly renaming the file, but don't see any other solution (other than renaming the directory, which I don't like either).

comment:40 Changed 2 years ago by rjollos

If you put a pair of empty double-quotes as an option value in trac.ini, the value of the option will be an empty string. If you put a pair of single-quotes as an option value in trac.ini, the value of the option will be a pair of single-quotes. So, although this is a corner case, itamarost was right in his original patch, that we need to protect against this. Only, I don't like doing this in two places, so I'll make a property to handle this, and also strip off single quotes in case the user enters something like ',' as the separator value in trac.ini.

comment:41 Changed 2 years ago by rjollos

(In [12101]) Refs #8141:

  • Strip single quotes from the multipleseparator Option value. This allows the user to specify a single whitespace separator as ' ' (which is not necessary since a single whitespace is the default), or even dream up separators such as double whitespace as ' '. A single whitespace character is used as the separator in the case the user specifies ''. Thanks to itamarost for pointing out this corner case in his original patch.
  • Wired up unit tests.
  • Renamed keywords.py to web_ui.py, to work around an issue described in comment:39, in which the keywordsuggest.keywordsuggest module was not being found by the keywordsuggest.tests.keywordsuggest module when running unit tests.

NOTE: If you have enabled the plugin through webadmin or by editing trac.ini with keywordsuggest.keywordsuggest.* = enabled, you will need to re-enable the plugin through webadmin, or edit the line to keywordsuggest.web_ui.* = enabled (or keywordsuggest.web_ui.keywordsuggestmodule = enabled).

comment:42 Changed 2 years ago by rjollos

It would be nice if we could move that JS code string out of the py file, for the sake of maintainability. add_script_data isn't available in Trac 0.11, but Jun described an approach in comment:6:ticket:10240.

comment:43 Changed 2 years ago by rjollos

(In [12103]) Refs #8141:

  • Moved generation of keywords string into a private method.
  • Several other refactorings, backed by unit tests.
  • In Trac 1.0 and later, jQuery-UI from the Trac core is added to the page. The ability to add jQuery-UI to the page was added in Trac 1.0.

comment:44 Changed 2 years ago by rjollos

jQuery-UI should be upgraded from 1.8.16 to 1.8.24 before we release version 0.5 of the plugin.

comment:45 Changed 18 months ago by rjollos

  • Status changed from assigned to new

Add Comment

Modify Ticket

Action
as new The owner will remain rjollos.
Author


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

 
Note: See TracTickets for help on using tickets.