Remove extra state and use Android APIs in activities with checkable lists #303 #480

dcbriccetti · 2017-03-01T08:21:59Z

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.

…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

…lity. AdminPreferencesActivity: Move keys to new AdminKeys class.

…enceActivity.

…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

…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.

grzesiek2010 · 2017-03-02T15:03:18Z

I think we should follow field naming conventions from https://source.android.com/source/code-style.html insetad of using "this.something".

dcbriccetti · 2017-03-02T15:58:27Z

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 CheckedItemInfo? Are you talking about the “m” prefix? If so, did you consider that the fields are public, and that rule doesn’t apply?

grzesiek2010 · 2017-03-02T16:04:24Z

@dcbriccetti here for example https://github.com/opendatakit/collect/pull/480/files#diff-4bbc6fa3ceedd6337f7fcd9d3fbe5928R13 shouldn't that be mGeneralKey?

lognaturel · 2017-03-02T16:37:40Z

@grzesiek2010 @dcbriccetti I guess the question is whether the m convention should apply to "plain old Java objects." I think the current code base isn't very consistent so I'm ok accepting both and making that decision as part of an eventual concretizing of the style guidelines.

dcbriccetti · 2017-03-02T17:39:23Z

“Non-public, non-static field names start with m.”
These fields are public. Of course, a JavaBean purist would say that they should be private and I should have made getter methods.

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.

lognaturel · 2017-03-02T17:43:44Z

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.

nribeka · 2017-03-02T17:59:18Z

I'm with Jake Wharton on this "m" issue 👎

https://twitter.com/jakewharton/status/497243553311907840

…rings.

…ity.

lognaturel · 2017-03-02T18:35:47Z

collect_app/src/main/java/org/odk/collect/android/activities/AppListActivity.java

+        int[] checkedPositions = new int[checkedItemCount];
+        long[] checkedIds = new long[checkedItemCount];
+        int resultIndex = 0;
+        for (int posIdx = 0; posIdx < itemCount; ++posIdx) {


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.

lognaturel · 2017-03-02T18:40:21Z

collect_app/src/main/java/org/odk/collect/android/activities/AppListActivity.java

+    }
+
+    /** Returns the IDs of the checked items, as an array of Long */
+    @NonNull


I thought that @NonNull was the default. Is there a reason to make it explicit here?

lognaturel · 2017-03-02T18:52:37Z

collect_app/src/main/java/org/odk/collect/android/activities/InstanceUploaderList.java

-
-    private static final String BUNDLE_SELECTED_ITEMS_KEY = "selected_items";
+public class InstanceUploaderList extends AppListActivity implements OnLongClickListener {
+    private static final String t = "InstanceUploaderList";


…ed item positions.

…tion change (orientation).

lognaturel · 2017-03-08T21:38:28Z

collect_app/src/main/java/org/odk/collect/android/preferences/AdminKeys.java

+    // keys for each preference
+
+    // main menu
+    public  static String KEY_EDIT_SAVED                = "edit_saved";


@dcbriccetti I noticed during your pairing session that the keys aren't final here. I think they probably should be, right?

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?

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

dcbriccetti added 16 commits February 26, 2017 23:02

Make mFontSizePreference a local variable.

3ae773d

Move preference keys to a new class, PreferenceKeys. This shortens Pr…

8ed494b

…eferencesActivity, and removes dependencies on it from those classes which only need the key strings.

Encapsulate the findPreference deprecation warning and allow shorter …

fff4cfb

…code to get ListPreferences

Move preference keys to a new class, PreferenceKeys. This shortens Pr…

b3ded2d

…eferencesActivity, and removes dependencies on it from those classes which only need the key strings. Inline a variable.

Merge remote-tracking branch 'remotes/upstream/master'

b7fc893

# Conflicts: # collect_app/src/main/java/org/odk/collect/android/preferences/PreferencesActivity.java

Merge remote-tracking branch 'remotes/upstream/master'

a2ddbd6

# Conflicts: # collect_app/src/main/java/org/odk/collect/android/preferences/PreferencesActivity.java

Merge remote-tracking branch 'upstream/master'

e72bd39

PreferencesActivity: Remove duplicate code. Organize. Improve readabi…

796970c

…lity. AdminPreferencesActivity: Move keys to new AdminKeys class.

Further improve the code that removes disabled preferences.

6227962

Fix comments.

31c92c3

Make minor improvements to DisabledPreferencesRemover

c0574a9

Push up common preferences code to new abstract superclass, AppPrefer…

e28bdb8

…enceActivity.

Merge remote-tracking branch 'upstream/master' into PreferencesActivi…

7dd22ac

…ty-improvements-2

Further improve the automatic removal of general preferences based on…

5adc77f

… the values of the admin preferences. Clean up more code.

Inline a field

71e149f

Remove extra state and use Android APIs in activities with checkable …

2563f9d

…lists #303. First stage.

dcbriccetti changed the title ~~303 activities checkable lists~~ Remove extra state and use Android APIs in activities with checkable lists #303 Mar 1, 2017

dcbriccetti added 5 commits March 1, 2017 11:15

Merge remote-tracking branch 'upstream/master' into 303-activities-ch…

9a2ed90

…eckable-lists # Conflicts: # collect_app/src/main/java/org/odk/collect/android/activities/InstanceUploaderList.java

Merge remote-tracking branch 'upstream/master' into 303-activities-ch…

4c3b602

…eckable-lists # Conflicts: # collect_app/src/main/java/org/odk/collect/android/activities/InstanceUploaderList.java

Remove extra state and use Android APIs in activities with checkable …

7ce7075

…lists #303. Create a common superclass for these list classes. This could join with or replace ListViewUtils later.

Remove extra state and use Android APIs in activities with checkable …

ed53604

…lists #303. This completes most of the work. More testing is needed.

Discard checkbox checked state saving and restoring code, but keep th…

f4e3446

…at for buttons’ enabled state, which is not saved automatically by Android.

lognaturel mentioned this pull request Mar 2, 2017

Issue #441, Refactor preference classes, Part 2 #475

Closed

dcbriccetti added 2 commits March 2, 2017 10:08

Fix my global change error, and make Log tag objects simple static st…

e277ac5

…rings.

Move remaining ListViewUtils code to abstract base class AppListActiv…

7020f7f

…ity.

lognaturel requested changes Mar 2, 2017

View reviewed changes

dcbriccetti added 6 commits March 2, 2017 11:00

Simplify AppListActivity getCheckedItemInfo, now that we no longer ne…

ce5ffd6

…ed item positions.

Remove @nonnull. Change ++i to i++.

3250e22

Change ++i to i++.

ba4e5f8

Restore the enabled state of the Select/Clear All button on configura…

2865e1d

…tion change (orientation).

Remove unused import

bd27bd4

Restore inadvertently-removed line.

cd04af1

lognaturel approved these changes Mar 2, 2017

View reviewed changes

lognaturel merged commit 2d14564 into getodk:master Mar 2, 2017

dcbriccetti deleted the 303-activities-checkable-lists branch March 4, 2017 20:32

lognaturel reviewed Mar 8, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

dcbriccetti commented Mar 1, 2017

grzesiek2010 commented Mar 2, 2017

dcbriccetti commented Mar 2, 2017

grzesiek2010 commented Mar 2, 2017 •

edited

Loading

lognaturel commented Mar 2, 2017

dcbriccetti commented Mar 2, 2017

lognaturel commented Mar 2, 2017 •

edited

Loading

nribeka commented Mar 2, 2017

lognaturel Mar 2, 2017

lognaturel Mar 2, 2017

lognaturel Mar 2, 2017

lognaturel Mar 8, 2017

dcbriccetti Mar 9, 2017

lognaturel Mar 9, 2017

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

dcbriccetti commented Mar 1, 2017

grzesiek2010 commented Mar 2, 2017

dcbriccetti commented Mar 2, 2017

grzesiek2010 commented Mar 2, 2017 • edited Loading

lognaturel commented Mar 2, 2017

dcbriccetti commented Mar 2, 2017

lognaturel commented Mar 2, 2017 • edited Loading

nribeka commented Mar 2, 2017

lognaturel Mar 2, 2017

Choose a reason for hiding this comment

lognaturel Mar 2, 2017

Choose a reason for hiding this comment

lognaturel Mar 2, 2017

Choose a reason for hiding this comment

lognaturel Mar 8, 2017

Choose a reason for hiding this comment

dcbriccetti Mar 9, 2017

Choose a reason for hiding this comment

lognaturel Mar 9, 2017

Choose a reason for hiding this comment

grzesiek2010 commented Mar 2, 2017 •

edited

Loading

lognaturel commented Mar 2, 2017 •

edited

Loading