976925 - UI after adding a Home Panel from Settings

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Chenxia Liu [:liuche]]

Two main options:
* Exit to about:home and display the new panel
* Display a toast and update the Home Panels settings list to include the new panel

Comment 1

[Lucas Rocha (:lucasr)]

I think we should do *something* about this for Fx30. Setting as P1.

Comment 2

[:Margaret Leibovic]

(In reply to Chenxia Liu [:liuche] from comment #0)
> Two main options:
> * Display a toast and update the Home Panels settings list to include the
> new panel

I vote this.

Comment 3

[Ian Barlow (:ibarlow)]

Works for me

Comment 4

[Chenxia Liu [:liuche]]

Attachment: Screenshot: Toast after adding panel

Ian, any changes to the string?

Comment 5

[Chenxia Liu [:liuche]]

Attachment: Patch: Display toast and refresh panels on new panel install

Comment 6

[Lucas Rocha (:lucasr)]

Comment on attachment 8385677 [details] [diff] [review]
Patch: Display toast and refresh panels on new panel install

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

Looks great but needs some final tweaks.

::: mobile/android/base/home/HomeConfigInvalidator.java
@@ +133,1 @@
>          handlePanelInstall(panelConfig);

I'd rather change handlePanelInstall to take an InvalidationMode and use IMMEDIATE here and DELAYED when handling HomePanels:Install.

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +92,5 @@
>  <!ENTITY pref_category_home_panels "Panels">
>  <!ENTITY pref_home_add_panel "Add panel">
>  <!ENTITY home_add_panel_title "Add new panel">
>  <!ENTITY home_add_panel_empty "Sorry, we couldn\'t find any panels for you to add.">
> +<!ENTITY home_add_panel_installed "New panel added">

I wonder if we should use the Panel's name in the message for clarity? 'Panel Name' panel added. Check with ibarlow.

::: mobile/android/base/preferences/GeckoPreferences.java
@@ +280,5 @@
>            case HomePanelPicker.REQUEST_CODE_ADD_PANEL:
>                switch (resultCode) {
>                    case Activity.RESULT_OK:
> +                      final Context context = this;
> +                      final int stringRes = R.string.home_add_panel_installed;

Move stringRes to the Runnable code and use GeckoPreferences.this instead?

ThreadUtils.postToUiThread(new Runnable () {
    @Override
    public void run() {
        final int messageId = R.string.home_add_panel_installed;
        Toast.makeText(GeckoPreferences.this messageId, Toast.LENGTH_SHORT).show();
    }
});

@@ +289,5 @@
> +                              Toast.makeText(context, stringRes, Toast.LENGTH_SHORT).show();
> +                          }
> +                      });
> +
> +                      mPanelsPreferenceCategory.reloadHomeConfig();

Maybe just 'refresh()' for simplicity?

::: mobile/android/base/preferences/PanelsPreferenceCategory.java
@@ +73,5 @@
> +    /**
> +     * Reload the Home Panels list from HomeConfig.
> +     */
> +    public void reloadHomeConfig() {
> +        int prefCount = getPreferenceCount();

nit: place this line after the comment, just before the loop.

Comment 7

[Ian Barlow (:ibarlow)]

(In reply to Chenxia Liu [:liuche] from comment #4)
> Created attachment 8385675 [details]
> Screenshot: Toast after adding panel
> 
> Ian, any changes to the string?

Can we use the name of the panel in the toast? So, something like 

"Nameofpanel added to homepage"

Comment 8

[Chenxia Liu [:liuche]]

Attachment: Patch: Display toast and refresh panels on new panel install

Comment 9

[Chenxia Liu [:liuche]]

Attachment: Screenshot: Toast after adding panel including panel name

This defaults to "New panel added to homepage" if there isn't a panel name for some reason.

Comment 10

[Lucas Rocha (:lucasr)]

Comment on attachment 8387490 [details] [diff] [review]
Patch: Display toast and refresh panels on new panel install

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

Also, one thing I missed in my previous review: maybe the code to show the toast should be in the HomePanelPicker itself as it will get triggered from different places and we don't want to make every different caller responsible for showing the toast.

::: mobile/android/base/home/HomeConfigInvalidator.java
@@ +131,2 @@
>       */
> +    public void installPanel(PanelConfig panelConfig, boolean delayed) {

No need to add this extra boolean argument. Always use IMMEDIATE for the installPanel() case. We can change that if we end up needing a delayed version in the future.

::: mobile/android/base/preferences/GeckoPreferences.java
@@ +281,5 @@
>            case HomePanelPicker.REQUEST_CODE_ADD_PANEL:
>                switch (resultCode) {
>                    case Activity.RESULT_OK:
> +                      final String panelName = data.getExtras().getString(HomePanelPicker.EXTRAS_PANEL_NAME);
> +                      ThreadUtils.postToUiThread(new Runnable () {

I'd be surprised if onActivityResult() was not called in the UI thread. Why do you need to postToUiThread() here?

@@ +288,5 @@
> +                              String successMsg = getResources().getString(R.string.home_add_panel_installed);
> +
> +                              // Substitute actual panel name if provided.
> +                              if (panelName != null) {
> +                                  successMsg = successMsg.replace(NEW_PANEL_SUBSTRING, "\"" + panelName + "\"");

This approach will break once the string gets translated as NEW_PANEL_SUBSTRING will not be "New panel" anymore. You'll have to use something like a format string parameter e.g. &formatS;

Comment 11

[Chenxia Liu [:liuche]]

Attachment: Patch: Display toast and refresh panels on new panel install

(In reply to Lucas Rocha (:lucasr) from comment #10)
> Comment on attachment 8387490 [details] [diff] [review]
> Patch: Display toast and refresh panels on new panel install
> 
> Review of attachment 8387490 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Also, one thing I missed in my previous review: maybe the code to show the
> toast should be in the HomePanelPicker itself as it will get triggered from
> different places and we don't want to make every different caller
> responsible for showing the toast.

Very good point - that actually makes the code much cleaner too. Added in this version of the patch.
> 
> ::: mobile/android/base/home/HomeConfigInvalidator.java
> @@ +131,2 @@
> >       */
> > +    public void installPanel(PanelConfig panelConfig, boolean delayed) {
> 
> No need to add this extra boolean argument. Always use IMMEDIATE for the
> installPanel() case. We can change that if we end up needing a delayed
> version in the future.
> 

Done.


> @@ +288,5 @@
> > +                              String successMsg = getResources().getString(R.string.home_add_panel_installed);
> > +
> > +                              // Substitute actual panel name if provided.
> > +                              if (panelName != null) {
> > +                                  successMsg = successMsg.replace(NEW_PANEL_SUBSTRING, "\"" + panelName + "\"");
> 
> This approach will break once the string gets translated as
> NEW_PANEL_SUBSTRING will not be "New panel" anymore. You'll have to use
> something like a format string parameter e.g. &formatS;

Gah, good call, I don't know what I was thinking. Fixed this and also added a localization note.

Comment 12

[Chenxia Liu [:liuche]]

Attachment: Screenshot: Toast after adding panel including panel name

If no panel name is provided, the text will read "New panel added to homepage"

Comment 13

[Chenxia Liu [:liuche]]

Attachment: Patch: Display toast and refresh panels on new panel install

Comment 14

[Lucas Rocha (:lucasr)]

Comment on attachment 8387813 [details] [diff] [review]
Patch: Display toast and refresh panels on new panel install

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

Looks good with the suggested changed.

::: mobile/android/base/home/HomePanelPicker.java
@@ +49,5 @@
>  
> +    /**
> +     * Intent extra key for name of new panel.
> +     */
> +    public static final String EXTRAS_PANEL_NAME = "panelName";

Unused, remove.

@@ +142,5 @@
>          final PanelConfig newPanelConfig = panelInfo.toPanelConfig();
>          HomeConfigInvalidator.getInstance().installPanel(newPanelConfig);
>  
> +        String panelName = panelInfo.getTitle();
> +        if (panelName == null) {

Just use newPanelConfig.getTitle() instead which is guaranteed to be set because we validate every new PanelConfig instance. This means you don't really need R.string.home_add_panel_default_name.

@@ +148,5 @@
> +        }
> +
> +        // Display toast.
> +        final String successMsg = getResources().getString(R.string.home_add_panel_installed, panelName);
> +        Toast.makeText(this, successMsg, Toast.LENGTH_SHORT).show();

nit: Factor out the code to show the toast notification into a separate showToastForNewPanel(PanelConfig) to keep things nice and tidy here.

Comment 15

[Chenxia Liu [:liuche]]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This apparently needed a clobber on Android: https://tbpl.mozilla.org/php/getParsedLog.php?id=35906791&tree=Fx-Team

Comment 18

[Nick Alexander :nalexander [he/him]]

(In reply to Wes Kocher (:KWierso) from comment #17)
> This apparently needed a clobber on Android:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=35906791&tree=Fx-Team

I believe that Bug 979388 has addressed this issue.