Closed Bug 529605 Opened 15 years ago Closed 13 years ago

Places data should be visible in content

Categories

(Firefox :: Bookmarks & History, defect)

Product:
Firefox
Component:
Bookmarks & History
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WONTFIX
Milestone:
Firefox 4.0

People

(Reporter: adw, Unassigned)

References

Details

Attachments

(2 files, 3 obsolete files)

Mockup
305.24 KB, image/png
Details
WIP 3
51.16 KB, patch
Details | Diff | Splinter Review 
Places data needs to be visible in content for 3.7.  Probably this will end up being a meta bug, but opening now to investigate what's needed.  Bug 529597 covers the wiring of the protocol handler, but once a protocol handler is assumed there's the business of displaying the data, with issues like security, writing the HTML and CSS, etc.
bug 523524 is the metabug for this work. I started a conversation here: https://bugzilla.mozilla.org/show_bug.cgi?id=523524#c7
Ah, didn't see that.  From what I understand, we're keeping separate UI bugs like that one and their impl bugs like this one.
Oh, and if you'd like to take this bug, feel free. :)
before we even get to places data in content, do we also need a protocol handler to block this bug as well as the query api work? I suppose we can use mock objects for implementation once we have settled on a new JS api in bug 522572.
Bug 529597, which also blocks the UI bug 523524.  I didn't make it block this one since they're really separate issues.
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
Blocks: 523526
Blocks: 524068
Blocks: 524060
Blocks: 524049
Blocks: 523523
Attached patch WIP 1 (obsolete) — — Splinter Review 
Early WIP, but it's very usable already.  Combined with the patch in bug 529597, you can click around folders, tags, etc. to view their contents.  I wrote a little wrapper around some DOM API to make node creation a bit simpler.  Not sure if it was worth it in the end.

* Do we need separate views tailored to folders, history, tags, queries -- i.e., each node type?  Or is a simple, single one like the one in this patch sufficient?
* l10n...
* Hmm, thought I had more thoughts than this.

See also the discussion about HTML generation in bug 529597 comment 18.
Attached image WIP 1 screenshot (obsolete) — 
This shows the bookmarks menu, but it can just as easily show history, a search, any Places query.  All the items are links.  Oh,

* Unless we use generic titles (e.g., "History Search", "Folder"), we need a way to make l10n'ed titles from queries.  e.g., places:folder=BOOKMARKS_MENU -> "Bookmarks Menu".
I spent an hour or so looking over Chrome's history page, chrome://history/.  Some notes:

- they use a really simple "template engine" inspired by jstemplates
  (http://code.google.com/p/trimpath/wiki/JavaScriptTemplates).
  - template engine is not used to generate dom, only textual
    substitution for l10n.  (jstemplates, however, can be used to generate
    dom.)
  - elements can have two special attrs, "i18n-content" and "i18n-values".
    content is the elt's content, values its attrs.  the values of these
    two i18n attrs can refer to keys.  a process() method is called on the
    element and turns keys into values.  the mapping of keys to values is
    just a simple object.
  - note in code says templates are i18n-aware.  the only l10n i see is:
    - text direction.  elements can have a "dir" attr.  this attr is set
      via the i18n-values template substitution described above.  they
      sniff for [dir="rtl"] in the css and then float things and change
      margins as appropriate.  really simple, but it suffices.
    - LocalStrings, a class whose comment says "the local strings get
      injected into the page using a variable name templateData."  english
      is hard-coded in the html, so some preprocessing tool that injects
      it?  it appears so.  templateData is a simple object, mapping
      keys -> english strings.  the view generation code simply calls
      LocalStrings.getString(key), which looks up the key in the
      templateData object.  this mechanism exists separately from the
      template engine.
- dom generation done by hand using dom api onload or when the search
  changes.  no string concat + innerHtml.  nothing fancy at all.  still,
  the dom-generating methods are small.
- "results-display" is the div that contains the data.  it's filled in
  via dom manip by func HistoryView.prototype.displayResults_.
- Page.prototype.getBrowseResultDOM is the function that generates a div
  for each result in the data.
> * Unless we use generic titles (e.g., "History Search", "Folder"), we need a
> way to make l10n'ed titles from queries.  e.g., places:folder=BOOKMARKS_MENU ->
> "Bookmarks Menu".

are window.PlacesUtils and window PlacesUIUtils available in your context? if so there should be no problem for folders (and you could use services from there), and we can eventually expand getBestTitle to build a title for a node in other cases, does the root node of a place:folder=XXX not have a .title set? it should still be possible to getConcreteItemId and getItemTitle.

a maybe stupid question, since you already did reaserch, why don't we point to a XUL file with bindings rather than xhtml? more form freedom?
Notes about third-party extensibility from the all-hands:

* Provide hooks for replacing the default view with another?  In what manner?
** Allow the target of the protocol handler (bug 529597) to be overridden?  As
   simple as a pref perhaps, something more robust would be better.
** Or hardcode a single target and provide the hook in the view?
* Is the view replacable wholesale?  And/or do we make it easy to plug in
  templates, widgets, Flash games, and what not?
* Injecting content into the default view should be straightforward enough for
  an extension (whether it's traditional or a jetpack) even without special
  hooks.  It's just a webpage.  We should take advantage of that and not create
  unnecessary work for ourselves.  Therefore, is good documentation with
  examples all we need?
* Marco raised a concern that traditional extension authors are more familiar
  with XUL, not HTML, so we should consider using XUL/XBL/overlays instead of
  HTML.
(In reply to comment #11)
> * Marco raised a concern that traditional extension authors are more familiar
>   with XUL, not HTML, so we should consider using XUL/XBL/overlays instead of
>   HTML.

Well not completely, i'm fine with xhtml, it is just matter that traditional xul way provides something like an "interface" they have to adhere, and the code is more ordered (we will have to add drag&drop handlers and so on). That said, i'd like to see the xhtml implementation, ideally it could have less overhead, let's try to give us to users as fast as possible to get feedback, and then we'll refine under-the-hood choices.
Alex, Dietrich wants to land this soon in some form so people can play with it.  I've seen [1], but do you have any UI direction for a first-pass implementation?  i.e., no thumbnails, no location bar breadcrumbs.  See the attached screenshot for what it basically looks like now.  We can do history, folders, tags, searches, pretty much everything already, but they all look alike so far.

[1] http://people.mozilla.com/~faaborg/files/20091012-personalWeb/bookmarksInContent.png
Attached image Mockup — 
Here is a quick mockup, basically just displaying the favicon, title and URL (or folder icon, folder title, and path), using the style of a standard content area page (ftp, network error, about:sessionrestore, about:privatebrowsing, etc).
Attached patch WIP 2 (obsolete) — — Splinter Review 
Saving my work, which now looks like Alex's mockup.

There's a problem with non-folder containers.  If you're looking at the bookmarks menu (in content), the Recent Tags query has a nice title.  When you click it, you navigate to place:sort=14&type=6&maxResults=10&queryType=1 and the title is lost.  Indeed all connection to its home in your bookmarks hierarchy is lost.

A solution I'm thinking of is to add a pseudo "itemId" query term, e.g., place:itemId=1337.  Instead of linking to place:sort=14&type=6&maxResults=10&queryType=1, link to place:itemId=1337 or whatever the Recent Tags item ID is.  The controller maps the itemId to its query result node.  Probably it would work like itemId -> place URI -> query result node, which would mean an extra database lookup.  Actually, do we not already have some way of getting nodes from item IDs, in a module or somewhere?
Attachment #416318 - Attachment is obsolete: true
Attachment #416319 - Attachment is obsolete: true
It's the same problem for history if we want to preserve the experience in the Library and sidebar, where you have a root history node with several predefined containers.

Now that it's landed I'm not sure we really needed a new protocol handler.  For one, place URLs aren't immediately translated into Places queries; the protocol handler is a layer of indirection.  And two, all we need is IDs, e.g., about:bookmarks?itemId=1337 and about:history?itemId=99, or about:place?id=10.  (The first two are nice because they're easy to type in, and we could show some home page for each.)  Or about:bookmarks/1337 -- the exact syntax doesn't matter.  History nodes don't exist in moz_bookmarks of course, but an ID would map to one of the predefined containers.

Another potential solution, using the protocol handler, is to piggyback item IDs using URL params, like place:terms=foo?itemId=11 for a bookmarked search.  That would still allow people to type in arbitrary place URIs, but I think that's a small use case.

Marco, what do you think?
Having an itemId is probably the way to go - the new PlacesQuery API has no knowledge of NavHistoryResultNodes anyway. I would prefer if we tried to build all of this new UI with little reliance on the existing Tree-centric system.
(In reply to comment #15)
> Actually, do we not
> already have some way of getting nodes from item IDs, in a module or somewhere?

hm, no, i think Mano was planning to add the opportunity to get just a single bookmark node, so we could drop the observers from the editing panel, and just use the result's auto-update capabilities. So i'd say it's plausible we want that feature.
On the other side, this would not freely solve the issue for generated queries, i mean, we plan to have History > Last 7 days, this can't be a real bookmark and it would not be able to get a title still. Even if we could create an hidden root with history queries in it (And that would probably solve the issue).

I don't completely recall if we are going to show queries in history or only in session-history, in the first case it would still be hard to get a good title.

If we want to preserve queries editability (for example i'm viewing Recent bookmarks, that is a query limited to 10 items, i could want to edit the place: uri to change limit to 20, 100) if we convert that to a simple place:itemid=XXX that will be lost. Not that it has ever been an high prio feature, though.

I'm maybe thinking to a new option to get title for a result like titlefrom=_itemid_ or even better titlefrom=_entityID_ so that we directly get title for a query from a good localization source (that could maybe also solve some interesting l10n issue we have).

while i'm all for having place:itemid=XXX working (for the above reasons), at that point we should be sure to always avoid presenting such uris to users, since they're completely meaningless and untweakable.

The question is if a query really needs to know everything about who created it, itemId piggyback would give such information, but do we need to know that? hierachy is maintained by the breadcrumb, are there other informations we need apart the title?

Finally a small question on the mockup, the folder we are in is already visible in the address bar breadcrumb (and most likely in the title bar and in the tab), i never had issues in Windows explorer understanding which folder i'm in without having a large title repetition in content, so why do we have to repeat that? is not that a loss of vertical space without any additional info (16:9 screens and netbooks screens don't forgive)?
Just collecting my thoughts... So what are we trying to do?  Display one node at a time.  In the common case the node is part of some hierarchy: bookmarks, history containers, tags, etc.  The node also has metadata: at least a title, and for bookmarks, keyword, description, etc.  When we display a node, we also need to show its place in its hierarchy and its metadata.  Bookmark nodes are the only nodes that know that they belong to a hierarchy and have metadata.  Hierarchy info and metadata are stored in the same location, a row in moz_bookmarks.  Our problem here comes up because you can now navigate to a single node outside of its larger context.  You can't currently do that in Firefox.

(In reply to comment #18)
> hm, no, i think Mano was planning to add the opportunity to get just a single
> bookmark node, so we could drop the observers from the editing panel, and just
> use the result's auto-update capabilities. So i'd say it's plausible we want
> that feature.

I wrote a helper that does this, item ID -> place URI -> query -> node.  Is it something we should add to one of the services?

> On the other side, this would not freely solve the issue for generated queries,
> i mean, we plan to have History > Last 7 days, this can't be a real bookmark
> and it would not be able to get a title still. Even if we could create an
> hidden root with history queries in it (And that would probably solve the
> issue).

Yeah, a hidden root that treated the history entries as bookmarks would work.  But now we're abusing the concept of a bookmark.  We just need a way to associate a node with its metadata and hierarchy.  As a hack we could persistently store that mapping in moz_bookmarks, but a separate table specifically for UI would be better I think.  We could also do it in code alone by associating arbitrary agreed-upon IDs to metadata and hierarchy info, and locating that code in one place like a module or service.

> If we want to preserve queries editability (for example i'm viewing Recent
> bookmarks, that is a query limited to 10 items, i could want to edit the place:
> uri to change limit to 20, 100) if we convert that to a simple place:itemid=XXX
> that will be lost. Not that it has ever been an high prio feature, though.

Yeah, with 350 million users, a very small portion of people would be doing things like that -- especially since eventually the location bar will show breadcrumbs.  There are other ways of letting people make those tweaks if we decide they're important, like through UI.  A better counterpoint I think is that by using only an ID, we can't show the user some arbitrary dynamically generated query that isn't bookmarked.  That's simple to fix: just allow both IDs for the normal case and full query URIs for the other cases.

> I'm maybe thinking to a new option to get title for a result like
> titlefrom=_itemid_ or even better titlefrom=_entityID_ so that we directly get

This is kind of a lose association.  I could type in a query and an unrelated titlefrom and get a weird page.

> while i'm all for having place:itemid=XXX working (for the above reasons), at
> that point we should be sure to always avoid presenting such uris to users,
> since they're completely meaningless and untweakable.

That's why I've been recently thinking about about:bookmarks and about:history.  Easy to remember, easy to type in, fits in with the many other "about" pages, and a nice "homepage" for each.  For individual nodes, about:bookmarks?id=1, about:history?id=1.  For non-bookmarked queries, maybe about:place?terms=foo&...&title=A_Nice_l10n_Title?  Alternatively, about:places if we don't want to separate history and bookmarks, but that URL is less intuitive for users.

> The question is if a query really needs to know everything about who created
> it, itemId piggyback would give such information, but do we need to know that?
> hierachy is maintained by the breadcrumb, are there other informations we need
> apart the title?

Without an ID of some sort, there's no breadcrumb.  A query URI by itself has no connection to its place in its hierarchy or metadata.

> Finally a small question on the mockup, the folder we are in is already visible
> in the address bar breadcrumb (and most likely in the title bar and in the
> tab)

This is a first-pass impl, no breadcrumbs.  True, there's the title bar and tab though.
(In reply to comment #19)
> I wrote a helper that does this, item ID -> place URI -> query -> node.  Is it
> something we should add to one of the services?

well, i think the query handler should be able to translate place:itemid=xxx to a single node, the problem is that the result is built around a root container, not a root generic type of node. Maybe it should just generate a fake root container with just one child, but using a queryContainer would not be efficient with updates, we would probably refresh it too often (it should just observe changes to 1 single itemid).

> > I'm maybe thinking to a new option to get title for a result like
> > titlefrom=_itemid_ or even better titlefrom=_entityID_ so that we directly get
> 
> This is kind of a lose association.  I could type in a query and an unrelated
> titlefrom and get a weird page.

it is a lose association, but i would not care much, it does not bring to any dangerous situation, i could build a query with the wrong title... so what, i can just edit it.
We have a bunch of l10n issues due to the fact queries titles are associated in backend, or hardcoded in bookmarks, this could be a solution to associate an entity to a query in a quick way.
So, would be cool to being able to avoid having a "last 7 days" titled bookmark, and being able to hook in frontend a "last X days" where i can tweak the number of days.
Am i asking for a "translate a place query to a meaningful title" helper in frontend? It's possible, that would be pretty cool but hard (too many grammar rules differ around the world). But yeah, a way to tell "associate this entity to this query, replacing params with X and Y" would be hot (and maybe i'm a bit OT).

>  Easy to remember, easy to type in, fits in with the many other "about" pages,
> and a nice "homepage" for each.  For individual nodes, about:bookmarks?id=1,
> about:history?id=1.  For non-bookmarked queries, maybe
> about:place?terms=foo&...&title=A_Nice_l10n_Title?  Alternatively, about:places
> if we don't want to separate history and bookmarks, but that URL is less
> intuitive for users.

i don't think overloading about: with tons of functionality is really nice, place: uris are already there, just not exposed, so there should be no problem in using them, and we can expand them as we like.

> Without an ID of some sort, there's no breadcrumb.  A query URI by itself has
> no connection to its place in its hierarchy or metadata.

hm, well the breadcrumb in itself has no interest in asking hierarchy to each node, since it will maintain a hierachy itself (i suppose a result? tbd), so it will know the hierarchy for each node by itself.
So i suppose the problem you're pointing at is instead that once i click on a link to visit a container in content page, i'm just asking to visit a place uri, if it's a query i ask to visit place:terms=foo, and the breadcrumb widget cannot know if this is part of a bookmark hieararchy or i just typed that in the locationbar. So yes, we most likely need to add a param to a query to associate it with its itemid.

Remaining question is place:itemid=XXX against place:option1=AAA&option2=BBB?itemid=XXX. We can potentially do both, right? the former would return a single-node result, the latter would just tell "this query comes from item with id = XXX".
i think both are nice for various kind of usage.

> This is a first-pass impl, no breadcrumbs.  True, there's the title bar and tab
> though.

well my question was mostly toward faaborg, please clarify titles question with him if possible.
(In reply to comment #20)
> not a root generic type of node. Maybe it should just generate a fake root
> container with just one child, but using a queryContainer would not be

Adding an itemid term to query URIs is one option, but how about an outside mechanism that, given an ID, returns a node of the appropriate type?  nsINavHistoryService.idToNode()?  Simpler than dummy roots, no results or queries or parent nodes needed, at least not from the caller's perspective.

> i don't think overloading about: with tons of functionality is really nice,

about URLs are indeed common in Firefox, but that's a feature, not a problem.  about can't be overloaded any more than http can.  Basically I think I made a mistake in jumping to a new scheme.  Just because we use place URIs internally doesn't mean we need to expose them as-is through a new scheme.  If the place scheme has some advantage over the already familiar about scheme then OK, but of course it doesn't -- it's just different letters, plus an entirely new protocol handler.

> place: uris are already there, just not exposed, so there should be no problem
> in using them, and we can expand them as we like.

The problem we're discussing points to the fact that place URIs aren't sufficient.  So now we have to muddy the waters and extend them with pseudo terms like itemid or title or titlefrom that don't actually mean anything to our internal uses of place URIs.

In other words there are two separate issues: place URIs perform one function for us internally, but it's clear now that they aren't the best way to make navigable URLs.
(In reply to comment #21)
> Adding an itemid term to query URIs is one option, but how about an outside
> mechanism that, given an ID, returns a node of the appropriate type? 
> nsINavHistoryService.idToNode()?  Simpler than dummy roots, no results or
> queries or parent nodes needed, at least not from the caller's perspective.

it would not live-update, that is what we need for various parts of the UI.
Updates are done by the result.

> In other words there are two separate issues: place URIs perform one function
> for us internally, but it's clear now that they aren't the best way to make
> navigable URLs.

They always had problems with titles and always needed to be able to point to a single item, so we would just fix existing problems, their primary purpose was to be exposed to the user, that we never made.

So, you suggest something like about:bookmark/1234/a+nice+title or about:bookmark/1234/entityid ?
it would require to check if it's a query, if so we will have to get the query uri and then build a result. if it's a simple bookmark will require support for place:itemId=1234. in the best case it will require a query.
the alternative would be place:terms=foo?itemid=1234&titlefrom=entityid, this one tells us everything we need in one shot.

i like the idea of about:bookmark/1234/entityid, because it's clean, but i'm worried by the additional work required to convert it to a proper internal query.
Also we would still like to allow users to type a place: query in the locationbar and see results live in content, this solution would not allow that at all.
(In reply to comment #22)

> Also we would still like to allow users to type a place: query in the
> locationbar and see results live in content, this solution would not allow that
> at all.

This seems like it would be corner case. I cannot imagine users typing arcane place: queries

in this day and age the web is all about clean, easy to remember urls
Yeah... if we want to minimize the number of trips to the DB, I don't see any way around having to add a new itemId term to query URIs.  As you say, there would need to be some dummy root in the result with the single child. :(  (Actually, since we're planning to show only containers in content right now, the item itself could be the root, but I hope we leave open the opportunity of navigating to non-container bookmarks later.)

Otherwise, no matter what URL format we decide on, for bookmarked items there are two DB lookups:  one for the place URI to get the query result node and one for the itemId to get the corresponding item's metadata.

A dummy root is a kludge... ideally a result's "root" would be just a node, and if it's a container, great, you open it...
All of this sounds like we are coding around cruft. We should be doing this new UI implementation with a clean slate. 

This needs a query api that is built for this UI, not the other way around.
(In reply to comment #24)
> Otherwise, no matter what URL format we decide on, for bookmarked items there
> are two DB lookups:  one for the place URI to get the query result node and one
> for the itemId to get the corresponding item's metadata.

Actually the two DB lookups come from 1) looking up moz_places.url given a moz_bookmarks.id to construct a place URI and 2) getting the query result node from the place URI.  That's the case -- no matter what URL format we decide on -- unless we add an itemId term to place URIs so the two steps can be combined.
Attached patch WIP 3 — — Splinter Review 
I don't think this is working out.  I had to modify the query URI tokenization stuff to ignore URL args.  There are a few URL args I've been using: itemId, title, and parentUri.  They're needed to maintain a node's connection to its correct title, icon, parent, and such.  If they're not stripped, as soon as you open a container with a bunch of nodes with place:...?foo URIs, you get lots of unrecognized token warnings.  History containers in particular contain a bunch of nodes with place:...?foo URIs, because you've been browsing around your Places in content.  Since the URL args are stripped, the nodes lose all the connections that the args were supposed to maintain.  So when you browse your history, a bunch of nodes have no titles and generic icons.
Attachment #420027 - Attachment is obsolete: true
I'm closing this bug.  I haven't worked on it in a long time, and nobody's made it a priority.  It can be reopened or cloned if it becomes a priority in the future, or if someone else wants to pick it up.
Assignee: adw → nobody
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → WONTFIX
Blocks: 737046
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: