Closed Bug 1264347 Opened 4 years ago Closed 3 years ago

Refactor SnackbarHelper to use builder pattern

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 50
Tracking Status
firefox50 --- fixed

People

(Reporter: sebastian, Assigned: twointofive, Mentored)

References

Details

Attachments

(1 file)

We add more and more showSnackbar() methods for different configuration to SnackbarHelper. A builder-like approach might be better here.
Duplicate of this bug: 1264791
Mentor: s.kaspari
Is this what you had in mind?
Assignee: nobody → twointofive
Comment on attachment 8770198 [details]
Bug 1264347 - Refactor SnackbarHelper to use builder pattern.

https://reviewboard.mozilla.org/r/63718/#review60726

<p>Yes! This is pretty awesome! :)</p>
<p>I think we could get rid of SnackbarHelper and its methods completely and move some more of the functionality inside the builder.</p>

::: mobile/android/base/java/org/mozilla/gecko/SnackbarHelper.java:87
(Diff revision 1)
> +        private Integer actionColor;
> +
> +        /**
> -     * @param activity Activity to show the snackbar in.
> +         * @param activity Activity to show the snackbar in.
> +         */
> +        public SnackbarBuilder(final Activity activity) {

Can we now replace the functionality of SnackbarHelper completely? If possible it would be nice to just have a SnackbarBuilder class.

::: mobile/android/base/java/org/mozilla/gecko/SnackbarHelper.java:158
(Diff revision 1)
> +        }
> +
> +        /**
> +         * @param object Populate the builder with data from a Gecko Snackbar:Show event.
> +         */
> +        public SnackbarBuilder object(final NativeJSObject object) {

Can we try to find a more descriptive name for this method? Something like fromEvent() or just event()?

::: mobile/android/base/java/org/mozilla/gecko/SnackbarHelper.java:179
(Diff revision 1)
> +            }
> +            return this;
> +        }
>  
> -        showSnackbarWithActionAndColors(activity,
> +        public void buildAndShow() {
> +            SnackbarHelper.showSnackbarWithActionAndColors(

If we inline this method then we do not need SnackbarHelper anymore and can rename this to SnackbarBuilder, right?

::: mobile/android/base/java/org/mozilla/gecko/delegates/BookmarkStateChangeDelegate.java:142
(Diff revision 1)
>                  showBookmarkDialog(browserApp);
>              }
>          };
>  
> -        SnackbarHelper.showSnackbarWithAction(browserApp,
> -                browserApp.getResources().getString(R.string.bookmark_added),
> +        SnackbarHelper.snackbarBuilder(browserApp)
> +                .message(browserApp.getResources().getString(R.string.bookmark_added))

It would be nice to have a message() and action() implementation that takes an (int) id as parameter and does resolve the string internally.

The StringRes annotation could be useful here:
https://developer.android.com/reference/android/support/annotation/StringRes.html
Attachment #8770198 - Flags: review?(s.kaspari)
https://reviewboard.mozilla.org/r/63718/#review60726

> It would be nice to have a message() and action() implementation that takes an (int) id as parameter and does resolve the string internally.
> 
> The StringRes annotation could be useful here:
> https://developer.android.com/reference/android/support/annotation/StringRes.html

It looks like some of these, like the one you reference here, use Resources' getString, while others, like
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java#824
use Context's getString - the two getString descriptions are slightly different, so is that going to be an issue?  (Everything else sounds great, thanks!)
(In reply to Tom Klein from comment #5)
> https://reviewboard.mozilla.org/r/63718/#review60726
> 
> > It would be nice to have a message() and action() implementation that takes an (int) id as parameter and does resolve the string internally.
> > 
> > The StringRes annotation could be useful here:
> > https://developer.android.com/reference/android/support/annotation/StringRes.html
> 
> It looks like some of these, like the one you reference here, use Resources'
> getString, while others, like
> https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/
> mozilla/gecko/BrowserApp.java#824
> use Context's getString - the two getString descriptions are slightly
> different, so is that going to be an issue?  (Everything else sounds great,
> thanks!)

No, this shouldn't be an issue and should have the same result. In fact getString() is only a shortcut for getResources().getString():
https://github.com/android/platform_frameworks_base/blob/master/core/java/android/content/Context.java#L399

In your case you already have a Context (Activity) from the SnackbarBuilder constructor so you should have everything you need to lookup Strings, colors, drawables, .. :)
Comment on attachment 8770198 [details]
Bug 1264347 - Refactor SnackbarHelper to use builder pattern.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63718/diff/1-2/
Attachment #8770198 - Flags: review?(s.kaspari)
Attachment #8770198 - Flags: review?(s.kaspari) → review+
Comment on attachment 8770198 [details]
Bug 1264347 - Refactor SnackbarHelper to use builder pattern.

https://reviewboard.mozilla.org/r/63718/#review61878

This is great. I triggered a try run from reviewboard.
(In reply to Sebastian Kaspari (:sebastian) from comment #8)
> This is great. I triggered a try run from reviewboard.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed4ef0234ea8
Pushed by twointofive@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/cbf648dae9a0
Refactor SnackbarHelper to use builder pattern. r=sebastian
https://hg.mozilla.org/mozilla-central/rev/cbf648dae9a0
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
You need to log in before you can comment on or make changes to this bug.