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

feat: Change name of Preferences > Filters > Tabs and move them to Account Preferences(#3536) #4115

Merged

Conversation

sanao1006
Copy link
Contributor

@sanao1006 sanao1006 commented Nov 20, 2023

Overview

In the previous code, when you open preferences, there is a section headed "Filters" with a section called "Tabs"

This is confusing.

Changes

  • Change the section title from "Filters" to "Per-timeline preferences."
  • Change the current "Tabs" section to "Home timeline" since it is only for home timelines

Screenshots

account preference screen detail screen

Note

  • Maybe string resources should have a new property? (for translation)

Related link

Fixes #3536

In the previous code, when you open preferences, there is a section headed "Filters" with a section called "Tabs"

This is confusing.

Therefore, make the following changes
- Change the section title from "Filters" to "Per-timeline preferences."
- Change the current "Tabs" section to "Home timeline" since it is only for home timelines

 Fixes tuskyapp#3536
Remove the following sharedPreferences as they are unnecessary.
- TAB_FILTER_HOME_REPLIES
- TAB_FILTER_HOME_BOOSTS
- TAB_SHOW_HOME_SELF_BOOSTS

Also, update sharedPreferences accordingly.

 Fixes tuskyapp#3536
@@ -45,7 +45,7 @@ enum class AppTheme(val value: String) {
*
* - Adding a new preference that does not change the interpretation of an existing preference
*/
const val SCHEMA_VERSION = 2023082301
const val SCHEMA_VERSION = 2023112001
Copy link
Contributor Author

@sanao1006 sanao1006 Nov 20, 2023

Choose a reason for hiding this comment

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

I don't know if this way of updating SharedPreferences is correct, so this commit is a bit suspect

So, please point out if I am wrong!

Copy link
Collaborator

Choose a reason for hiding this comment

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

You did it correctly

@sanao1006 sanao1006 marked this pull request as ready for review November 20, 2023 04:20
@sanao1006
Copy link
Contributor Author

Are you possibly forgetting about this PR?
I appreciate if you can give me any response!

Copy link
Collaborator

@connyduck connyduck left a comment

Choose a reason for hiding this comment

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

Very nice, thank you.

"Per-timeline preferences" is now shown twice on the account preferences which is a bit confusing. I think one of these should be renamed. Suggestion: Call the new one at the botton "local filters".

@sanao1006
Copy link
Contributor Author

sanao1006 commented Nov 26, 2023

It is true that it would be better to rename it as you said!

@sanao1006
Copy link
Contributor Author

sanao1006 commented Nov 26, 2023

@connyduck
How about setting the new one to "Per-timeline preferences" and the old one back to "Filters"?
As shown in this image
image

Of course, setting the new one to "Local filters" is no problem at all.
Please let me know your opinion!

@connyduck
Copy link
Collaborator

I'm fine with both

@sanao1006
Copy link
Contributor Author

Fixed as shown in the image above.

@sanao1006 sanao1006 requested a review from connyduck November 26, 2023 15:11
@connyduck connyduck changed the title feat: Change name of Preferences > Filters > Tabs (#3536) feat: Change name of Preferences > Filters > Tabs and move them to Account Preferences(#3536) Nov 26, 2023
@mcclure
Copy link
Collaborator

mcclure commented Nov 28, 2023

Can you explain to me the meaning of the second commit, "update: Remove unnecessary sharedPreferences"? Were these fields already or are they made irrelevant by this patch changing them to per-account values? Thanks.

Also, I have put up a PR #4128 , if that is accepted this PR will need to change its version number to acommodate it.

@sanao1006
Copy link
Contributor Author

are they made irrelevant by this patch changing them to per-account values?

Yes, exactly.

Also, I have put up a PR #4128 , if that is accepted this PR will need to change its version number to acommodate it.

In other words, this PR needs to update its DB version to 56 instead of 55, is that right?

@mcclure
Copy link
Collaborator

mcclure commented Nov 29, 2023

Thanks for explaining.

Also, I have put up a PR #4128 , if that is accepted this PR will need to change its version number to acommodate it.

In other words, this PR needs to update its DB version to 56 instead of 55, is that right?

If the PR is accepted. (But there were no objections either here or on Matrix over a while so I guess it is getting accepted?)

@mcclure
Copy link
Collaborator

mcclure commented Jan 3, 2024

Updated PR with 55->56 change as discussed. I tested this builds but did not test running it or performing an upgrade.

Sanao, this will probably cause your local copy to crash until you do an uninstall/reinstall cycle (because the code knows how to update 54->56 but not from your 55 to 56)

@connyduck connyduck merged commit e8e7bad into tuskyapp:develop Jan 3, 2024
3 checks passed
connyduck added a commit that referenced this pull request Jan 3, 2024
Posted this as issue #3999 before. The reasoning is personal experiments
and forks may add database fields and must bump the database number to
do so, but this causes massive merge difficulties when Tusky then
inevitably itself bumps the number. To alleviate this, Tusky official
should use only even database numbers, so odd versions are available for
third party scribbling.

There was little discussion positive or negative in #3999 (one proposal
we switch to a date-based number system, which would work but also could
be unnecessarily complicated). With PR #4115 we now have to make a
decision because that's the first post-proposal PR to bump the database
number odd. So, since I see no outright objections, I'd like to
implement this.

@connyduck suggested the best way to implement the proposal would be to
add a comment to the version number's home in AppDatabase.java.

Co-authored-by: Konrad Pozniak <connyduck@users.noreply.github.com>
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.

Change name of Preferences > Filters > Tabs
3 participants