Monster issue

Related to this is the number of characters entered before a match starts. Right now, the native node search does 1, the Drupal search does 3, and I'm not sure what solr does. Further, the Drupal search generates errors with less than 3.

As part of this issue, I plan to increase the number of characters typed before searching starts to 3. Currently the code deletes the matches when there are 0 (unless prepend is on) and matches when there are >0 characters.

I propose:
0: delete the results
1-2: leave results as they are
3: match

I could see the logic in deleting the results for 1-2, though. Anyone have any thoughts on this?

In working on this, I've expanded the scope of issue. I plan to offer a patch for comment which:
- Triggers search upon page load, providing eliminating duplicate effort in PHP and fixing a bug with duplicate content
- Limits results to N; queries on N+1 and adds "... and more" after the item list
- Starts out with the inline fieldset collapsed, and expands it if there are matches. Block is unchanged.
- Replaces message inside field set with "success" message if there is no duplicated content and title exists

My focus is on the "replace contents" option. I'm unsure of the use case for "prepend, and am unsure of the desired behavior, so definition and/or work will be needed for that.

My goal is to provide a coherent upgrade to giving good interactive feedback without warning the user unnecessarily.

The attached patch is relative to:

1. 6.x-1.x-dev (Sept 22, 2010) +
2. 722118-6-search-modes.patch +
3. 729430-17-inline-widget.patch +
4. 899492-2-documentation.patch +
5. 941944-5-flexible-urls.patch

It incorporates functionality from Enhanced functionality when title is empty versus matching versus unique.

1. User Interface
1. Uniqueness scope - all nodes or just within content type
2. Results replaced or prepend - existing functionality relocated to Search fieldset
3. Max Results - limit the number of responses. Validated to be within 1..100, or 1..10 for Drupal core search.
4. Default Description - added help text (form '#description')
2. Widget operation
1. "uniqueness-dyn-span" changed to general purpose notifier, and changed to p element to prevent mixed content. If using Replace type search, notifier is placed below list to prevent content flickering during search. For prepend, it left above the list because the list may be very long.
2. Search is only triggered when there are 3 or more characters in the title. This both prevents excess searches early in the typing process and also prevent the query from being too short for drupal core, generating errors.
3. An initial search is triggered upon page load based on title field (if the title is 3 characters or longer).
4. Fieldset starts out collapsed and is automatically expanded when there is relevant contents within it.
5. Default description is replaced with "Success! No duplicates found." if a valid title (>= 3 characters) returns no duplicates. "Success!" maybe themed with CSS. Recommend .uniqueness-success be color #393 and bold, or similar.
6. If sufficient search results are returned that it is clear there are additional results, then "... and others." is added after the list of results.
7. Server-side generation of duplicate results has been disabled (but not removed). This fixes a bug where changing the title while previewing shows two versions of the related contents (one static, one live). It also supports "edit node" forms.
8. Support for (optionally) matching within content type and (always) not matching the current node for node and drupal core searches. Solr needs implementing by others (if desired).

I have tested the cases of most interest to me: node searches, in-line widget, replace results. I have done some testing for prepend, but since I'm not sure of the use case, there may be corrections needed. I have tried to not introduce bugs for features I'm not using (i.e. solr, prepend, block widget, and taxonomy search), but I'm not sure that the features I've introduced are implemented either fully or correctly for these other cases. This is not because I don't care, but either because I'm not sure what the desired behavior is (prepend and taxonomy in particular) or because I don't have the ability to code and test (e.g. solr). Also, some of these features may be abandoned (unsure).

Please consider this patch for comment. I assume that other users may want things implemented differently or may find defects. If the patch is largely satisfactory, it may be most efficient to commit it (or a revision) so that other contributors can start with it as a base for their revisions and further work.

Reply to bforchhammer from Enhanced functionality ... thread:

However I prefer separate patches ...

If you look at the diff, I think you'll see how intertwined the features out. They would have to be individually submitted, reviewed and committed serially -- at a huge cost in time. Please consider accepting this as a "version++" of sorts.

[The uniqueness in-line fieldset starts out collapsed.] ... Not sure I agree with your use case

Suggest you try it before adding a config option. Works beautifully for my use. There is nothing distracting to read if you start out with unique content. You only see the related content when there is some. My use case is that I need unique titles and I don't want to pester the user if they are already unique.

Duplicate-results bug?

Didn't bother reporting the bug since it was fixed as a side effect of something that I needed to implement anyway (for edit). The bug is if you preview and then type more in the title, you see two copies of the "related content". The static one was generated by the server and the dynamic one was generated by the widget. It it were highly desirable to avoid the jQuery request upon page load (and I don't think it is), then the widget would have to look for HTML generated by the server before adding it itself, and the server would have to be updated to generate identical content (which it doesn't). Assuming this patch is committed, I suggest we ignore the bug that doesn't exist anymore, or alternatively create the issue and then report it as fixed by this issue's patch.

Needs to be implemented for Solr, definitely needs configuration option.

I suggest we commit this patch (once reviewed and corrected as needed), then someone else can work on implementing solr. I have passed the parameters to the solr code and added an @todo for someone else. There is no configuration work needed, to my knowledge.

... and translation support ... make separate issues

Agreed. My code is no more or less translatable than the previous code. Whereas "Searching ..." was hard coded, two more English phrases are. Again, I suggest we work on that after this patch is committed and we have a common codebase. I'll open an issue for it.

That 10 search result limit with Drupal needs a note ...

Yes. Note is already implemented. Hard coded in core. Can't be changed without duplicating node_search

See #899480: Configurable search content types if you haven't already.

Yes. Took my functional specs from there. Implemented as envisioned in 899480's last post (simplified -- content types match within themselves, optionally). As an example, the content type and current node id are communicated in the URL: uniqueness-search/CONTENT-TYPE/NID/?title=SEARCHED-FOR. It would be difficult to implement these two features separately since the same lines of code create and decode the URL.

Thanks very much for stepping up as co-maintainer. I look forward to working with you. If you start by committing your first three patches, I can re-roll my two to apply serially without offset or fuzz. Flexible URLs should be committed before this patch.

Thanks for the review.

1) I too prefer having content-type be the default scope. I only suggested "all" for compatibility. I'll change it.
2) I just moved the block, but I'll fix the grammatical error. ;)
3) Good improvement, but would be even better if condition were put first. In this way, the user can determine whether the sentence relates to him/her without having to read the whole thing. So maybe:
Limit the number of search results. (For "Drupal Search", must be 10 or fewer.)
4) Also good, but I don't think it is help text and "being" implies action without providing more info. Maybe:
Heading displayed above the results within the widget.
5) Thanks for catching the "be". There is, however, no need for sanitizing a numeric value calculated in the line above. ! is the correct placeholder, I believe.
6) I think we need an option. If <3, then there is a bug with Drupal Search because a stream of errors will be displayed on the next page load, generated by the core search routine. In my use case typing a letter causes tons of results.
7) I considered where to communicate type and nid. I decided that the values array is best used to communicate the query specs from what the user typed -- what is needed based upon what the user did. The type and NID are static within one page, and would seem to be best conveyed in the URL. They could also go in the session, but I think that may be undesirable. (I'm no expert, but I have been reading that anonymous sessions may interfere with high performance caching. While node editing isn't usually anonymous, it could be.) Absent a good reason to change, I think it is implemented appropriately.

3) The comma is optional after the prepositional phrase, but helpful for separation and corresponds to a natural pause when said aloud. The comma should probably go inside the quotes as required by proper punctuation rules, though. (Often this isn't done in computer use because the quoted text may be something that should be typed literally.)

4) I think the default "signal-to-noise" text should go, but I let someone else tend to that if they want. To me, it is a heading introducing exactly what is below. When used with the collapsed fieldset. the text is hidden when you start, displayed when you're in an "invalid" state with duplicates, and then replaced with the "Success!" message. Maybe:
Explanation displayed within the widget before the list of duplicates.
Or "Introduction displayed ..."?

6) I strenuously don't want it for < 3 characters for node title. If I want this, I bet other users will too. I think an option is in order.

7) I'll move the search specs to the values array. Of course, it could be "q=CONTENT-TYPE/NID". j/k

This patch incorporates the feedback from above.

1. I did not implement Solr -- specifically not matching the current node, not limiting to content types, and not limiting the number of results. Someone else who knows and can test Solr will have to do this.
2. I did not include an option to start the widget's fieldset expanded. It is only collapsed, however, when it has nothing useful to say: when there is nothing to match on yet or when the match is initially successful. I can add an option if your use case requires it.
3. I removed the unneeded code for preview and for server-side theming as part of the cleanup.

N.B. Depends upon current head + 941944-2-add-edit.patch (aka Flexible URLs).

As far as I can tell, there are only three people who can test this -- the three active contributors. I have this patch deployed on my live site at Kindred Cocktails. You are welcome to create an account and create a test cocktail. Please delete it when you are done. And of course, you folks can install the patch for your own use and test it.

I can easily resolve the translation issues if you tell me how you want to proceed.

If you tell me you want to customize the fieldset starting state, I'll include that. I think our use cases are different because in my use I can't imagine wanting it visible in its initial state -- rather like putting up a warning sign for something that hasn't happened yet.

Clearly we both need different settings for minimum characters, so I think that is resolved.

As for Solr, I simply can't implement it. If no one else is going to implement it, I can validate the inimplemented setting combinations, but this is putting in code to go the wrong direction (which would be implementing it).

Since this is pre-release, I think we would be far better off with this patch (and flexible URLs) as a basis for polishing off any remaining issues. I'm not saying you shouldn't test it, but that it moves the module much closer to something that is universally useful to a larger audience.

I have added:

1. Success message now stored in variable table.
2. Removed the <span ...> from default success message, since user can now enter anything they want.
3. Created a one-line helper function uniqueness_filter_xss_inline, which allows only reasonable in-line tags. Needed because filter_xss doesn't allow span and filter_xss_admin allows many unreasonable block elements.
4. Use uniqueness_filter_xss for fieldset legend and block title; standard description; and unique description (success message).
5. Added Drupal.t() around "Search ..." and "... and more" in JavaScript.

I will re-roll once the translation patch (in whatever form it takes) and flexible url patches are committed.

Ben - would you be willing to implement the new features for solr -- limiting the number of responses, eliminating the current node from the results, and matching on content type?

Benedikt - would you be willing to look at the above patch to see if the collapsed fieldset meets your needs?

Once 948442-translation is committed, I will rework the patch for this issue to:

1) Incorporate the theming changes from 948442.

2) Move the generation of the inline fieldset into the theming function to offer complete user flexibility for the inline HTML. They can start the field set out collapsed, expanded, or omit it entirely, all without having to complicate the admin UI further.

3) This patch already addresses the issue of generating the initial content for node-edit form and removes the server-side related content list generation, which fixes the theming differences between what the JS generates and what the drupal PHP theme generates.

The patch represent work begun 3-4 patches ago and not yet committed. Much of the changes are interrelated, dealing with the same (or nearly same) lines of code. Read the patch and you'll see.

I haven't updated the batch because there isn't consensus on at least one feature. If we get agreement, then I can update the patch accordingly.

This patch eliminates the server-side content generation, relying on the Javascript for all list theming. The advantage is that a) the work is done only once -- less code / more reliable / simpler and b) the generated HTML is guaranteed to be identical between the two cases (initial page load versus interactive). The disadvantage is that the functionality is lost for non-JS user agents, such as screen readers.

We could do it all in the client, revert to list theming in both the server and client, or we could do it all in the server.

If we do it all in the server, there is a complication of keeping the list of already-processed nodes to prevent duplicates for the prepend case (from AJAX request to AJAX request). There would also be the option to send more strings to the client from the theme function. Some strings aren't initially visible. These could possibly be sent with inline CSS to hide them. The client would then extract the DOM elements, remove the CSS, and use them as needed. (This technique could also be used for the other implementation strategies as well.)

Continuing in this vein, the entire work of output generation could be moved to the server, with the client just sending the query (title) and getting fully-formatted HTML block content or inline content (including fieldset, for example). This would put more work onto the server and might be slower. It would have the advantage of putting all the logic that one might want to customize, and all the strings, into the theme function so that a theme developer could adapt it. It would remove the need to have some of the lesser-needed options in the admin UI. (The "Searching..." string comes immediately to mind.)

If it is a requirement that we support non-JS user agents, then something has to change. Speaking only for myself, I am OK with requiring JS because I feel this feature is helpful but non-essential functionality. However, obviously the maintainers need to set the requirements.

Note: I have two nearly back-to-back weeks of vacation coming up, so I don't expect to have much time in the next 3 weeks or so.

I now have an immediate need for multiple appearance settings -- one for each content type. These settings are the ones in the fieldset in the patch above. This provides the ability to have prompts and text that is specific to the content type.

I suggest that the global settings be left as above, and a checkbox added to the settings for each content type to the effect of "override global uniqueness settings" (so similar). This would enable and expand a fieldset below containing the same settings. Then the code would be refactored to use the content-specific setting if override is enabled for that content type.

This will be a fair bit of editing of the code as the settings are scattered here and there. However, I'm working on a fork since the above patch has not been committed. If there is a road map, I can work toward it. If my needs have diverged from others, I can simply continue in the fork, with the possibility of future merging harder and/or less likely.

If the above patch is OK to commit with the following changes, I will make them:
1) Reinstate the PHP theming so that javascript-less clients can see the duplicated content list.
2) Document which features do not work with SOLR. Since I think SOLR will eventually be implemented, I suggest this be in a readme or install.txt file, rather than code itself.

I appreciate the effort to cherry-pick this patch and commit portions of it. When this is complete and committed, we can see if there are any remaining tidbits of meat on the carcass worth including, commit them, and then use that as the basis for future work, such as custom settings (including prompts) per content type.