GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing static-codereview.wikimedia.org — r94702 MediaWiki - Code Review archive r94702 MediaWiki - Code Review archive
Diff [purge]
Follow-up revisions
Past revisions this follows-up on
CommentsSummary: works. Further comment regarding the way, the highlighter works (it splits the "needle" into space-separated "needles", which are then searched and marked). Hi, I succesfully and for the first time used http://localhost/tests/qunit/?filter=jquery.highlightText%3A%20Check . this is a neat tool, thanks for the hint. In my local copy I added some more test vectors, which I will commit later. The current function $.highlightText says clearly in the beginning "// Split our pattern string at spaces and run our highlight function on the results" Thus the result of the highlighter of "Blue Öyster Haystack" with needle "Blue Ö" is <span class="highlight">Blue</span> <span class="highlight">Ö</span>yster Haystack i.e. "Blue" and "Ö" matched and marked separately. Others (like me) would (perhaps) expect to receive <span class="highlight">Blue Ö</span>yster Haystack which looks smarter when you have the highlight function in suggestions.js with extension Vector. I just wanted to point out, that the current function indeed treats a sequence of words as single space-separated terms, which are then searched, and the matches are highlighted separately. I am going to add test vectors and expected results for this behaviour. highlight still fails for test case 025a committed in rev r95341 That highlight appears to work; it just matches in two places, while your expected text only lists one. "« L" was the "needle" - why should it match with "..... l´île", where the "«" is not present? the highlighter matches space separated string separately... thus it looks for "«" and for "L" Well now we're getting into the ultimate problem: this highlighter code doesn't necessarily know anything about how the matches are being made by the search engine. Do multiple words/symbols/tokens need to appear in the same order? Not according to what jquery.highlight does, since it breaks them into separate tokens and looks for them individually. The default opensearch prefix search implementation does raw prefix matches only, so would not tend to match something like that -- what you get out of Lucene / MWSearch might be different. tagging for back porting to 1.18 and also 1.17wmf1 Why is this tagged 1.17wmf1? The revision that it fixes doesn't seem to have been deployed. I wanted to say that Brion's fix + var match = node.data.match( new RegExp( "(^|\\s)" + $.escapeRE( pat ), "i" ) ); + if ( match ) { + var pos = match.index + match[1].length; // include length of any matched spaces is fine and should be backported to 1.17wmf1. I was not sure how exactly to communicate this to you. In my view it should be patched into this http://svn.wikimedia.org/viewvc/mediawiki/branches/REL1_17/phase3/resources/jquery/jquery.highlightText.js?revision=79129&view=markup revision. Aha, I do see the code in 1.17wmf1 now. I'll merge it later. I've merged this along with r90092 to 1.18, 1.17, and 1.17wmf1 now. (1.18 had the earlier commit but not its correction here) Status & tagging log
| |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| You are visitor 01402503 — GeezerVison does not store the pages you look up. Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||