Skip to content

Add nullability annotations to ContentSet methods. #650 #651

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.

Sign up for GitHub

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

Open
lemonboston wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add nullability annotations to ContentSet methods. #650 #651

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
42c9b8f
Add nullability annotations to ContentSet methods. #650
Jan 25, 2018
4154099
Remove mAfterKeys compatibility workaround from ContentSet.
Jan 26, 2018
4453861
Add javadocs about the key parameter of the listener registration met…
Jan 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions .idea/codeInsightSettings.xml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

79 changes: 44 additions & 35 deletions opentasks/src/main/java/org/dmfs/tasks/model/ContentSet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
import android.net.Uri;
import android.os.Parcel;
import android.os.Parcelable;
import android.support.annotation.NonNull;
import android.support.annotation.Nullable;
import android.util.Log;

import org.dmfs.tasks.utils.AsyncContentLoader;
Expand Down Expand Up @@ -112,13 +114,8 @@ private ContentSet()
* @param uri
* A content URI, either a directory URI or an item URI.
*/
public ContentSet(Uri uri)
public ContentSet(@NonNull Uri uri)
{
if (uri == null)
{
throw new IllegalArgumentException("uri must not be null");
}

mUri = uri;
}

Expand All @@ -129,13 +126,8 @@ public ContentSet(Uri uri)
* @param other
* The {@link ContentSet} to clone.
*/
public ContentSet(ContentSet other)
public ContentSet(@NonNull ContentSet other)
{
if (other == null)
{
throw new IllegalArgumentException("other must not be null");
}

if (other.mBeforeContentValues != null)
{
mBeforeContentValues = new ContentValues(other.mBeforeContentValues);
Expand All @@ -159,7 +151,7 @@ public ContentSet(ContentSet other)
* @param mapper
* The {@link ContentValueMapper} to use when loading the values.
*/
public void update(Context context, ContentValueMapper mapper)
public void update(@NonNull Context context, @NonNull ContentValueMapper mapper)
{
String itemType = context.getContentResolver().getType(mUri);
if (itemType != null && !itemType.startsWith(ContentResolver.CURSOR_DIR_BASE_TYPE))
Expand All @@ -175,7 +167,7 @@ public void update(Context context, ContentValueMapper mapper)


@Override
public void onContentLoaded(ContentValues values)
public void onContentLoaded(@NonNull ContentValues values)
{
mBeforeContentValues = values;
mLoading = false;
Expand All @@ -200,7 +192,7 @@ public boolean isLoading()
* @param context
* A context.
*/
public void delete(Context context)
public void delete(@NonNull Context context)
{
if (mUri != null)
{
Expand All @@ -226,10 +218,16 @@ public void delete(Context context)
}


public Uri persist(Context context)
@NonNull
public Uri persist(@NonNull Context context)
{
if (mAfterContentValues == null || mAfterContentValues.size() == 0)
{
if (mUri == null)
{
throw new IllegalStateException("Task uri is null, cannot persist(). (Called after delete()?)");
}

// nothing to do here
return mUri;
}
Expand All @@ -247,6 +245,11 @@ else if (isUpdate())

mAfterContentValues = null;

if (mUri == null)
{
throw new IllegalStateException("Task uri is null, cannot persist(). (Called after delete()?)");
}

return mUri;
}

Expand All @@ -263,26 +266,26 @@ public boolean isUpdate()
}


public boolean containsKey(String key)
public boolean containsKey(@NonNull String key)
{
return mAfterContentValues != null && mAfterContentValues.containsKey(key) || mBeforeContentValues != null && mBeforeContentValues.containsKey(key);
}


public boolean persistsKey(String key)
private boolean persistsKey(@NonNull String key)
{
return mAfterContentValues != null && mAfterContentValues.containsKey(key);
}


public boolean updatesAnyKey(Set<String> keys)
public boolean updatesAnyKey(@NonNull Set<String> keys)
{
if (mAfterContentValues == null)
{
return false;
}

Set<String> keySet = new HashSet<String>(mAfterKeys);
Set<String> keySet = new HashSet<>(mAfterKeys);

keySet.retainAll(keys);

Expand All @@ -291,7 +294,7 @@ public boolean updatesAnyKey(Set<String> keys)
}


public void ensureUpdates(Set<String> keys)
public void ensureUpdates(@NonNull Set<String> keys)
{
if (mBeforeContentValues == null || keys == null || keys.isEmpty())
{
Expand Down Expand Up @@ -328,6 +331,7 @@ public void ensureUpdates(Set<String> keys)
}


@NonNull
private ContentValues ensureAfter()
{
ContentValues values = mAfterContentValues;
Expand All @@ -336,13 +340,13 @@ private ContentValues ensureAfter()
values = new ContentValues();
mAfterContentValues = values;
// also create mAfterKeys
mAfterKeys = new HashSet<String>();
mAfterKeys = new HashSet<>();

@dmfs dmfs Jan 25, 2018

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Please remove mAfterKeys while you're at it. It was only a workaround for the absence of ContentValues.keySet() on Android 2.x. Since we don't support Android 2 any more you can remove it in most places and replace by mAfterContentValues.keySet() where it's being read.

@lemonboston lemonboston Jan 26, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok, I've removed mAfterKeys.

}
return values;
}


public void put(String key, Integer value)
public void put(@NonNull String key, @Nullable Integer value)
{
Integer oldValue = getAsInteger(key);
if (value != null && !value.equals(oldValue) || value == null && oldValue != null)
Expand All @@ -367,7 +371,8 @@ public void put(String key, Integer value)
}


public Integer getAsInteger(String key)
@Nullable
public Integer getAsInteger(@NonNull String key)
{
final ContentValues after = mAfterContentValues;
if (after != null && after.containsKey(key))
Expand All @@ -378,7 +383,7 @@ public Integer getAsInteger(String key)
}


public void put(String key, Long value)
public void put(@NonNull String key, @Nullable Long value)
{
Long oldValue = getAsLong(key);
if (value != null && !value.equals(oldValue) || value == null && oldValue != null)
Expand All @@ -402,7 +407,8 @@ public void put(String key, Long value)
}


public Long getAsLong(String key)
@Nullable
public Long getAsLong(@NonNull String key)
{
final ContentValues after = mAfterContentValues;
if (after != null && after.containsKey(key))
Expand All @@ -413,7 +419,7 @@ public Long getAsLong(String key)
}


public void put(String key, String value)
public void put(@NonNull String key, @Nullable String value)
{
String oldValue = getAsString(key);
if (value != null && !value.equals(oldValue) || value == null && oldValue != null)
Expand All @@ -437,7 +443,8 @@ public void put(String key, String value)
}


public String getAsString(String key)
@Nullable
public String getAsString(@NonNull String key)
{
final ContentValues after = mAfterContentValues;
if (after != null && after.containsKey(key))
Expand All @@ -448,7 +455,7 @@ public String getAsString(String key)
}


public void put(String key, Float value)
public void put(@NonNull String key, @Nullable Float value)
{
Float oldValue = getAsFloat(key);
if (value != null && !value.equals(oldValue) || value == null && oldValue != null)
Expand All @@ -472,7 +479,8 @@ public void put(String key, Float value)
}


public Float getAsFloat(String key)
@Nullable
public Float getAsFloat(@Nullable String key)
{
final ContentValues after = mAfterContentValues;
if (after != null && after.containsKey(key))
Expand All @@ -489,6 +497,7 @@ public Float getAsFloat(String key)
*
* @return The {@link Uri}.
*/
@Nullable
public Uri getUri()
{
return mUri;
Expand Down Expand Up @@ -529,7 +538,7 @@ public void finishBulkUpdate()
* @param key
* The key of the value to remove.
*/
public void remove(String key)
public void remove(@NonNull String key)
{
if (mAfterContentValues != null)
{
Expand All @@ -544,7 +553,7 @@ else if (mBeforeContentValues != null && mBeforeContentValues.get(key) != null)
}


public void addOnChangeListener(OnContentChangeListener listener, String key, boolean notify)
public void addOnChangeListener(@NonNull OnContentChangeListener listener, @Nullable String key, boolean notify)

@dmfs dmfs Jan 25, 2018

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I think it doesn't make sense to add a listener for changes to a null key. So key should be @NonNull.

@lemonboston lemonboston Jan 26, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There are usages of these methods with null key, so that's why I used Nullable. I've checked now and null actually has a special usage here, see:

opentasks/opentasks/src/main/java/org/dmfs/tasks/model/ContentSet.java

Lines 69 to 73 in 4453861

/**
* A {@link Map} for the {@link OnContentChangeListener}s. A listener registers for a specific key in a content set or for <code>null</code> to e notified
* of full reloads.
*/
private final Map<String, Set<OnContentChangeListener>> mOnChangeListeners = new HashMap<String, Set<OnContentChangeListener>>();

So now I've just added some javadoc to the methods about the key parameter.
Let me know if you'd prefer it to be changed somehow.

@dmfs dmfs Jan 26, 2018

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I right, I forgot about that. Passing null will cause the listener to be called for any update on any key. Did you check if we actually use that?

@lemonboston lemonboston Jan 26, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, they are called with null key from ViewTaskFragment and EditTaskFragment.

{
Set<OnContentChangeListener> listenerSet = mOnChangeListeners.get(key);
if (listenerSet == null)
Expand All @@ -563,7 +572,7 @@ public void addOnChangeListener(OnContentChangeListener listener, String key, bo
}


public void removeOnChangeListener(OnContentChangeListener listener, String key)
public void removeOnChangeListener(@NonNull OnContentChangeListener listener, @Nullable String key)

@dmfs dmfs Jan 25, 2018

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

same here, key is @NonNull.

{
Set<OnContentChangeListener> listenerSet = mOnChangeListeners.get(key);
if (listenerSet != null)
Expand All @@ -573,7 +582,7 @@ public void removeOnChangeListener(OnContentChangeListener listener, String key)
}


private void notifyUpdateListeners(String key)
private void notifyUpdateListeners(@NonNull String key)
{
Set<OnContentChangeListener> listenerSet = mOnChangeListeners.get(key);
if (listenerSet != null)
Expand Down Expand Up @@ -632,7 +641,7 @@ public void writeToParcel(Parcel dest, int flags)
}


public void readFromParcel(Parcel source)
private void readFromParcel(Parcel source)
{
ClassLoader loader = getClass().getClassLoader();
mUri = source.readParcelable(loader);
Expand Down
3 changes: 2 additions & 1 deletion opentasks/src/main/java/org/dmfs/tasks/utils/OnContentLoadedListener.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package org.dmfs.tasks.utils;

import android.content.ContentValues;
import android.support.annotation.NonNull;


/**
Expand All @@ -32,5 +33,5 @@ public interface OnContentLoadedListener
* @param values
* The loaded {@link ContentValues}.
*/
public void onContentLoaded(ContentValues values);
void onContentLoaded(@NonNull ContentValues values);
}