1228414 - Replace remaining Toast.makeText() calls with snackbar

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

After bug 1224521 we have replaced all toasts from JavaScript (browser.js) and calls to the toast method in BrowserApp.

There are still calls to Toast.makeText(..).show() scattered all over our code base. This bug is about finding and replacing them with snackbars. This might be a bit harder: Toasts only needed a context. Snackbars need a view to find the root layout.

https://dxr.mozilla.org/mozilla-central/search?q=Toast.makeText&redirect=true&case=false

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

Alex: Maybe you want to continue with the toasts here. Some of them might be hard or impossible to replace. Start with the easy ones.

Comment 2

[Alex Johnson (:alex_johnson)]

(In reply to Sebastian Kaspari (:sebastian) from comment #1)
> Alex: Maybe you want to continue with the toasts here. Some of them might be
> hard or impossible to replace. Start with the easy ones.

Yeah, I'd pick this up.  Could you assign it to me?

Comment 3

[Alex Johnson (:alex_johnson)]

Attachment: MozReview Request: Bug 1228414 - Replaced all the possible Toast.makeTexts with Snackbars. r+sebastian

Bug 1228414 - Replaced Toaster.makeText() calls in GeckoApp and BrowserApp r?sebastian

Comment 4

[Alex Johnson (:alex_johnson)]

Attachment: MozReview Request: Bug 1228414 - Replaced clear private data toast with snackbar r?sebastian

Bug 1228414 - Replaced clear private data toast with snackbar r?sebastian

Comment 5

[Alex Johnson (:alex_johnson)]

Attachment: MozReview Request: Bug 1228414 - Replaced the last search engine toast. r?sebastian

Bug 1228414 - Replaced the last search engine toast. r?sebastian

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8693225 [details]
MozReview Request: Bug 1228414 - Replaced all the possible Toast.makeTexts with Snackbars. r+sebastian

https://reviewboard.mozilla.org/r/26413/#review23891

Good job! r+ with the changes mentioned below.

::: mobile/android/base/GeckoApp.java:1063
(Diff revision 1)
> +
> +                String set_image_path_fail = getResources().getString(R.string.set_image_path_fail);
> +
>                  if (!dcimDir.mkdirs() && !dcimDir.isDirectory()) {
> -                    Toast.makeText((Context) this, R.string.set_image_path_fail, Toast.LENGTH_SHORT).show();
> +                    showSnackbar(set_image_path_fail, Snackbar.LENGTH_SHORT, null, null);
>                      return;
>                  }
>                  String path = Media.insertImage(getContentResolver(),image, null, null);
>                  if (path == null) {
> -                    Toast.makeText((Context) this, R.string.set_image_path_fail, Toast.LENGTH_SHORT).show();
> +                    showSnackbar(set_image_path_fail, Snackbar.LENGTH_SHORT, null, null);

It's okay to use getString() inside showSnackbar. No need to create this String early. Most of the time I might not be needed.

::: mobile/android/base/GeckoApp.java:1064
(Diff revision 1)
> +                String set_image_path_fail = getResources().getString(R.string.set_image_path_fail);

It looks like this String is not used?

::: mobile/android/base/GeckoApp.java:1089
(Diff revision 1)
> -                Toast.makeText((Context) this, R.string.set_image_fail, Toast.LENGTH_SHORT).show();
> +                showSnackbar(getResources().getString(R.string.set_image_fail), Snackbar.LENGTH_SHORT, null, null);

NIT: In an activity you can just call getString() without getResources().

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8693225 [details]
MozReview Request: Bug 1228414 - Replaced all the possible Toast.makeTexts with Snackbars. r+sebastian

https://reviewboard.mozilla.org/r/26413/#review23893

::: mobile/android/base/GeckoApp.java:2274
(Diff revision 1)
> -        Toast.makeText(this, message, Toast.LENGTH_LONG).show();
> +        showSnackbar(message, Snackbar.LENGTH_LONG, null, null);

This needs to be a toast. It's shown if the current system is not supported. We finish the activity immediately in this situation. So there's no UI to show a snackbar.

Comment 8

[Alex Johnson (:alex_johnson)]

Comment on attachment 8693225 [details]
MozReview Request: Bug 1228414 - Replaced all the possible Toast.makeTexts with Snackbars. r+sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26413/diff/1-2/

Comment 9

[Alex Johnson (:alex_johnson)]

Comment on attachment 8693226 [details]
MozReview Request: Bug 1228414 - Replaced clear private data toast with snackbar r?sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26407/diff/1-2/

Comment 10

[Alex Johnson (:alex_johnson)]

Comment on attachment 8693227 [details]
MozReview Request: Bug 1228414 - Replaced the last search engine toast. r?sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26415/diff/1-2/

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8693226 [details]
MozReview Request: Bug 1228414 - Replaced clear private data toast with snackbar r?sebastian

https://reviewboard.mozilla.org/r/26407/#review23901

::: mobile/android/base/preferences/GeckoPreferences.java:626
(Diff revision 1)
> -                        Toast.makeText(context, stringRes, Toast.LENGTH_SHORT).show();
> +                        final FragmentManager fragmentManager = getFragmentManager();
> +                        int id = getResources().getIdentifier("android:id/prefs", null, null);
> +                        final Fragment fragment = fragmentManager.findFragmentById(id);
> +                        Snackbar.make(fragment.getView(), stringRes, Snackbar.LENGTH_SHORT).show();

This probably does not work on Gingerbread where we do not use fragments - can you test this? Maybe you can use the root view (android.R.id.content) instead.

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8693227 [details]
MozReview Request: Bug 1228414 - Replaced the last search engine toast. r?sebastian

https://reviewboard.mozilla.org/r/26415/#review23903

::: mobile/android/base/preferences/SearchEnginePreference.java:97
(Diff revision 2)
> +                    Fragment fragment = activity.getFragmentManager()
> +                            .findFragmentById(getContext().getResources()
> +                                    .getIdentifier("android:id/prefs", null, null));
> +
> +                    Snackbar.make(fragment.getView(), R.string.pref_search_last_toast, Snackbar.LENGTH_SHORT).show();

Like in the previous commit: I assume this does not work on Gingerbread.

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8693225 [details]
MozReview Request: Bug 1228414 - Replaced all the possible Toast.makeTexts with Snackbars. r+sebastian

https://reviewboard.mozilla.org/r/26413/#review23905

Comment 14

[Alex Johnson (:alex_johnson)]

Comment on attachment 8693226 [details]
MozReview Request: Bug 1228414 - Replaced clear private data toast with snackbar r?sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26407/diff/2-3/

Comment 15

[Alex Johnson (:alex_johnson)]

Comment on attachment 8693227 [details]
MozReview Request: Bug 1228414 - Replaced the last search engine toast. r?sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26415/diff/2-3/

Comment 16

[Alex Johnson (:alex_johnson)]

Attachment: MozReview Request: Bug 1228414 - Replaced EditBookmark toast with snackbar. r?sebastian

Bug 1228414 - Replaced EditBookmark toast with snackbar. r?sebastian

Comment 17

[Alex Johnson (:alex_johnson)]

Attachment: MozReview Request: Bug 1228414 - Replaced toasts in GeckoAppShell and HomeFragment r?sebastian

Bug 1228414 - Replaced toasts in GeckoAppShell and HomeFragment r?sebastian

Comment 18

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8693226 [details]
MozReview Request: Bug 1228414 - Replaced clear private data toast with snackbar r?sebastian

https://reviewboard.mozilla.org/r/26407/#review24137

::: mobile/android/base/preferences/GeckoPreferences.java:626
(Diff revision 3)
> -                        Toast.makeText(context, stringRes, Toast.LENGTH_SHORT).show();
> +                        Snackbar.make(findViewById(android.R.id.content), stringRes, Snackbar.LENGTH_SHORT).show();

So this worked? Did you test this on Gingerbread too?

Comment 19

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8693227 [details]
MozReview Request: Bug 1228414 - Replaced the last search engine toast. r?sebastian

https://reviewboard.mozilla.org/r/26415/#review24139

Comment 20

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8693621 [details]
MozReview Request: Bug 1228414 - Replaced EditBookmark toast with snackbar. r?sebastian

https://reviewboard.mozilla.org/r/26505/#review24141

::: mobile/android/base/EditBookmarkDialog.java:220
(Diff revision 1)
> -                        Toast.makeText(context, R.string.bookmark_updated, Toast.LENGTH_SHORT).show();
> +                        View view = ((Activity)context).findViewById(R.id.root_layout);
> +                        Snackbar.make(view, R.string.bookmark_updated, Snackbar.LENGTH_SHORT).show();

"root_layout" is going away eventually (bug 1107636). So we better do not rely on it anymore. 

You are assuming that the activity is using a layout with "root layout". This means you are basically saying that this is the BrowserApp activity. So I wonder if we should just cast to BrowserActivity and call showSnackbar().

Comment 21

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8693622 [details]
MozReview Request: Bug 1228414 - Replaced toasts in GeckoAppShell and HomeFragment r?sebastian

https://reviewboard.mozilla.org/r/26507/#review24143

::: mobile/android/base/GeckoAppShell.java:2601
(Diff revision 1)
> -        Toast toast = Toast.makeText(getContext(),
> -                                     getContext().getResources().getString(R.string.share_image_failed),
> +        View rootView = ((Activity)getContext()).findViewById(R.id.root_layout);
> +        Snackbar.make(rootView, R.string.share_image_failed, Snackbar.LENGTH_SHORT).show();

See previous review.

::: mobile/android/base/home/HomeFragment.java:408
(Diff revision 1)
> -            Toast.makeText(mContext, R.string.page_removed, Toast.LENGTH_SHORT).show();
> +            View rootView = ((Activity)mContext).findViewById(R.id.root_layout);
> +            Snackbar.make(rootView, R.string.page_removed, Snackbar.LENGTH_SHORT).show();

see above

Comment 22

[Alex Johnson (:alex_johnson)]

https://reviewboard.mozilla.org/r/26407/#review24137

> So this worked? Did you test this on Gingerbread too?

It worked on my phone, but I'm having an exceptionally hard time building Fennec for Gingerbread.

Comment 23

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Alex Johnson(:alex_johnson) from comment #22)
> https://reviewboard.mozilla.org/r/26407/#review24137
> 
> > So this worked? Did you test this on Gingerbread too?
> 
> It worked on my phone, but I'm having an exceptionally hard time building
> Fennec for Gingerbread.

You should be able to run a normal Fennec build on Gingerbread. You get closer to a release Gingerbread build with the following things set in your mozconfig:

   ac_add_options --with-android-min-sdk=9
   ac_add_options --with-android-max-sdk=10
   ac_add_options --enable-android-resource-constrained

Note that with this config you can build with 'mach' but you can't build with Android Studio / IntelliJ

Comment 24

[Alex Johnson (:alex_johnson)]

Comment on attachment 8693225 [details]
MozReview Request: Bug 1228414 - Replaced all the possible Toast.makeTexts with Snackbars. r+sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26413/diff/2-3/

Comment 25

[Alex Johnson (:alex_johnson)]

Comment on attachment 8693226 [details]
MozReview Request: Bug 1228414 - Replaced clear private data toast with snackbar r?sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26407/diff/3-4/

Comment 26

[Alex Johnson (:alex_johnson)]

Comment on attachment 8693227 [details]
MozReview Request: Bug 1228414 - Replaced the last search engine toast. r?sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26415/diff/3-4/

Comment 27

[Alex Johnson (:alex_johnson)]

Comment on attachment 8693621 [details]
MozReview Request: Bug 1228414 - Replaced EditBookmark toast with snackbar. r?sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26505/diff/1-2/

Comment 28

[Alex Johnson (:alex_johnson)]

Comment on attachment 8693622 [details]
MozReview Request: Bug 1228414 - Replaced toasts in GeckoAppShell and HomeFragment r?sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26507/diff/1-2/

Comment 29

[Alex Johnson (:alex_johnson)]

https://reviewboard.mozilla.org/r/26505/#review24141

> "root_layout" is going away eventually (bug 1107636). So we better do not rely on it anymore. 
> 
> You are assuming that the activity is using a layout with "root layout". This means you are basically saying that this is the BrowserApp activity. So I wonder if we should just cast to BrowserActivity and call showSnackbar().

Casting to BrowserActivity isn't working because the mLayout in GeckoApp can not be accessed from the EditBookmark activity.  So, I decided to just switch to android.R.id.content.

Comment 30

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8693621 [details]
MozReview Request: Bug 1228414 - Replaced EditBookmark toast with snackbar. r?sebastian

https://reviewboard.mozilla.org/r/26505/#review24719

Comment 31

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8693622 [details]
MozReview Request: Bug 1228414 - Replaced toasts in GeckoAppShell and HomeFragment r?sebastian

https://reviewboard.mozilla.org/r/26507/#review24723

Comment 32

[Carsten Book [:Tomcat]]

Hi, this failed to apply:

apply changeset? [ynmpcq?]: y
applying 1772a6c68b74
unable to find 'mobile/android/base/BrowserApp.java' for patching
4 out of 4 hunks FAILED -- saving rejects to file mobile/android/base/BrowserApp.java.rej
unable to find 'mobile/android/base/GeckoApp.java' for patching
3 out of 3 hunks FAILED -- saving rejects to file mobile/android/base/GeckoApp.java.rej
patch failed to apply
abort: fix up the merge and run hg transplant --continue

can you take a look, thanks!

Comment 33

[Alex Johnson (:alex_johnson)]

Comment on attachment 8693225 [details]
MozReview Request: Bug 1228414 - Replaced all the possible Toast.makeTexts with Snackbars. r+sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26413/diff/3-4/

Comment 34

[Alex Johnson (:alex_johnson)]

Fixed!

Comment 35

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/a2ad9909ffa0

Comment 36

[Phil Ringnalda (:philor)]

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

Comment 37

[Teodora Vermesan (:TeoVermesan)]

A snackbar is displayed after:
> - a page is removed from reading list
> - a bookmark is removed
> - a link is bookmarked
> - an already bookmark is added as bookmark 
> - a bookmark is updated
> - a page is removed from home panels

Verified as fixed using:
Device: Nexus 6 (Android 6.0)
Build: Firefox for Android 45.0a2 and 46.0a1 (2015-12-22) 

Leaving this RESOLVED FIXED until bug 1233412 gets fixed.