? Success

User tests: Successful: Unsuccessful:

avatar dgt41
dgt41
17 Nov 2014

Replaces the mootool with jquery

This is a replacement of the mootools script with a jquery equivalent.
The script was taken from here and is providing the same functionality with the old one!

Test

Make sure you have data and you indexed it
1. In admin create a menu for finder
2. In the finders options enable suggestions but disable highlighting (it will load mootools)
3. Try it in the front end

screen shot 2014-11-17 at 9 11 18

avatar dgt41 dgt41 - open - 17 Nov 2014
avatar jissues-bot jissues-bot - change - 17 Nov 2014
Labels Added: ?
avatar dgt41 dgt41 - change - 17 Nov 2014
The description was changed
avatar dgt41
dgt41 - comment - 17 Nov 2014

@infograf768 @Bakual Any thoughts on including someone else’s code? I used this script https://github.com/devbridge/jQuery-Autocomplete which is MIT licensed.

avatar chrisdavenport
chrisdavenport - comment - 17 Nov 2014

JavaScript which is MIT licensed should be fine. We already include other such libraries. However, I think the 3rd-party code should be unmodified (we don't want to have to maintain 3rd-party code) and should be placed in the /media directory.

avatar dgt41
dgt41 - comment - 17 Nov 2014

@chrisdavenport It is not modified at all, simple copy paste here! I guess it’s better to use their minified version and not the uncompressed that I included...

avatar infograf768
infograf768 - comment - 17 Nov 2014

@dgt41
It is always better to include both as we use the uncompressed when debug

avatar dgt41 dgt41 - change - 17 Nov 2014
The description was changed
avatar dgt41
dgt41 - comment - 17 Nov 2014

@chrisdavenport @infograf768 I did include both scripts (norm, min) and adjust the call to reflect that. Now should be clear that these are unmodified

avatar chrisdavenport
chrisdavenport - comment - 17 Nov 2014

@test failed

Multiple word searches don't work. I think you need to change line 61 of components/com_finder/models/suggestions.php to use "string" filter instead of "word".

Smart Search module code also needs to be changed to use the new autocompleter.

avatar dgt41
dgt41 - comment - 17 Nov 2014

@chrisdavenport Good catch, updating both

avatar chrisdavenport
chrisdavenport - comment - 17 Nov 2014

Another thought has just occurred to me. Many people, myself included, will have used template overrides to customise the component and module output. Since the output from suggestions.php is now different, any site using such overrides will break.

In order to maintain backwards compatibility we will need to continue providing the same output for Ajax calls and continue providing the Mootools autocompleter. Consequently, the jQuery version will need to use a different URI for its Ajax calls so that both versions will work.

avatar dgt41
dgt41 - comment - 17 Nov 2014

Good point. I am about to leave my desk now, but I will provide the changes needed for that later tonight

avatar dgt41
dgt41 - comment - 17 Nov 2014

@chrisdavenport I pushed some code with:
1. Keep all the old stuff (controller, model, css and js)
2. I named the new controller and model: suggester (not my brightest moment :) )
3. I added some logger for the model (do we have to include for every method? )
B/C should be solid

avatar chrisdavenport
chrisdavenport - comment - 17 Nov 2014

I'm seeing a strange problem with the autocompleter behaviour. With the default sample data, I tried entering the search term "the installation". As I type part way through "installation" this brings up two possibilities: "the installation process" and "the installation". However, as soon as I finish typing "the installation", both suggestions disappear! If I then try to type a space it gets removed. If I try to enter the search term "the installation process" it always gets shown as "the installationprocess" and the search fails. Very strange. Seems to happen whenever I try to enter a search term which brings up a suggestion with more than 2 words in it. In other words, it's impossible to search for a 3 word phrase.

The new code also seems to be generating a lot more Ajax requests than the old code. I think it gets called on every keystroke whereas the old code had some sort of timer so a fast typist could not overwhelm it.

Also, could you change the POST to a GET? No point in propagating bad practice into the new code.

Note: Also need to check that it works in an environment with multi-byte characters.

avatar dgt41
dgt41 - comment - 17 Nov 2014

@chrisdavenport Can you test it without this line ?

avatar chrisdavenport
chrisdavenport - comment - 17 Nov 2014

Which line? Your link takes me to a diff with no particular line highlighted.

avatar dgt41
dgt41 - comment - 17 Nov 2014

line 69 default_form.php mute this: delimiter: /(,|;)\s*/,

avatar chrisdavenport
chrisdavenport - comment - 17 Nov 2014

No, still not working. :-(

avatar chrisdavenport
chrisdavenport - comment - 17 Nov 2014

Sorry, I amended the wrong file! Yes, that fixes the issue.

avatar dgt41
dgt41 - comment - 17 Nov 2014

This is ultra weird, i just tested myself. The think is that their demo site accepts spaces, try United Arab Emirates. Should be some setting, down to the manual...

avatar dgt41
dgt41 - comment - 17 Nov 2014

The old code had delay: 1000 but there is a timer here as well so i will set it back to 1 sec.
Also the get comes in the next push.
I have a greek site, somewhere, i’ll try to test it there

avatar chrisdavenport
chrisdavenport - comment - 17 Nov 2014

I've just been playing with deferRequestBy. I think 1000 seems a bit sluggish and I prefer 500. What do you think?

avatar dgt41
dgt41 - comment - 17 Nov 2014

Yes, 1000 is far too slow, 500 seems perfect to me

avatar chrisdavenport
chrisdavenport - comment - 18 Nov 2014

Okay, this is working well for me. I'm going to look at code style now. Nearly done, thanks for hanging in there. :-)

0339bd2 18 Nov 2014 avatar dgt41 CS
avatar dgt41
dgt41 - comment - 18 Nov 2014

@chrisdavenport As I promised I tested this with Greek language and it works like a charm

screen shot 2014-11-18 at 4 53 24

avatar roland-d roland-d - test_item - 18 Nov 2014 - Tested successfully
avatar roland-d roland-d - alter_testresult - 18 Nov 2014 - chrisdavenport: Tested successfully
avatar roland-d
roland-d - comment - 18 Nov 2014

@test: All good. Well done.

@chrisdavenport You mentioned it should be in the /media folder and @dgt41 has done this but in a subfolder named com_finder. Should it be there or in a more vendor specific folder?

This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5128.

avatar chrisdavenport
chrisdavenport - comment - 18 Nov 2014

@roland-d Good question. The autocompleter is useful more generally than just Smart Search. What folder do you suggest?

avatar dgt41
dgt41 - comment - 18 Nov 2014

I vote for /media/jui/js

avatar chrisdavenport
chrisdavenport - comment - 18 Nov 2014

/media/jui/js sounds good to me.

avatar chrisdavenport
chrisdavenport - comment - 18 Nov 2014

Sorry, I'm going to throw a small spanner in the works. I think the code can be simplified quite a bit...

By adding "paramName: 'q'" to the autocompleter Ajax call, you can avoid having to create new FinderControllerSuggester and FinderModelSuggester classes. If you add a suggest() method to the existing FinderControllerSuggestions class which does basically the same thing as the existing display() method, except for the slightly different JSON output, then call it using "&task=suggestions.suggest" instead of "&task=suggestions.display", I think that should work.

You can even extract out the common parts of the display() and suggest() methods into an internal protected method in order to avoid the code duplication entirely.

And if you can figure out how to get the autocompleter to use the existing JSON then you can avoid having to add the suggest() method entirely. Not sure if that's possible though, I didn't understand the documentation on that point.

avatar dgt41
dgt41 - comment - 18 Nov 2014

But the script expects

{
   “suggestions:”
    [
        JSON data
    ]
}

Probably we can alter our output in the client side and add the missing parts… Let’s give it a try

avatar dgt41
dgt41 - comment - 18 Nov 2014

This code works:

    public function suggest()
    {
        $return = array();

        $params = JComponentHelper::getParams('com_finder');
        if ($params->get('show_autosuggest', 1))
        {
            // Get the suggestions.
            $model = $this->getModel('Suggestions', 'FinderModel');
            $return = $model->getItems();
        }

        // Check the data.
        if (empty($return))
        {
            $return = array();
        }

        // Use the correct json mime-type
        header('Content-Type: application/json');

        // Send the response.
        echo '{ "suggestions": ' . json_encode($return) . ' }';
        JFactory::getApplication()->close();
    }
avatar chrisdavenport
chrisdavenport - comment - 18 Nov 2014

Yes, that's what I was suggesting (no pun intended).

Now, you have two methods, display() and suggest() that are identical except for the last couple of lines, so consider extracting the duplicate code into a separate (protected) method, getSuggestions(), say. So you end up with something like this

public function suggest()
{
    $suggestions = $this->getSuggestions();

    // Use the correct json mime-type
    header('Content-Type: application/json');

    // Send the response.
    echo '{ "suggestions": ' . json_encode($suggestions) . ' }';
    JFactory::getApplication()->close();
}

and similarly for the display() method.

But if you can get the new script to work with the old JSON output, then even better.

avatar dgt41
dgt41 - comment - 18 Nov 2014

Would you like to review, before I push the code:

<?php
/**
 * @package     Joomla.Site
 * @subpackage  com_finder
 *
 * @copyright   Copyright (C) 2005 - 2014 Open Source Matters, Inc. All rights reserved.
 * @license     GNU General Public License version 2 or later; see LICENSE
 */

defined('_JEXEC') or die;

/**
 * Suggestions JSON controller for Finder.
 *
 * @since       2.5
 */
class FinderControllerSuggestions extends JControllerLegacy
{
    /**
     * Method to find search query suggestions. Uses jQuery and autocopleter.js
     *
     * @return  void
     *
     * @since   3.4
     */
    public function suggest()
    {
        $suggestions = $this->getSuggestions();

        // Use the correct json mime-type
        header('Content-Type: application/json');

        // Send the response.
        echo '{ "suggestions": ' . json_encode($suggestions) . ' }';
        JFactory::getApplication()->close();
    }

    /**
     * Method to find search query suggestions. Uses Mootools and autocompleter.js
     *
     * @param   boolean  $cachable   If true, the view output will be cached
     * @param   array    $urlparams  An array of safe url parameters and their variable types, for valid values see {@link JFilterInput::clean()}.
     *
     * @return  void
     *
     * @since   2.5
     * @deprecated 3.4
     */
    public function display($cachable = false, $urlparams = false)
    {

        $suggestions = $this->getSuggestions();

        // Send the response.
        echo json_encode($suggestions);
        JFactory::getApplication()->close();
    }

    /**
     * Method to retrieve the data from the database
     *
     * @return  array The suggested words
     *
     * @since 3.4
     */
    protected function getSuggestions()
    {
        $return = array();

        $params = JComponentHelper::getParams('com_finder');
        if ($params->get('show_autosuggest', 1))
        {
            // Get the suggestions.
            $model = $this->getModel('Suggestions', 'FinderModel');
            $return = $model->getItems();
        }

        // Check the data.
        if (empty($return))
        {
            $return = array();
        }

        return $return;
    }
}
avatar chrisdavenport
chrisdavenport - comment - 18 Nov 2014

You need to put "header('Content-Type: application/json');" into the display method too.

There's a couple of trivial code style issues, but otherwise looks good.

avatar roland-d
roland-d - comment - 18 Nov 2014

/media/jui/js sounds good to me too.

avatar dgt41
dgt41 - comment - 19 Nov 2014

@roland-d @chrisdavenport I already put the scripts in media/jui/js. One small consideration: in case that com_finder becomes a candidate for removal, the same story as weblinks, these files should still remain there, otherwise moving them there is really useless. Just a thought...

avatar roland-d
roland-d - comment - 20 Nov 2014

@dgt41 We should keep the library there even after removal of the components. Since it is there, 3rd party developers may start using it as well.

avatar brianteeman brianteeman - change - 20 Nov 2014
Category JavaScript Search
avatar smanzi
smanzi - comment - 25 Nov 2014

@test success @dgt41 Very nice job Dimitris! :+1:

Have you tried it in a multilingual environment? There used to be a bug that when one language was selected, content flagged as "All" (languages) was not searched. I don't have a multilingual setup now, but as soon as I'll have the time I'll build one and let you know...

avatar smanzi smanzi - test_item - 25 Nov 2014 - Tested successfully
avatar dgt41
dgt41 - comment - 25 Nov 2014

@smanzi I think for this responsible is the code in the model, and this PR didn’t touch that. I tested this with greek lang, but only in terms of picking the right words (accents, punctuation etc). Looking at the model and the code used here I can see this:

        // Set the query language
        if (JLanguageMultilang::isEnabled())
        {
            $lang = JFactory::getLanguage()->getTag();
        }
        else
        {
            $lang = FinderIndexerHelper::getDefaultLanguage();
        }

        $lang = FinderIndexerHelper::getPrimaryLanguage($lang);
        $this->setState('language', $lang);
avatar smanzi
smanzi - comment - 25 Nov 2014

@dgt41
I'm seeing something strange: suggestion for a term that appears only inside an <h1> tag ("English") is displayed also out of context and concatenated (no space) with other terms.

See attached screenshot.
capture

I'm unsure if this is a problem with the indexer or your suggestion JS.

How can I help? Going on Skype now...

avatar smanzi
smanzi - comment - 25 Nov 2014

Tested without this PR: same result. So this is not a problem introduced by your PR!

avatar chrisdavenport
chrisdavenport - comment - 25 Nov 2014

@smanzi Please open a new issue on the tracker. I suspect it might be due to a known problem in the HTML parser. I've been working on a fix for it, but not quite finished yet.

avatar smanzi
smanzi - comment - 25 Nov 2014

OK, will do later (going to lunch now...) and... there is more not functioning in com_finder (especially in a multilingual environment....)!

avatar smanzi
smanzi - comment - 25 Nov 2014

@chrisdavenport: I just created issue #5204 for the above and other com_finder issues...

Thanks!

Sergio

avatar waader
waader - comment - 27 Nov 2014

Can this be marked as RTC or does it need further testing?

avatar smanzi
smanzi - comment - 27 Nov 2014

This has three tests... I don't know why it is not RTC yet...

avatar roland-d roland-d - change - 27 Nov 2014
Status Pending Ready to Commit
avatar roland-d
roland-d - comment - 27 Nov 2014

Moving to RTC, 3 successful tests.

This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5128.

avatar brianteeman brianteeman - change - 29 Nov 2014
Labels Added: ?
avatar zero-24 zero-24 - close - 13 Dec 2014
avatar wilsonge wilsonge - close - 13 Dec 2014
avatar wilsonge wilsonge - change - 13 Dec 2014
Status Ready to Commit Closed
Closed_Date 0000-00-00 00:00:00 2014-12-13 20:37:58
avatar wilsonge
wilsonge - comment - 13 Dec 2014

Merged - thanks!

avatar zero-24 zero-24 - change - 14 Oct 2015
Labels Removed: ?
avatar buzzword1224
buzzword1224 - comment - 21 Apr 2021

I'm unsure if this is a problem with the indexer or your suggestion JS.
My site had the same problem, site https://fortegrp.com/


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/5128.

avatar dgrammatiko
dgrammatiko - comment - 21 Apr 2021

My site had the same problem, site https://fortegrp.com/

You don't describe what's the problem... Also, this PR was merged 7 years ago and it wasn't meant to solve any other problem apart from the fact that it was dropping Mootools and used jQuery.

Add a Comment

Login with GitHub to post a comment