419216 - Use URL of page as the title if no title exists

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P2)

Description

[Reed Loden [:reed]]

Bug 406528 caused bad titles to be removed. That's great, but it makes history look very awkward. I think a better approach would be to use the URL of the page as the title so it isn't just completely blank.

Another possibility is saying "(no title)" like the Places Library does for titleness pages instead of just being blank.

Screenshot to be attached.

Comment 1

[Reed Loden [:reed]]

Attachment: Screenshot

Comment 2

[John P Baker]

This is very strange for text/plain visits; For instance http://wwwdev.bris.ac.uk/robots.txt gives 'robots.txt' on 2 and is blank on 3.0b4pre (cf images 'xyz.jpg (JPEG image, 123 x 145 pixels)'

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

I agree with Reed, but don't think that we should be *setting* the title to the URL, just *using* the URL when there's no title information to display. Nicer still would be to strip the protocol so it's just the domain name.

I note that the back/forward drop down already seems to do this.

Comment 4

[Marco Bonardo [:mak]]

Attachment: screenshot with ellipsis

should be an easy fix on menus
what about showing host and filename with ellipsis? do you think that the full path would be more useful?

Comment 5

[Marco Bonardo [:mak]]

Attachment: screenshot of alternative styles

Comment 6

[Marco Bonardo [:mak]]

dietrich i can take this, i only need final UI team definition on the content to put there.

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 305991 [details]
screenshot with ellipsis

nice, ui-r=beltzner

Comment 8

[Marco Bonardo [:mak]]

Attachment: patch

implements host/ellipsis/filename solution

Comment 9

[Asaf Romano (gone)]

Comment on attachment 307953 [details] [diff] [review]
patch

>Index: browser/components/places/content/utils.js
>===================================================================

>   getFormattedString: function PU_getFormattedString(key, params) {
>     return this._bundle.formatStringFromName(key, params, params.length);
>   },
> 
>   getString: function PU_getString(key) {
>     return this._bundle.GetStringFromName(key);
>   },
> 
>+  getEllipsis: function PU_getEllipsis() {
>+    if (!this._ellipsis) {
>+      try {
>+        var pref = Components.classes["@mozilla.org/preferences-service;1"].
>+                              getService(Components.interfaces.nsIPrefBranch);
>+        this._ellipsis = pref.getComplexValue("intl.ellipsis",
>+                          Components.interfaces.nsIPrefLocalizedString).data;
>+      } catch (ex) {
>+        return this._ellipsis = "\u2026";
>+      }
>+    }
>+    return this._ellipsis;
>+  },
>+

Please use a smart getter here (delete on first access). What's the try/catch block for? isn't it set by default

Cc/Ci, too.

>   /**
>    * Determines whether or not a ResultNode is a Bookmark folder.
>    * @param   aNode
>    *          A result node
>    * @returns true if the node is a Bookmark folder, false otherwise
>    */
>   nodeIsFolder: function PU_nodeIsFolder(aNode) {
>     NS_ASSERT(aNode, "null node");
>@@ -1872,17 +1886,26 @@ var PlacesUtils = {
>         element.appendChild(popup);
>         if (aContainersMap)
>           aContainersMap.push({ resultNode: aNode, domNode: popup });
>         element.className = "menu-iconic bookmark-item";
>       }
>       else
>         throw "Unexpected node";
> 
>-      element.setAttribute("label", aNode.title);
>+      if (!aNode.title && aNode.uri) {

This should rather be an uriTypes.indexOf check, otherwise this would label untitled folders as well!

Comment 10

[Marco Bonardo [:mak]]

Attachment: patch

fixed comments

Comment 11

[Asaf Romano (gone)]

Comment on attachment 308387 [details] [diff] [review]
patch

I still prefer an explicit check against urlTypes (like we do for containerTypes in the same function). r=mano either way.

Comment 12

[Marco Bonardo [:mak]]

what's the difference? nodeIsURI does that check... Do you think that future changes to nodeIsURI could create problems here?

Comment 13

[Marco Bonardo [:mak]]

Attachment: for check-in

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Won't this throw for non-nsStandardURLs, data: URIs for example?

Comment 15

[Marco Bonardo [:mak]]

Attachment: patch

thank you Gavin, while fixing the possible throw, i ended up thinking at other cases. 

So:

- for non standard uris we show the full uri

- if we don't have a fileName we should use the path (hiding the scheme as originally requested by beltzner, so only host + path), so we can easily distinguish www.mozilla.org/one/ from www.mozilla.org/two/, while before we had only www.mozilla.org in both cases, and that was bad

- ellipsis should be used only if we have both host and fileName

since something more has changed i'm asking a new review

Comment 16

[Asaf Romano (gone)]

> - for non standard uris we show the full uri

I think we should rather use treeview's (no title) in these edge cases. very long data: uris aren't so useful.

Comment 17

[Marco Bonardo [:mak]]

Comment on attachment 308399 [details] [diff] [review]
patch

will change the edge case of non standard uris with (no title), since they could be very long. We still have the tooltip and statusbartext to distinguish them.

Comment 18

[Marco Bonardo [:mak]]

Attachment: v2

next step, we are going to use the same title algo on all menupopup/tree views in places. This patch works fine with liveUpdate too.

For items directly on toolbar we still use the empty title, to allow users have toolbars made up of only icons.

For menupopups and trees (sidebar/Library) we are using a new util that act as following:

- if node has a title we use that
else
- if node has a valid host and fileName, use host + ellipsis + fileName
- if no fileName, use host + path
- if node has non-standard uri use (no title)
- if node does not have an uri (ie: folder) use (no title)

Comment 19

[Asaf Romano (gone)]

Comment on attachment 308478 [details] [diff] [review]
v2

r=mano

Comment 20

[Reed Loden [:reed]]

Checking in browser/components/places/content/menu.xml;
/cvsroot/mozilla/browser/components/places/content/menu.xml,v  <--  menu.xml
new revision: 1.117; previous revision: 1.116
done
Checking in browser/components/places/content/toolbar.xml;
/cvsroot/mozilla/browser/components/places/content/toolbar.xml,v  <--  toolbar.xml
new revision: 1.140; previous revision: 1.139
done
Checking in browser/components/places/content/treeView.js;
/cvsroot/mozilla/browser/components/places/content/treeView.js,v  <--  treeView.js
new revision: 1.56; previous revision: 1.55
done
Checking in browser/components/places/content/utils.js;
/cvsroot/mozilla/browser/components/places/content/utils.js,v  <--  utils.js
new revision: 1.118; previous revision: 1.117
done

Comment 21

[John P Baker]

I've lost track, a bit, on what this is supposed to do; It is showing |bugzilla.mozilla.org/../attachment.cgi| on the history menu - as I would expect - but I am seeing the full URL in the back/forward drop down [However this does match the tab title]

Comment 22

[Marco Bonardo [:mak]]

this affects history menu, bookmark menu, toolbar menus

Comment 23

[Tracy Walker [:tracy]]

verified with: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b5pre) Gecko/2008031704 Minefield/3.0b5pre and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre) Gecko/2008031704 Minefield/3.0b5pre

Comment 26

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv