970247 - Stub ButtonToast view

Status: RESOLVED FIXED

(Firefox for Android :: General, defect)

Description

[Lucas Rocha (:lucasr)]

Lazy-inflate it on demand.

Comment 1

[Lucas Rocha (:lucasr)]

Attachment: Move mToast.show() call behind a GeckoApp API (r=wesj)

Comment 2

[Lucas Rocha (:lucasr)]

Attachment: Stub the ButtonToast view (r=wesj)

Comment 3

[Lucas Rocha (:lucasr)]

Comment on attachment 8373241 [details] [diff] [review]
Move mToast.show() call behind a GeckoApp API (r=wesj)

So that we centralize all show() calls under GeckoApp's API.

Comment 4

[Wesley Johnston (:wesj)]

Comment on attachment 8373241 [details] [diff] [review]
Move mToast.show() call behind a GeckoApp API (r=wesj)

Review of attachment 8373241 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/BrowserApp.java
@@ +2258,5 @@
>                      Toast.makeText(this, R.string.bookmark_removed, Toast.LENGTH_SHORT).show();
>                      item.setIcon(R.drawable.ic_menu_bookmark_add);
>                  } else {
>                      tab.addBookmark();
> +                    showButtonToast(

I think I like lazy getters better than this. Can we just do getButtonToast().show(...), and then have getButtonToast do the lazy inflation?

Comment 5

[Lucas Rocha (:lucasr)]

Attachment: Stub the ButtonToast view (r=wesj)

Comment 6

[Lucas Rocha (:lucasr)]

Comment on attachment 8373966 [details] [diff] [review]
Stub the ButtonToast view (r=wesj)

(In reply to Wesley Johnston (:wesj) from comment #4)
> Comment on attachment 8373241 [details] [diff] [review]
> Move mToast.show() call behind a GeckoApp API (r=wesj)
> 
> Review of attachment 8373241 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/BrowserApp.java
> @@ +2258,5 @@
> >                      Toast.makeText(this, R.string.bookmark_removed, Toast.LENGTH_SHORT).show();
> >                      item.setIcon(R.drawable.ic_menu_bookmark_add);
> >                  } else {
> >                      tab.addBookmark();
> > +                    showButtonToast(
> 
> I think I like lazy getters better than this. Can we just do
> getButtonToast().show(...), and then have getButtonToast do the lazy
> inflation?

Fair enough. Here it is.

Comment 7

[Lucas Rocha (:lucasr)]

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

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Backed out for robocop NPEs.
https://hg.mozilla.org/integration/fx-team/rev/400b0e44fbc0

https://tbpl.mozilla.org/php/getParsedLog.php?id=34563031&tree=Fx-Team

Comment 9

[Lucas Rocha (:lucasr)]

Got it, the ViewStub should not have the same id than its inflated view. Pushed again.

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Backed out for robocop NPEs (again). Please run this through Try before pushing again.
https://hg.mozilla.org/integration/fx-team/rev/926feb30decc

https://tbpl.mozilla.org/php/getParsedLog.php?id=34568481&tree=Fx-Team

Comment 11

[Brian Nicholson (:bnicholson)]

This patches changes mToast to a ViewStub, right? So mToast will be null if we don't show a toast before saving state.

Side note: we might want to move mToast.onSaveInstanceState() into GeckoApp#onSaveInstanceState instead of BrowserApp#onSaveInstanceState since mToast is defined in GeckoApp.

Comment 12

[Lucas Rocha (:lucasr)]

(In reply to Brian Nicholson (:bnicholson) from comment #11)
> This patches changes mToast to a ViewStub, right? So mToast will be null if
> we don't show a toast before saving state.
> 
> Side note: we might want to move mToast.onSaveInstanceState() into
> GeckoApp#onSaveInstanceState instead of BrowserApp#onSaveInstanceState since
> mToast is defined in GeckoApp.

You're right. Updating my patch accordingly.

Comment 13

[Lucas Rocha (:lucasr)]

Attachment: Stub the ButtonToast view (r=wesj)

Comment 14

[Lucas Rocha (:lucasr)]

Comment on attachment 8375619 [details] [diff] [review]
Stub the ButtonToast view (r=wesj)

Move the mToast.onSaveInstanceState() call to GeckoApp with a null check.

Comment 15

[Lucas Rocha (:lucasr)]

Review ping?

Comment 16

[Lucas Rocha (:lucasr)]

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

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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