387740 - move the toolbar items in the organizer to an "Organize" menu in the search bar

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Dietrich Ayala (:dietrich)]

see the mockup in the bug URL.

Comment 1

[Ray Kiddy]

Attachment: full log of build, would probably help.

full log of build

Comment 2

[Dietrich Ayala (:dietrich)]

(In reply to comment #1)
> Created an attachment (id=271920) [details]
> full log of build, would probably help.
> 
> full log of build
> 

wrong bug?

Comment 3

[Alex Faaborg [:faaborg] (Firefox UX)]

Here is how I propose we migrate the menu bar commands into a vista style toolbar:

Organize menu = moved to the new Organize button
REMOVE = entirely gone from the interface
Menu bar only = only appears in the menu bar, which is hidden by default (on windows)
NEW = added to the interface

File > New Bookmark...   [organize menu]
File > New Live Bookmark [REMOVE]
File > New Folder        [organize menu]
File > New Separator     [organize menu]
----------------------------------------
File > Import            [organize menu]
File > Export            [organize menu]
----------------------------------------
File > Close             [organize menu]
----------------------------------------
Edit > Undo              [organize menu]
Edit > Redo              [organize menu]
----------------------------------------
Edit > Cut               [organize menu]
Edit > Copy              [organize menu]
Edit >Paste              [organize menu]
Edit > Delete            [organize menu]
----------------------------------------
Edit > Select All        [organize menu]
----------------------------------------
Edit > Find in Bookmarks [REMOVE]
Edit > Find in 'selected'[REMOVE]
----------------------------------------
Edit > Move Bookmarks    [menu bar only]
Edit > Copy Bookmarks    [menu bar only]
Edit > Set as Bookmarks Toolbar Folder  [REMOVE]
----------------------------------------
Edit > Reload Live Bookmark  [REMOVE]
Edit > Reload Live Title [REMOVE]
Edit > Reload            [NEW, organize menu / menu bar]
----------------------------------------
Edit > Properties        [organize menu]
----------------------------------------
View > Toolbar           [menu bar only]
View > Show Columns      [menu bar only]
----------------------------------------
View > Unsorted          [menu bar only]
View > Sort by *         [menu bar only]
----------------------------------------
View > A > Z Sort Order  [menu bar only]
View > Z > A Sort Orde   [menu bar only]


Notes on removed items

File > New Live Bookmark

We should just check if the location is a feed, and if so, make a live bookmark.  This break being able to bookmark RSS feeds, but who wants to regularly read XML?
----------------------------------------
Edit > Find in *

There is now a very large search field in the toolbar.  All this command does is focus hte field.
----------------------------------------
Edit > Move Bookmarks

We should keep this in the menu bar (and add Edit > Copy Bookmarks) for accessiblity.
----------------------------------------
Edit > Set as Bookmarks Toolbar Folder

This produces added complexity with the new bookmarks hierarchy, and I don't believe users need this level of control.
----------------------------------------
Edit > Reload Live Bookmark
Edit > Reload Live Title

These are both collapsed into a single "Edit > Reload"
----------------------------------------
View > *

All of these commands can be achieved through direct manipulation on the column headers, making them needlessly redundant.  However, I would imagine a lot of users would complain if we removed them, so I'm setting them to menu bar only.

Comment 4

[Alex Faaborg [:faaborg] (Firefox UX)]

Attachment: Mockup of the proposed organize menu

Here is the proposed menu under the organize button

Comment 5

[Christine Yen]

(In reply to comment #3)
> Here is how I propose we migrate the menu bar commands into a vista style
> toolbar:

"Migrate" -> are we removing the menu bar, then (so the browser menu bar items are still available when the Bookmarks Manager has focus, a la Download Manager)?

Comment 6

[Christine Yen]

Rather - I think a better question is, what is left in the menu bar? Just the items marked [menu bar only]..?

Comment 7

[Alex Faaborg [:faaborg] (Firefox UX)]

Sorry, I wasn't really clear on that: everything marked "organize menu", "menu bar only", and "NEW" are left in the menu bar, in their original position (still have a File menu, Edit menu, etc).  The only things being removed are the ones marked "remove."  The menu bar is hidden by default on Windows.

Comment 8

[Christine Yen]

Attachment: checkpt 1

All the menu items are working, except those under the "Layout" menu which depend on the two UI bugs for the details pane (bug 387749) and file browser (bug 387746)

Comment 9

[Christine Yen]

Attachment: checkpt 2

reflecting recent changes in CVS and the updated patch for Steve's item detail pane bug 387749.

Alex - what should the "Menu Bar" menuitem toggle under the "Layout" menu in the Organize button?

Comment 10

[Alex Faaborg [:faaborg] (Firefox UX)]

Sorry for the delay in replying, I've been out sick.  The "Menu Bar" menuitem toggle displays the classic menu bar above the toolbar.  I'll attach a screen shot of explorer in both states.

Comment 11

[Alex Faaborg [:faaborg] (Firefox UX)]

Attachment: Menubar in Vista's explorer

Here is a mockup of how the classic menubar appears in Vista's explorer.

Comment 12

[Alex Faaborg [:faaborg] (Firefox UX)]

Two additional notes:

-The menubar toggle should not appear on OS X (since the menu bar is always visible)
-If the user hits the alt key, the menu bar should appear.  After the user executes a command in the menu bar, it disappears again.

Comment 13

[Alex Faaborg [:faaborg] (Firefox UX)]

Ideally the changes outlined in this bug will only appear on Vista.  I'm going to start a discussion in dev.apps.firefox about platform specific UI design.

Comment 14

[Alex Faaborg [:faaborg] (Firefox UX)]

Regarding the last comment, we've decided to use this interface across all platforms, changing only the visual styling.

Comment 15

[Alex Faaborg [:faaborg] (Firefox UX)]

Here is the latest iteration of the Places Organizer:

http://people.mozilla.com/~faaborg/files/granParadisoUI/placesOrganizer_i5WindowLayout.png

Comment 16

[Dietrich Ayala (:dietrich)]

christine, are you going to be able to work on this for M8?

Comment 17

[Dietrich Ayala (:dietrich)]

Attachment: fix v1

- up to date w/ latest mockup
- removed the detail pane code
- removed the grouping UI (for now)
- some whitespace cleanup

Comment 18

[Asaf Romano (gone)]

Hrm, this is really a vista-thing, and unlikely something we're willing to do on mac.

Comment 19

[Alex Faaborg [:faaborg] (Firefox UX)]

>this is really a vista-thing, and unlikely something we're willing
>to do on mac.

Actually Vista copied OS X pretty throughly with their Explorer UI.  The Organize menu is essentially an analog of the gear in the Finder.  There are some semantic differences (Properties/Get Info, Delete/Move to Trash), the items are in a different order, and a few are missing on OS X (like cut).  However, the contents of the menus are otherwise largely the same.

On OS X we might want to swap the order of the View menu button and the Organize "Gear" so that the View menu button is on the left.  The only remaining platform parity problem is that Views are in a drop down menu instead of a line of radio-style button controls, but I don't think this will be too big of a deal.  The actual menu bar on OS X will match the structure of the Organize and View menu buttons.

Once we make those minor changes, and throw in some leopard-style monochrome icons and a unified toolbar, I think our mac users will be rather happy.  When I get time I'll post a round of Places mockups for OS X.

I should note that while this UI works well on Vista and OS X, it will be a little odd for Windows XP users.

Comment 20

[Dietrich Ayala (:dietrich)]

Alex, can you add a gear icon to bug 392682 for this? After we get all the changes in (this bug + "view" menu, "import" menu and the navigation buttons) we can tweak the order, and icon vs text label buttons, etc based on feedback.

Comment 21

[Dietrich Ayala (:dietrich)]

Attachment: fix v2

per mconnor and faaborg, remove the old menus altogether.

Comment 22

[Dietrich Ayala (:dietrich)]

Attachment: fix v2

this time with strings

Comment 23

[Asaf Romano (gone)]

+      <toolbarbutton id="organizeButton"

style="min-width:0px !important;"

?

+                    type="menu" label="Organize"

?

Comment 24

[Asaf Romano (gone)]

On OS X, you probably want to overlay macBrowserOverlay on this window.

Comment 25

[Dietrich Ayala (:dietrich)]

Attachment: fix v3

(In reply to comment #24)
> On OS X, you probably want to overlay macBrowserOverlay on this window.
> 

like this? the overlays there are already in macBrowserOverlay.

also, note the file.label change. would it be better to change the prefix on all those entities to "organize"? does that make things easier or harder for localizers? :/

Comment 26

[Dietrich Ayala (:dietrich)]

Attachment: screenshot - organize, view and i/b menus

Comment 27

[Dietrich Ayala (:dietrich)]

Attachment: screenshot mac - browser menus, b/f nav, icons and buttons, no statusbar

Comment 28

[Dietrich Ayala (:dietrich)]

Attachment: screenshot win - browser menus, b/f nav, icons and buttons, no statusbar

Comment 29

[Dietrich Ayala (:dietrich)]

Attachment: combined menu patch v1

- combines patches from bug 387742, bug 392648, bug 392664
- the navigation history should probably use actual session history
- using the temporary icons in bug 392682
- including browserMountPoints.inc kills keyboard shortcuts

Comment 30

[Dietrich Ayala (:dietrich)]

Attachment: combined menu patch v2

still has shortcut problem.

Comment 31

[Dietrich Ayala (:dietrich)]

Attachment: combined menu patch v3

per mano:

- re-enabled title area
- removed pluggable views support

Comment 32

[Dietrich Ayala (:dietrich)]

Attachment: combined menu patch v4

re-enabled titlebar styles

Mano: was that what you meant, or something else?

Comment 33

[Dietrich Ayala (:dietrich)]

Attachment: combined menu patch v5

mano, not sure if you're finished scanning yet, but here's the patch with the commandset back in place.

Comment 34

[Dietrich Ayala (:dietrich)]

Attachment: combined menu patch v6

[7:00pm] Mano: dietrich: there are still some hardcoded entites in your patch
[7:00pm] Mano: dietrich: plus this bogus "observes" element

fixed

Comment 35

[Dietrich Ayala (:dietrich)]

note: keyboard shortcuts in the organizer are still broken on mac with this patch. they work ok on windows.

Comment 36

[Dietrich Ayala (:dietrich)]

Attachment: combined menu patch v7 (unrotted, moved the backup/restore menuitems)

Comment 37

[Dietrich Ayala (:dietrich)]

Attachment: combined menu patch v8 (unrotted again)

Comment 38

[Asaf Romano (gone)]

Comment on attachment 281258 [details] [diff] [review]
combined menu patch v8 (unrotted again)


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

Please Avoid rev'ing this file here if there are no actual modifications.

>Index: browser/components/places/content/places.js
>===================================================================
> 
>   destroy: function PO_destroy() {
>     OptionsFilter.destroy();
>   },
> 
>+  _location: null,
>+  get location() {
>+    return this._location;
>+  },
>+
>+  set location(aLocation) {
>+    LOG("Node URI: " + aLocation);
>+
>+    if (!aLocation)
>+      return;
>+

return aLocation;

>+    if (this.location)
>+      this._backHistory.unshift([this.location]);

why do you wrap the string in an array?

>+
>+    // Deserialize and reserialize to take OptionsFilter-ing into
>+    // account.
>+    // XXX need to implement nsIPlacesView interface, and require that
>+    // views implement it. Then we can assume that all views have .load()

You could get rid of this comment now that we don't support that.

>+    var queriesRef = { }, optionsRef = { };
>+    PlacesUtils.history.queryStringToQueries(aLocation,
>+                                             queriesRef, { }, optionsRef);
>+    var options = OptionsFilter.filter(queriesRef.value, optionsRef.value, null);
>+    this._location =
>+      PlacesUtils.history.queriesToQueryString(queriesRef.value,
>+                                               queriesRef.value.length,
>+                                               options);
>+    // pass to view
>+    this._content.place = this._location;
>+

why is this querystring->query->auerystring mess?

>+    // update navigation commands
>+    if (this._backHistory.length == 0)
>+      document.getElementById("OrganizerCommand:Back").setAttribute("disabled", true);
>+    else
>+      document.getElementById("OrganizerCommand:Back").removeAttribute("disabled");
>+    if (this._forwardHistory.length == 0)
>+      document.getElementById("OrganizerCommand:Forward").setAttribute("disabled", true);
>+    else
>+      document.getElementById("OrganizerCommand:Forward").removeAttribute("disabled");
>+  },
>+

must |return aLocation| here too.

>+  _backHistory: [],
>+  _forwardHistory: [],
>+  back: function PO_back() {
>+    this._forwardHistory.unshift([this.location]);
>+    var historyEntry = this._backHistory.shift();
>+    this._location = null;
>+    this.location = historyEntry[0];

>+  },
>+  forward: function PO_forward() {
>+    var historyEntry = this._forwardHistory.shift();
>+    this.location = historyEntry[0];
>+  },
>+

don't forget to update this too.


>   /**
>    * Loads the place URI entered in the debug 
>    */
>   loadPlaceURI: function PO_loadPlaceURI() {
>+
>+    // clear forward history
>+    this._forwardHistory.splice(0);
>+
>     var placeURI = document.getElementById("placeURI");
>+
>     var queriesRef = { }, optionsRef = { };
>     PlacesUtils.history.queryStringToQueries(placeURI.value, 
>                                              queriesRef, { }, optionsRef);
>     
>+    // for debug panel
>     var autoFilterResults = document.getElementById("autoFilterResults");
>     if (autoFilterResults.checked) {
>       var options = 
>         OptionsFilter.filter(queriesRef.value, optionsRef.value, null);
>     }
>     else
>       options = optionsRef.value;
>-    this._content.load(queriesRef.value, options);
>+
>+    placeURI =
>+      PlacesUtils.history.queriesToQueryString(queriesRef.value,
>+                                               queriesRef.value.length,
>+                                               options);
>+
>+    this.location = placeURI;
> 
>     this.setHeaderText(this.HEADER_TYPE_SHOWING, "Debug results for: " + placeURI.value);
> 
>     this.updateLoadedURI();
> 
>     placeURI.focus();
>   },
>

this method likely throws now (string has no focus() api ;)).

>+    // update location
>+    this.location = PlacesUtils.history.queriesToQueryString(queries, queries.length, options);


see my earlier comment about querystring->queries->querystring

>   showHideColumn: function VM_showHideColumn(element) {
>     const PREFIX = "menucol_";
>     var columnID = element.id.substr(PREFIX.length, element.id.length);
>-    var column = document.getElementById(columnID);
>+    // get the column from the places content tree
>+    var content = document.getElementById("placeContent");
>+    var columns = content.columns;
>+    var column = null;
>+    for (var i = 0; i < columns.count; ++i) {
>+      column = columns.getColumnAt(i).element;
>+      if (column.id == columnID)
>+        break;
>+    }

what's this for?

>Index: browser/components/places/content/places.xul
>===================================================================
> <!DOCTYPE window [
> <!ENTITY % placesDTD SYSTEM "chrome://browser/locale/places/places.dtd">
> %placesDTD;
> <!ENTITY % editMenuOverlayDTD SYSTEM "chrome://global/locale/editMenuOverlay.dtd">
> %editMenuOverlayDTD;
> ]>
> 
>@@ -61,22 +65,22 @@
>         xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>         onload="PlacesOrganizer.init();"
>         onunload="PlacesOrganizer.destroy();"
>         width="700" height="500" screenX="10" screenY="10"
>         persist="width height screenX screenY sizemode">
> 
>   <script type="application/x-javascript"
>           src="chrome://browser/content/places/places.js"/>
>-  
>-  <stringbundleset id="stringbundleset"/>
> 
>-  <commandset id="editMenuCommands"/>
>-  <commandset id="baseMenuCommandSet"/>
>+#ifdef XP_MACOSX
>+#include ../../../base/content/browserMountPoints.inc
>+#else
>   <commandset id="placesCommands"/>
>+#endif
>   

don't you need <commandset id="editMenuCommands"/> XP?

>   <commandset id="organizerCommandSet">
>     <command id="OrganizerCommand_find:all"
>              label="&cmd.findInBookmarks.label;"
>              accesskey="&cmd.findInBookmarks.accesskey;"
>              oncommand="PlacesSearchBox.findAll();"/>
>     <command id="OrganizerCommand_find:current"
>              label="&cmd.findCurrent.label;"
>@@ -89,25 +93,33 @@
>     <command id="OrganizerCommand_backup"
>              oncommand="PlacesOrganizer.backupBookmarks();"/>
>     <command id="OrganizerCommand_restoreFromFile"
>              oncommand="PlacesOrganizer.restoreFromFile();"/>
>     <command id="OrganizerCommand_search:save"
>              oncommand="PlacesOrganizer.saveSearch();"/>
>     <command id="OrganizerCommand_search:moreCriteria"
>              oncommand="PlacesQueryBuilder.addRow();"/>
>+    <command id="OrganizerCommand:Back"
>+             oncommand="PlacesOrganizer.back();"/>
>+    <command id="OrganizerCommand:Forward"
>+             oncommand="PlacesOrganizer.forward();"/>
>   </commandset>
> 

>-  <keyset id="baseMenuKeyset"/>
>+  <broadcasterset id="placesViewBroadcasters">
>+    <broadcaster id="placesView:Default"
>+                 checked="true"/>
>+  </broadcasterset>
>+

unused.

>   <keyset id="placesOrganizerKeyset">
>     <!-- Instantiation Keys -->
>     <key id="placesKey_close" key="&cmd.close.key;" modifiers="accel" 
>          oncommand="close();"/>
> 
>     <key id="placesKey_show:debug"
>          key="d" modifiers="accel,shift"
>          oncommand="PlacesOrganizer.toggleDebugPanel();"/>
>@@ -157,37 +169,173 @@
>     <key id="key_delete2" keycode="VK_BACK" command="cmd_delete"/>
> #endif
>   </keyset>
> 
>   <popupset id="placesPopupset">
>     <popup id="placesContext"/>
>   </popupset>
> 
>-  <toolbox>
>-    <menubar id="placesMenubar">
>-      <menu id="fileMenu" label="&file.label;" accesskey="&file.accesskey;">
>-        <menupopup>
>-          <menuitem id="fileNewBookmark"
>+  <toolbox id="placesToolbox">
>+    <toolbar class="chromeclass-toolbar" id="placesToolbar">
>+      <toolbarbutton id="back-button" class="toolbarbutton-1 chromeclass-toolbar-additional"
>+                     command="OrganizerCommand:Back"
>+                     tooltiptext="&backButton.tooltip;"
>+                     accesskey="&backCmd.accesskey;"
>+                     disabled="true">
>+        <observes element="placesOrganizer:Back" attribute="disabled"/>

What's this for?

>+      </toolbarbutton>
>+    
>+      <toolbarbutton id="forward-button" class="toolbarbutton-1 chromeclass-toolbar-additional"
>+                     command="OrganizerCommand:Forward"
>+                     tooltiptext="&forwardButton.tooltip;"
>+                     accesskey="&forwardCmd.accesskey;"
>+                     disabled="true">
>+        <observes element="placesOrganizer:Forward" attribute="disabled"/>

ditto


>Index: browser/locales/en-US/chrome/browser/places/places.dtd
>===================================================================
>RCS file: /cvsroot/mozilla/browser/locales/en-US/chrome/browser/places/places.dtd,v
>retrieving revision 1.29
>diff -u -8 -p -r1.29 places.dtd
>--- browser/locales/en-US/chrome/browser/places/places.dtd	10 Sep 2007 01:02:41 -0000	1.29
>+++ browser/locales/en-US/chrome/browser/places/places.dtd	17 Sep 2007 19:36:30 -0000
>@@ -1,36 +1,40 @@
> <!ENTITY places.bm.title
>-  "Bookmarks Manager">
>+  "Places Organizer">

rev the entity name.

> <!ENTITY file.label
>-  "File">
>+  "Organize">

ditto.

> <!ENTITY view.label
>-  "View">
>+  "Views">

and ditto.

>Index: browser/locales/en-US/profile/localstore.rdf
>===================================================================
>RCS file: /cvsroot/mozilla/browser/locales/en-US/profile/localstore.rdf,v
>retrieving revision 1.4
>diff -u -8 -p -r1.4 localstore.rdf
>--- browser/locales/en-US/profile/localstore.rdf	14 Sep 2007 04:52:47 -0000	1.4
>+++ browser/locales/en-US/profile/localstore.rdf	17 Sep 2007 19:36:30 -0000

is that bug back? :(



>Index: browser/themes/pinstripe/browser/places/places.css
>===================================================================
>RCS file: /cvsroot/mozilla/browser/themes/pinstripe/browser/places/places.css,v
>retrieving revision 1.12
>diff -u -8 -p -r1.12 places.css
>--- browser/themes/pinstripe/browser/places/places.css	17 Jun 2007 15:09:44 -0000	1.12
>+++ browser/themes/pinstripe/browser/places/places.css	17 Sep 2007 19:36:30 -0000
>@@ -1,47 +1,80 @@
>+/* Toolbar */
>+#placesToolbar {
>+  border-bottom: 0px;
>+}
>+

nit: s/0px/none/

>+/* organize button */
>+#organizeButton {
>+  list-style-image: url("chrome://browser/skin/wrench.png");
>+}
>+
>+/* view button */
>+#viewMenu {
>+  list-style-image: url("chrome://browser/skin/wrench.png");
>+}
>+
>+/* maintenance button */
>+#maintenanceButton {
>+  list-style-image: url("chrome://browser/skin/wrench.png");
>+}
>+

use a class?


> #titlebar {
>   background-color: -moz-Dialog;
>   border-bottom: 2px solid;
>   -moz-border-bottom-colors: ThreeDHighlight ThreeDShadow;
>+  padding: 0px;

nit: s/0px/0/

>Index: browser/themes/winstripe/browser/places/places.css
>===================================================================

See my comments for the pinstripe version, also:

> #placesList {
>-  -moz-appearance: listbox;
>-  margin: 6px 0px 7px 6px;
>+  -moz-appearance: none;
>+  margin: 0px;
>+  border: 0px;
>+  padding: 0px;

none and 0 as above.

>   width: 160px;

this belongs to the xul file (as a "width" attribute) if we want to persist the splitter state...

Comment 39

[Dietrich Ayala (:dietrich)]

Attachment: combined menu patch v9 - addresses comments

note: i'm about to boot into windows to test these latest changes there.

> 
> why do you wrap the string in an array?

leftover from when the view was being stored as well.

i've removed it for now.

> >+    var queriesRef = { }, optionsRef = { };
> >+    PlacesUtils.history.queryStringToQueries(aLocation,
> >+                                             queriesRef, { }, optionsRef);
> >+    var options = OptionsFilter.filter(queriesRef.value, optionsRef.value, null);
> >+    this._location =
> >+      PlacesUtils.history.queriesToQueryString(queriesRef.value,
> >+                                               queriesRef.value.length,
> >+                                               options);
> >+    // pass to view
> >+    this._content.place = this._location;
> >+
> 
> why is this querystring->query->auerystring mess?

to take options filtering into account, stored sorting state, etc.

> 
> >   showHideColumn: function VM_showHideColumn(element) {
> >     const PREFIX = "menucol_";
> >     var columnID = element.id.substr(PREFIX.length, element.id.length);
> >-    var column = document.getElementById(columnID);
> >+    // get the column from the places content tree
> >+    var content = document.getElementById("placeContent");
> >+    var columns = content.columns;
> >+    var column = null;
> >+    for (var i = 0; i < columns.count; ++i) {
> >+      column = columns.getColumnAt(i).element;
> >+      if (column.id == columnID)
> >+        break;
> >+    }
> 
> what's this for?

hrm, i experienced a bug where the column returned for the id was from the left-tree not the content tree. however i'm not able to reproduce it now, so removed. i'll take it up in another bug if i can reproduce it again.

> >@@ -157,37 +169,173 @@
> >     <key id="key_delete2" keycode="VK_BACK" command="cmd_delete"/>
> > #endif
> >   </keyset>
> > 
> >   <popupset id="placesPopupset">
> >     <popup id="placesContext"/>
> >   </popupset>
> > 
> >-  <toolbox>
> >-    <menubar id="placesMenubar">
> >-      <menu id="fileMenu" label="&file.label;" accesskey="&file.accesskey;">
> >-        <menupopup>
> >-          <menuitem id="fileNewBookmark"
> >+  <toolbox id="placesToolbox">
> >+    <toolbar class="chromeclass-toolbar" id="placesToolbar">
> >+      <toolbarbutton id="back-button" class="toolbarbutton-1 chromeclass-toolbar-additional"
> >+                     command="OrganizerCommand:Back"
> >+                     tooltiptext="&backButton.tooltip;"
> >+                     accesskey="&backCmd.accesskey;"
> >+                     disabled="true">
> >+        <observes element="placesOrganizer:Back" attribute="disabled"/>
> 
> What's this for?
> 
> >+      </toolbarbutton>
> >+    
> >+      <toolbarbutton id="forward-button" class="toolbarbutton-1 chromeclass-toolbar-additional"
> >+                     command="OrganizerCommand:Forward"
> >+                     tooltiptext="&forwardButton.tooltip;"
> >+                     accesskey="&forwardCmd.accesskey;"
> >+                     disabled="true">
> >+        <observes element="placesOrganizer:Forward" attribute="disabled"/>
> 
> ditto

unused, removed
> 
> > <!ENTITY view.label
> >-  "View">
> >+  "Views">
> 
> and ditto.

changed these, but now the rest of the menuitem strings are "file.*", "view.*", etc, which bugs me.

is it more or less of a pain for localizers if we change all of these to match their top level item?

> >+/* organize button */
> >+#organizeButton {
> >+  list-style-image: url("chrome://browser/skin/wrench.png");
> >+}
> >+
> >+/* view button */
> >+#viewMenu {
> >+  list-style-image: url("chrome://browser/skin/wrench.png");
> >+}
> >+
> >+/* maintenance button */
> >+#maintenanceButton {
> >+  list-style-image: url("chrome://browser/skin/wrench.png");
> >+}
> >+
> 
> use a class?

these will all have unique icons once we get a proper icon set for the organizer.

Comment 40

[Asaf Romano (gone)]

Comment on attachment 281353 [details] [diff] [review]
combined menu patch v9 - addresses comments

I think we should nuke optionsfiltering at this point and find some other solution in a follow up. It wouldn't matter for back/forward interaction anyway since the previous "location" already includes the saved options.

As for the entities, either way is fine by me.

r=mano otherwise.

Comment 41

[Asaf Romano (gone)]

+      <toolbarbutton id="back-button" class="toolbarbutton-1 chromeclass-toolbar-additional"
+                     command="OrganizerCommand:Back"
+                     tooltiptext="&backButton.tooltip;"
+                     accesskey="&backCmd.accesskey;"
+                     disabled="true">
+      </toolbarbutton>
+    
+      <toolbarbutton id="forward-button" class="toolbarbutton-1 chromeclass-toolbar-additional"
+                     command="OrganizerCommand:Forward"
+                     tooltiptext="&forwardButton.tooltip;"
+                     accesskey="&forwardCmd.accesskey;"
+                     disabled="true">
+      </toolbarbutton>

nit: prefer <toolbarbutton/> form.

Comment 42

[Dietrich Ayala (:dietrich)]

Attachment: for checkin

Comment 43

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

Comment on attachment 281355 [details] [diff] [review]
for checkin

a=beltzner

Comment 44

[Dietrich Ayala (:dietrich)]

Checking in browser/components/places/content/places.js;
/cvsroot/mozilla/browser/components/places/content/places.js,v  <--  places.js
new revision: 1.103; previous revision: 1.102
done
Checking in browser/components/places/content/places.xul;
/cvsroot/mozilla/browser/components/places/content/places.xul,v  <--  places.xul
new revision: 1.83; previous revision: 1.82
done
Checking in browser/components/sessionstore/src/nsSessionStore.js;
/cvsroot/mozilla/browser/components/sessionstore/src/nsSessionStore.js,v  <--  nsSessionStore.js
new revision: 1.77; previous revision: 1.76
done
Checking in browser/locales/en-US/chrome/browser/places/places.dtd;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/places/places.dtd,v  <--  places.dtd
new revision: 1.30; previous revision: 1.29
done
Checking in browser/themes/pinstripe/browser/jar.mn;
/cvsroot/mozilla/browser/themes/pinstripe/browser/jar.mn,v  <--  jar.mn
new revision: 1.56; previous revision: 1.55
done
RCS file: /cvsroot/mozilla/browser/themes/pinstripe/browser/wrench.png,v
done
Checking in browser/themes/pinstripe/browser/wrench.png;
/cvsroot/mozilla/browser/themes/pinstripe/browser/wrench.png,v  <--  wrench.png
initial revision: 1.1
done
Checking in browser/themes/pinstripe/browser/places/places.css;
/cvsroot/mozilla/browser/themes/pinstripe/browser/places/places.css,v  <--  places.css
new revision: 1.13; previous revision: 1.12
done
Checking in browser/themes/winstripe/browser/jar.mn;
/cvsroot/mozilla/browser/themes/winstripe/browser/jar.mn,v  <--  jar.mn
new revision: 1.53; previous revision: 1.52
done
RCS file: /cvsroot/mozilla/browser/themes/winstripe/browser/wrench.png,v
done
Checking in browser/themes/winstripe/browser/wrench.png;
/cvsroot/mozilla/browser/themes/winstripe/browser/wrench.png,v  <--  wrench.png
initial revision: 1.1
done
Checking in browser/themes/winstripe/browser/places/places.css;
/cvsroot/mozilla/browser/themes/winstripe/browser/places/places.css,v  <--  places.css
new revision: 1.15; previous revision: 1.14
done

Comment 45

[Cédric Corazza]

There are two entities with the same name in places.dtd : view.columns.label , the second occurrence should be renamed.

Comment 46

[Asaf Romano (gone)]

File a bug.

Comment 47

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

> File a bug.

see bug #396689

Comment 48

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

(In reply to comment #44)
> Checking in browser/components/sessionstore/src/nsSessionStore.js;
> /cvsroot/mozilla/browser/components/sessionstore/src/nsSessionStore.js,v  <-- 
> nsSessionStore.js
> new revision: 1.77; previous revision: 1.76
> done

Was that supposed to be part of this bug? It looks unrelated to the rest of the patch.

Comment 49

[Dietrich Ayala (:dietrich)]

(In reply to comment #48)
> (In reply to comment #44)
> > Checking in browser/components/sessionstore/src/nsSessionStore.js;
> > /cvsroot/mozilla/browser/components/sessionstore/src/nsSessionStore.js,v  <-- 
> > nsSessionStore.js
> > new revision: 1.77; previous revision: 1.76
> > done
> 
> Was that supposed to be part of this bug? It looks unrelated to the rest of the
> patch.
> 

yes, was intended. that method errors out for windows not tracked by session restore. once the organizer started using the main menu set, the history menu wouldn't load due to the error.

Comment 50

[Henrik Skupin [:whimboo][⌚️UTC+1]]

A bit late but verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.2pre) Gecko/2008072004 GranParadiso/3.0.2pre ID:2008072004

Comment 51

[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