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

Add setting to disable automatic fetching of subscription feed #2632

Merged
merged 9 commits into from
Sep 30, 2022

Conversation

Aiz0
Copy link
Contributor

@Aiz0 Aiz0 commented Sep 28, 2022

Add setting to disable automatic fetching of subscription feed

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

closes #2515
part of #539

Description

Main changes

Adds a new setting in subscription settings to disable automatic fetching of the subscription feed.
Prevents calls to the method getSubscriptions() in Subscriptions.js in to places.

  1. When the subscriptions component is mounted
  2. In getProfileSubscriptions method, when the users switches profile.

When the users switches profile and has disabled auto fetching 2 things can happen

  1. the video list is emptied.
  2. if the video list is saved in profileSubscriptions then it will be loaded instead.

other stuff

In Subscriptions.vue I made some changes to the messages that show when the video list is empty

  1. The regular "You have not subscribed to any channel" message will now only show when you have in fact not subscribed to any channel.
  2. Added a new message that only shows if you have disabled automatic fetching.
  3. Added a new message that will show when the user is subscribed to channels that have not uploaded any videos.

message 2 will block message 3 from being displayed even if they have attempted two fetch subscriptions.
This will probably not be a huge deal since I imagine it rarely happens.
The alternatives is to check if the user has attempted to refresh subscriptions.

Screenshots

This is the message that shows when the setting is disabled and the user starts the app or navigates to a new profile.
2022-09-28_T02:27:41

Testing

I created 2 new profiles
one without any channels to test if message 1 is displayed
the second one included only a channel without any videos to test if message 3 is displayed.

with the new setting disabled
I tested to make sure subscriptions where not fetched when freetube is started.
I switched between profiles to make sure subscriptions where not fetched automatically
I fetched subscriptions from one profile switched to another and the switched back to see if the video list was kept.

chosen api or rss should not affect anything since this check is done before that part of the code.

Desktop

  • OS: Arch Linux
  • OS Version: 5.19.8-arch1-1
  • FreeTube version: 0.17.1

Additional context

It's likely that I've missed something or implemented this weirdly since I am unfamiliar with this part of the code.

I had to remove the newline for the Your Subscription list is currently empty string in the locale file to get the new strings to work.

If disabling fetching when switching profile is unwanted I could make a new setting for that or just remove it.

@PrestonN PrestonN enabled auto-merge (squash) September 28, 2022 00:03
@Aiz0 Aiz0 closed this Sep 28, 2022
auto-merge was automatically disabled September 28, 2022 00:03

Pull request was closed

@Aiz0
Copy link
Contributor Author

Aiz0 commented Sep 28, 2022

Accidentally opened this before filling in the template... It's filled in now so I'll reopen it.

@Aiz0 Aiz0 reopened this Sep 28, 2022
@PrestonN PrestonN enabled auto-merge (squash) September 28, 2022 01:14
@Aiz0 Aiz0 changed the title Setting auto fetch feed Add setting to disable automatic fetching of subscription feed Sep 28, 2022
@ChunkyProgrammer ChunkyProgrammer added B: inconsistent behavior PR: waiting for review For PRs that are complete, tested, and ready for review and removed B: inconsistent behavior labels Sep 28, 2022
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.

Code changes look fine
Still need to test locally tomorrow (21:08 here)

PikachuEXE
PikachuEXE previously approved these changes Sep 29, 2022
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.

Tested

  • Main changes
  • Profile with no channel for message 1
  • Switching between profiles to see fetched video list kept

message 3 not tested since no channel with no video provided

@@ -14,6 +14,12 @@
:tooltip="$t('Tooltips.Subscription Settings.Fetch Feeds from RSS')"
@change="updateUseRssFeeds"
/>
<ft-toggle-switch
Copy link
Member

Choose a reason for hiding this comment

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

I think you should add a switch column grid and two switch columns so it matches the layout of other setting components (parental control settings, distraction free settings, privacy settings, player settings)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added one now, It does look nicer.
2022-09-29_T18:49:07
Should I change their order?

@Aiz0
Copy link
Contributor Author

Aiz0 commented Sep 29, 2022

message 3 not tested since no channel with no video provided

Sorry forgot to include a one. Didn't want to pick a random users channel.
Youtube music channel works though since it's a special channel without any videos uploaded.
https://www.youtube.com/channel/UC-9-kyTW8ZkZNDHQJ6FgpwQ

This matches the layout other settings components
auto-merge was automatically disabled September 29, 2022 16:53

Head branch was pushed to by a user without write access

@PrestonN PrestonN enabled auto-merge (squash) September 29, 2022 16:53
auto-merge was automatically disabled September 29, 2022 17:06

Head branch was pushed to by a user without write access

@PrestonN PrestonN enabled auto-merge (squash) September 29, 2022 17:07
used so the disable automatic fetch message doesn't block the
empty channels message.
auto-merge was automatically disabled September 29, 2022 17:14

Head branch was pushed to by a user without write access

@PrestonN PrestonN enabled auto-merge (squash) September 29, 2022 17:15
@Aiz0
Copy link
Contributor Author

Aiz0 commented Sep 29, 2022

I was over complicating this in my mind...
The correct message should now display in all cases.

2022-09-29_T19:15:14

absidue
absidue previously approved these changes Sep 29, 2022
@@ -768,6 +770,8 @@ Tooltips:
Fetch Feeds from RSS: When enabled, FreeTube will use RSS instead of its default
method for grabbing your subscription feed. RSS is faster and prevents IP blocking,
but doesn't provide certain information like video duration or live status
Fetch Automatically: When enabled, FreeTube will automatically fetch
your subscription feed on start and when switching profile.
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if important to mention but feed will also be fetched when opening a new window/tab
Also not a big fan of the space between this setting and the RSS setting

Copy link
Member

Choose a reason for hiding this comment

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

The visual space between them can be fixed by adding the :compact="true" attribute to all 3 switches.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if important to mention but feed will also be fetched when opening a new window/tab

I don't think "on start" is the best wording.
I'll see if I can think of something better or if there's any suggestions.
Maybe "when a window is opened" or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The visual space between them can be fixed by adding the :compact="true" attribute to all 3 switches.

Thanks, I'll add that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've made the switches compact and reworded the tooltip.

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.

Some observations

auto-merge was automatically disabled September 29, 2022 22:12

Head branch was pushed to by a user without write access

@Aiz0 Aiz0 dismissed stale reviews from ChunkyProgrammer and absidue via ddde14b September 29, 2022 22:12
@PrestonN PrestonN enabled auto-merge (squash) September 29, 2022 22:12
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 locally

@PrestonN PrestonN merged commit 6ddbce2 into FreeTubeApp:development Sep 30, 2022
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Sep 30, 2022
@Aiz0 Aiz0 deleted the setting-auto-fetch-feed branch October 8, 2022 00:48
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.

[Feature Request]: Optionally disable automatic subscription fetching
6 participants