1262285 - Update copy for empty state of Combined History to promote Sync

Status: RESOLVED INCOMPLETE

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

Comment 1

[Chenxia Liu [:liuche]]

Attachment: Mock: New empty state

Comment 2

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: prev_empty3.png

Comment 3

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: prev_devicefolder_empty.png

Matej, here are the mocks that we talked about!

NI-ing for context

Comment 4

[Matej Novak [:matej]]

(In reply to Anthony Lam (:antlam) from comment #2)
> Created attachment 8739190 [details]
> prev_empty3.png

Copy looks good. I'm not sure if it's just because it's orange and italic, but the tip copy looks bigger than the other line. Would it make sense to make that a little smaller?

Comment 5

[Matej Novak [:matej]]

(In reply to Anthony Lam (:antlam) from comment #3)
> Created attachment 8739192 [details]
> prev_devicefolder_empty.png
> 
> Matej, here are the mocks that we talked about!
> 
> NI-ing for context

Do we have to say "logins" on this one? That's not copy we normally use with Sync and it stands out. I'm not totally clear what it refers to and I worry it may confuse users.

Comment 6

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Matej Novak [:matej] from comment #4)
> (In reply to Anthony Lam (:antlam) from comment #2)
> > Created attachment 8739190 [details]
> > prev_empty3.png
> 
> Copy looks good. I'm not sure if it's just because it's orange and italic,
> but the tip copy looks bigger than the other line. Would it make sense to
> make that a little smaller?

It should be the same size, but we can see how this looks on device.

(In reply to Matej Novak [:matej] from comment #5)
> (In reply to Anthony Lam (:antlam) from comment #3)
> > Created attachment 8739192 [details]
> > prev_devicefolder_empty.png
> > 
> > Matej, here are the mocks that we talked about!
> > 
> > NI-ing for context
> 
> Do we have to say "logins" on this one? That's not copy we normally use with
> Sync and it stands out. I'm not totally clear what it refers to and I worry
> it may confuse users.

It does also make the sentence read differently (4 things rather than 3).

I'd be OK removing it for this screen since you're right, it doesn't really apply in this context.

Comment 8

[Chenxia Liu [:liuche]]

This bug is to update the strings in the empty views in the combined history layout [1] so they look like the mocks. They would be the layouts used in id/home_history_empty_view and id/home_clients_empty_view. The strings in the history empty view are set in CombinedHistoryPanel [2] while the strings for clients are set in the xml file.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/home_combined_history_panel.xml
[2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryPanel.java#176

Comment 9

[amol1994mane]

I'd like to pick this as my first bug to fix. What steps do I take from here?

Comment 10

[amol1994mane]

Attachment: Changes I made to string values

Here's my patch to fix this bug. It changes the string values of fxaccount_getting_started_description2, home_most_recent_empty, and home_most_recent_emptyhint

Would love some feedback!

Comment 11

[Chenxia Liu [:liuche]]

Comment on attachment 8760101 [details] [diff] [review]
Changes I made to string values

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

Thanks for the patch! A few notes:

Beyond just changing the dtd files, you also need to update the references to those strings. If you use dxr.mozilla.org, you can find that those strings are referenced in strings.xml.in.

Additionally, we'd like to make these UI changes that are present in the mock (the spacing, etc). You'll have to look at the xml files, and update the padding/spacers there. If you can't get the spacing/padding correct from the mocks without breaking other things because the layout is used in multiple places, I'd suggest making a separate empty layout for the history (because it has the text as well as the clickable hint, vs the text and an image - see the empty states for the other panels, such as bookmarks or recent tabs). Ideally, we'd like to keep these layouts simple, so try to add as few hard-coded dimensions or padding as you can (not to mean you won't use any, but be sparing with them).

Some misc stuff:
- When you want to flag for review or feedback, set the flag to ? to request it and enter the email of the person you're flagging for review (usually this is their irc nick, so for me it'd be :liuche) - you should only do use one of these flag
- The commit message should match the name of the bug, with the reviewer appended to the end. So in this case, it should be:
Bug 1262285: Update copy for empty state of Combined History to promote Sync. r=liuche

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +578,1 @@
>  <!-- Localization note (home_most_recent_emptyhint2): "Psst" is a sound that might be used to attract someone's attention unobtrusively, and intended to hint at Private Browsing to the user.

Update this localization note, since it no longer matches either the string being used or the string variable.

Comment 12

[Chenxia Liu [:liuche]]

Comment on attachment 8760101 [details] [diff] [review]
Changes I made to string values

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

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +573,5 @@
>  <!ENTITY home_closed_tabs_title "Recently closed tabs">
>  <!ENTITY home_last_tabs_title "Tabs from last time">
>  <!ENTITY home_last_tabs_empty "Your recent tabs show up here.">
>  <!ENTITY home_open_all "Open all">
> +<!ENTITY home_most_recent_empty "Websites you visit will show up here.">

Also, I forgot to mention, if you're updating strings in the dtd files, you should increment the number at the end so that the changes get picked up for our localizers.

Comment 13

[amol1994mane]

> Beyond just changing the dtd files, you also need to update the references
> to those strings. If you use dxr.mozilla.org, you can find that those
> strings are referenced in strings.xml.in.

I don't understand what you mean by this. Could you elaborate more on it? These are the corresponding lines in strings.xml.in :
<string name="fxaccount_getting_started_description">&fxaccount_getting_started_description2;</string><string name="home_most_recent_empty">&home_most_recent_empty;</string>
<string name="home_most_recent_emptyhint">&home_most_recent_emptyhint2;</string>

Comment 14

[amol1994mane]

> Also, I forgot to mention, if you're updating strings in the dtd files, you
> should increment the number at the end so that the changes get picked up for
> our localizers.

Which number are you referring to? I'm not too familiar with how localizers work.

Comment 15

[Chenxia Liu [:liuche]]

(In reply to amol1994mane from comment #14)
> > Also, I forgot to mention, if you're updating strings in the dtd files, you
> > should increment the number at the end so that the changes get picked up for
> > our localizers.
> 
> Which number are you referring to? I'm not too familiar with how localizers
> work.

At the end of the variable names in the .dtd file, you should either add a number, or increment the number that exists. For example, if you update the string home_most_recent_emptyhint2, you should increment that number so the variable name is now home_most_recent_emptyhint3. That way when the localizers are able to see that there is a "new string", and re-localize that string. And since that string is referenced in strings.xml.in, you'll need to update usages in that file too.

Feel free to send more questions my way if you have them! Also, so that I see your comments, be sure to flag me (Need info, r?, f? - whichever is appropriate for the situation) if you need a response so I'll get a notification and remember to check your comment.

Comment 16

[amol1994mane]

Attachment: Changed string values, references, and updated UI

Updated the references. Made a new layout file for the empty history view and updated the layout file for sync. 

I tried putting some of the style attributes (padding, margins, etc.) in the style.xml file instead of the layout files, but for some reason the changes wouldn't show in the UI.

Comment 17

[Chenxia Liu [:liuche]]

Comment on attachment 8760101 [details] [diff] [review]
Changes I made to string values

When you add a new patch, check the box to obsolete old patches so there's no confusion.

Comment 18

[Chenxia Liu [:liuche]]

Comment on attachment 8761464 [details] [diff] [review]
Changed string values, references, and updated UI

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

Good start, a few comments to fix. Also, this no longer applies onto fx-team, so re-pull and resolve the conflicts, and then flag me for review again.

(For UI bugs, can you also add a screenshot so UX can make sure it matches what they want?)

::: mobile/android/base/resources/layout/history_empty_panel.xml
@@ +1,1 @@
> +<?xml version="1.0" encoding="utf-8"?>

You'll want to add the license that's present in other xml files whenever you make a new file.

@@ +1,2 @@
> +<?xml version="1.0" encoding="utf-8"?>
> +<!--<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"-->

Remove this commented out stuff.

@@ +13,5 @@
> +              android:paddingLeft="50dp"
> +              android:paddingRight="50dp">
> +
> +    <TextView android:id="@+id/home_empty_text"
> +    android:layout_width="match_parent"

Indent these android:* lines to match the first one, for both the textviews.

@@ +20,5 @@
> +    android:layout_marginTop="56dp"
> +    android:gravity="top|center"
> +    android:textAppearance="@style/TextAppearance.EmptyHistoryMessage"/>
> +
> +    <TextView android:id="@+id/home_empty_hint"

You can remove the home_empty_hint from the original layout that no longer needs to handle hints anymore.

Comment 19

[amol1994mane]

Attachment: Proper indentation, updated comments

I didn't understand what you meant by "Also, this no longer applies onto fx-team, so re-pull and resolve the conflicts, and then flag me for review again." I just did:
 
hg pull -u 

Was that what you meant? No merge conflicts.

I'll upload the screenshots in my next comment. btw thanks for your help so far!

Comment 20

[amol1994mane]

Attachment: New Empty History Page 1

Comment 21

[amol1994mane]

Attachment: New Sync Page 1

Comment 22

[Chenxia Liu [:liuche]]

I just tried to apply your new patch to the new fx-team, and it still had conflicts. Maybe you're not re-applying the patch to the tip of the new tree?

Comment 23

[Chenxia Liu [:liuche]]

Comment on attachment 8762193 [details]
New Empty History Page 1

Looks good! I also want to make sure this looks okay in landscape, small screens, etc. Feel free to upload a screenshot of landscape mode for both of these screens too! I'm less concerned about those states, but would like to make sure.

Comment 24

[Chenxia Liu [:liuche]]

Hi amol, any change on this? This patch really is almost there :)

Comment 25

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Unassigning due to inactivity. Let me know if you'd still like to work on this amol.

Comment 26

[mddrill]

How can I work on this? I'm new to open source.

Comment 27

[Sivaranjani Subramanian]

Hi, I'd like to work on this bug. Should I start from scratch or is the issue partly resolved? I see that the patch was almost ready.

Comment 28

[Ramu]

Hi All,
Is anybody working on this or could I put my hands on this to make some contribution???
Please suggest.

Thanks!

Regards,
Sriram

Comment 29

[:Grisha Kruglov]

Is this still relevant, antlam? It's a bit confusing, since the mocks you've uploaded look pretty much like what we currently have in nightly.

Comment 30

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Looking at Chenxia's comment 23 and what we have in the product right now, it looks like we still have some work to do.

Can we pick up from the screenshot in comment 23? That would be the best place to start. 

The main difference is the spacing and the text style of "Websites you visited most recently show up here."

The text style there (in comment 23) is the right one. There were also concerns for landscape view and smaller device screens.

Could we make sure those all work?

Comment 31

[Vito]

Is this bug outdated? I was investigating it and (I do not know how to post an attachment) the main differences between what is in the product right now and what the screenshots in comment 23 shows are:
-The phrase "Websites you visited most recently show up here." is replaced by "Websites you visit will show up here." and it has a smaller font.
-The phrase "Psst: using a New Private Tab won't save your history." is replaced by "Tip: Use Private Browsing for pages you don't want saved."
-In the screenshot, some space seems to have been added between the two phrases, but this may be simply due to the size reduction of the first phrase.

Are these the required changes? Is this bug still relevant?

Also, this is my first comment and attempt at a contribution so don't hesitate to tell me what I am doing wrong.

Comment 32

[a.bissarinova]

Hello! I see that amol1994mane had done most of the work here apart from the compatibility for different screen sizes. Is this bug still needs to be fixed. If yes, I would like to work on it as my first bug.

Comment 33

[archit garg[:archit]]

@liuche is this bug still open? can i take it?

Comment 34

[Chenxia Liu [:liuche]]

If someone wants to finish this bug up, the remaining work to be done is:

- Apply these patches to a fresh pull of mozilla-central (they are quite out of date)
- Make screenshots of these applied patches in landscape, portrait (on an emulator is fine) on both a small screen and a large screen device
- Don't include the *.orig file
- Use a proper apostrophe (’ instead of ') so that it doesn't need to be escaped

The spacing of the view should look like that of comment #23.

Since this patch really is almost there and should be easy to verify, I'll mentor it through.

When picking this bug up, please make sure you have done the following:

- Set up a build through these instructions: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_for_Android_build
- Applied the patches in this bug to your local mozilla-central repo and resolved any conflicts in applying
- Post the screenshots or make a reviewboard request (I'll assign the bug to you)

Some additional resources are here:
https://wiki.mozilla.org/Mobile/Get_Involved#Firefox_for_Android_Resources

Comment 35

[Adam David Burch]

Attachment: landscape_big

Comment 36

[Adam David Burch]

Attachment: portrait_big.jpg

Comment 37

[Adam David Burch]

Attachment: patch.diff

I've decided to take on this bug. As you can see from the screen shots I've attached, it works fine on my Samsung tablet.  I followed the quick start guide https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_for_Android_build and that lets me build and deploy at the command line

However, I can't seem to get my Android Studio to deploy my local code to an emulator or even my device! When I try to run 'app' in Android Studio I get a very baffling error message -

Installation failed with message Failed to finalize session : INSTALL_FAILED_INVALID_APK: /data/app/vmdl1949052835.tmp/11_app-local-withGeckoBinaries-minApi__-photon-debug version code 2015550793 inconsistent with 0.
It is possible that this issue is resolved by uninstalling an existing version of the apk if it is present, and then re-installing.

WARNING: Uninstalling will remove the application data!

Do you want to uninstall the existing application?

Even when I hit 'OK', I get the same error message. This is preventing me from getting screen shots on a smaller device. Some Googling makes it look like similar people have fixed this by rooting their devices, but I don't want to do that. It's especially baffling to me as running the mach build, package, install, and run cycle works properly. Does anyone have bandwidth to help me through this error?

Comment 38

[Adam David Burch]

I've done some digging with logcat to narrow down my build/deploy error.

04-04 10:27:09.018 31639 31639 E AndroidRuntime: FATAL EXCEPTION: main
04-04 10:27:09.018 31639 31639 E AndroidRuntime: Process: org.mozilla.fennec_adam, PID: 31639
04-04 10:27:09.018 31639 31639 E AndroidRuntime: java.lang.UnsatisfiedLinkError: dalvik.system.PathClassLoader[DexPathList[[zip file "/data/app/org.mozilla.fennec_adam-1/base.apk"],nativeLibraryDirectories=[/data/app/org.mozilla.fennec_adam-1/lib/arm, /system/lib, /vendor/lib]]] couldn't find "libmozglue.so"
04-04 10:27:09.018 31639 31639 E AndroidRuntime: 	at java.lang.Runtime.loadLibrary0(Runtime.java:984)
04-04 10:27:09.018 31639 31639 E AndroidRuntime: 	at java.lang.System.loadLibrary(System.java:1567)
04-04 10:27:09.018 31639 31639 E AndroidRuntime: 	at org.mozilla.gecko.mozglue.GeckoLoader.doLoadLibraryExpected(GeckoLoader.java:348)
04-04 10:27:09.018 31639 31639 E AndroidRuntime: 	at org.mozilla.gecko.mozglue.GeckoLoader.doLoadLibrary(GeckoLoader.java:374)
04-04 10:27:09.018 31639 31639 E AndroidRuntime: 	at org.mozilla.gecko.mozglue.GeckoLoader.loadMozGlue(GeckoLoader.java:435)
04-04 10:27:09.018 31639 31639 E AndroidRuntime: 	at org.mozilla.gecko.GeckoApp.onCreate(GeckoApp.java:953)
04-04 10:27:09.018 31639 31639 E AndroidRuntime: 	at org.mozilla.gecko.BrowserApp.onCreate(BrowserApp.java:743)
04-04 10:27:09.018 31639 31639 E AndroidRuntime: 	at android.app.Activity.performCreate(Activity.java:6973)
04-04 10:27:09.018 31639 31639 E AndroidRuntime: 	at android.app.Instrumentation.callActivityOnCreate(Instrumentation.java:1126)
04-04 10:27:09.018 31639 31639 E AndroidRuntime: 	at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:2946)
04-04 10:27:09.018 31639 31639 E AndroidRuntime: 	at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:3064)
04-04 10:27:09.018 31639 31639 E AndroidRuntime: 	at android.app.ActivityThread.-wrap14(ActivityThread.java)
04-04 10:27:09.018 31639 31639 E AndroidRuntime: 	at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1659)
04-04 10:27:09.018 31639 31639 E AndroidRuntime: 	at android.os.Handler.dispatchMessage(Handler.java:102)
04-04 10:27:09.018 31639 31639 E AndroidRuntime: 	at android.os.Looper.loop(Looper.java:154)
04-04 10:27:09.018 31639 31639 E AndroidRuntime: 	at android.app.ActivityThread.main(ActivityThread.java:6823)
04-04 10:27:09.018 31639 31639 E AndroidRuntime: 	at java.lang.reflect.Method.invoke(Native Method)
04-04 10:27:09.018 31639 31639 E AndroidRuntime: 	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:1557)
04-04 10:27:09.018 31639 31639 E AndroidRuntime: 	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1445)


I'm using the artifact build, as recommend, and all the steps listed under https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_for_Android_build#Troubleshooting work fine for me. 

mcomella, I see liuche isn't actively working on firefox mobile. Can I get a different mentor/reviewer for this bug?

Comment 39

[Adam David Burch]

So I had the bright idea of trying to install the apk from the command line onto a virtual device and get the screenshots that way. But after I pulled the latest code and rebased[1], I get a brand new build error when I try to run ./mach install

Error running mach:

    ['install']

The error occurred in code that was called by the mach command. This is either
a bug in the called code itself or in the way that mach is calling it.

You should consider filing a bug for this issue.

If filing a bug, please include the full output of mach, including this error
message.

The details of the failure are as follows:

ImportError: cannot import name _psutil_linux

  File "/home/adam/Documents/Open-Sauce/firefox-android/mozilla-central/python/mozbuild/mozbuild/mach_commands.py", line 786, in install
    from mozrunner.devices.android_device import verify_android_device
  File "/home/adam/Documents/Open-Sauce/firefox-android/mozilla-central/build/mach_bootstrap.py", line 363, in __call__
    module = self._original_import(name, globals, locals, fromlist, level)
  File "/home/adam/Documents/Open-Sauce/firefox-android/mozilla-central/testing/mozbase/mozrunner/mozrunner/devices/android_device.py", line 11, in <module>
    import psutil
  File "/home/adam/Documents/Open-Sauce/firefox-android/mozilla-central/build/mach_bootstrap.py", line 363, in __call__
    module = self._original_import(name, globals, locals, fromlist, level)
  File "/home/adam/Documents/Open-Sauce/firefox-android/mozilla-central/third_party/python/psutil/psutil/__init__.py", line 100, in <module>
    from . import _pslinux as _psplatform
  File "/home/adam/Documents/Open-Sauce/firefox-android/mozilla-central/build/mach_bootstrap.py", line 363, in __call__
    module = self._original_import(name, globals, locals, fromlist, level)
  File "/home/adam/Documents/Open-Sauce/firefox-android/mozilla-central/third_party/python/psutil/psutil/_pslinux.py", line 26, in <module>
    from . import _psutil_linux as cext

I've tried install psutil to python, to no avail. 

[1]hg pull --rebase

Comment 40

[Adam David Burch]

Attachment: landscape_small.png

Comment 41

[Adam David Burch]

Attachment: portrait_small.png

Comment 42

[Adam David Burch]

Thanks to the IRC chat (and nalexander in particular) I was able to get a build deploying to an emulator. The problem was that I was mixing architectures - my device and builds were for ARM, but the emulator was x86. I still a reviewer for this bug, but now it's ready to ship!

Comment 43

[Adam David Burch]

*I still NEED a reviewer for this bug

Comment 44

[:sdaswani]

Very sorry Adam - what kind of review is this? UX or code?

Comment 45

[Adam David Burch]

I think this would be a UX review.

Comment 46

[:sdaswani]

Adam, I am really not sure what the state of this ticket is, or who on UX could review it. I'm only working on stability issues, and this doesn't fit that designation. Finally I can't provide any guidance on UX review.

We really appreciate your contribution, but I will check with Product if this is something we want fixed any longer.

Comment 47

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Sorry that I haven't gotten back to this – I'm also only on Fennec for critical issues. Susheel, it doesn't look like Barbara is checking BZ and I don't want to leave our contributors hanging, could you follow up? fwiw, I think antlam is responsible for UX on fennec.

Comment 48

[:sdaswani]

Bran is now Mike. I will add this bug to my meeting with Product about prioritization.

Comment 49

[Barbara Bermes [:barbara]]

Sorry for the delay, I think this could just be considered a copy/content review.

Brian Jones can assist here and we could push this out. I don't think it takes that long.

Comment 50

[Firefox Bug Husbandry Bot]

Re-triaging per https://bugzilla.mozilla.org/show_bug.cgi?id=1473195

Needinfo :susheel if you think this bug should be re-triaged.

Comment 51

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.