Added ability to hide buttons/alert views #1251

devotaaabel · 2018-08-31T16:04:41Z

Closes #1217 and relates to #1216

danesfeder

@devotaaabel this is looking great - I love that we are able to remove some code from InstructionView.

Just some general comments and cleanup. Can you add javadoc for all public interfaces and methods? Also, can we update the method declaration to public > protected > private -- saw some out of order (SoundButton) and figured I'd call it out before @Guardiola31337 does 😉

...i/src/main/java/com/mapbox/services/android/navigation/ui/v5/instruction/FeedbackButton.java

+public class FeedbackButton extends ConstraintLayout implements NavigationButton {
+  private FloatingActionButton feedbackFab;
+  private List<OnClickListener> onClickListeners;
+


danesfeder · 2018-09-10T13:27:46Z

.../src/main/java/com/mapbox/services/android/navigation/ui/v5/instruction/InstructionView.java

@@ -586,29 +476,25 @@ private void addBottomSheetListener() {
  }

  private void initializeClickListeners() {


We can update the naming here to better describe the button setup functions it's calling

danesfeder · 2018-09-10T13:28:07Z

.../src/main/java/com/mapbox/services/android/navigation/ui/v5/instruction/InstructionView.java

@@ -809,7 +695,7 @@ private void hideThenStepLayout() {
  }

  @Nullable
-  private FragmentManager obtainSupportFragmentManger() {
+  private FragmentManager obtainSupportFragmentManager() {


Nice typo catch here 😅

danesfeder · 2018-09-10T13:30:02Z

.../main/java/com/mapbox/services/android/navigation/ui/v5/instruction/NavigationAlertView.java

+    show(NavigationConstants.FEEDBACK_SUBMITTED, 3000, false);
+  }
+
+  public void showReportProblemFor3Seconds() {


Rather than hardcode this to 3Seconds can we update to showReportProblemFor(int seconds), passing in the constant at the call site?

devotaaabel · 2018-09-10T19:42:21Z

@danesfeder ready for re-review

danesfeder

@devotaaabel thanks for the updates 🚀

Just a couple minor comments / questions

danesfeder · 2018-09-13T15:01:17Z

.../src/main/java/com/mapbox/services/android/navigation/ui/v5/instruction/InstructionView.java

@@ -342,6 +327,14 @@ public void setDistanceFormatter(DistanceFormatter distanceFormatter) {
    }
  }

+  public NavigationButton retrieveSoundButton() {


Still missing some javadoc here and retrieveFeedbackButton

danesfeder · 2018-09-13T15:04:29Z

.../main/java/com/mapbox/services/android/navigation/ui/v5/instruction/NavigationAlertView.java

+   *
+   * @param navigationViewModel to set
+   */
+  public void setModel(NavigationViewModel navigationViewModel) {


To align method naming here, does it make sense to update this to subscribe like InstructionView#subscribe(NavigationViewModel)?

devotaaabel · 2018-09-18T14:58:24Z

@danesfeder ready for re-review

Guardiola31337 · 2018-09-20T09:55:30Z

Originally reported by @zeallat in #1301

I want to create custom SummaryBottomSheet and InstructionView, but it seems no features are provide for this. So I want to hide original SummaryBottomSheet and InstructionView and generate other.

How can I set visibility(especially View.GONE) of SummaryBottomSheet and InstructionView?
Many thanks for read this issue.

Is it possible to include this feature request as part of this PR @devotaaabel? Does that make sense at all? If not, feel free to re-open #1301 and we can fix in a different PR. What do you think @devotaaabel?

devotaaabel · 2018-09-20T12:43:08Z

@Guardiola31337 I think we should communicate with the developer about creating their own custom view to allow for custom components before adding the ability to hide.

Guardiola31337 · 2018-09-20T13:14:33Z

@devotaaabel

I think we should communicate with the developer about creating their own custom view to allow for custom components before adding the ability to hide.

Followed up in 👉 #1301 (comment)

Let's hold off in adding this feature here and revisit later if finally needed (seems overkill anyways).

Will give a final round of 👀 and we can merge here 🚀

Guardiola31337

Overall this looks good @devotaaabel Great work 👏

Left some minor comments / questions and cleanup before merging here.

Guardiola31337 · 2018-09-20T14:19:35Z

...vigation-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/NavigationButton.java

+  void addOnClickListener(View.OnClickListener onClickListener);
+
+  /**
+   * Hides the widget


Minor thing - Should this be button (instead of widget) to better reflect what's state in the interface name? The other way around could also work i.e. naming the interface NavigationWidget but not sure if that would make sense though for all the cases in which it's implemented. It seems there are all "buttons" after all.

Guardiola31337 · 2018-09-20T14:20:51Z

...vigation-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/NavigationButton.java

+  void hide();
+
+  /**
+   * Shows the widget


Same here ☝️

Guardiola31337 · 2018-09-20T14:22:56Z

...navigation-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/NavigationView.java

+  }
+
+  /**
+   * Returns the feedback button for sending feedback about the maps


Minor thing - about the *navigation (instead of maps) would better describe what the feedback button allows you to do.

Guardiola31337 · 2018-09-20T14:25:11Z

...navigation-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/NavigationView.java

+    return instructionView.retrieveFeedbackButton();
+  }
+
+  public NavigationButton retrieveRecenterButton() {


Still missing some Javadoc here.

Guardiola31337 · 2018-09-20T15:01:42Z

...navigation-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/RecenterButton.java

+  private void setupOnClickListeners() {
+    onClickListeners = new ArrayList<>();
+
+    setOnClickListener(new OnClickListener() {


What about extracting the listener into a class property?

Same thing happens in FeedbackButton and SoundButton.

Guardiola31337 · 2018-09-20T16:36:05Z

.../src/main/java/com/mapbox/services/android/navigation/ui/v5/instruction/InstructionView.java

+  }
+
+  private void setupSoundButton() {
+    soundButton.addOnClickListener(new View.OnClickListener() {


Same here ☝️

Minor thing - View. is not needed (OnClickListener is enough as in setupFeedbackButton ☝️)

Guardiola31337 · 2018-09-20T16:40:14Z

.../main/java/com/mapbox/services/android/navigation/ui/v5/instruction/NavigationAlertView.java

+   * Shows this alert view for when feedback is submitted
+   */
+  public void showFeedbackSubmitted() {
+    show(NavigationConstants.FEEDBACK_SUBMITTED, 3000, false);


NIT Magic number

Guardiola31337 · 2018-09-20T16:42:13Z

.../main/java/com/mapbox/services/android/navigation/ui/v5/instruction/NavigationAlertView.java

+
+  private void showReportProblem() {
+    show(NavigationConstants.REPORT_PROBLEM,
+      NavigationConstants.ALERT_VIEW_PROBLEM_DURATION, true);


NIT unnecessary new line

Guardiola31337 · 2018-09-20T16:42:42Z

.../main/java/com/mapbox/services/android/navigation/ui/v5/instruction/NavigationAlertView.java

+  @Override
+  protected void onFinishInflate() {
+    super.onFinishInflate();
+    setOnClickListener(new View.OnClickListener() {


What about extracting the listener into a class property?

Guardiola31337 · 2018-09-20T16:43:15Z

...n-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/instruction/SoundButton.java

+  }
+
+  private void initializeAnimation() {
+    Animation fadeIn = new AlphaAnimation(0, 1);


Magic numbers ☝️ 👇

Guardiola31337

Thanks for the updates @devotaaabel 🚀

Just a couple minor comments / questions

Guardiola31337 · 2018-09-24T17:29:00Z

...navigation-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/FeedbackButton.java

+import android.util.AttributeSet;
+
+public class FeedbackButton extends ConstraintLayout implements NavigationButton {
+  private FloatingActionButton feedbackFab;


Private field 'feedbackFab' is assigned but never accessed

@Guardiola31337 I meant to add the listener to the FAB and not the whole widget. It is corrected

Guardiola31337 · 2018-09-24T17:30:35Z

...navigation-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/FeedbackButton.java

+  }
+
+  /**
+   * Hides the widget


👀 #1251 (comment)

Guardiola31337 · 2018-09-24T17:31:03Z

...navigation-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/FeedbackButton.java

+  }
+
+  /**
+   * Shows the widget


👀 #1251 (comment)

Guardiola31337 · 2018-09-24T17:34:38Z

...tion-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/MultiOnClickListener.java

+
+class MultiOnClickListener implements View.OnClickListener {
+  private Set<View.OnClickListener> onClickListeners;
+


NIT unnecessary new line

Guardiola31337 · 2018-09-24T17:49:14Z

...tion-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/MultiOnClickListener.java

+    onClickListeners.add(onClickListener);
+  }
+
+  void removeListener(View.OnClickListener onClickListener) {


Saw that although we're adding listeners using the above API addListener

mapbox-navigation-android/libandroid-navigation-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/instruction/InstructionView.java

Line 480 in 2f5d53a

feedbackButton.addOnClickListener(new OnClickListener() {

mapbox-navigation-android/libandroid-navigation-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/instruction/InstructionView.java

Line 490 in 2f5d53a

soundButton.addOnClickListener(new View.OnClickListener() {

mapbox-navigation-android/libandroid-navigation-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/NavigationView.java

Line 562 in 2f5d53a

recenterBtn.addOnClickListener(new RecenterBtnClickListener(navigationPresenter));

we're not calling the removeListener counterpart at all 🤔

Shouldn't we add calls where appropriate to avoid potential memory leaks?

Guardiola31337 · 2018-09-24T18:06:23Z

...id-navigation-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/SoundButton.java

+    if (android.os.Build.VERSION.SDK_INT < android.os.Build.VERSION_CODES.LOLLIPOP) {
+      Drawable soundChipBackground = DrawableCompat.wrap(soundChipText.getBackground()).mutate();
+      int navigationViewPrimaryColor = ThemeSwitcher.retrieveThemeColor(getContext(),
+        R.attr.navigationViewPrimary);


👀 #1251 (comment)

Guardiola31337 · 2018-09-24T18:09:57Z

.../src/main/java/com/mapbox/services/android/navigation/ui/v5/instruction/InstructionView.java

+   * Gets the sound button which is used for muting/unmuting, for uses such as adding listeners and
+   * hiding the button.
+   *
+   * @return Sound button with {@link NavigationButton} API


Minor thing - sound should be lowercase

Guardiola31337 · 2018-09-24T18:10:07Z

.../src/main/java/com/mapbox/services/android/navigation/ui/v5/instruction/InstructionView.java

+   * Gets the feedback button which is used for sending feedback, for uses such as adding listeners
+   * and hiding the button.
+   *
+   * @return Feedback button with {@link NavigationButton} API


Minor thing - feedback should be lowercase

Guardiola31337 · 2018-09-24T18:11:24Z

.../src/main/java/com/mapbox/services/android/navigation/ui/v5/instruction/InstructionView.java

+  }
+
+  private void setupSoundButton() {
+    soundButton.addOnClickListener(new View.OnClickListener() {


Minor thing - View. is not needed (OnClickListener is enough as in setupFeedbackButton ☝️)

Guardiola31337 · 2018-09-24T18:16:50Z

.../main/java/com/mapbox/services/android/navigation/ui/v5/instruction/NavigationAlertView.java

+      @Override
+      public void run() {
+        show(NavigationConstants.REPORT_PROBLEM,
+          NavigationConstants.ALERT_VIEW_PROBLEM_DURATION, true);


👀 #1251 (comment)

Guardiola31337

Thanks for addressing all the comments @devotaaabel

devotaaabel added the ✓ ready for review label Aug 31, 2018

devotaaabel added this to the 0.18.0 milestone Aug 31, 2018

devotaaabel self-assigned this Aug 31, 2018

devotaaabel requested review from Guardiola31337 and danesfeder August 31, 2018 16:04

devotaaabel force-pushed the devota-allow-hiding-of-components branch from ce8fe34 to 81e80b8 Compare August 31, 2018 16:19

danesfeder modified the milestones: 0.18.0, 0.19.0 Sep 4, 2018

devotaaabel added ⚠️ DO NOT MERGE PR should not be merged! and removed ✓ ready for review labels Sep 4, 2018

devotaaabel force-pushed the devota-allow-hiding-of-components branch 7 times, most recently from 0ee06db to 858d8af Compare September 7, 2018 18:58

devotaaabel added ✓ ready for review and removed ⚠️ DO NOT MERGE PR should not be merged! labels Sep 7, 2018

danesfeder reviewed Sep 10, 2018

View reviewed changes

devotaaabel force-pushed the devota-allow-hiding-of-components branch from 858d8af to 8d1c28e Compare September 10, 2018 19:41

devotaaabel force-pushed the devota-allow-hiding-of-components branch 3 times, most recently from 5d30c0d to 2d256c2 Compare September 11, 2018 19:49

danesfeder reviewed Sep 13, 2018

View reviewed changes

devotaaabel force-pushed the devota-allow-hiding-of-components branch from fcf2bdc to 71837ba Compare September 18, 2018 14:21

Guardiola31337 mentioned this pull request Sep 20, 2018

How can I set visibility of SummaryBottomSheet and InstructionView? #1301

Closed

devotaaabel force-pushed the devota-allow-hiding-of-components branch from 71837ba to 8b88cc3 Compare September 20, 2018 12:43

devotaaabel force-pushed the devota-allow-hiding-of-components branch from 8b88cc3 to 0e2ab24 Compare September 20, 2018 13:24

Guardiola31337 reviewed Sep 20, 2018

View reviewed changes

devotaaabel force-pushed the devota-allow-hiding-of-components branch from 0e2ab24 to d49c154 Compare September 21, 2018 15:41

Guardiola31337 reviewed Sep 24, 2018

View reviewed changes

devotaaabel force-pushed the devota-allow-hiding-of-components branch from 2f5d53a to 190e8b2 Compare September 24, 2018 19:46

Added ability to hide buttons/alert views

d45f212

devotaaabel force-pushed the devota-allow-hiding-of-components branch from 190e8b2 to d45f212 Compare September 24, 2018 19:57

Guardiola31337 approved these changes Sep 24, 2018

View reviewed changes

devotaaabel merged commit 2d92c9b into master Sep 24, 2018

devotaaabel deleted the devota-allow-hiding-of-components branch September 24, 2018 20:26

danesfeder mentioned this pull request Sep 26, 2018

[Question] How can i hide alert view and feedback? #634

Closed

		@@ -586,29 +476,25 @@ private void addBottomSheetListener() {
		}

		private void initializeClickListeners() {


		class MultiOnClickListener implements View.OnClickListener {
		private Set<View.OnClickListener> onClickListeners;

Added ability to hide buttons/alert views #1251

Added ability to hide buttons/alert views #1251

Conversation

devotaaabel commented Aug 31, 2018 • edited Loading

danesfeder left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devotaaabel commented Sep 10, 2018

danesfeder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devotaaabel commented Sep 18, 2018

Guardiola31337 commented Sep 20, 2018

devotaaabel commented Sep 20, 2018

Guardiola31337 commented Sep 20, 2018

Guardiola31337 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Guardiola31337 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Guardiola31337 left a comment

Choose a reason for hiding this comment

devotaaabel commented Aug 31, 2018 •

edited

Loading