Skip to content
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

[mod] new preference: query_in_title #485

Merged
merged 1 commit into from
Nov 24, 2021
Merged

Conversation

dalf
Copy link
Member

@dalf dalf commented Nov 6, 2021

What does this PR do?

  • disable by default
  • settings.yml: ui.query_in_title
  • in /preferences: privacy tab

when enabled, the result page's title contains the user query.

previously:

  • oscar theme: the query was always included
  • simple theme: the query was included with the GET method

image

image

Why is this change important?

Some browser records the page history using the HTML title pages.

See #434 (comment)

How to test this PR locally?

With the simple and oscar themes:

  • make a query : check there is NO title
  • change the preferences: check there is a title
  • reset the preferences
  • in settings.yml: set ui.query_in_title: true
  • check there is a title
  • in settings.yml: set lock: - query_in_title
  • /preferences: check the option has disappeared.

Author's checklist

Related issues

Related to #434

Copy link
Member

@mrpaulblack mrpaulblack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest changing the line in results.html in both oscar and simple to (This way there is a space between the query and the - like so: time - SearXNG; right now the title looks like this in firefox: time- SearXNG):

{% block title %}{% if query_in_title %}{{ q|e }} - {% endif %}{% endblock %}
  • 👍 enabeling the setting in pref. in oscar and simple works
  • 👍 disabeling the setting in pref. in oscar and simple works
  • 👍 reseting the settings work
  • 👎 enabeling query in title does NOT work through settings.yml (query_in_title: true in ui section; this is the case for both themes)
  • 👍 locking the setting in settings.yml works for both themes

@dalf
Copy link
Member Author

dalf commented Nov 7, 2021

👎 enabeling query in title does NOT work through settings.yml (query_in_title: true in ui section; this is the case for both themes)

Fixed. (the GitHub workflow fails)
Also, the values is stored in query_in_title (underscore) in settings, preferences and Jinja context (no mismatch with query-in-title).

@unixfox : the default value is off. So the title will disappear from page's title when the instances are upgraded unless the user explicitly set the value to on.

Possible better implementation:

  • set the default value to "on" for FF browser, "off" otherwise.
  • in settings.yml: query_in_title: auto (false, true are still possible, auto is additional value)
  • the function searx.webapp.pre_request set the value according to the user agent (something similar is already for the HTTP method).

@dalf
Copy link
Member Author

dalf commented Nov 7, 2021

make test.robot fails because of this line:

<option value="" {% if not query_in_title %}selected="selected"{% endif %}>{{ _('Disabled')}}</option>

More precisely, removing {% if not query_in_title %}selected="selected"{% endif %} from this line fixes this error:

Error occured: Traceback (most recent call last):
  File "/home/alexandre/code/searx/tests/robot/__main__.py", line 69, in main
    run_robot_tests([getattr(test_webapp, x) for x in dir(test_webapp) if x.startswith('test_')])
  File "/home/alexandre/code/searx/tests/robot/__main__.py", line 62, in run_robot_tests
    test(browser)
  File "/home/alexandre/code/searx/tests/robot/test_webapp.py", line 55, in test_preferences_engine_select
    assert browser.find_by_xpath('//input[@id="engine_general_dummy__general"]').first.checked
AssertionError

I can't figure out why.

Looking at the HTML page:

export SEARXNG_SETTINGS_PATH=$(pwd)/tests/robot/settings_robot.yml
make run
xdg-open http://localhost:11111

I don't see any issue.

@mrpaulblack
Copy link
Member

mrpaulblack commented Nov 7, 2021

Also: Setting anything it the settings on the user device through the /preferences endpoint does not work anymore. It seems the POST request when pressing the save btn does not work anymore:

image

Also the debug log:

ERROR   searx.webapp                  : Invalid choice: 
Traceback (most recent call last):
  File "/home/local/code/external/searxng/searx/webapp.py", line 519, in pre_request
    preferences.parse_dict(request.form)
  File "/home/local/code/external/searxng/searx/preferences.py", line 460, in parse_dict
    self.key_value_settings[user_setting_name].parse(user_setting)
  File "/home/local/code/external/searxng/searx/preferences.py", line 206, in parse
    raise ValidationException('Invalid choice: {0}'.format(data))
searx.preferences.ValidationException: Invalid choice: 
DEBUG   searx.webapp                  : http%3A//127.0.0.1%3A8888/preferences uses locale `en`
INFO    werkzeug                      : 127.0.0.1 - - [07/Nov/2021 11:01:05] "POST /preferences HTTP/1.1" 302 -
DEBUG   searx.webapp                  : http%3A//127.0.0.1%3A8888/ uses locale `en`

* disable by default
* settings.yml: ui.query_in_title
* in /preferences: privacy tab

when enabled, the result page's title contains the user query.

previously:
* oscar theme: the query was always included
* simple theme: the query was included with the GET method
@dalf dalf marked this pull request as draft November 8, 2021 20:50
@dalf
Copy link
Member Author

dalf commented Nov 9, 2021

@mrpaulblack the issue you describe is exactly why the tests fail :

  • the preferences are saved : it fails silently.
  • then code checks the preferences are changed

I've added

settings['ui']['query_in_title'],

(image_proxy preference definition has the "workaround")

So now it should work.


Perhaps with Firefox, the default value should be true and false in other cases.

@mrpaulblack
Copy link
Member

FYI: Firefox does actually log the page title in history

image

I think this setting should be off by default...

@dalf
Copy link
Member Author

dalf commented Nov 24, 2021

FYI: Firefox does actually log the page title in history

even wit the POST method ?

It is off by default, see searx/settings_defaults.py

@mrpaulblack
Copy link
Member

Woow you are right; It does not log page title when using POST method with query in page title enabled mb...

@dalf
Copy link
Member Author

dalf commented Nov 24, 2021

By default, for a Firefox user, this setting can be enabled when the method is POST, and disabled in all other cases.
It is doable but.... that's a mess from the user point of view: why suddenly the title has disappear?

So as you said, it should be disabled by default.

There are an inconsistent settings:

  • in the /preferences, it is in the privacy section.
  • in settings.yml, the preferences in under the ui section (there is no privacy section).

Copy link
Member

@mrpaulblack mrpaulblack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the inconsistency in settings.yml is fine; since its the only privacy setting from the theme in settings.yml and still kinda fits into that category (if there are more privacy items we should probably think about adding the category to settings.yml...). I also retested and everything works now 👍 (settings.yml change in simple and oscar and also changing in /pref. in simple and oscar).... The only thing I would change is to add a space after the query and before the -:

image

(But dont know how and its not that important if not tbh)

@dalf dalf marked this pull request as ready for review November 24, 2021 12:10
@dalf dalf merged commit 42c1a93 into searxng:master Nov 24, 2021
@dalf dalf deleted the pref_query_in_title branch November 24, 2021 12:10
@dalf dalf mentioned this pull request Nov 24, 2021
@unixfox
Copy link
Member

unixfox commented Nov 29, 2021

image

There is a space missing before the -, not sure where to fix that 🤔

return42 added a commit to return42/searxng that referenced this pull request Nov 29, 2021
Suggested-by: @unixfox searxng#485 (comment)
Signed-off-by: Markus Heiser <markus.heiser@darmarit.de>
@return42
Copy link
Member

There is a space missing

Discussion moved to #560

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants