From bef4ba8d22f1fb889a8ab7b47e8cd8b122d95a89 Mon Sep 17 00:00:00 2001 From: Roman Tsisyk Date: Sun, 3 Dec 2023 18:00:26 +0200 Subject: [PATCH] [android] Refactor handling of links on PlacePage - Make PlacePageLinksFragment.getLink() non-static as requested on one of the previous code reviews. - Move getWebsiteUrl() and getKayakUrl() to MapObject. - Force @NonNull String invariant for getKayakUrl(). Currently all getMetadata() methods return @NonNull Strings. Replacing empty strings with null objects and removing TextUtils.isEmpty() clutter is very error-prone and requires a separate refactoring. - Fix few lint warnings. Signed-off-by: Roman Tsisyk --- .../organicmaps/bookmarks/data/MapObject.java | 50 ++++-- .../organicmaps/bookmarks/data/Metadata.java | 5 - .../organicmaps/car/screens/PlaceScreen.java | 16 +- .../app/organicmaps/car/util/UiHelpers.java | 3 - .../widget/placepage/PlacePageView.java | 11 +- .../sections/PlacePageLinksFragment.java | 149 ++++++------------ .../sections/PlacePageWikipediaFragment.java | 8 +- 7 files changed, 101 insertions(+), 141 deletions(-) diff --git a/android/app/src/main/java/app/organicmaps/bookmarks/data/MapObject.java b/android/app/src/main/java/app/organicmaps/bookmarks/data/MapObject.java index b0c75ce774..c2349aedbf 100644 --- a/android/app/src/main/java/app/organicmaps/bookmarks/data/MapObject.java +++ b/android/app/src/main/java/app/organicmaps/bookmarks/data/MapObject.java @@ -9,15 +9,19 @@ import androidx.annotation.NonNull; import androidx.annotation.Nullable; import androidx.core.os.ParcelCompat; +import app.organicmaps.Framework; import app.organicmaps.routing.RoutePointInfo; import app.organicmaps.search.Popularity; +import app.organicmaps.util.Utils; import app.organicmaps.widget.placepage.PlacePageData; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; import java.util.ArrayList; import java.util.Arrays; +import java.util.Date; import java.util.List; +import java.util.Objects; // TODO(yunikkk): Refactor. Displayed information is different from edited information, and it's better to // separate them. Simple getters from jni place_page::Info and osm::EditableFeature should be enough. @@ -46,6 +50,9 @@ public class MapObject implements PlacePageData public static final int OPENING_MODE_DETAILS = 2; public static final int OPENING_MODE_FULL = 3; + private static String kHttp = "http://"; + private static String kHttps = "https://"; + @NonNull private final FeatureId mFeatureId; @MapObjectType @@ -54,10 +61,11 @@ public class MapObject implements PlacePageData private String mTitle; @Nullable private final String mSecondaryTitle; - private String mSubtitle; + private final String mSubtitle; private double mLat; private double mLon; private final String mAddress; + @NonNull private final Metadata mMetadata; private final String mApiId; private final RoutePointInfo mRoutePointInfo; @@ -98,7 +106,7 @@ public class MapObject implements PlacePageData mAddress = address; mLat = lat; mLon = lon; - mMetadata = metadata; + mMetadata = metadata != null ? metadata : new Metadata(); mApiId = apiId; mRoutePointInfo = routePointInfo; mOpeningMode = openingMode; @@ -111,7 +119,7 @@ public class MapObject implements PlacePageData protected MapObject(@MapObjectType int type, Parcel source) { - this(ParcelCompat.readParcelable(source, FeatureId.class.getClassLoader(), FeatureId.class), // FeatureId + this(Objects.requireNonNull(ParcelCompat.readParcelable(source, FeatureId.class.getClassLoader(), FeatureId.class)), // FeatureId type, // MapObjectType source.readString(), // Title source.readString(), // SecondaryTitle @@ -123,8 +131,8 @@ public class MapObject implements PlacePageData source.readString(), // ApiId; ParcelCompat.readParcelable(source, RoutePointInfo.class.getClassLoader(), RoutePointInfo.class), // RoutePointInfo source.readInt(), // mOpeningMode - ParcelCompat.readParcelable(source, Popularity.class.getClassLoader(), Popularity.class), - source.readString(), + Objects.requireNonNull(ParcelCompat.readParcelable(source, Popularity.class.getClassLoader(), Popularity.class)), + Objects.requireNonNull(source.readString()), source.readInt(), null // mRawTypes ); @@ -258,9 +266,30 @@ public class MapObject implements PlacePageData return res == null ? "" : res; } - public boolean hasMetadata() + @NonNull + public String getWebsiteUrl(boolean strip) { - return mMetadata != null && !mMetadata.isEmpty(); + final String website = getMetadata(Metadata.MetadataType.FMD_WEBSITE); + final int len = website.length(); + if (strip && len > 1) + { + final int start = website.startsWith(kHttps) ? kHttps.length() : (website.startsWith(kHttp) ? kHttp.length() : 0); + final int end = website.endsWith("/") ? len - 1 : len; + return website.substring(start, end); + } + return website; + } + + @NonNull + public String getKayakUrl() + { + final String uri = getMetadata(Metadata.MetadataType.FMD_EXTERNAL_URI); + if (TextUtils.isEmpty(uri)) + return ""; + final Date firstDay = new Date(); + final Date lastDay = new Date(firstDay.getTime() + (1000 * 60 * 60 * 24)); + final String res = Framework.nativeGetKayakHotelLink(Utils.getCountryCode(), uri, firstDay, lastDay); + return res == null ? "" : res; } public String getApiId() @@ -306,11 +335,6 @@ public class MapObject implements PlacePageData return mMapObjectType == BOOKMARK; } - public final boolean isApiPoint() - { - return mMapObjectType == API_POINT; - } - @Nullable public RoutePointInfo getRoutePointInfo() { @@ -385,7 +409,7 @@ public class MapObject implements PlacePageData return mFeatureId.hashCode(); } - public static final Creator CREATOR = new Creator() + public static final Creator CREATOR = new Creator<>() { @Override public MapObject createFromParcel(Parcel source) diff --git a/android/app/src/main/java/app/organicmaps/bookmarks/data/Metadata.java b/android/app/src/main/java/app/organicmaps/bookmarks/data/Metadata.java index f5a2b88e2d..fa8687dfaf 100644 --- a/android/app/src/main/java/app/organicmaps/bookmarks/data/Metadata.java +++ b/android/app/src/main/java/app/organicmaps/bookmarks/data/Metadata.java @@ -97,11 +97,6 @@ public class Metadata implements Parcelable return mMetadataMap.get(type); } - boolean isEmpty() - { - return mMetadataMap.isEmpty(); - } - @Override public int describeContents() { diff --git a/android/app/src/main/java/app/organicmaps/car/screens/PlaceScreen.java b/android/app/src/main/java/app/organicmaps/car/screens/PlaceScreen.java index 55e677e794..66f3d9beba 100644 --- a/android/app/src/main/java/app/organicmaps/car/screens/PlaceScreen.java +++ b/android/app/src/main/java/app/organicmaps/car/screens/PlaceScreen.java @@ -6,6 +6,7 @@ import static android.text.Spanned.SPAN_INCLUSIVE_INCLUSIVE; import android.content.Intent; import android.net.Uri; import android.text.SpannableString; +import android.text.TextUtils; import androidx.annotation.NonNull; import androidx.annotation.Nullable; @@ -198,9 +199,10 @@ public class PlaceScreen extends BaseMapScreen implements OnBackPressedCallback. { Objects.requireNonNull(mMapObject); - final String phoneNumber = getFirstPhoneNumber(mMapObject); - if (!phoneNumber.isEmpty()) + final String phones = mMapObject.getMetadata(Metadata.MetadataType.FMD_PHONE_NUMBER); + if (!TextUtils.isEmpty(phones)) { + final String phoneNumber = phones.split(";", 1)[0]; final Action.Builder openDialBuilder = new Action.Builder(); openDialBuilder.setIcon(new CarIcon.Builder(IconCompat.createWithResource(getCarContext(), R.drawable.ic_phone)).build()); openDialBuilder.setOnClickListener(() -> getCarContext().startCarApp(new Intent(Intent.ACTION_DIAL, Uri.parse("tel:" + phoneNumber)))); @@ -242,16 +244,6 @@ public class PlaceScreen extends BaseMapScreen implements OnBackPressedCallback. mRoutingController.rebuildLastRoute(); } - @NonNull - private static String getFirstPhoneNumber(@NonNull MapObject mapObject) - { - if (!mapObject.hasMetadata()) - return ""; - - final String phones = mapObject.getMetadata(Metadata.MetadataType.FMD_PHONE_NUMBER); - return phones.split(";", 1)[0]; - } - @Override public void onBackPressed() { diff --git a/android/app/src/main/java/app/organicmaps/car/util/UiHelpers.java b/android/app/src/main/java/app/organicmaps/car/util/UiHelpers.java index 54f49c8563..5e09d50f8b 100644 --- a/android/app/src/main/java/app/organicmaps/car/util/UiHelpers.java +++ b/android/app/src/main/java/app/organicmaps/car/util/UiHelpers.java @@ -103,9 +103,6 @@ public final class UiHelpers @Nullable public static Row getPlaceOpeningHoursRow(@NonNull MapObject place, @NonNull CarContext context) { - if (!place.hasMetadata()) - return null; - final String ohStr = place.getMetadata(Metadata.MetadataType.FMD_OPEN_HOURS); final Timetable[] timetables = OpeningHours.nativeTimetablesFromString(ohStr); final boolean isEmptyTT = (timetables == null || timetables.length == 0); diff --git a/android/app/src/main/java/app/organicmaps/widget/placepage/PlacePageView.java b/android/app/src/main/java/app/organicmaps/widget/placepage/PlacePageView.java index 981545a0cb..06dd20c4ab 100644 --- a/android/app/src/main/java/app/organicmaps/widget/placepage/PlacePageView.java +++ b/android/app/src/main/java/app/organicmaps/widget/placepage/PlacePageView.java @@ -144,13 +144,12 @@ public class PlacePageView extends Fragment implements View.OnClickListener, private PlacePageViewModel mViewModel; private MapObject mMapObject; - private static void refreshMetadataOrHide(String metadata, View metaLayout, TextView metaTv) + private static void refreshMetadataOrHide(@Nullable String metadata, @NonNull View metaLayout, @NonNull TextView metaTv) { if (!TextUtils.isEmpty(metadata)) { metaLayout.setVisibility(VISIBLE); - if (metaTv != null) - metaTv.setText(metadata); + metaTv.setText(metadata); } else metaLayout.setVisibility(GONE); @@ -319,14 +318,14 @@ public class PlacePageView extends Fragment implements View.OnClickListener, private void updateLinksView() { - final boolean hasLinkAvailable = PlacePageLinksFragment.hasLinkAvailable(mMapObject); - updateViewFragment(PlacePageLinksFragment.class, LINKS_FRAGMENT_TAG, R.id.place_page_links_fragment, hasLinkAvailable); + updateViewFragment(PlacePageLinksFragment.class, LINKS_FRAGMENT_TAG, R.id.place_page_links_fragment, true); } private void updateOpeningHoursView() { final String ohStr = mMapObject.getMetadata(Metadata.MetadataType.FMD_OPEN_HOURS); - updateViewFragment(PlacePageOpeningHoursFragment.class, OPENING_HOURS_FRAGMENT_TAG, R.id.place_page_opening_hours_fragment, !ohStr.isEmpty()); + updateViewFragment(PlacePageOpeningHoursFragment.class, OPENING_HOURS_FRAGMENT_TAG, + R.id.place_page_opening_hours_fragment, !TextUtils.isEmpty(ohStr)); } private void updatePhoneView() diff --git a/android/app/src/main/java/app/organicmaps/widget/placepage/sections/PlacePageLinksFragment.java b/android/app/src/main/java/app/organicmaps/widget/placepage/sections/PlacePageLinksFragment.java index 37db55e8ec..be1839c4f8 100644 --- a/android/app/src/main/java/app/organicmaps/widget/placepage/sections/PlacePageLinksFragment.java +++ b/android/app/src/main/java/app/organicmaps/widget/placepage/sections/PlacePageLinksFragment.java @@ -20,15 +20,11 @@ import app.organicmaps.bookmarks.data.MapObject; import app.organicmaps.bookmarks.data.Metadata; import app.organicmaps.util.UiUtils; import app.organicmaps.util.Utils; -import app.organicmaps.util.log.Logger; import app.organicmaps.widget.placepage.PlacePageUtils; import app.organicmaps.widget.placepage.PlacePageViewModel; import java.util.ArrayList; -import java.util.Arrays; -import java.util.Date; import java.util.List; -import java.util.Objects; import static android.view.View.GONE; import static android.view.View.VISIBLE; @@ -37,17 +33,6 @@ public class PlacePageLinksFragment extends Fragment implements Observer supportedLinks = Arrays.asList( - Metadata.MetadataType.FMD_WEBSITE, - Metadata.MetadataType.FMD_EMAIL, - Metadata.MetadataType.FMD_WIKIMEDIA_COMMONS, - Metadata.MetadataType.FMD_EXTERNAL_URI, - Metadata.MetadataType.FMD_CONTACT_FACEBOOK, - Metadata.MetadataType.FMD_CONTACT_INSTAGRAM, - Metadata.MetadataType.FMD_CONTACT_TWITTER, - Metadata.MetadataType.FMD_CONTACT_VK, - Metadata.MetadataType.FMD_CONTACT_LINE); - private View mFrame; private View mFacebookPage; private TextView mTvFacebookPage; @@ -71,51 +56,40 @@ public class PlacePageLinksFragment extends Fragment implements Observer { - final String url = Objects.requireNonNull(getExternalUrl(mMapObject)); + final String url = mMapObject.getKayakUrl(); final MwmActivity activity = (MwmActivity) requireActivity(); - activity.openKayakLink(url); + if (!TextUtils.isEmpty(url)) + activity.openKayakLink(url); }); mKayak.setOnLongClickListener((v) -> { - final String url = Objects.requireNonNull(getExternalUrl(mMapObject)); - PlacePageUtils.copyToClipboard(requireContext(), mFrame, url); + final String url = mMapObject.getKayakUrl(); + if (!TextUtils.isEmpty(url)) + PlacePageUtils.copyToClipboard(requireContext(), mFrame, url); return true; }); @@ -149,8 +125,11 @@ public class PlacePageLinksFragment extends Fragment implements Observer Utils.sendTo(requireContext(), getLink(mMapObject, - Metadata.MetadataType.FMD_EMAIL))); + mEmail.setOnClickListener(v -> { + final String email = mMapObject.getMetadata(Metadata.MetadataType.FMD_EMAIL); + if (!TextUtils.isEmpty(email)) + Utils.sendTo(requireContext(), email); + }); mEmail.setOnLongClickListener((v) -> copyUrl(mEmail, Metadata.MetadataType.FMD_EMAIL)); mWikimedia = mFrame.findViewById(R.id.ll__place_wikimedia); @@ -184,30 +163,26 @@ public class PlacePageLinksFragment extends Fragment implements Observer copyUrl(mLinePage, Metadata.MetadataType.FMD_CONTACT_LINE)); } - private boolean isSocialUsername(Metadata.MetadataType type) - { - return !mMapObject.getMetadata(type).contains("/"); - } - private void openUrl(Metadata.MetadataType type) { - final String url = getLink(mMapObject, type); + final String url = getLink(type); if (!TextUtils.isEmpty(url)) Utils.openUrl(requireContext(), url); } private boolean copyUrl(View view, Metadata.MetadataType type) { - final String url = getLink(mMapObject, type); + final String url = getLink(type); + if (TextUtils.isEmpty(url)) + return false; final List items = new ArrayList<>(); items.add(url); - final String metadata = type == Metadata.MetadataType.FMD_WEBSITE - ? getWebsiteUrl(mMapObject, false /* strip */) - : mMapObject.getMetadata(type); + final String title = type == Metadata.MetadataType.FMD_WEBSITE ? + mMapObject.getWebsiteUrl(false /* strip */) : mMapObject.getMetadata(type); // Add user names for social media if available - if (!metadata.equals(url) && isSocialUsername(type)) - items.add(metadata); + if (!TextUtils.isEmpty(title) && !title.equals(url) && !title.contains("/")) + items.add(title); if (items.size() == 1) PlacePageUtils.copyToClipboard(requireContext(), mFrame, items.get(0)); @@ -216,59 +191,33 @@ public class PlacePageLinksFragment extends Fragment implements Observer 1) - { - final int start = website.startsWith(kHttps) ? kHttps.length() : (website.startsWith(kHttp) ? kHttp.length() : 0); - final int end = website.endsWith("/") ? len - 1 : len; - return website.substring(start, end); - } - return website; - } - private void refreshLinks() { - UiUtils.showIf(getExternalUrl(mMapObject) != null, mKayak); - refreshMetadataOrHide(getWebsiteUrl(mMapObject, true /* strip */), mWebsite, mTvWebsite); + UiUtils.showIf(!TextUtils.isEmpty(mMapObject.getKayakUrl()), mKayak); + refreshMetadataOrHide(mMapObject.getWebsiteUrl(true /* strip */), mWebsite, mTvWebsite); + String wikimedia_commons = mMapObject.getMetadata(Metadata.MetadataType.FMD_WIKIMEDIA_COMMONS); String wikimedia_commons_text = TextUtils.isEmpty(wikimedia_commons) ? "" : getResources().getString(R.string.wikimedia_commons); refreshMetadataOrHide(wikimedia_commons_text, mWikimedia, mTvWikimedia); refreshMetadataOrHide(mMapObject.getMetadata(Metadata.MetadataType.FMD_EMAIL), mEmail, mTvEmail); - refreshSocialPageLink(mMapObject, mFacebookPage, mTvFacebookPage, Metadata.MetadataType.FMD_CONTACT_FACEBOOK); - refreshSocialPageLink(mMapObject, mInstagramPage, mTvInstagramPage, Metadata.MetadataType.FMD_CONTACT_INSTAGRAM); - refreshSocialPageLink(mMapObject, mTwitterPage, mTvTwitterPage, Metadata.MetadataType.FMD_CONTACT_TWITTER); - refreshSocialPageLink(mMapObject, mVkPage, mTvVkPage, Metadata.MetadataType.FMD_CONTACT_VK); - refreshSocialPageLink(mMapObject, mLinePage, mTvLinePage, Metadata.MetadataType.FMD_CONTACT_LINE); + final String facebook = mMapObject.getMetadata(Metadata.MetadataType.FMD_CONTACT_FACEBOOK); + refreshMetadataOrHide(facebook, mFacebookPage, mTvFacebookPage); + + final String instagram = mMapObject.getMetadata(Metadata.MetadataType.FMD_CONTACT_INSTAGRAM); + refreshMetadataOrHide(instagram, mInstagramPage, mTvInstagramPage); + + final String twitter = mMapObject.getMetadata(Metadata.MetadataType.FMD_CONTACT_TWITTER); + refreshMetadataOrHide(twitter, mTwitterPage, mTvTwitterPage); + + final String vk = mMapObject.getMetadata(Metadata.MetadataType.FMD_CONTACT_VK); + refreshMetadataOrHide(vk, mVkPage, mTvVkPage); + + final String line = mMapObject.getMetadata(Metadata.MetadataType.FMD_CONTACT_LINE); + refreshMetadataOrHide(line, mLinePage, mTvLinePage); + + final String kayak = mMapObject.getKayakUrl(); + UiUtils.showIf(!TextUtils.isEmpty(kayak), mKayak); } @Override diff --git a/android/app/src/main/java/app/organicmaps/widget/placepage/sections/PlacePageWikipediaFragment.java b/android/app/src/main/java/app/organicmaps/widget/placepage/sections/PlacePageWikipediaFragment.java index 51b0a5544a..8ba23cc115 100644 --- a/android/app/src/main/java/app/organicmaps/widget/placepage/sections/PlacePageWikipediaFragment.java +++ b/android/app/src/main/java/app/organicmaps/widget/placepage/sections/PlacePageWikipediaFragment.java @@ -105,9 +105,13 @@ public class PlacePageWikipediaFragment extends Fragment implements Observer Utils.openUrl(requireContext(), wikipediaLink)); + mWiki.setOnClickListener((v) -> { + if (!TextUtils.isEmpty(wikipediaLink)) + Utils.openUrl(requireContext(), wikipediaLink); + }); mWiki.setOnLongClickListener((v) -> { - PlacePageUtils.copyToClipboard(requireContext(), mFrame, wikipediaLink); + if (!TextUtils.isEmpty(wikipediaLink)) + PlacePageUtils.copyToClipboard(requireContext(), mFrame, wikipediaLink); return true; }); }