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

Make history page remember last query string & search limit #5192

Open
wants to merge 22 commits into
base: development
Choose a base branch
from

Conversation

PikachuEXE
Copy link
Collaborator

@PikachuEXE PikachuEXE commented May 30, 2024

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

Description

Make several page types remember last query string & search limit to deal with

  • Search in history
  • Found a few videos
  • View one of them, go back last page, oops not filtered

Page types:

  • History page
  • Channel tab (Channel List for current profile)
  • User Playlists page
  • Single User Playlist page
  • Single Channel page

Screenshots

No video I am lazy

Testing

  • Ensure last used search query and data limit restored when revisiting history page
  • Ensure they are not restored after clearing query text / in new window
  • Ensure no 2 time rendering on page initial render

Desktop

  • OS:
  • OS Version:
  • FreeTube version:

Additional context

There are 2 commits & first one is refactor
Might easier to review per commit

@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label May 30, 2024
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) May 30, 2024 06:34
@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

efb4f5ff-1298-471a-8973-3d47447115dc commented May 30, 2024

I understand where you're are coming from but this behavior is also not apparent in Playlists tab, Channel tab, and a Channel page.

Channel page one also applies to Nr 6 on the list of issue #2446 which is a way bigger issue

Just to clarify im not against this but I think we should do it for all pages instead of one for the sake of a consistent UX

@kommunarr
Copy link
Collaborator

I'd also like to know rationale, as I don't see any requests for this. I'm not necessarily opposed to it, but I don't have any reason to believe that the app remembering my last query would cause fewer irksome moments than the app not remembering it.

@PikachuEXE
Copy link
Collaborator Author

Making it work like search query page
I realize this when searching in history (and several videos popup), click one of them, not the one I want, go back, oops
Though I can always open in new windows (but mobile can't do it?)
Up to you guys

@kommunarr
Copy link
Collaborator

kommunarr commented May 31, 2024

If you want, you can share the build & I can try it out for a while in personal use. It sounds nice in theory, but it feels more horizontal of a change for the use cases where you want to go to the initial page state & now have to click to clear the input. The search bar is also omnipresent, so you wouldn't ever forget what was going on, but here I could see it leading to momentary confusion and re-familiarizing yourself with the context of what's going on & what you were searching beforehand. Seems like maybe "open in a new tab" covers the type of use case you're mentioning better (albeit I guess not if you didn't expect to misclick).

@PikachuEXE PikachuEXE force-pushed the feature/history/remember-search-query branch from 434c7d5 to cfbda54 Compare June 3, 2024 05:49
@PikachuEXE
Copy link
Collaborator Author

PikachuEXE commented Jun 3, 2024

Used router.replace to ensure the query text restored only on clicking back
Clicking on history link won't restore it

About the catch after replace:
https://v3.router.vuejs.org/guide/advanced/navigation-failures.html

Update 1: Use my custom build if you want - https://github.com/PikachuEXE/FreeTube/actions/runs/9345433122

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

Just to clarify im not against this but I think we should do it for all pages instead of one for the sake of a consistent UX

Just retested it but my opinion on this hasnt changed.

@PikachuEXE
Copy link
Collaborator Author

@efb4f5ff-1298-471a-8973-3d47447115dc
Can you list all the page types so I won't miss any on next update?

@kommunarr
Copy link
Collaborator

That sounds like it would be a pretty sizable change to the codebase for a small feature. I would personally advise putting this one on the backburner just because I'm not sure if the feature would provide enough utility versus the effort to maintain it.

Copy link
Contributor

This PR is stale because it has been open 28 days with no activity. Remove stale label or comment or this will be closed in 14 days.

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

@efb4f5ff-1298-471a-8973-3d47447115dc Can you list all the page types so I won't miss any on next update?

Sure:

  • FT Channel tab
  • User Playlist tab main view
  • Inside User Playlist
  • Creator Channel page

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

efb4f5ff-1298-471a-8973-3d47447115dc commented Aug 1, 2024

Q: Why does it take a while for the thumbnails to appear when navigating back to the History tab? Why isnt it instantly shown?

VirtualBoxVM_bZgbgCWrV7.mp4

@github-actions github-actions bot added PR: merge conflicts / rebase needed and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Aug 16, 2024
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@PikachuEXE PikachuEXE force-pushed the feature/history/remember-search-query branch from 3c71c71 to 987b10a Compare August 20, 2024 01:12
@PikachuEXE
Copy link
Collaborator Author

Q: Why does it take a while for the thumbnails to appear when navigating back to the History tab? Why isnt it instantly shown?

Fixed

Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@PikachuEXE
Copy link
Collaborator Author

FT Channel tab
User Playlist tab main view
Inside User Playlist
Creator Channel page

Also done but for the Creator Channel page one I am not really confident (even though I tested it a bit

@PikachuEXE PikachuEXE added the PR: waiting for review For PRs that are complete, tested, and ready for review label Aug 20, 2024
@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

efb4f5ff-1298-471a-8973-3d47447115dc commented Oct 15, 2024

It briefly shows that there are 0 results for the search i made before showing the actual search results

VirtualBoxVM_POiZPMF3AU.mp4

Edit: it also seems to do this on dev branch so i dont think its related to this PR but i could be wrong.

* development: (30 commits)
  Translated using Weblate (Azerbaijani)
  Translated using Weblate (French)
  Translated using Weblate (Estonian)
  Bump the babel group with 3 updates (FreeTubeApp#5858)
  Bump electron-builder from 25.1.7 to 25.1.8 (FreeTubeApp#5862)
  Bump stylelint from 16.9.0 to 16.10.0 in the stylelint group (FreeTubeApp#5860)
  Bump the eslint group with 2 updates (FreeTubeApp#5859)
  Bump globals from 15.10.0 to 15.11.0 (FreeTubeApp#5861)
  Bump sass from 1.79.4 to 1.79.5 (FreeTubeApp#5863)
  Bump electron from 32.1.2 to 32.2.0 (FreeTubeApp#5864)
  Translated using Weblate (Icelandic)
  Translated using Weblate (Russian)
  Translated using Weblate (Hebrew)
  Translated using Weblate (Chinese (Traditional Han script))
  Translated using Weblate (Italian)
  Translated using Weblate (Chinese (Simplified Han script))
  Translated using Weblate (Serbian)
  Translated using Weblate (Czech)
  Translated using Weblate (Japanese)
  Translated using Weblate (Portuguese (Brazil))
  ...
@PikachuEXE
Copy link
Collaborator Author

@efb4f5ff-1298-471a-8973-3d47447115dc
I just fixed this for search tab only
Since the data isElementListLoading for all tabs if any tab's data finished loading it shows the result too early
I introduced another state for search tab only (no need for other tabs yet coz PR is not for remembering other tabs)

@absidue absidue added the DO NOT MERGE UNTIL AFTER RELEASE Do not merge before the next release as this is not a bug fix label Oct 16, 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 we are now adding the data limits to the URLs, there isn't really a need to duplicate them into session storage too, so we can probably remove that code.

Additionally the code for the channel page seems a bit messy, but in an unusual move for me I think I would be okay with it, assuming that it will be refactored soon anyway when we migrate the component to the composition API or start saving the selected tab in the URL, whichever happens sooner.

Copy link
Member

Choose a reason for hiding this comment

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

To future after RC reviewer me: LGTM

@PikachuEXE
Copy link
Collaborator Author

@absidue
dataLimit and searchDataLimit and separated and I don't intend to introduce more changes to this already big PR causing potentially more stuff to test = more chances to break something

I can make a PR later for storing dataLimit into route instead of session storage (which might cause behaviour change, another reason to not do so in this PR

@ChunkyProgrammer ChunkyProgrammer removed the DO NOT MERGE UNTIL AFTER RELEASE Do not merge before the next release as this is not a bug fix label Oct 27, 2024
@github-actions github-actions bot added PR: merge conflicts / rebase needed and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Nov 23, 2024
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

* development: (273 commits)
  Translated using Weblate (Serbian)
  Translated using Weblate (Czech)
  Translated using Weblate (French)
  Translated using Weblate (German)
  Translated using Weblate (Spanish)
  Fix text colour in player overflow menu (FreeTubeApp#6213)
  Add warning message to proxy settings (FreeTubeApp#6099)
  Migrate SubscribedChannels view to Composition API (FreeTubeApp#6131)
  Translated using Weblate (Croatian)
  Translated using Weblate (English (United Kingdom))
  Translated using Weblate (Afrikaans)
  Translated using Weblate (Afrikaans)
  Better player error handling (FreeTubeApp#6180)
  Translated using Weblate (Arabic)
  Translated using Weblate (Afrikaans)
  local API: Fix playlists and podcasts not appearing in search results (FreeTubeApp#6196)
  Translated using Weblate (Serbian)
  Translated using Weblate (Hungarian)
  Bump shaka-player from 4.11.11 to 4.12.1 (FreeTubeApp#6193)
  Translated using Weblate (French)
  ...

# Conflicts:
#	src/renderer/views/SubscribedChannels/SubscribedChannels.js
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@PikachuEXE PikachuEXE added the PR: waiting for review For PRs that are complete, tested, and ready for review label Nov 24, 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.

I haven't got round to testing the pull request yet, so for the moment here is a code review with a bunch suggestions and one question.

src/renderer/components/ChannelDetails/ChannelDetails.vue Outdated Show resolved Hide resolved
src/renderer/views/Channel/Channel.vue Outdated Show resolved Hide resolved
Comment on lines 268 to 277
;(() => {
document.addEventListener('keydown', keyboardShortcutHandler)
getSubscription()
})
const oldQuery = route.query.searchQueryText ?? ''
if (oldQuery !== null && oldQuery !== '') {
// `handleQueryChange` must be called after `filterHistoryDebounce` assigned
handleQueryChange(oldQuery, true)
}
})()
Copy link
Member

Choose a reason for hiding this comment

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

Could you please explain why you changed this from an onMounted callback to an immediately invoked function?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was changed from mounted to created but in composition style either it's in setup block
However oldQuery is only used inside created so I wrap it with an immediately invoked function

Copy link
Member

Choose a reason for hiding this comment

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

The keyboard shortcut handler should be moved back into onMounted as we shouldn't be intercepting key presses before the user can actually see the component. As for the other stuff I don't think it's worth making it more complicated with an immediately invoked function because setup is compiled to a function, so the variable will get garbage collected as soon as it reaches the bottom of it anyway.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated

src/renderer/views/Channel/Channel.js Outdated Show resolved Hide resolved
@PikachuEXE
Copy link
Collaborator Author

Other points are addressed except created

* development: (41 commits)
  Translated using Weblate (Afrikaans)
  Translated using Weblate (Portuguese)
  Translated using Weblate (Welsh)
  Translated using Weblate (Spanish)
  Translated using Weblate (Polish)
  Translated using Weblate (Urdu)
  Translated using Weblate (Chinese (Traditional Han script))
  Switch the data import to the Web File System API (FreeTubeApp#6222)
  Translated using Weblate (Serbian)
  Translated using Weblate (Basque)
  Translated using Weblate (Basque)
  Translated using Weblate (Icelandic)
  Translated using Weblate (Basque)
  Translated using Weblate (French)
  Translated using Weblate (Italian)
  Translated using Weblate (Chinese (Simplified Han script))
  Bump electron from 32.2.5 to 33.2.0 (FreeTubeApp#6231)
  Fix incompatibility with upcoming YouTube.js version (FreeTubeApp#6233)
  Translated using Weblate (Estonian)
  Translated using Weblate (Portuguese (Brazil))
  ...
@PikachuEXE PikachuEXE requested review from ChunkyProgrammer and removed request for ChunkyProgrammer December 3, 2024 00:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: waiting for review For PRs that are complete, tested, and ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants