Closed Bug 976925 Opened 8 years ago Closed 8 years ago

UI after adding a Home Panel from Settings

Categories

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

ARM
Android
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 30

People

(Reporter: liuche, Assigned: liuche)

References

Details

Attachments

(2 files, 5 obsolete files)

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
I think we should do *something* about this for Fx30. Setting as P1.
Priority: -- → P1
(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.
Flags: needinfo?(ibarlow)
Works for me
Flags: needinfo?(ibarlow)
Assignee: nobody → liuche
Attached image Screenshot: Toast after adding panel (obsolete) —
Ian, any changes to the string?
Flags: needinfo?(ibarlow)
Attachment #8385677 - Flags: review?(lucasr.at.mozilla)
Status: NEW → ASSIGNED
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.
Attachment #8385677 - Flags: review?(lucasr.at.mozilla) → feedback+
(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"
Flags: needinfo?(ibarlow)
Attachment #8385677 - Attachment is obsolete: true
Attachment #8387490 - Flags: review?(lucasr.at.mozilla)
This defaults to "New panel added to homepage" if there isn't a panel name for some reason.
Attachment #8385675 - Attachment is obsolete: true
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;
Attachment #8387490 - Flags: review?(lucasr.at.mozilla) → review-
(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.
Attachment #8387490 - Attachment is obsolete: true
Attachment #8387492 - Attachment is obsolete: true
Attachment #8387807 - Flags: review?(lucasr.at.mozilla)
If no panel name is provided, the text will read "New panel added to homepage"
Attachment #8387807 - Attachment is obsolete: true
Attachment #8387807 - Flags: review?(lucasr.at.mozilla)
Attachment #8387813 - Flags: review?(lucasr.at.mozilla)
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.
Attachment #8387813 - Flags: review?(lucasr.at.mozilla) → review+
https://hg.mozilla.org/integration/fx-team/rev/affa309bdf7a
Target Milestone: --- → Firefox 30
https://hg.mozilla.org/mozilla-central/rev/affa309bdf7a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
(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.
Flags: needinfo?(nalexander)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.