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

Fix handling of DeArrow titles #3825

Merged
merged 1 commit into from
Aug 3, 2023

Conversation

absidue
Copy link
Member

@absidue absidue commented Jul 29, 2023

Fix handling of DeArrow titles

Pull Request Type

  • Bugfix

Related issue

PR that added the DeArrow titles: #3688

Description

This pull request addresses two issues with the current DeArrow titles implementation:

  1. The titles should only be modified at display time, so that they disappear when you disable the dearrow titles setting, the current implementation unfortunately stores them in the watch history and playlists databases if the video is favourited or marked as watched from an ft-list-video component, so they stay modified even when the setting is disabled
  2. The current implementation results in the data display being delayed until the dearrow title was fetched, that means that the components got rendered empty first and then recreated with the data when the title was fetched, not only causing extra component updates but also meaning that people with slow connections would see empty components until the API requests are done. (see screenshot below)

This pull request addresses the first issue by only overriding the title in the displayTitle computed property, instead of overriding the title property that is among other things used for the mark as watched button.

It addresses the second issue by switching the component creation back to being synchronous and fetching the DeArrow title asynchronously, if the setting is enabled and the video isn't listed in the cache. This approach means that if the DeArrow titles setting is disabled or the video exists in the cache, the initial component render is completely synchronous without any updates (we can use the cached title during the initial render). If the setting is enabled and the video doesn't exist in the cache, the initial render will stay synchronous using the original data, once the API response is complete only the components that have a modified title get rerendered.
So on slow connections instead of seeing nothing first, you will now see the original data and then the display title will be replaced once that API has responded.

Screenshots

Empty component
rendering_issue

Simulated throttling setting
throttling-setting

Testing

Testing the worst case rendering scenario

  1. Make sure that the DeArrow titles setting is disabled because we want to start with an empty cache
  2. Load your subscriptions if they aren't already loaded (make sure you have a channel that has clickbate titles e.g. Linus Tech Tips)
  3. In the same window, switch to the settings tab and enable the DeArrow titles setting
  4. In the network tab in the devtools set the throttling option to the "Slow 3G" preset (see screenshot)
  5. Switch back to the subscriptions tab
  6. With this pull request you should see the components render straight away, with the titles gradually getting replaced with the DeArrow ones where available.
    Without this pull request you will see empty components and then they will start rendering in when the DeArrow API responses arrive, even components without a replacement title will suffer from the issue.

Testing that the modified titles don't get stored

  1. Enable the DeArrow titles setting
  2. In the 3 dots menu next to a video with a modified title, click "Mark as Watched"
  3. In the history tab, check that the video is shown with the modified title
  4. Disable the DeArrow titles setting
  5. In the history tab, check that the video is shown with the original title

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) July 29, 2023 16:30
@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Jul 29, 2023
Copy link
Collaborator

@PikachuEXE PikachuEXE left a comment

Choose a reason for hiding this comment

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

All tested

@ajayyy
Copy link
Contributor

ajayyy commented Jul 31, 2023

The current implementation results in the data display being delayed until the dearrow title was fetched, that means that the components got rendered empty first and then recreated with the data when the title was fetched, not only causing extra component updates but also meaning that people with slow connections would see empty components until the API requests are done.

It's up to you how you want to implement it, but I feel like first showing an original title, then swapping, could be annoying.

I guess for titles it is not too annoying, but if DeArrow thumbnails are implemented, I feel a syncronous approach with it swapping would definitely be jarring.

Up to you though, if you prefer the swapping, that's fine.

@absidue
Copy link
Member Author

absidue commented Jul 31, 2023

The current approach involves the user seeing a completely broken UI, which then suddenly loads in on a video by video basis, which I personally think is a worse user experience than a bit of text changing. Especially as it only changes the first time that video item is seen for that FreeTube session, for every render after that, it uses the cached version directly upon creation.

When/if the thumbnails are implemented, they could be faded or a placeholder could be shown while it's loading and then swapped out with a fade.

The big reason why the implementation shouldn't stay as it is, is because it worsens the performance of item lists, even when DeArrow is disabled, which I think we can all agree is not desirable.

@FreeTubeBot FreeTubeBot merged commit 2ff8133 into FreeTubeApp:development Aug 3, 2023
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Aug 3, 2023
@absidue absidue deleted the fix-dearrow-titles branch August 3, 2023 12:57
PikachuEXE added a commit to PikachuEXE/FreeTube that referenced this pull request Aug 5, 2023
* development: (33 commits)
  Miscellaneous CSS cleanup (FreeTubeApp#3847)
  Fix empty channels showing up as errored with RSS (FreeTubeApp#3824)
  Fix author for album playlists on the playlist page (FreeTubeApp#3838)
  Update Snap Source Host Location (FreeTubeApp#3844)
  * Show error message in popular tab when instance does not support it (FreeTubeApp#3841)
  Use video durations from the watch history for RSS (FreeTubeApp#3839)
  ! Fix unnecessary error message display in toast when paused before video started playing on load (FreeTubeApp#3835)
  Use emit and props instead of $parent (FreeTubeApp#3834)
  Add custom toast event bus for Vue 3 compatiblity (FreeTubeApp#3833)
  Fix handling of DeArrow titles (FreeTubeApp#3825)
  * Update top nav bar icon to remove focus state style (FreeTubeApp#3792)
  Update ft-input for top navbar search input to behave more like Youtube one (FreeTubeApp#3793)
  Translated using Weblate (Hungarian)
  Fix: importing subscriptions with terminated channels (FreeTubeApp#3816)
  Fix outdated subscription cache clearing code when "Remove All Subscriptions / Profiles" performed (FreeTubeApp#3817)
  Translated using Weblate (Croatian)
  Bump eslint-plugin-import from 2.27.5 to 2.28.0 (FreeTubeApp#3827)
  Bump eslint from 8.45.0 to 8.46.0 (FreeTubeApp#3829)
  Bump eslint-plugin-unicorn from 48.0.0 to 48.0.1 (FreeTubeApp#3828)
  Bump lefthook from 1.4.6 to 1.4.7 (FreeTubeApp#3830)
  ...

# Conflicts:
#	src/renderer/components/ft-list-video/ft-list-video.js
#	src/renderer/components/playlist-info/playlist-info.js
#	src/renderer/components/playlist-info/playlist-info.vue
#	src/renderer/components/watch-video-info/watch-video-info.js
PikachuEXE added a commit to PikachuEXE/FreeTube that referenced this pull request Aug 5, 2023
* feature/playlist-2023-05: (31 commits)
  Miscellaneous CSS cleanup (FreeTubeApp#3847)
  Fix empty channels showing up as errored with RSS (FreeTubeApp#3824)
  Fix author for album playlists on the playlist page (FreeTubeApp#3838)
  Update Snap Source Host Location (FreeTubeApp#3844)
  * Show error message in popular tab when instance does not support it (FreeTubeApp#3841)
  Use video durations from the watch history for RSS (FreeTubeApp#3839)
  ! Fix unnecessary error message display in toast when paused before video started playing on load (FreeTubeApp#3835)
  Use emit and props instead of $parent (FreeTubeApp#3834)
  Add custom toast event bus for Vue 3 compatiblity (FreeTubeApp#3833)
  Fix handling of DeArrow titles (FreeTubeApp#3825)
  * Update top nav bar icon to remove focus state style (FreeTubeApp#3792)
  Update ft-input for top navbar search input to behave more like Youtube one (FreeTubeApp#3793)
  Translated using Weblate (Hungarian)
  Fix: importing subscriptions with terminated channels (FreeTubeApp#3816)
  Fix outdated subscription cache clearing code when "Remove All Subscriptions / Profiles" performed (FreeTubeApp#3817)
  Translated using Weblate (Croatian)
  Bump eslint-plugin-import from 2.27.5 to 2.28.0 (FreeTubeApp#3827)
  Bump eslint from 8.45.0 to 8.46.0 (FreeTubeApp#3829)
  Bump eslint-plugin-unicorn from 48.0.0 to 48.0.1 (FreeTubeApp#3828)
  Bump lefthook from 1.4.6 to 1.4.7 (FreeTubeApp#3830)
  ...
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.

6 participants