1264347 - Refactor SnackbarHelper to use builder pattern

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

We add more and more showSnackbar() methods for different configuration to SnackbarHelper. A builder-like approach might be better here.

Comment 2

[Tom Klein]

Attachment: Bug 1264347 - Refactor SnackbarHelper to use builder pattern.

Review commit: https://reviewboard.mozilla.org/r/63718/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63718/

Comment 3

[Tom Klein]

Is this what you had in mind?

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

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

Comment 5

[Tom Klein]

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!)

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

(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 7

[Tom Klein]

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/

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

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.

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

(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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cbf648dae9a0
Refactor SnackbarHelper to use builder pattern. r=sebastian

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cbf648dae9a0