329269 - Places Organizer

Status: RESOLVED FIXED

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

Description

[Ben Goodger (use ben at mozilla dot org for email)]

Per: http://wiki.mozilla.org/Places:User_Interface#Places_Organizer

- Places organizer window
- Requirements too detailed to list, see link for details

Menus: http://wiki.mozilla.org/Places:User_Interface#Menus

Comment 1

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: partial work

Comment 2

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 214641 [details] [diff] [review]
partial work

r=[e-mail]

Comment 3

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 214641 [details] [diff] [review]
partial work

wtf am I doing

requesting review. 

I need a vacation.

Comment 4

[Annie Sullivan]

Comment on attachment 214641 [details] [diff] [review]
partial work

I don't see a selType property in tree.xml, shouldn't there be one?

The patch doesn't include new files, I assume there aren't any changes to advancedSearch.inc?

>+  _clean: function VM__clean(popup, startID, endID) {
>+    if (startID) {
>+      var startElement = document.getElementById(startID);
>+      ASSERT(startElement.parentNode == popup, "startElement is not in popup");
>+      ASSERT(startElement, 
>+             "startID does not correspond to an existing element");
>+      var endElement = null;
>+      if (endID) {
>+        endElement = document.getElementById(endID);
>+        ASSERT(endElement.parentNode == popup, "endElement is not in popup");
>+        ASSERT(endElement, 
>+               "endID does not correspond to an existing element");
>+      }
>+      while (startElement.nextSibling != endElement)
>+        popup.removeChild(startElement.nextSibling);
>+      return endElement;
>+    }
>+    else {
>+      while(popup.hasChildNodes())
>+        popup.removeChild(popup.firstChild);  
>+    }
>+    return null;
>+  },

It looks like _clean() removes all of the elements if startID is null and endID is NOT null.  Is this intended?

>+#ifndef XP_MACOSX
>+          <menuseparator/>
>+          <menuitem id="properties" command="placesCmd_show:info"/>
>+#endif

Why no show info command on mac?

Comment 5

[Ben Goodger (use ben at mozilla dot org for email)]

(In reply to comment #4)
> (From update of attachment 214641 [details] [diff] [review] [edit])
> I don't see a selType property in tree.xml, shouldn't there be one?
> 
> The patch doesn't include new files, I assume there aren't any changes to
> advancedSearch.inc?

Nope. Sorry! I'll attach a new patch in a moment. 

> It looks like _clean() removes all of the elements if startID is null and endID
> is NOT null.  Is this intended?

Yes, but that's an invalid sitaution, so I added an ASSERT for it. 

> Why no show info command on mac?

It's in another menu. 

selType is a property of the toolkit tree.xml (toolkit/content/widgets/tree.xml) - I was bringing this to our widgets to make the checking code in controller simpler

Comment 6

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: updated for checkin

Comment 7

[Ben Goodger (use ben at mozilla dot org for email)]

landed this part, will continue working on this next week. 

Comment 8

[Nickolay_Ponomarev]

The checkin introduced calls to unexistent functions PlacesOrganizer.groupBySite() and PlacesOrganizer.groupByPage(). They used to be calls to PlacesController.* before.

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/components/places/content/places.js#1024

Comment 9

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: more improvements to view initialization

Comment 10

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 215315 [details] [diff] [review]
more improvements to view initialization

* Built in, Asynchronous initialization for browser window places views.
  - support the asyncinit="true" attribute on the toolbar/menupopup XUL elements. 
  - when this is set, the constructor of the binding sets a 0ms timeout and inits after that,
    similar to delayedStartup but without needing to be called explicitly. 
  - set this attribute on the two instantiations in browser-menubar.inc and browser.xul
  - remove the explicit calls to init from browser.js:delayedStartup. 
  - rename public init methods to private _init methods now that no one outside the binding
    calls them. 
* General Style Nazi Reformatting
* Opening Places Organizer API
  - browser.js:showPlacesOrganizer modified to search for an existing organizer instead
    of opening a new one
  - 'bookmarks' or 'history' modes replaced by API where organizer expects window argument
    to be a place: URI to select in the left list
  - define the default place: URIs for the bookmarks menu and history roots in controller.js
    and use these throughout the browser code where the organizer is opened showing history
    or bookmarks. 
* Cleanup
  - move MENU_URI constant used only by menu.xml into a local definition within that file. 
  - move code around: ViewConfig definition closer to the ViewConfigurator object. 
  - move code around: GroupingConfig closer to the Grouping handling. 
  - move code around: #include debug.js to end of file to preserve line numbering. 
  - remove unused _strings property in controller
  - remove unused getPlaceURI method in controller
  - remove all grouping-related functions and fields from the controller. 
  - add names to anonymous functions. 
* Grouping
  - Grouping will be a feature offered exclusively through the organizer for now. As a result
    I want to remove all traces of it from the controller. 
* ViewConfigurator
  - some views, e.g. the places list, have restrictions on how users can interact with
    them, e.g. cannot drop before index 4, only support dropping items of certain types,
    etc. This is pseudo-model metadata, but is not expressed in the place: URIs. However
    this information should follow these models to any view they're instantiated in, e.g.
    you should not be able to drag into the static area if the view is loaded in a menu
    vs. a tree. 
  - So create a generic ViewConfigurator that is used to apply rules about certain roots 
    within the model (e.g. the places root) to any view that shows that root. 
  - Changed field names on the ViewConfig object to better describe their function. 
* Filter/Page Transactions
  - The Filtering Transaction Manager that supported the Places View when it was loaded in
    the browser area was removed, however the flags on all the transactions that indicated
    whether or not the transaction should be filtered by the filtering transaction manager
    stuck around. Removed all of them. 
* AVI Changes
  - Remove the isBookmarks property, since it's just a shorthand for nodeIsFolder anyway.
  - Add a new property: ".place" which sets/gets the string place: URI loaded in the
    view. Setting this will reload the view. Make the PlacesOrganizer use this in its
    initialization to simplify it a lot. Also update the PlacesOrganizer select function to
    use this property to reload the content view as the Places List selection changes. Also
    make the tree's "load" method private, since this is now handled by this property. Also
    remove the "loadFolder" method since it's redundant. 
  - Make sure the ViewConfig field name changes propagate through to every view 
    implementation.
* Style Tweaks
  - When you selected a place in the places list the two views would "jump" resize. This
    is because the width allocated to each is calculated according to:
      (availWidth - intrinsicChildWidth)/flex
    ... and when the intrinsicChildWidth changes (the text label in "Showing: Bookmarks Menu"
    changes width) the view resizes, so set the intrinsic width to 0px for that label to 
    cause it to be predictably availWidth/flex. 
  - Padding on right side of favicons. 
* Remove Calendar from display
  - Remove the calendar from the view for now. 
  - Remove all the functions associated with it (range updating, view reload notifications,
    etc). 
* Close Keybinding - Ctrl+W for places organizer
* Tree Initialization/Cleanup
  - Make the tree use the controller's |history| field instead of getting its own ref.
  - Attach the controller in the constructor, rather than in the window. 
  - Get rid of unused init method (handled by the ViewConfigurator now)
  - Get rid of _fireEvent (not needed now there's no calendar)
  - make _load private
  - get rid of redundant loadFolder
  - add methods to set the selected node. 
  - remove isBookmarks property (see above)
  - move AVI method impls around so they're closer to each other
* Change the default history query to include the last 30 days now the calendar is gone. 

IGNORE THE CHANGE TO NSNAVHISTORYQUERY.CPP!

I will handle that in a separate bug.

Comment 11

[Annie Sullivan]

Comment on attachment 215315 [details] [diff] [review]
more improvements to view initialization


>     // TODO: check for an existing one and focus it instead. 
>-    openDialog("chrome://browser/content/places/places.xul", 
>-               "", "dialog=no,resizable", mode);
>+    var wm = 
>+        Cc["@mozilla.org/appshell/window-mediator;1"].
>+        getService(Ci.nsIWindowMediator);
>+    var organizer = wm.getMostRecentWindow("Places:Organizer");

Seems like you can get rid of the TODO comment now?

>+const PLACE_HISTORY = "place:beginTime=0&beginTimeRef=1&endTime=0&endTimeRef=2&sort=4&type=1";
>+const PLACE_BOOKMARKS = "";

Why is PLACE_BOOKMARKS empty?

>-/*
>- 
>- AVI rules:
>- 

I'll admit this comment didn't help much because it didn't explain enough about what the abstract view interface is and what all the required fields/properties/methods represent, but where does the documentation for the AVI rules live now?

Also, can you add <!-- AVI Method --> comments to toolbar.xml and menu.xml like the ones you added to tree.xml?

>-      <method name="init">
>+      <method name="_init">
>         <body><![CDATA[
>         this._places = 
>           Cc["@mozilla.org/browser/nav-history-service;1"].
>           getService(Ci.nsINavHistoryService);
>         this._bms = 
>           Cc["@mozilla.org/browser/nav-bookmarks-service;1"].
>           getService(Ci.nsINavBookmarksService);
>           

Now that the toolbar init gets called from the constructor, it should handle the case where the "place" attribute isn't set.

Comment 12

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: patch

fixes an additional issue with bookmarkProperties.js (tree initialization). 

modifies init functions and place properties for toolbar/menu

removes load function from toolbar (unused)

Comment 13

[Ben Goodger (use ben at mozilla dot org for email)]

This one landed branch and trunk. 

Comment 14

[Doug Shelton]

snip
> * Remove Calendar from display
>   - Remove the calendar from the view for now. 
>   - Remove all the functions associated with it (range updating, view reload
> notifications,
>     etc). 
>

Just curious - why remove the calendar?  This seemed to be one of the most useful features of Places; it added a dimension of time and a simple way to navigate.

Comment 15

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: command cleanup - patch for review, not for applying (-wb)

Many improvements to command enabling/visibility rules for places views and the context menu. 

Consolidate transaction manager command updating (undo/redo) into a separate updater. 

Make it so that dangerous commands cannot be performed on "read only" items in the left list - these are in the "system area" and cannot be cut/deleted/pasted amongst etc. 

Split apart the onCommandUpdate function due to its growing complexity into classes of sub-functions which handle enabling different sets of commands. 

In the sub functions, ensure the commands are updated reasonably. 

New "canPaste" method which checks to see if the data on the clipboard can actually be inserted into the view...e.g. if there are URLs. If any chunk of the data fails to parse as a URL, then the paste command should be disabled since it's just raw text or some other format we don't recognize. (This section is incomplete and needs more work)

Improve _buildSelectionMetadata to not set mixed for non-mixed selections (*), add a query attribute, use the nodeIs* functions instead of manually inspecting type, import annotations as properties, etc. This allows us to show and enable the "Reload" function on livemark items. 

(* mixed = selection containing different node types, or nodes at different levels in the hierarchy). 

Also, show "Sort by: " prefix on menuitems in organizer's View menu, so that it's obvious what the menu items do. 

Make pressing ENTER/RETURN open a link. 

Fix bugs in: 
- isCommandEnabled - was returning true if the disabled attribute == "true"!
- hasRemovableSelection - don't call nodeIsReadOnly (see comment)
- createTransactions - js error invoking this.nodeIsURI in an embedded function, should use self instead. 

Remove dead code:
- canPaste

Comment 16

[Annie Sullivan]

Comment on attachment 215827 [details] [diff] [review]
command cleanup - patch for review, not for applying (-wb)


>+  /**
>+   * Updates commands for opening links
>+   * @param   inSysArea
>+   *          true if the selection intersects the read only "system" area.
>+   * @param   hasSingleSelection
>+   *          true if only one item is selected in the view
>+   * @param   selectedNode
>+   *          The selected nsINavHistoryResultNode
>+   * @param   canInsert
>+   *          true if the item is a writable container that can be inserted 
>+   *          into
>+   */
>+  _updateSortCommands: 
>+  function PC__updateSortCommands(inSysArea, hasSingleSelection, selectedNode, 
>+                                  canInsert) {

nit: "Updates commands for opening links" comment is incorrect.

>+    // Depending on the selection, the persistent sort command sorts the 
>+    // contents of the current folder (when the selection is mixed or leaf 
>+    // items like individual bookmarks are selected) or the contents of the
>+    // selected folder (if a single folder is selected).     
>+    var sortingChildren = false;
>+    var name = result.root.title;
>+    if (selectedNode) 
>+      name = selectedNode.parent.title;
>+    else if (hasSingleSelection && this.nodeIsFolder(selectedNode)) {
>+      name = selectedNode.title;
>+      sortingChildren = true;
>+    }

So if selectedNode is null, you check this.nodeIsFolder(selectedNode)?  Won't that always assert?  And why would you check if a null node is a folder anyway?  I don't understand what this is supposed to do.

Comment 17

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: address issues

Oops! I meant if () not else if ()

I updated the comment too.

Comment 18

[Ben Goodger (use ben at mozilla dot org for email)]

e.g. 

if there's no selected node, use the title of the result/view root
if there's a selected node, use the title of the select node's parent
if there's a single selected node that's a folder, use the title of that folder instead. 

From a user's point of view:

- right clicking on a view when there's no selection still should imply that that view can be sorted, even if there's no individual item selected (first case)
- right clicking on a leaf item should mean sort the contents of that leaf item's parent (i.e. sort the leaf item and all its peers)
- right clicking on a folder should show an option to sort the contents of that folder. 

Comment 19

[Annie Sullivan]

Comment on attachment 215839 [details] [diff] [review]
address issues


>+    if (selectedNode) 
>+      name = selectedNode.parent.title;
>+    if (hasSingleSelection && this.nodeIsFolder(selectedNode)) {
>+      name = selectedNode.title;
>+      sortingChildren = true;
>+    }

This looks much better, assuming that selectedNode can't be null if hasSingleSelection is set?

Comment 20

[Ben Goodger (use ben at mozilla dot org for email)]

correct.

thanks, annie!

Comment 21

[Nickolay_Ponomarev]

bug 331784 might have been caused by this. BTW, nice job ignoring comment 8 (it's now filed as bug 330567).

Comment 22

[Ben Goodger (use ben at mozilla dot org for email)]

Basic Organizer functionality now in place. 

Comment 23

[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