-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
autocomplete has a bug #3337
Comments
I think we should avoid code that mutates objects / this part should be dropped: Lines 316 to 321 in e2af3e4
Instead of mutating the RawQuery object in place and thus making it unusable for subsequent code, a new RawQuery object should be created whenever needed. |
@return42 since I was digging with answerers already, I'll try to take this up (unless it's a more complicated rework that's better suited for dalf). To be explicit, the expected result is to change the changeQuery method to return a new RawTextQuery object? |
I would prefer @dalf to take a look at it, he knows best what the intention was.
yes, but that was never the question / the question is: what is the benefit for the user to have answers in the autocompletion? |
With the following patch, changeQuery creates a new RawTextQuery instead of changing the current one. diff --git a/searx/query.py b/searx/query.py
index ae68d0da2..b7fdba275 100644
--- a/searx/query.py
+++ b/searx/query.py
@@ -308,26 +308,26 @@ class RawTextQuery:
self.autocomplete_location = last_index_location
- def get_autocomplete_full_query(self, text):
+ def get_autocomplete_full_query(self, text: str) -> str:
qlist, position = self.autocomplete_location
qlist[position] = text
return self.getFullQuery()
- def changeQuery(self, query):
- self.user_query_parts = query.strip().split()
- self.query = self.getFullQuery()
- self.autocomplete_location = (self.user_query_parts, len(self.user_query_parts) - 1)
- self.autocomplete_list = []
- return self
+ def changeQuery(self, query: str) -> "RawTextQuery":
+ full_query = "{0} {1}".format(" ".join(self.query_parts), query).strip()
+ new_rawtextquery = RawTextQuery(full_query, self.disabled_engines)
+ new_rawtextquery.autocomplete_location = (self.user_query_parts, len(self.user_query_parts) - 1)
+ new_rawtextquery.autocomplete_list = []
+ return new_rawtextquery
- def getQuery(self):
+ def getQuery(self) -> str:
return ' '.join(self.user_query_parts)
- def getFullQuery(self):
+ def getFullQuery(self) -> str:
"""
get full query including whitespaces
"""
- return '{0} {1}'.format(' '.join(self.query_parts), self.getQuery()).strip()
+ return "{0} {1}".format(" ".join(self.query_parts), self.getQuery()).strip()
def __str__(self):
return self.getFullQuery()
diff --git a/searx/webapp.py b/searx/webapp.py
index a6cadcf6c..9aa9950b7 100755
--- a/searx/webapp.py
+++ b/searx/webapp.py
@@ -840,8 +840,6 @@ def autocompleter():
backend_name = request.preferences.get_value('autocomplete')
for result in search_autocomplete(backend_name, sug_prefix, sxng_locale):
- # attention: this loop will change raw_text_query object and this is
- # the reason why the sug_prefix was stored before (see above)
if result != sug_prefix:
results.append(raw_text_query.changeQuery(result).getFullQuery())
diff --git a/tests/unit/test_query.py b/tests/unit/test_query.py
index b4f5f8a0d..cff70fec9 100644
--- a/tests/unit/test_query.py
+++ b/tests/unit/test_query.py
@@ -55,8 +55,8 @@ class TestQuery(SearxTestCase): # pylint:disable=missing-class-docstring
query_text = '<8 the query'
query = RawTextQuery(query_text, [])
another_query = query.changeQuery('another text')
- self.assertEqual(query, another_query)
- self.assertEqual(query.getFullQuery(), '<8 another text')
+ self.assertNotEqual(query, another_query)
+ self.assertEqual(another_query.getFullQuery(), '<8 another text')
class TestLanguageParser(SearxTestCase): # pylint:disable=missing-class-docstring
Because the answers are supposed to be offline and quick to get, so why not have them directly in the autocompletion. I like the fact there are data models for the answers, and in this case, I'm ok to remove them from the autocompletion if they don't fit into the autocompletion. |
Visiting this again, I'll look into applying Dalfs patch into a dedicated PR. The topic on if they should be included or not are interested so I opened a discussion here. |
As #3316 pointed out to me, we have the answerers I didn't aware before / may since it has never been worked (AFAIK).
It was introduced in
The OpenSearch part was fixed in
the main issue is the
raw_text_query.changeQuery(result)
..searxng/searx/webapp.py
Lines 843 to 846 in e2af3e4
.. which modifies the result query for all following usages .. I tried to understand the why this side effect is needed here .. but the autocompleter implementation is to complex and I gave up after 4h of investigation.
I have a doubt we really want to have answers in the autocompletion, a simple fix will look like this:
Here by example the random string from
searxng/searx/answerers/random/answerer.py
Lines 21 to 22 in e2af3e4
is shown in the autocompletion ..
.. but I can't imagine that this would be of any advantage to the user / especially as he would first have to select the value in order to copy it.
Our autocompleter only supports simple strings and is therefore not necessarily suitable for displaying "results" other than completion-suggestions in a user-friendly way.
The text was updated successfully, but these errors were encountered: