778216 - create a pref that will show the full URL in the awesomebar instead of the title

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Ian Melven :imelven]

A security researcher friend of mine asked me if there was a pref for this. This pref would be off by default but would let more advanced users always see the URL in the awesomebar instead of the page title. Wesj has cooked up an addon to do this, by the way.

Comment 2

[Wesley Johnston (:wesj)]

If we wanted to do this, rather than a pref I've thought about allowing users to flick the title up or down to flip between the title and url.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Or adding a "Show Address" / "Show Title" toggle in the URLBar long tap menu

Comment 4

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Mark Finkle (:mfinkle) from comment #3)
> Or adding a "Show Address" / "Show Title" toggle in the URLBar long tap menu

If we do implement this, then I think we had better collect some usage data and see which users prefer.

Comment 5

[Wesley Johnston (:wesj)]

Attachment: Patch v1

Adds a context menu item on the urlbar to toggle between showing the title and url. Stored as a java pref. Affects all tabs forever once its set.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 735020 [details] [diff] [review]
Patch v1

>diff --git a/mobile/android/base/BrowserToolbar.java b/mobile/android/base/BrowserToolbar.java

>+        SharedPreferences settings = activity.getSharedPreferences(GeckoApp.PREFS_NAME, 0);
>+        settings.registerOnSharedPreferenceChangeListener(this);
>+        mShowUrl = settings.getBoolean(GeckoApp.PREFS_SHOW_URL, false);

I wonder if this causes StrictMode violations? Just about anything related to SharedPreferences does. If so, use a background thread.
Is a listener gettign all pref changes? Will that cause pref problems?

>                 if (Tabs.getInstance().isSelectedTab(tab)) {
>-                    setTitle(tab.getDisplayTitle());
>+                    setTitle(tab.getDisplayTitle(mShowUrl));

Let's not make any Tab.getDisplayTitle changes. Instead:

setTitle(mShowUrl ? tab.getUrl() : tab.getDisplayTitle());

(or similar)

>-                    setTitle(tab.getDisplayTitle());
>+                    setTitle(tab.getDisplayTitle(mShowUrl));

Same

>     public void refresh() {

>             String url = tab.getURL();
>-            setTitle(tab.getDisplayTitle());
>+            setTitle(tab.getDisplayTitle(mShowUrl));

Same


>+    public void onSharedPreferenceChanged(SharedPreferences sharedPreferences, String key) {
>+        if (key.equals(GeckoApp.PREFS_SHOW_URL)) {
>+            mShowUrl = sharedPreferences.getBoolean(key, false);
>+            refresh();

refresh is not lightweight. Can't we just change the title? just call setTitle here?

>diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java

>         GeckoAppShell.openUriExternal(url, "text/plain", "", "",
>-                                      Intent.ACTION_SEND, tab.getDisplayTitle());
>+                                      Intent.ACTION_SEND, tab.getDisplayTitle(false));

Another reason why we do not want to change Tab.getDisplayTitle

>+                ThreadUtils.postToBackgroundThread(new Runnable() {
>+                    @Override
>+                    public void run() {
>+                        SharedPreferences settings = getSharedPreferences(PREFS_NAME, 0);
>+                        SharedPreferences.Editor editor = settings.edit();
>+                        editor.putBoolean(PREFS_SHOW_URL, item.getItemId() == R.id.show_url);
>+                        editor.commit();
>+                    }
>+                });

I do wonder about the performance implications with using a listener

>diff --git a/mobile/android/base/locales/en-US/android_strings.dtd b/mobile/android/base/locales/en-US/android_strings.dtd

>+<!ENTITY contextmenu_show_url "Show URL">
>+<!ENTITY contextmenu_show_title "Show Title">

"Display Address"
"Display Title"

?

I almost think that is too little caption. Maybe

"Display Address in Toolbar"
"Display Title in Toolbar"

Comment 7

[Wesley Johnston (:wesj)]

Attachment: Patch v2

I'm coming around to this.

> I wonder if this causes StrictMode violations? Just about anything related
> to SharedPreferences does. If so, use a background thread.
> Is a listener gettign all pref changes? Will that cause pref problems?
The listener only gets changes to the pref type we registerd for. I created a set of BrowserToolbarPrefs for this now, so we should (for now) only be getting this one pref change.

> setTitle(mShowUrl ? tab.getUrl() : tab.getDisplayTitle());
Done. I'll move the formatting from the other bug into setTitle();

> refresh is not lightweight. Can't we just change the title? just call
> setTitle here?
Switched

> I do wonder about the performance implications with using a listener
Everything is background threaded as much as possible now I think.

> "Display Address in Toolbar"
> "Display Title in Toolbar"
I did this, but I don't think I really like it. It feels redudant for the menu to refer to the thing the menu was attached to. Sorta like "Show the Address in that thing you just clicked on there". I'd rather just Display Address and Display Title.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 735280 [details] [diff] [review]
Patch v2

>diff --git a/mobile/android/base/BrowserToolbar.java b/mobile/android/base/BrowserToolbar.java

>-    public BrowserToolbar(BrowserApp activity) {
>+    public BrowserToolbar(final BrowserApp activity) {

Does this need to be final?

>+        final BrowserToolbar self = this;
>+        (new UiAsyncTask<Void, Void, Void>(ThreadUtils.getBackgroundHandler()) {
>+            @Override
>+            public synchronized Void doInBackground(Void... params) {
>+                SharedPreferences settings = activity.getSharedPreferences(PREFS_NAME, 0);
>+                settings.registerOnSharedPreferenceChangeListener(self);

I think you could use:
settings.registerOnSharedPreferenceChangeListener(BrowserToolbar.this);

and remove:
final BrowserToolbar self = this;

Maybe. Maybe not since it's a thread.

>diff --git a/mobile/android/base/locales/en-US/android_strings.dtd b/mobile/android/base/locales/en-US/android_strings.dtd

>+<!ENTITY contextmenu_show_url "Display Address in Toolbar">
>+<!ENTITY contextmenu_show_title "Display Title in Toolbar">

I'd be fine with dropping the "in Toolbar" from the strings. I just worry a little about users not knowing that this menu will change the text in the URLBar. That's one reason I like "Display" over "Show". We use "Show" to pop things up in front of the user.

Can you make a build and let's get Ian and others to give feedback before landing.

r+, but this is contingent on getting UX to OK as well.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Ian - We need your thoughts on this approach. Wes is going to get a build to try out.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Ian, Wes: If this approach seems too "in your face" or confusing, we could try:
* Visible setting?
* Add enough internal support to allow an add-on to "flip the switch". Maybe add a Gecko pref which could be set via about:config _or_ and add-on?

Comment 11

[Lucas Rocha (:lucasr)]

Comment on attachment 735280 [details] [diff] [review]
Patch v2

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

::: mobile/android/base/BrowserToolbar.java
@@ +468,5 @@
>      public void onTabChanged(Tab tab, Tabs.TabEvents msg, Object data) {
>          switch(msg) {
>              case TITLE:
>                  if (Tabs.getInstance().isSelectedTab(tab)) {
> +                    setTitle(mShowUrl ? tab.getURL() : tab.getDisplayTitle());

Drive-by: "mShowUrl ? tab.getURL() : tab.getDisplayTitle()" should probably be factored out into a method to avoid the code duplication.

Comment 12

[Ian Barlow (:ibarlow)]

(In reply to Mark Finkle (:mfinkle) from comment #10)
> Ian, Wes: If this approach seems too "in your face" or confusing, we could
> try:
> * Visible setting?
> * Add enough internal support to allow an add-on to "flip the switch". Maybe
> add a Gecko pref which could be set via about:config _or_ and add-on?

I would suggest making the strings sentence case (Display title in address bar)

Wes, can you please post a build for me to try?

Comment 13

[Lucas Rocha (:lucasr)]

(In reply to Mark Finkle (:mfinkle) from comment #10)
> Ian, Wes: If this approach seems too "in your face" or confusing, we could
> try:
> * Visible setting?
> * Add enough internal support to allow an add-on to "flip the switch". Maybe
> add a Gecko pref which could be set via about:config _or_ and add-on?

I'm not keen on adding this option to the toolbar context menu because it currently only deals with actions on the current website (share, copy URL, site settings, etc), not the UI.

I'd personally prefer to see this under our "Privacy and Security" settings with the wording suggested by Ian ("Display XXXXX in address bar"). Not sure if XXXXX should use "title" (on by default) or "URL" (off by default) though. URL is maybe too technical.

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Lucas Rocha (:lucasr) from comment #13)
> (In reply to Mark Finkle (:mfinkle) from comment #10)
> > Ian, Wes: If this approach seems too "in your face" or confusing, we could
> > try:
> > * Visible setting?
> > * Add enough internal support to allow an add-on to "flip the switch". Maybe
> > add a Gecko pref which could be set via about:config _or_ and add-on?
> 
> I'm not keen on adding this option to the toolbar context menu because it
> currently only deals with actions on the current website (share, copy URL,
> site settings, etc), not the UI.
> 
> I'd personally prefer to see this under our "Privacy and Security" settings
> with the wording suggested by Ian ("Display XXXXX in address bar"). Not sure
> if XXXXX should use "title" (on by default) or "URL" (off by default)
> though. URL is maybe too technical.

I am leaning towards a Setting too. But instead of a checkbox, I think a list would be better, since it shows both states: Title, Address

Comment 15

[Wesley Johnston (:wesj)]

Sorry for the delay. Build at:

http://people.mozilla.com/~wjohnston/showurl.apk

it has both the prefernce and the context menu item (to compare, not because I think we should ship both). I like the context menu better, but mostly because I like the instant feedback. It makes it clearer to me what the pref does.

Comment 16

[Wesley Johnston (:wesj)]

Also, my builds on this machine seem to crash anytime you load a page with plugin content. :( Just be careful.

Comment 17

[Ian Barlow (:ibarlow)]

(In reply to Wesley Johnston (:wesj) from comment #15)

> it has both the prefernce and the context menu item (to compare, not because
> I think we should ship both). I like the context menu better, but mostly
> because I like the instant feedback. It makes it clearer to me what the pref
> does.

The instant feedback is nice, I agree, but I also don't see this as a feature people are going to be toggling constantly. I see this more as a 'set it and forget it', which would probably belong in Settings as Lucas suggested.

Other than that this seems to work as intended. The strings read a bit strangely though. Maybe we just need to simplify: "Title bar", with the subheads "Show page title" or "Show URL".

Comment 18

[Wesley Johnston (:wesj)]

Attachment: Patch v3

Mostly the same. Just uses the pref instead. Updated the strings to match ian's. I'm not sure if we use "Titlebar" or "Title bar", but this uses the former.

Comment 19

[Wesley Johnston (:wesj)]

Attachment: Patch v4

Cleaned up the prefs stuff a bit.

Comment 20

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 738152 [details] [diff] [review]
Patch v4

>diff --git a/mobile/android/base/BrowserApp.java b/mobile/android/base/BrowserApp.java

>-    public boolean onContextItemSelected(MenuItem item) {
>+    public boolean onContextItemSelected(final MenuItem item) {

Does this need to be final?

>diff --git a/mobile/android/base/BrowserToolbar.java b/mobile/android/base/BrowserToolbar.java

>+        final BrowserToolbar self = this;
>+        (new UiAsyncTask<Void, Void, Void>(ThreadUtils.getBackgroundHandler()) {
>+            @Override
>+            public synchronized Void doInBackground(Void... params) {
>+                SharedPreferences settings = activity.getSharedPreferences(PREFS_NAME, 0);
>+                settings.registerOnSharedPreferenceChangeListener(self);

I think you could use:
settings.registerOnSharedPreferenceChangeListener(BrowserToolbar.this);

and remove:
final BrowserToolbar self = this;

Maybe. Maybe not since it's a thread.

r- but make these changes and we'll get this landed

Comment 21

[Wesley Johnston (:wesj)]

Attachment: Patch v2

Updated to address comments.

Comment 22

[Wesley Johnston (:wesj)]

Attachment: Patch v2.1

Now without cruft!

Comment 23

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 742065 [details] [diff] [review]
Patch v2.1

>diff --git a/mobile/android/base/BrowserToolbar.java b/mobile/android/base/BrowserToolbar.java

>+    public static final String PREFS_NAME = "BrowserToolbarPrefs";

To be consistent with GeckoApp, let's rename this to "BrowserToolbar"   (no "Prefs" suffix)

r+ with that change

Comment 24

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/19d8817e53bb

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/19d8817e53bb

Comment 26

[Teodora Vermesan (:TeoVermesan)]

Test Case added in moztrap:
https://moztrap.mozilla.org/manage/case/8075/
https://moztrap.mozilla.org/manage/case/8076/

Comment 27

[Lukas Blakk [:lsblakk] use ?needinfo]

This has been noted in the Aurora 23 release notes:

http://www.mozilla.org/en-US/mobile/23.0a2/auroranotes/

If you would like to make any changes or have questions/concerns please contact me directly.