KeywordSuggestPlugin does not work well with Ffx4.0 betas

[dustin@…]

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?

[Ryan J Ollos]

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

[Ryan J Ollos]

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.

[Ryan J Ollos]

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

[Ryan J Ollos]

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.

[Ryan J Ollos]

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?

[Ryan J Ollos]

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.

[Ryan J Ollos]

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

• Upgraded from the ​jQuery Autocomplete Plugin to ​jQuery UI 1.8.16.
  • The mustmatch option is currently disabled for this version.

[Ryan J Ollos]

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

[Ryan J Ollos]

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

[Ryan J Ollos]

(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.

[Ryan J Ollos]

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

[Ryan J Ollos]

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.

[Ryan J Ollos]

(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.

[Ryan J Ollos]

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.

[Ryan J Ollos]

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

[Ryan J Ollos]

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

[Ryan J Ollos]

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

[Ryan J Ollos]

(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.

[Ryan J Ollos]

(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

[Itamar Ostricher]

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?

[Itamar Ostricher]

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.

[Ryan J Ollos]

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.

[Itamar Ostricher]

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

[Itamar Ostricher]

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.

[Ryan J Ollos]

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.

[Ryan J Ollos]

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

[Ryan J Ollos]

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

[Ryan J Ollos]

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

[Ryan J Ollos]

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

'multipleseparator': self.multipleseparator or ' ', 

Thoughts?

[Ryan J Ollos]

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.

[Ryan J Ollos]

(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).

[Ryan J Ollos]

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

[Ryan J Ollos]

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

[Steffen Hoffmann]

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?

[Ryan J Ollos]

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 ;)

[Ryan J Ollos]

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).

[Ryan J Ollos]

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.

[Ryan J Ollos]

(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).

[Ryan J Ollos]

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.

[Ryan J Ollos]

(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.

[Ryan J Ollos]

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