Skip to content
This repository has been archived by the owner on Mar 16, 2021. It is now read-only.

Exception handling and return values for put/remove #51

Conversation

jannisveerkamp
Copy link
Contributor

@passsy

We should add more tests for failed put/remove actions. But I couldn't find a way to let them fail at all.

Jannis Veerkamp added 2 commits October 14, 2015 18:30
…ion was successful

- added some exception handling within the constructor of a Preference
@codecov-io
Copy link

codecov-io commented Oct 15, 2015

Current coverage is 100%

Sunburst

No coverage report found for develop at 0d912db.

Powered by Codecov. Last updated by 0d912db...f8313b6

@passsy passsy force-pushed the feature/exception_handling_and_put_remove_improvements branch from e495f49 to 7ccca30 Compare October 15, 2015 12:04
Jannis Veerkamp added 3 commits October 15, 2015 14:08
@passsy
Copy link
Contributor

passsy commented Oct 15, 2015

in PreferenceStorage

  • setVersion() should return boolean
  • wipe() should return boolean
  • clear() should return boolean

In PreferenceAccessor

  • clear() should return boolean
  • wipe() should return boolean

assertFalse(providerHelper.persist(uri, STRING_A, null));
assertFalse(providerHelper.persist(MODULE_A, STRING_A, null));
assertFalse(providerHelper.persist(MODULE_A, STRING_A, null, null));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nice one! I struggled with this test

@passsy
Copy link
Contributor

passsy commented Oct 22, 2015

I thought about this and I'm not sure if this is the correct approach.

Tray is currently very heavy when it comes to performance.

  • A simple new MyTrayPreference(context) causes a ContentProvider request (~8ms) for the current version.
  • A pref.put("key", "value") costs ~12ms on a high end phone.

This performance problem should be addressed soon with a memory cache #22. The memory cache would make put and get methods which are void async. Additionally we have to add something like forcePut and forceGet to force a multiprocess sync to make the multiprocess functionality work as it does now. Those force methods return a boolean as proposed here.

Solving the version check in the constructor
We could build it similar to Androids SQLiteOpenHelper which opens the database after calling getWritableDatabase() (the version check happens there). This means new could is very cheap (maybe starting the async version check). The version needs to be checked before making a read/write request with the same error handling proposed here.

I don't want to merge it right now because of the API change which has to be reverted with the in-memory cache implementation.

@passsy
Copy link
Contributor

passsy commented Nov 5, 2015

@jannisveerkamp I'm now 100% sure the listener work (see #53), even in a multi process environment. I'm focusing now on a memory cache (#22)

passsy and others added 10 commits November 5, 2015 18:09
The equality check before the deletion of migrated
preferences only worked for strings as Tray stores
everything as a string. The `toString()`
ensures it works for boxed types as well.
This reverts commit 62817ca.
…g-prefs

Fix deletion of non string migrated shared preferences.
@passsy
Copy link
Contributor

passsy commented Jul 4, 2016

I'm not worried about the API change anymore. SharedPreferences.Editor#commit() also returns a boolean. I'm looking forward to merge this.
//cc @jannisveerkamp @Mobbit

Pascal Welsch added 3 commits July 5, 2016 15:49
…lean indicating success

getVersion() throws TrayException when not successful
simplifyed isVersionChangeChecked()
moved all direct contentprovider access from ContentProviderStorage into TrayProviderHelper automatically catching all possible exceptions from the database
@passsy
Copy link
Contributor

passsy commented Jul 10, 2016

merged with #69

@passsy passsy closed this Jul 10, 2016
@passsy passsy deleted the feature/exception_handling_and_put_remove_improvements branch July 10, 2016 18:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

4 participants