notes-android icon indicating copy to clipboard operation
notes-android copied to clipboard

Store category sorting for recent, favorites and uncategorized per account

Open AlpAcA0072 opened this issue 4 years ago • 22 comments

Store category sorting for recent, favorites and uncategorized per account.

AlpAcA0072 avatar May 16 '21 07:05 AlpAcA0072

I'm still working on deleting meta category when deleting an account.

AlpAcA0072 avatar May 16 '21 07:05 AlpAcA0072

Reading and updating are realized as I said in the relevant issue. As for deleting, I get the SharedPreferences of current context and uses sharedPreferences.remove() to remove the related value by the key.

AlpAcA0072 avatar May 16 '21 07:05 AlpAcA0072

There's stiil a problem that I don't know how to test the modified code, so I check the keys mannually to make sure they are the same. The NotesRepositoryTest is passed, however, it seems that the relevant tests contain no test about SharedPreferences or meta category.

AlpAcA0072 avatar May 16 '21 07:05 AlpAcA0072

Yeah, we don't have that much tests (yet). I am working from time to time on the coverage, but that's a huge load of work, given that we started with 0 tests...

I have added a very basic unit test for the delete method of the NotesRepository (see master branch). You should be able to extend it for your use case after rebasing your fork or merging the latest master.

stefan-niedermann avatar May 16 '21 10:05 stefan-niedermann

Yeah, we don't have that much tests (yet). I am working from time to time on the coverage, but that's a huge load of work, given that we started with 0 tests...

I have added a very basic unit test for the delete method of the NotesRepository (see master branch). You should be able to extend it for your use case after rebasing your fork or merging the latest master.

Thank you very much for your hard work and feedback! I've rebased my fork and merging the latest master to my branch, finished with the new test testDeleteAccount and passed all the tests in NotesRepositoryTest.java.

AlpAcA0072 avatar May 16 '21 11:05 AlpAcA0072

I’ve modified the code according to the above suggestions and passed the latest added tests. 😄

AlpAcA0072 avatar May 16 '21 12:05 AlpAcA0072

I don't know exactly what to do in the migration. Should I refresh the sharedPreferences? Or should I uses the refreshed sharedPreferences to update the database using execSQL?

AlpAcA0072 avatar May 17 '21 12:05 AlpAcA0072

You basically need to read all account IDs from the database, migrate sharedPreferences as described here and then remove the old sharedPreferences which were not account specific

stefan-niedermann avatar May 17 '21 12:05 stefan-niedermann

You basically need to read all account IDs from the database, migrate sharedPreferences as described here and then remove the old sharedPreferences which were not account specific

How can I know the structure of the table of database? Without knowing it I could not make a query.

AlpAcA0072 avatar May 18 '21 08:05 AlpAcA0072

How can I know the structure of the table of database?

We are using the Room library as ORM. The entities are defined in NotesDatabase at the very top, each of the class mentioned there matches one table in the database. Each property of a class matches one column of the corresponding table.

Everything you need is to fetch the set of all account IDs and iterate it to add sharedPrefs for each of them. A similar query is made in the Migration_22_23.

Don't forget to increment the database version, otherwise your migration will not be applied (i forgot it a few times myself and was frustrated because it didn't work :wink: )

stefan-niedermann avatar May 18 '21 09:05 stefan-niedermann

I think I've finished with that. Please check whether there is a problem. Thanks a lot!😆

AlpAcA0072 avatar May 18 '21 09:05 AlpAcA0072

How can I know the structure of the table of database?

We are using the Room library as ORM. The entities are defined in NotesDatabase at the very top, each of the class mentioned there matches one table in the database. Each property of a class matches one column of the corresponding table.

Everything you need is to fetch the set of all account IDs and iterate it to add sharedPrefs for each of them. A similar query is made in the Migration_22_23.

Don't forget to increment the database version, otherwise your migration will not be applied (i forgot it a few times myself and was frustrated because it didn't work 😉 )

Do I need to add the modified sharedPreferences to the database? I've finished the Migration_23_24.java and want to know is there anything wrong with my implementation

AlpAcA0072 avatar May 18 '21 11:05 AlpAcA0072

Do I need to add the modified sharedPreferences to the database?

No.

But you are removing the keys too early. This way it will only work for one set up account, not in a multi accoubt scenario. You must only remove them after looping over all accounts.

Also please use always hardcoded strings in Migrations (instead of getString(R.string.xxx)) because the values might change over the time but need to stay the same in migrations forever 🙂

stefan-niedermann avatar May 18 '21 11:05 stefan-niedermann

Do I need to add the modified sharedPreferences to the database?

No.

But you are removing the keys too early. This way it will only work for one set up account, not in a multi accoubt scenario. You must only remove them after looping over all accounts.

Also please use always hardcoded strings in Migrations (instead of getString(R.string.xxx)) because the values might change over the time but need to stay the same in migrations forever 🙂

If I hardcoded the string, for example "Uncategorized" to replace the R.string.action_uncategorized. For any languages other than English, how to guarantee that this method will also work?

AlpAcA0072 avatar May 18 '21 12:05 AlpAcA0072

This version satisfy all the requirements above.😄

AlpAcA0072 avatar May 18 '21 12:05 AlpAcA0072

You are absolutely right, what a pity that we use localized strings a keys.

We should move away from them during this migration and i unfortunately can not see any other option then gathering a list of all available translated values (see strings.xml files), put them into an array in the migration and iterate them.

As a new key you can use something meaningful like

  • category_sorting_ + favorite_ + 1
  • category_sorting_ (prefix to distinguish from other sharedPrefs) + favorite_ (meta category) + 1 (account id)

You can either store those new keys as

public static final String XYZ = "category_sorting_";

or alternatively in the values/strings.xml as

<string name="XYZ" translatable="false">category_sorting_</string>

Actually what you have found here is a bug from my point of view, because if one changes the language of his Android device, the sorting of the meta categories won't work.

Some extra miles to go, but worth it.

stefan-niedermann avatar May 18 '21 13:05 stefan-niedermann

You are absolutely right, what a pity that we use localized strings a keys.

We should move away from them during this migration and i unfortunately can not see any other option then gathering a list of all available translated values (see strings.xml files), put them into an array in the migration and iterate them.

As a new key you can use something meaningful like

  • category_sorting_ + favorite_ + 1
  • category_sorting_ (prefix to distinguish from other sharedPrefs) + favorite_ (meta category) + 1 (account id)

You can either store those new keys as

public static final String XYZ = "category_sorting_";

or alternatively in the values/strings.xml as

<string name="XYZ" translatable="false">category_sorting_</string>

Actually what you have found here is a bug from my point of view, because if one changes the language of his Android device, the sorting of the meta categories won't work.

Some extra miles to go, but worth it.

So actually what I need to do is to defined three untranslatable meta categories, and migrate the old one to the new one. Because the newly defined one is staticlly stored in sharedPreferences, we don't have to worry about internationalization. Am I right? That is such a wonderful idea!

AlpAcA0072 avatar May 18 '21 13:05 AlpAcA0072

Yes, it can be considered being a bug to use translated values as keys (which we currently do). Therefore we should replace them with static values while we are migrating them anyway.

stefan-niedermann avatar May 18 '21 13:05 stefan-niedermann

I guess now we don't have to worry about hardcoded strings in Migrations. I added four elements to Strings.xml add using them in the Migration_23_24.java, does this meets the above requiements?

AlpAcA0072 avatar May 19 '21 02:05 AlpAcA0072

@stefan-niedermann Do I have to resolve the conflicts shown below? Is there anything I can do anymore?

AlpAcA0072 avatar May 19 '21 08:05 AlpAcA0072

I did not forget you, i am just quite busy with my work and my private life at the moment - sorrs for the delay.

stefan-niedermann avatar May 28 '21 06:05 stefan-niedermann

Thank you for your attention. I am also very busy these days and I hope the repair of the issue will not be delayed.

AlpAcA0072 avatar May 31 '21 02:05 AlpAcA0072