From dd673c23b9dec33e25ca27c5cc8082c49555af15 Mon Sep 17 00:00:00 2001 From: Sergey Magidovich Date: Fri, 1 Jul 2016 21:48:13 +0300 Subject: [PATCH] Code review. --- .../jni/com/mapswithme/core/jni_helper.hpp | 4 +- .../maps/editor/EditorFragment.java | 72 ++++++++----------- .../maps/editor/EditorHostFragment.java | 47 +++++++----- .../maps/editor/LanguagesFragment.java | 18 +++-- .../maps/editor/MultilanguageAdapter.java | 36 +++++----- .../mapswithme/maps/editor/StreetAdapter.java | 22 +++--- .../maps/editor/data/LocalizedName.java | 3 + indexer/editable_map_object.hpp | 1 - 8 files changed, 102 insertions(+), 101 deletions(-) diff --git a/android/jni/com/mapswithme/core/jni_helper.hpp b/android/jni/com/mapswithme/core/jni_helper.hpp index 7e26c3b292..6ac621c37c 100644 --- a/android/jni/com/mapswithme/core/jni_helper.hpp +++ b/android/jni/com/mapswithme/core/jni_helper.hpp @@ -9,8 +9,6 @@ #include "std/string.hpp" #include "std/shared_ptr.hpp" -#include "base/logging.hpp" - extern jclass g_mapObjectClazz; extern jclass g_bookmarkClazz; @@ -71,7 +69,7 @@ jobjectArray ToJavaArray(JNIEnv * env, jclass clazz, TIt begin, TIt end, size_t template jobjectArray ToJavaArray(JNIEnv * env, jclass clazz, TContainer const & src, TToJavaFn && toJavaFn) { - return ToJavaArray(env, clazz, begin(src), end(src), src.size(), move(toJavaFn)); + return ToJavaArray(env, clazz, begin(src), end(src), src.size(), forward(toJavaFn)); } jobjectArray ToJavaStringArray(JNIEnv * env, vector const & src); diff --git a/android/src/com/mapswithme/maps/editor/EditorFragment.java b/android/src/com/mapswithme/maps/editor/EditorFragment.java index a1723e5cc0..8335e8f5c5 100644 --- a/android/src/com/mapswithme/maps/editor/EditorFragment.java +++ b/android/src/com/mapswithme/maps/editor/EditorFragment.java @@ -38,6 +38,8 @@ import org.solovyev.android.views.llm.LinearLayoutManager; public class EditorFragment extends BaseMwmFragment implements View.OnClickListener, EditTextDialogFragment.OnTextSaveListener { + final static String LAST_LOCALIZED_NAME_INDEX = "LastLocalizedNameIndex"; + private TextView mCategory; private View mCardName; private View mCardAddress; @@ -46,7 +48,8 @@ public class EditorFragment extends BaseMwmFragment implements View.OnClickListe private RecyclerView mLocalizedNames; - private final RecyclerView.AdapterDataObserver mLocalizedNamesObserver = new RecyclerView.AdapterDataObserver() { + private final RecyclerView.AdapterDataObserver mLocalizedNamesObserver = new RecyclerView.AdapterDataObserver() + { @Override public void onChanged() { @@ -207,17 +210,6 @@ public class EditorFragment extends BaseMwmFragment implements View.OnClickListe setEdits(); } - private void writeNames() - { - LinearLayoutManager lm = (LinearLayoutManager) mLocalizedNames.getLayoutManager(); - for (int i = 0; i < mLocalizedNamesAdapter.getItemCount(); ++i) - { - View nameItem = lm.findViewByPosition(i); - EditText name = (EditText) nameItem.findViewById(R.id.input); - mParent.setLocalizedNameTo(name.getText().toString(), i + 1); // +1 Skip default name. - } - } - boolean setEdits() { if (!validateFields()) @@ -232,8 +224,6 @@ public class EditorFragment extends BaseMwmFragment implements View.OnClickListe Editor.nativeSetEmail(mEmail.getText().toString()); Editor.nativeSetHasWifi(mWifi.isChecked()); Editor.nativeSetOperator(mOperator.getText().toString()); - - writeNames(); Editor.nativeSetLocalizedNames(mParent.getLocalizedNamesAsArray()); return true; @@ -351,36 +341,34 @@ public class EditorFragment extends BaseMwmFragment implements View.OnClickListe refreshLocalizedNames(); final Bundle args = getArguments(); - if (args.containsKey(EditorHostFragment.kLastLocalizedNameIndex)) - { - showLocalizedNames(true); - UiUtils.waitLayout(mLocalizedNames, new ViewTreeObserver.OnGlobalLayoutListener() - { - @Override - public void onGlobalLayout() - { - LinearLayoutManager lm = (LinearLayoutManager) mLocalizedNames.getLayoutManager(); - int position = args.getInt(EditorHostFragment.kLastLocalizedNameIndex); - - View nameItem = mLocalizedNames.getLayoutManager().findViewByPosition(position); - - int cvNameTop = view.findViewById(R.id.cv__name).getTop(); - int nameItemTop = nameItem.getTop(); - - view.scrollTo(0, cvNameTop + nameItemTop); - - // TODO(mgsergio): Uncomment if focus and keyboard are required. - // TODO(mgsergio): Keyboard doesn't want to hide. Only pressing back button works. - // View nameItemInput = nameItem.findViewById(R.id.input); - // nameItemInput.requestFocus(); - // InputUtils.showKeyboard(nameItemInput); - } - }); - } - else + if (args == null || !args.containsKey(LAST_LOCALIZED_NAME_INDEX)) { showLocalizedNames(false); + return; } + showLocalizedNames(true); + UiUtils.waitLayout(mLocalizedNames, new ViewTreeObserver.OnGlobalLayoutListener() + { + @Override + public void onGlobalLayout() + { + LinearLayoutManager lm = (LinearLayoutManager) mLocalizedNames.getLayoutManager(); + int position = args.getInt(LAST_LOCALIZED_NAME_INDEX); + + View nameItem = lm.findViewByPosition(position); + + int cvNameTop = view.findViewById(R.id.cv__name).getTop(); + int nameItemTop = nameItem.getTop(); + + view.scrollTo(0, cvNameTop + nameItemTop); + + // TODO(mgsergio): Uncomment if focus and keyboard are required. + // TODO(mgsergio): Keyboard doesn't want to hide. Only pressing back button works. + // View nameItemInput = nameItem.findViewById(R.id.input); + // nameItemInput.requestFocus(); + // InputUtils.showKeyboard(nameItemInput); + } + }); } private void initViews(View view) @@ -388,7 +376,7 @@ public class EditorFragment extends BaseMwmFragment implements View.OnClickListe final View categoryBlock = view.findViewById(R.id.category); categoryBlock.setOnClickListener(this); // TODO show icon and fill it when core will implement that - // UiUtils.hide(categoryBlock.findViewById(R.id.icon)); + UiUtils.hide(categoryBlock.findViewById(R.id.icon)); mCategory = (TextView) categoryBlock.findViewById(R.id.name); mCardName = view.findViewById(R.id.cv__name); mCardAddress = view.findViewById(R.id.cv__address); diff --git a/android/src/com/mapswithme/maps/editor/EditorHostFragment.java b/android/src/com/mapswithme/maps/editor/EditorHostFragment.java index d8c8fd815b..116d900d61 100644 --- a/android/src/com/mapswithme/maps/editor/EditorHostFragment.java +++ b/android/src/com/mapswithme/maps/editor/EditorHostFragment.java @@ -27,14 +27,12 @@ import com.mapswithme.util.Utils; import com.mapswithme.util.statistics.Statistics; import java.util.ArrayList; -import java.util.Arrays; +import java.util.List; public class EditorHostFragment extends BaseMwmToolbarFragment implements OnBackPressListener, View.OnClickListener, LanguagesFragment.Listener { private boolean mIsNewObject; - final static String kExistingLocalizedNames = "ExistingLocalizedNames"; - final static String kLastLocalizedNameIndex = "LastLocalizedNameIndex"; enum Mode { @@ -47,33 +45,46 @@ public class EditorHostFragment extends BaseMwmToolbarFragment private Mode mMode; - /// A list of localized names added by a user and those that were in metadata. - private ArrayList mLocalizedNames; + /** + * A list of localized names added by a user and those that were in metadata. + */ + private static final List sLocalizedNames = new ArrayList<>(); - /// Used in MultilanguageAdapter to show, select and remove items. - ArrayList getLocalizedNames() + /** + * Used in MultilanguageAdapter to show, select and remove items. + */ + List getLocalizedNames() { - return mLocalizedNames; + return sLocalizedNames; } public LocalizedName[] getLocalizedNamesAsArray() { - return mLocalizedNames.toArray(new LocalizedName[mLocalizedNames.size()]); + return sLocalizedNames.toArray(new LocalizedName[sLocalizedNames.size()]); } void setLocalizedNames(LocalizedName[] names) { - mLocalizedNames = new ArrayList(Arrays.asList(names)); + sLocalizedNames.clear(); + for (LocalizedName name : names) + { + if (name.code == LocalizedName.DEFAULT_LANG_CODE) + continue; + sLocalizedNames.add(name); + } } - void setLocalizedNameTo(String name, int index) + /** + * Sets .name of an index item to name. + */ + void setName(String name, int index) { - mLocalizedNames.get(index).name = name; + sLocalizedNames.get(index).name = name; } void addLocalizedName(LocalizedName name) { - mLocalizedNames.add(name); + sLocalizedNames.add(name); } @Nullable @@ -152,7 +163,7 @@ public class EditorHostFragment extends BaseMwmToolbarFragment mToolbarController.setTitle(getTitle()); Bundle args = new Bundle(); if (focusToLastLocalizedName) - args.putInt(kLastLocalizedNameIndex, mLocalizedNames.size() - 2); // -1 for zero-based and one more -1 for ignoring default name. + args.putInt(EditorFragment.LAST_LOCALIZED_NAME_INDEX, sLocalizedNames.size() - 1); final Fragment editorFragment = Fragment.instantiate(getActivity(), EditorFragment.class.getName(), args); getChildFragmentManager().beginTransaction() .replace(R.id.fragment_container, editorFragment, EditorFragment.class.getName()) @@ -179,10 +190,10 @@ public class EditorHostFragment extends BaseMwmToolbarFragment protected void addLocalizedLanguage() { Bundle args = new Bundle(); - String[] languages = new String[mLocalizedNames.size()]; - for (int i = 0; i < mLocalizedNames.size(); ++i) - languages[i] = mLocalizedNames.get(i).lang; - args.putStringArray(kExistingLocalizedNames, languages); + ArrayList languages = new ArrayList<>(sLocalizedNames.size()); + for (LocalizedName name : sLocalizedNames) + languages.add(name.lang); + args.putStringArrayList(LanguagesFragment.EXISTING_LOCALIZED_NAMES, languages); editWithFragment(Mode.LANGUAGE, R.string.choose_language, args, LanguagesFragment.class, false); } diff --git a/android/src/com/mapswithme/maps/editor/LanguagesFragment.java b/android/src/com/mapswithme/maps/editor/LanguagesFragment.java index c7b77ed663..d1e8952a23 100644 --- a/android/src/com/mapswithme/maps/editor/LanguagesFragment.java +++ b/android/src/com/mapswithme/maps/editor/LanguagesFragment.java @@ -8,13 +8,17 @@ import com.mapswithme.maps.editor.data.Language; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; import java.util.Comparator; import java.util.HashSet; +import java.util.List; +import java.util.Set; -import static java.util.Collections.sort; public class LanguagesFragment extends BaseMwmRecyclerFragment { + final static String EXISTING_LOCALIZED_NAMES = "ExistingLocalizedNames"; + public interface Listener { void onLanguageSelected(Language language); @@ -24,17 +28,17 @@ public class LanguagesFragment extends BaseMwmRecyclerFragment protected RecyclerView.Adapter createAdapter() { Bundle args = getArguments(); - HashSet existingLanguages = new HashSet<>(Arrays.asList(args.getStringArray(EditorHostFragment.kExistingLocalizedNames))); + Set existingLanguages = new HashSet<>(args.getStringArrayList(EXISTING_LOCALIZED_NAMES)); - ArrayList languages = new ArrayList<>(); + List languages = new ArrayList<>(); for (Language lang : Editor.nativeGetSupportedLanguages()) { - if (existingLanguages.contains(lang.code)) - continue; - languages.add(lang); + if (!existingLanguages.contains(lang.code)) + languages.add(lang); } - sort(languages, new Comparator() { + Collections.sort(languages, new Comparator() + { @Override public int compare(Language lhs, Language rhs) { return lhs.name.compareTo(rhs.name); diff --git a/android/src/com/mapswithme/maps/editor/MultilanguageAdapter.java b/android/src/com/mapswithme/maps/editor/MultilanguageAdapter.java index 46815455e8..4fc1eca797 100644 --- a/android/src/com/mapswithme/maps/editor/MultilanguageAdapter.java +++ b/android/src/com/mapswithme/maps/editor/MultilanguageAdapter.java @@ -8,35 +8,21 @@ import android.widget.EditText; import com.mapswithme.maps.R; import com.mapswithme.maps.editor.data.LocalizedName; +import com.mapswithme.util.StringUtils; import com.mapswithme.util.UiUtils; -import java.util.ArrayList; +import java.util.List; public class MultilanguageAdapter extends RecyclerView.Adapter { - private ArrayList mNames; + private final List mNames; private EditorHostFragment mHostFragment; // TODO(mgsergio): Refactor: don't pass the whole EditorHostFragment. MultilanguageAdapter(EditorHostFragment hostFragment) { mHostFragment = hostFragment; - mNames = new ArrayList<>(); - ArrayList names = mHostFragment.getLocalizedNames(); - - // Skip default name. - for (int i = 1; i < names.size(); i++) - mNames.add(names.get(i)); - } - - public void setNames(ArrayList names) - { - mNames = names; - } - - public ArrayList getNames() - { - return mNames; + mNames = hostFragment.getLocalizedNames(); } @Override @@ -57,7 +43,10 @@ public class MultilanguageAdapter extends RecyclerView.Adapter