-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Remove extra state and use Android APIs in activities with checkable lists #303 #480
Remove extra state and use Android APIs in activities with checkable lists #303 #480
Conversation
…eferencesActivity, and removes dependencies on it from those classes which only need the key strings.
…code to get ListPreferences
…eferencesActivity, and removes dependencies on it from those classes which only need the key strings. Inline a variable.
# Conflicts: # collect_app/src/main/java/org/odk/collect/android/preferences/PreferencesActivity.java
# Conflicts: # collect_app/src/main/java/org/odk/collect/android/preferences/PreferencesActivity.java
…lity. AdminPreferencesActivity: Move keys to new AdminKeys class.
…ty-improvements-2
… the values of the admin preferences. Clean up more code.
…lists #303. First stage.
…eckable-lists # Conflicts: # collect_app/src/main/java/org/odk/collect/android/activities/InstanceUploaderList.java
…eckable-lists # Conflicts: # collect_app/src/main/java/org/odk/collect/android/activities/InstanceUploaderList.java
…lists #303. Create a common superclass for these list classes. This could join with or replace ListViewUtils later.
…lists #303. This completes most of the work. More testing is needed.
…at for buttons’ enabled state, which is not saved automatically by Android.
I think we should follow field naming conventions from https://source.android.com/source/code-style.html insetad of using "this.something". |
Hi @grzesiek2010. Thanks for looking at the change. I can’t see what you’re referring to. (Did you attach your comment to a specific line of code?) Is it |
@dcbriccetti here for example https://github.com/opendatakit/collect/pull/480/files#diff-4bbc6fa3ceedd6337f7fcd9d3fbe5928R13 shouldn't that be mGeneralKey? |
@grzesiek2010 @dcbriccetti I guess the question is whether the |
“Non-public, non-static field names start with m.” This is not very relevant, but I really dislike the “m” convention. Modern IDEs make it very clear what’s what. “this” is usually needed only in the constructor, for copying an argument’s value to a field with the same name. To me, the “m” is noise. It slows me down. Examining an object in the debugger, nearly everything starts with an m. |
Ah, yes, I was assuming they were private. 😊 But the convention also is very much an Android thing. Like I said, I'm not terribly worried about that kind of stylistic thing for now. We need to get a checkstyle file we can all agree in and just make formatting and checking automatic. In fact, I have one to propose and I will create an issue for that shortly. |
I'm with Jake Wharton on this "m" issue 👎 |
int[] checkedPositions = new int[checkedItemCount]; | ||
long[] checkedIds = new long[checkedItemCount]; | ||
int resultIndex = 0; | ||
for (int posIdx = 0; posIdx < itemCount; ++posIdx) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe using a prefix increment here is any different than using a postfix one. I think postfix is more familiar and that this could be surprising.
} | ||
|
||
/** Returns the IDs of the checked items, as an array of Long */ | ||
@NonNull |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that @NonNull
was the default. Is there a reason to make it explicit here?
|
||
private static final String BUNDLE_SELECTED_ITEMS_KEY = "selected_items"; | ||
public class InstanceUploaderList extends AppListActivity implements OnLongClickListener { | ||
private static final String t = "InstanceUploaderList"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TAG
…ed item positions.
…tion change (orientation).
// keys for each preference | ||
|
||
// main menu | ||
public static String KEY_EDIT_SAVED = "edit_saved"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dcbriccetti I noticed during your pairing session that the keys aren't final here. I think they probably should be, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good observation. I missed that myself. They were that way when I moved them to the new file, but I missed an opportunity to improve them. Want to take any action now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll file it as a quick win for one of our new contributors! 😊 Just wanted to make sure I wasn't missing something.
…le-lists getodk#303, Remove extra state and use Android APIs in activities with checkable lists getodk#441, Refactor preference classes
This is branched off another pending branch, but the new commits start Mar 1.
I managed to remove the state and use the ListView methods, as requested, so far for one class:
InstanceUploaderList
. The rest will follow.