362292 - [places] implement places-based-bookmarks sidebar

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[(not reading, please use [e-mail] instead)]

[places] implement places-based-bookmarks sidebar.

this is a much simpler viewer into places that the massive organize bookmarks dialog.

we should be able to do this without defining MOZ_PLACES_BOOKMARKS, but keep it behind MOZ_PLACES (like the places based history sidebar is currently)

see bug #356487, for the places-based-history sidebar work.  see also history-panel.xul / history-panel.js in the places content.

note, the history panel is not complete, see bug #359462 for an incomplete patch that tries to wed the edit menu overlay with the places controller.  (this patch needs work.)

note, for eventual for 2.0 -> 3.0 migration, we'll need the new sidebar to live at the same chrome location as the old sidebar (but we don't need to this just yet.)  again, see the history-on-places sidebar for how I did this.

Comment 1

[Joey Minta]

I attached a work-in-progress patch for something like this in bug 360029 (I didn't find this one.)

Comment 2

[Dan Mills [:thunder]]

Hey Joey,

I've also been working on a sidebar.  I'll upload what I have now, so we can sync up if possible.

Comment 3

[Dan Mills [:thunder]]

Attachment: jar.mn and content/tree.xml changes

As the comment in tree.xml says, I'd like to use the applyFilter method, but I needed some slightly different options, so I wrote searchBookmarks.  I'll look into it.

2 more files coming up (cvs diff -N doesn't work, at least for files that haven't been added.  anyone know a workaround?).

Comment 4

[Dan Mills [:thunder]]

Attachment: component/bookmarksPanel.js

Comment 5

[Dan Mills [:thunder]]

Attachment: content/bookmarksPanel.xul

Comment 6

[Dan Mills [:thunder]]

Attachment: Bookmarks sidebar

Here's a cleaner version of the patch.

If the contents of sidebarUtils.js look good, I can change history-panel.* to use it.  Not sure if that should be included in this bug, though.

Some outstanding issues:
* DND doesn't always work correctly.  At least when dropping into a closed container, it seems to break.
* A few commands aren't working yet.  Delete, for one.
* When DNDing a livemark into the bookmarks tree, the livemark gets moved, not copied.

I can spin off bugs for any/all of the above, if that seems appropriate.

Comment 7

[Dan Mills [:thunder]]

Hrm, I'm not sure we want the browser-menubar.inc portion of my patch yet.  I was testing that and it snuck in.

Comment 8

[Asaf Romano (gone)]

Comment on attachment 252859 [details] [diff] [review]
Bookmarks sidebar

>Index: components/bookmarks/jar.mn
>===================================================================
>+#ifndef MOZ_PLACES_BOOKMARKS
> *  content/browser/bookmarks/bookmarksPanel.xul               (content/bookmarksPanel.xul)
> *  content/browser/bookmarks/bookmarksPanel.js                (content/bookmarksPanel.js)
>+#endif

We don't build the legacy bookmarks component at all when this is defined ;)


So I Hope.

>Index: components/places/jar.mn
>===================================================================
> # keep the Places version of the history sidebar at history/history-panel.xul 
> # to prevent having to worry about between versions of the browser
> *   content/browser/history/history-panel.xul            (content/history-panel.xul) 
> *   content/browser/places/history-panel.js              (content/history-panel.js)
>+# ditto for the bookmarks sidebar
>+*   content/browser/bookmarks/bookmarksPanel.xul         (content/bookmarksPanel.xul)
>+*   content/browser/bookmarks/bookmarksPanel.js          (content/bookmarksPanel.js)
>+*   content/browser/bookmarks/sidebarUtils.js            (content/sidebarUtils.js)
> *   content/browser/places/moveBookmarks.xul             (content/moveBookmarks.xul)
> *   content/browser/places/moveBookmarks.js              (content/moveBookmarks.js)

Hrm, we should only build these #ifdef MOZ_PLACES_BOOKMARKS, otherwise you may override the legacy-sidebar files in places-w/o-bookmarks builds.

>Index: components/places/content/bookmarksPanel.js
>===================================================================
>+function init() {
>+  gSidebarTree.place = BOOKMARKS_SIDEBAR_ROOT;

Use the place attribute instead.

>+function searchBookmarks(aSearchString) {
>+  if (aSearchString == "")

or just |if (aSearchString)|

>+    gSidebarTree.place = BOOKMARKS_SIDEBAR_ROOT;

|gSidebarTree.place = gSidebarTree.place| is cleaner IMO.

>+  else {
>+    var options = gSidebarTree.getBestOptions().clone();

why clone?


>+}
>\ No newline at end of file

new line.

>Index: components/places/content/bookmarksPanel.xul
>===================================================================
>+  <script type="application/x-javascript" 
>+          src="chrome://browser/content/bookmarks/sidebarUtils.js"/>
>+  <script type="application/x-javascript" 
>+          src="chrome://browser/content/bookmarks/bookmarksPanel.js"/>
>+  <script type="application/x-javascript"
>+          src="chrome://global/content/debug.js"/>

debug.js is #included by globalOverlay.js, which is loaded by placesOverlay.xul.

>+</page>
>\ No newline at end of file


new line.

>Index: components/places/content/sidebarUtils.js
>===================================================================
>+# The Initial Developer of the Original Code is Mozilla Corporation.
>+# Portions created by the Initial Developer are Copyright (C) 2007
>+# the Initial Developer. All Rights Reserved.
>+#
>+# Contributor(s):
>+#   Dan Mills <[e-mail]> (Ported from history-panel.js)

which means the initial developer is not MoCo.


>+var gSidebarTree;
>+
>+function sidebarInit(treeId, searchBoxId) {
>+  gSidebarTree = document.getElementById(treeId);
>+  document.getElementById(searchBoxId).focus();
>+}

I don't like this, we should simply pass the tree to the few shared functions.

>+function checkURLSecurity(aURL)
>+{
...

s/Components.classes/Cc/


>+function openURLIn(aWhere)
>+{
>+  // node will be null if there is a multiple selection 

not AFAICT, see tree.xml. Your tree doesn't support multiple selection at all though.


>+function openURL(aEvent)
>+{
...

Maybe move the security checks to this function and then call:

aTree.controller.openSelectedNodeIn(whereToOpenLink(aEvent));

then you could remove openURLIn altogether.

>\ No newline at end of file

new line.

Comment 9

[(not reading, please use [e-mail] instead)]

Comment on attachment 252859 [details] [diff] [review]
Bookmarks sidebar

clearing my review, as asaf is on the job.

Comment 10

[Dan Mills [:thunder]]

Attachment: Bookmarks sidebar, revised

Has the fixes above, plus a fix to use places.dtd instead of bookmarks.dtd (which I had in my tree, but is incorrect).

Comment 11

[Asaf Romano (gone)]

Comment on attachment 253713 [details] [diff] [review]
Bookmarks sidebar, revised

>diff -r c359f6f07adc browser/components/places/content/bookmarksPanel.js
>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>+++ b/browser/components/places/content/bookmarksPanel.js	Thu Feb 01 18:32:01 2007 -0800


>+function searchBookmarks(aSearchString) {
>+  tree = document.getElementById('bookmarks-view');
>+  if (!aSearchString)
>+    tree.place = tree.place;
>+  else {
>+    var options = tree.getBestOptions();
>+    var query = PlacesUtils.history.getNewQuery();
>+    query.searchTerms = aSearchString;
>+    query.onlyBookmarked = true;
>+    tree.load([query], options);

Cannot we use applyFilter here instead (see history-panel.js)?

>diff -r c359f6f07adc browser/components/places/content/bookmarksPanel.xul
>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>+++ b/browser/components/places/content/bookmarksPanel.xul	Thu Feb 01 18:32:01 2007 -0800

>+  <tree id="bookmarks-view" class="placesTree" type="places"
>+        flex="1"
>+        hidecolumnpicker="true"
>+        place="place:&amp;folder=1&amp;group=3&amp;excludeQueries=1"
>+        context="placesContext"
>+        onkeypress="if (event.keyCode == 13) openURL(document.getElementById('bookmarks-view'), event);"
>+        onclick="handleClick(document.getElementById('bookmarks-view'), event);">

you can pass |this| rather than |document.getElementById('bookmarks-view')| in both handlers.


>diff -r c359f6f07adc browser/components/places/content/sidebarUtils.js

>+function sidebarInit(treeId, searchBoxId) {
>+  gSidebarTree = document.getElementById(treeId);
>+  
>+}
>+

no longer used.


>+function checkURLSecurity(aURL)

Ugh.

See bug 224521 for the reason this is done in the history sidebar, we shouldn't do this in the bookmark sidebar (it sure breaks bookmarklets), thus i think you should move this back to history-panel.js

Comment 12

[Dan Mills [:thunder]]

(In reply to comment #11)

> Cannot we use applyFilter here instead (see history-panel.js)?

(tree.xml, right?)

Not as it is right now.  applyFilter copies the current sort options, which for the bookmarks tree will be GROUP_BY_HOST.  That eventually ends up in nsNavHistory::RecursiveGroup, which doesn't support that grouping method.

However, lxr tells me we don't call applyFilter from very many places - just places.js and history-panel.js.  So I could refactor applyFilter to do what we need in all those cases.

I'll comment more once I get more data.

> you can pass |this| rather than |document.getElementById('bookmarks-view')| in
> both handlers.

Oh, that's good to know!

> >+function checkURLSecurity(aURL)
> 
> Ugh.
> 
> See bug 224521 for the reason this is done in the history sidebar, we shouldn't
> do this in the bookmark sidebar (it sure breaks bookmarklets), thus i think you
> should move this back to history-panel.js

Hmm, I see.

How about this instead: I add a boolean to clickHandler to toggle checking security or not, possibly also merging openURL into clickHandler.

The click handlers are otherwise identical for both sidebars, and I'd like to share that code.

Comment 13

[Dan Mills [:thunder]]

Attachment: Bookmarks sidebar, v3

Here's a patch that uses applyFilter.  I didn't have applyFilter reset the place when the search terms are empty, because I'm not sure that's the right thing to do.  It seems like we might want to be able to run applyFilter to e.g. filter for only bookmarked nodes, without any search terms.

Let me know what you think of the checkSerurity thing too!

Thanks for the reviews so far...

Comment 14

[Asaf Romano (gone)]

Attachment: put the security-check in controller/utils (checked in)

This will especially help us if we ever expose history items in views other than the history sidebar.

I also moved the error string to places.properties.

Comment 15

[(not reading, please use [e-mail] instead)]

Comment on attachment 253853 [details] [diff] [review]
put the security-check in controller/utils (checked in)

r=sspitzer, but can you also fix this caller in the history sidebar (http://lxr.mozilla.org/seamonkey/source/browser/components/places/content/history-panel.js#117)
to use the one in PlacesUtils, and then remove the duplicate code in http://lxr.mozilla.org/seamonkey/source/browser/components/places/content/history-panel.js#72 and also remove the duplicated property?

Comment 16

[Asaf Romano (gone)]

I intentionally left that part to Dan so we don't patch the same code.

Comment 17

[Asaf Romano (gone)]

mozilla/browser/components/places/content/controller.js 1.121
mozilla/browser/components/places/content/utils.js 1.10
mozilla/browser/locales/en-US/chrome/browser/places/places.properties 1.1

Comment 18

[(not reading, please use [e-mail] instead)]

> I intentionally left that part to Dan so we don't patch the same code.

dan, can you make that cleanup part of your final patch?

Comment 19

[Dan Mills [:thunder]]

Attachment: Bookmarks sidebar, v4

* Includes the changes to the history sidebar to use the shared click handler, and (therefore) the shared security check.

* I put the shared sidebar code in a SidebarUtils object, to avoid global namespace pollution.

* Removes the leftover error property in history.properties, as well as the duplicated one in places.properties.

Comment 20

[Dan Mills [:thunder]]

Attachment: Bookmarks sidebar, v5

* Don't remove string from history.properties, that would break non-places builds.

No other changes.

Comment 21

[Asaf Romano (gone)]

Comment on attachment 254095 [details] [diff] [review]
Bookmarks sidebar, v5

reviewed in #places.

Comment 22

[Dan Mills [:thunder]]

Attachment: Bookmarks sidebar, v6

* Fix key handlers.
* Change applyFilter so it resets the tree to the original place url with an empty string.
* The 2.0 history sidebar (but not the bookmarks one) interpreted a click in the "gutter" (empty area to the left of the name/icon) as a select, not an action.  This patch adds a boolean to the click handler so we can still use the same handler for both sidebars.  We may want to have them behave the same, but that should be in its own bug.

Comment 23

[Dan Mills [:thunder]]

Attachment: Bookmarks sidebar, v7

* Revert the applyFilter change, we have a workaround for that in the sidebar.
* Make a few style/correctness changes.

Hope this is the last one :)

Comment 24

[Asaf Romano (gone)]

Comment on attachment 254229 [details] [diff] [review]
Bookmarks sidebar, v7

r=mano. Please file a bug on the applyFilter mess as discussed.

Comment 25

[Asaf Romano (gone)]

mozilla/browser/base/content/browser-menubar.inc 1.10
mozilla/browser/components/places/jar.mn 1.33
mozilla/browser/components/places/content/bookmarksPanel.js 1.1
mozilla/browser/components/places/content/bookmarksPanel.xul 1.1
mozilla/browser/components/places/content/history-panel.js 1.9
mozilla/browser/components/places/content/history-panel.xul 1.6
mozilla/browser/components/places/content/sidebarUtils.js 1.1
mozilla/browser/components/places/content/tree.xml 1.50
mozilla/browser/locales/en-US/chrome/browser/places/places.properties 1.14

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