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

Reword repository indicator fetch setting #17389

Merged
merged 8 commits into from
Sep 20, 2023

Conversation

tidy-dev
Copy link
Contributor

Related Issues:
#10687
GitHub Desktop fetching even with auto fetch disabled

Description

The "Periodically fetch and refresh status of all repositories" is to toggle the background fetching of repositories in order to display the status indicators in the repository list. Unfortunately, we have had users interpret the setting to mean it should stop all the periodic fetching including their currently open one. However, this is not true, GitHub Desktop will still periodically fetch the open repository in order to keep it in sync with it's remote.

Further explanation of why this doesn't turn off fetching in the currently open repository:

As part of these background fetches we fast forward all but the currently selected branch which in turn powers "Update from default branch" and the merge/rebase dialogs as they operate on local branches only. It's also what enables us to show the ahead/behind indicator in the push/pull toolbar (and what enables us to detect that we need to do a pull vs a fetch).

Therefore, having a setting to turn all periodic fetching off would require a bit of research to determine all the consequences and handle all the use cases of the open repository.

Screenshots

image

Release notes

Notes: [Improved] Clarified the outcome of toggling the setting under "Background Updates" in the "Advanced" settings.

Copy link
Member

@niik niik left a comment

Choose a reason for hiding this comment

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

This is already loads better than what we've got and I'd be happy to take this improvement. I though about trying an angle where we title the setting after what it enables and not what it does but I couldn't get it to not be too wordy

 [ ] Show which repositories have local or remote changes in the repository list

app/src/ui/preferences/advanced.tsx Outdated Show resolved Hide resolved
app/src/ui/preferences/advanced.tsx Outdated Show resolved Hide resolved
app/src/ui/preferences/advanced.tsx Outdated Show resolved Hide resolved
@oscarllann
Copy link

- Allows the display of up-to-date status indicators in the repository - list. Disabling this may improve performance with many repositories. +

+ This requires the periodic fetching of repositories that are not + currently open. Turning this off will not stop the periodic fetching + of your open repository.

>
This requires the periodic fetching of repositories that are not
currently open. Turning this off will not stop the periodic fetching
of your open repository.
Copy link
Member

Choose a reason for hiding this comment

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

I like @oscarllann angle on performance. Maybe it gets excessively wordy but adding something like... "but will improve overall performance of the app for users with many repositories" could be useful to our users.

@tidy-dev
Copy link
Contributor Author

tidy-dev commented Sep 18, 2023

[ ] Show which repositories have local or remote changes in the repository list

I do prefer this approach as it is more direct. I had something long on one iterations but was still using "status indicators", I like the explanation of the behavior better. Probably the only thing I prefer about "status indicators" is that it tells me that is likely referring to the "icon" stuff in the repository list. I think that this is going to be a verbose setting no matter what because it is hard to concisely communicate. I am going to mull on this a bit see if I can think of anything different.

may improve performance with many repositories.

Good call out -> having the reason for the setting makes sense.

up-to-date status indicators

I was leery of the "up-to-date" verbiage since it is a periodic fetch so it is not always up-to-date :D

tidy-dev and others added 3 commits September 18, 2023 12:49
Co-Authored-By: Markus Olsson <634063+niik@users.noreply.github.com>
@tidy-dev
Copy link
Contributor Author

tidy-dev commented Sep 18, 2023

Latest thoughts.. but the verboseness isn't pretty with the checkbox centering.. But could fix if we decide we like some wording that is two lines.

image

@sergiou87
Copy link
Member

Yeah that doesn't look good 😭 What do you think about moving some of that info to the description text?

[ ] Show status icons in the repository list

These icons will indicate whether repositories have local or remote changes. This requires the periodic fetching of…

@tidy-dev
Copy link
Contributor Author

Yeah. I definitely like a one line setting better. But, man, that wall of text. :P I split it into two paragraphs in hopes of making it scannable.

Using one line setting with long explanation split into two paragraphs.

@tidy-dev
Copy link
Contributor Author

tidy-dev commented Sep 19, 2023

Alternative to wall of text above .. more scannable wall of text?

Description broken into sections and bullets

@sergiou87
Copy link
Member

I prefer the two paragraphs better!

@tidy-dev tidy-dev force-pushed the background-fetch-setting-wording branch from 3b48e74 to 2d493e7 Compare September 19, 2023 15:21
@tidy-dev
Copy link
Contributor Author

PR currently

image

Copy link
Member

@sergiou87 sergiou87 left a comment

Choose a reason for hiding this comment

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

Love it! Thank you 😄

@tidy-dev tidy-dev merged commit 47e7d31 into development Sep 20, 2023
7 checks passed
@tidy-dev tidy-dev deleted the background-fetch-setting-wording branch September 20, 2023 15:25
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.

None yet

4 participants