Skip to content

Keyword search history #6414

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

Merged

Conversation

kommunarr
Copy link
Collaborator

@kommunarr kommunarr commented Dec 19, 2024

Keyword search history

Pull Request Type

  • Feature Implementation

Related issue

closes #1492

Description

Adopting some pieces from #5003, this PR implements search history with behavior very similar to that of YT.

  • When Enable Search Suggestions is enabled and the search bar is focused with no non-whitespace text entered, up to 14 of the most recent searches will display. When a query is entered, up to 4 of the most recent matching searches can display.
  • There is now a magnifying glass icon by YT search suggestion dropdown entries and a history icon + Remove button by search history entries. The Remove button will remove the search history entry and cache entry.
  • The Clear Search Cache setting is now a Clear Search History and Cache setting.
  • Renamed existing Remember History setting to Remember Watch History and added Remember Search History setting as per suggestions in the Matrix chat.

See Testing section for more specifics on the intended behaviors.

Screenshots

Screenshot_20241227_222328

Screenshot_20241228_001053

Testing

  • Test that repeating a search of a keyword (e.g., by clicking on it in history) moves it to the top of the dropdown
  • Test that multiple searches of the same keyword but with different filter parameters does not make multiple search entries appear in dropdown
  • Test that search queries with matching search history entry names have up to 4 of those matching search history entries appear in the results, and in any case, the total # of search results does not exceed 14
  • Test search history entry removal by clicking or Right Arrow key navigation + Enter works properly
  • Test that search history suggestions do not appear when Enable Search Suggestions is disabled
  • Test that deleting the search history and cache under Privacy Settings permanently deletes the search history (and cache)
  • Test that new search histories are not saved, but old search history entries still appear in the search results, when Remember Search History is disabled and Enable Search Suggestions is enabled

Desktop

  • OS: OpenSUSE
  • OS Version: TW

Additional context

The src/renderer/modules/search-history.js implementation is made in a certain way to accommodate possibilities of features like #5003 while still giving space for further optimization if other routes are taken. That's why we allow keeping >14 search history entries at a time even if the older entries are not currently accessible through the UI yet, to name an example.

I would also contend that separating search history by profile is too arguable and out of scope to include in this PR.

@kommunarr kommunarr added the PR: waiting for review For PRs that are complete, tested, and ready for review label Dec 19, 2024
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) December 19, 2024 21:41
@kommunarr kommunarr force-pushed the feat/keyword-search-history branch from c6df924 to d04c760 Compare December 19, 2024 21:50
Copy link
Member

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc left a comment

Choose a reason for hiding this comment

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

Manually removing all the characters does result in seeing the Recent Searches but using the Clear button doesnt.

VirtualBoxVM_Hh0rVfxTWh.mp4

Pure visually but it looks kind of weird that the search bar removes the Clear button for a sec causing the elements to move but makes the Clear button reappear after 1ms. Maybe it should behave more like 2nd video.

VirtualBoxVM_fiEaDHXf0w.mp4
VirtualBoxVM_UoPZAHcYaz.mp4

pressing spacebar throws errors

VirtualBoxVM_FmaVLiQHAv.mp4

Like seen in the vids above Linus Tech Tips was in the recent searches but sometimes i doesnt appear and i dont know why that happens.

image

EDIT: It wont show the search history after clicking on the search history

VirtualBoxVM_H70MVwsHCF.mp4

@kommunarr kommunarr added PR: WIP PR: waiting for review For PRs that are complete, tested, and ready for review and removed PR: waiting for review For PRs that are complete, tested, and ready for review PR: WIP labels Dec 20, 2024
@kommunarr
Copy link
Collaborator Author

@efb4f5ff-1298-471a-8973-3d47447115dc Thank you for your speedy and attentive testing! Fixed above issues now - the not showing of the current history entry when you're on that same page is actually intentional, but I can change that if it's confusing from a user perspective.

Fix focus being lost for a second during searching of search history entry causing clear text button to phase away for a moment. Fix clear text button event not being noticed by the logic in top-nav. Fix spacebar error issue by adding check in updateVisibleDataList function.
@efb4f5ff-1298-471a-8973-3d47447115dc

Pressing spacebar gives me 2 things from my history. I think i expect all of them or none as i didnt actually type something yet. I think showing all would be the better way to go

VirtualBoxVM_VbE9wRYI12.mp4

Press and Hold moves searchbar. This doesnt happen with search suggestions

VirtualBoxVM_mMyBuwjs2v.mp4

Pure visually but it looks kind of weird that the search bar removes the Clear button for a sec causing the elements to move but makes the Clear button reappear after 1ms. Maybe it should behave more like 2nd video.

This one doesnt seem to be addressed

the not showing of the current history entry when you're on that same page is actually intentional, but I can change that if it's confusing from a user perspective.

I think it was quite confusing as i just searched for it

@kommunarr kommunarr added PR: WIP and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Dec 20, 2024
@kommunarr
Copy link
Collaborator Author

kommunarr commented Dec 20, 2024

Fixed all of these issues now!

Edit: Also refactored to simplify some of the logic and testing criteria.

Fixes clear text button to appear and stay visible when the dropdown options are visible in an ft-input. Fixes updateVisibleDataList not using trim(), thus incorrectly causing filtering of search history when space bar was used.
@kommunarr kommunarr force-pushed the feat/keyword-search-history branch from b00b3b5 to dd97039 Compare December 20, 2024 03:43
… route

This is much more efficient for search history particularly. No need for app-specific routes and easier operations & logic due to name being the primary key for search history entries. This still allows for the route to be used as the _id for different kinds of search history (i.e., bookmarks) that could be added in the future.
@kommunarr kommunarr force-pushed the feat/keyword-search-history branch from dd97039 to a6375c7 Compare December 20, 2024 03:47
@kommunarr kommunarr added PR: waiting for review For PRs that are complete, tested, and ready for review and removed PR: WIP labels Dec 20, 2024
Copy link
Member

@absidue absidue left a comment

Choose a reason for hiding this comment

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

As the left and right arrow keys are already used for moving left and right in your search query and the enter key is used to trigger a search, it is now really easy to accidentally delete search history entries when using the arrow keys to adjust your search query and enter to search, especially because the highlighting of the remove link is very subtle, so most of the time you don't even realise that it is activated. Which I can say from personal experience as it happened multiple times while I was testing this pull request.

@kommunarr
Copy link
Collaborator Author

As the left and right arrow keys are already used for moving left and right in your search query and the enter key is used to trigger a search, it is now really easy to accidentally delete search history entries when using the arrow keys to adjust your search query and enter to search, especially because the highlighting of the remove link is very subtle, so most of the time you don't even realise that it is activated. Which I can say from personal experience as it happened multiple times while I was testing this pull request.

Thanks for the feedback - is this any different from YT's implementation, and/or do you have a suggestion on how to improve this?

@absidue
Copy link
Member

absidue commented Jan 5, 2025

I haven't personally experienced YouTube's implementation of it, but here are some suggestions off the top of my head:

  1. It would be to call .preventDefault() on the arrow key events when you are using them to change the focus so it doesn't move the cursor at the same time, that'll make it more obvious that something else is happening, you'll need to make sure that they still work normally when it isn't being used to change the focus (e.g. no entry selected or a search suggestion).
  2. To make the highlight more obvious we could potentially change the colour when it is focused or do something like the outline highlighting that we do for tabs (channels, trending, subscription page) when you focus them with the arrow keys.

@kommunarr
Copy link
Collaborator Author

kommunarr commented Jan 5, 2025

1 is very different from YT's implementation and similar apps, so it might be confusing to familiar users if we change it. Added bolding on selection of the entry now.

Screenshot_20250105_115533

@absidue
Copy link
Member

absidue commented Jan 5, 2025

At the moment when you press the right arrow key while a history suggestion is "focused" two things happen at the same time: the cursor in the text field moves one character to the right and the focus changes to the remove button.
Number 1 is about making it only do one thing at a time, either move to the remove button (call preventDefault like we do for the up and down arrow keys to prevent to cursor in the text box moving) or move the cursor in the text box when no remove button is available e.g. youtube suggestion or focus is in the text box.

@kommunarr
Copy link
Collaborator Author

kommunarr commented Jan 5, 2025

For now, let's stick to YT's approach and change it if it users dislike it:

Kooha-2025-01-05-15-50-44.webm

@absidue
Copy link
Member

absidue commented Jan 5, 2025

Okay wow that is weirdly unexpected that YouTube would have it like that, feels like an oversight on their part to me.
I agree that we can leave it like this as users are likely be used to that behaviour and change it if users dislike it, maybe I'm the only person that thinks it's weird and we'll hear nothing.

Copy link
Member

@absidue absidue left a comment

Choose a reason for hiding this comment

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

Please create a PR to the docs repo to add the new database file to the data location page, after this has been merged.

@PikachuEXE
Copy link
Collaborator

What's changed since last review...
This?

Test that new search histories are not saved, but old search history entries still appear in the search results, when Remember Search History is disabled and Enable Search Suggestions is enabled

@kommunarr
Copy link
Collaborator Author

That's correct

Are you sure you want to clear out your search cache?: Are you sure you want to
clear out your search cache?
Search cache has been cleared: Search cache has been cleared
Clear Search History and Cache: Clear Search History and Cache
Copy link
Member

Choose a reason for hiding this comment

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

Did we want to remove the old key for the other locales? Or is the plan to keep it for now so it's easier to update the translation?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I was thinking the latter.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good, approving now

@FreeTubeBot FreeTubeBot merged commit 5594fcf into FreeTubeApp:development Jan 7, 2025
5 checks passed
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Jan 7, 2025
@efb4f5ff-1298-471a-8973-3d47447115dc

A thought that popped up that i wanted to share: maybe allow importing exporting search history and importing from YT

SuperAKWA pushed a commit to SuperAKWA/FreeTube that referenced this pull request Jan 24, 2025
* Add search history DB integration

* Implement search history display logic and UI

* Modify search cache removal setting to remove search history as well

* Exclude current search route from history suggestions and populate input with matching query

* Modify new labels for clarity

* Fix issues detected during review

Fix focus being lost for a second during searching of search history entry causing clear text button to phase away for a moment. Fix clear text button event not being noticed by the logic in top-nav. Fix spacebar error issue by adding check in updateVisibleDataList function.

* Implement fixes from code review

Fixes clear text button to appear and stay visible when the dropdown options are visible in an ft-input. Fixes updateVisibleDataList not using trim(), thus incorrectly causing filtering of search history when space bar was used.

* Update logic to allowing storing and searching by name rather than by route

This is much more efficient for search history particularly. No need for app-specific routes and easier operations & logic due to name being the primary key for search history entries. This still allows for the route to be used as the _id for different kinds of search history (i.e., bookmarks) that could be added in the future.

* Add back clear statement

* Fix clear text button not working or appearing as active when arrowing through search history results

* Add 'Remove' option

* Implement search history entries showing up alongside YT search suggestions as queries are being typed

* Improve code commenting

* Implement code review changes

Fix preventDefault being called for arrow left/right in search input. Revert ellipsis changes for overly long search results. Adjust matching search history logic to be non-heuristic.

* Fix text overflow issue with long search history entries

* Remove 'name' field and unused DB methods

* Implement 'Remember Search History' setting and rename 'Remember History' to 'Remember Watch History'

* Apply suggestions from code review

Co-authored-by: absidue <[email protected]>

* Update to not show search history suggestions if 'Remember Search History' is disabled

* Update logic to make 'Enable Search Suggestions' being false still allow showing search history results

* Revert hiding old search history entries if rememberSearchHistory is disabled

* Fix searchOptions not closing in edge case

Bug scenario: 1. click search history entry, 2. click clear text button, 3. see state of searchOptions not being closeable until re-interacting with the input

* Update src/renderer/views/Search/Search.js

Co-authored-by: absidue <[email protected]>

* Add bolding to selected/focused 'Remove' button

---------

Co-authored-by: absidue <[email protected]>
@PikachuEXE
Copy link
Collaborator

PikachuEXE commented Feb 19, 2025

Test that search history suggestions do not appear when Enable Search Suggestions is disabled

This seems not true now (and I remember I tested this before approving
Something changed?

image

I saw this above

Test that new search histories are not saved, but old search history entries still appear in the search results, when Remember Search History is disabled and Enable Search Suggestions is enabled

But I have Enable Search Suggestions disabled so...

@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

efb4f5ff-1298-471a-8973-3d47447115dc commented Feb 19, 2025

This is all intended behavior

When Search Suggestions is disabled it only shows you the Search History, so the previously made search queries. If user makes a search then it will still be added to the History. If you would not like future searches to be added to the history go to Privacy Settings and disable Remember Search History.

@PikachuEXE
Copy link
Collaborator

Well the it's

Test that new search histories are not saved, but old search history entries still appear in the search results, when Remember Search History is disabled and Enable Search Suggestions is enabled/disabled

Fine for me it's just misleading to mention Enable Search Suggestions at all with this behaviour

OothecaPickle pushed a commit to OothecaPickle/FreeTube that referenced this pull request Apr 29, 2025
* Add search history DB integration

* Implement search history display logic and UI

* Modify search cache removal setting to remove search history as well

* Exclude current search route from history suggestions and populate input with matching query

* Modify new labels for clarity

* Fix issues detected during review

Fix focus being lost for a second during searching of search history entry causing clear text button to phase away for a moment. Fix clear text button event not being noticed by the logic in top-nav. Fix spacebar error issue by adding check in updateVisibleDataList function.

* Implement fixes from code review

Fixes clear text button to appear and stay visible when the dropdown options are visible in an ft-input. Fixes updateVisibleDataList not using trim(), thus incorrectly causing filtering of search history when space bar was used.

* Update logic to allowing storing and searching by name rather than by route

This is much more efficient for search history particularly. No need for app-specific routes and easier operations & logic due to name being the primary key for search history entries. This still allows for the route to be used as the _id for different kinds of search history (i.e., bookmarks) that could be added in the future.

* Add back clear statement

* Fix clear text button not working or appearing as active when arrowing through search history results

* Add 'Remove' option

* Implement search history entries showing up alongside YT search suggestions as queries are being typed

* Improve code commenting

* Implement code review changes

Fix preventDefault being called for arrow left/right in search input. Revert ellipsis changes for overly long search results. Adjust matching search history logic to be non-heuristic.

* Fix text overflow issue with long search history entries

* Remove 'name' field and unused DB methods

* Implement 'Remember Search History' setting and rename 'Remember History' to 'Remember Watch History'

* Apply suggestions from code review

Co-authored-by: absidue <[email protected]>

* Update to not show search history suggestions if 'Remember Search History' is disabled

* Update logic to make 'Enable Search Suggestions' being false still allow showing search history results

* Revert hiding old search history entries if rememberSearchHistory is disabled

* Fix searchOptions not closing in edge case

Bug scenario: 1. click search history entry, 2. click clear text button, 3. see state of searchOptions not being closeable until re-interacting with the input

* Update src/renderer/views/Search/Search.js

Co-authored-by: absidue <[email protected]>

* Add bolding to selected/focused 'Remove' button

---------

Co-authored-by: absidue <[email protected]>
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.

Keyword search history
6 participants