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

Implement compile-time flag to disable automatic update checks. #4854

Merged

Conversation

libklein
Copy link
Contributor

@libklein libklein commented Oct 1, 2023

Description

This PR adds a compile-time flag to disable automatic updates (cf. #4796). Specifically, it prevents the update button from being shown/updated if the Updater detects that an update is necessary. A potential drawback of this approach is that it also does not show the button if the user "requests" this, e.g., by enabling beta updates.

An alternative implementation could also simply disable the automatic update check on application startup.

I believe that the former approach is more desirable as it ensures that the Updater class is in it's expected state regardless of whether updates are disabled or not. We could, for instance, silently log that the application is out of state.

Copy link
Collaborator

@Mm2PL Mm2PL left a comment

Choose a reason for hiding this comment

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

Chatterino never updates automatically without the user's explicit action. It only checks for updates and displays a prompt (which I believe to be an important enough distinction to mention).
Please don't just hide the button. This approach results in wasted requests to the update server. Instead please look at src/singletons/Updates (notably Updates::checkForUpdates) and disable updates there.
Disabling updates at compile time IMO should never have the app call home, it's something I'd consider an antifeature.

Copy link
Member

@pajlada pajlada left a comment

Choose a reason for hiding this comment

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

  • what mm2pl mentioned, I think I prefer the network request not being run in the first place

src/CMakeLists.txt Outdated Show resolved Hide resolved
@libklein
Copy link
Contributor Author

libklein commented Oct 1, 2023

Sounds good!

I'd personally prefer disabling the updates by removing the call in https://github.com/Chatterino/chatterino2/blob/master/src/RunGui.cpp#L273 instead of modifying Updater.cpp directly. This way the class itself remains functional, it'll just not be triggered.

I'd also suggest disabling https://github.com/Chatterino/chatterino2/blob/master/src/Application.cpp#L190 to stay consistent - otherwise we'd have to trigger an update check on startup based on the betaUpdates setting. I actually think that hiding the beta checkbox/ignoring the setting altogether is probably in line with the PR, given that it's motiviation is to stop annoying users that source chatterino from a package repository with update notifications. I do see that this is debatable tough.

Happy to hear your opinion on this!

@pajlada
Copy link
Member

pajlada commented Oct 1, 2023

Instead of ifdefing out the calls to checkForUpdates, I'd rather have this check just early return in the checkForUpdates itself

@Felanbird Felanbird enabled auto-merge (squash) October 1, 2023 20:00
Copy link
Collaborator

@Mm2PL Mm2PL left a comment

Choose a reason for hiding this comment

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

lgtm

CMakeLists.txt Outdated Show resolved Hide resolved
@pajlada pajlada disabled auto-merge October 1, 2023 20:16
@pajlada pajlada requested a review from fourtf October 2, 2023 08:27
@pajlada pajlada merged commit 5b17ae3 into Chatterino:master Oct 2, 2023
15 checks passed
@pajlada
Copy link
Member

pajlada commented Oct 2, 2023

Thank you @libklein ! As a first-time contributor, you can now add yourself to the contributors list that's shown inside the About page in Chatterino.

If you want this, you can open a new PR where you modify the resources/contributors.txt file and add yourself to the list. Make sure to read the comments at the top for instructions.

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.

5 participants