bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

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

VERIFIED FIXED in Firefox 3 beta1

Status

()

Firefox
Bookmarks & History
VERIFIED FIXED
11 years ago
9 years ago

People

(Reporter: dietrich, Assigned: dietrich)

Tracking

Trunk
Firefox 3 beta1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [places-ui], URL)

Attachments

(5 attachments, 17 obsolete attachments)

136.32 KB, image/png
Details
63.97 KB, image/png
Details
140.43 KB, image/png
Details
72.32 KB, image/jpeg
Details
50.92 KB, patch
beltzner
: approval1.9+
Details | Diff | Splinter Review
(Assignee)

Description

11 years ago
see the mockup in the bug URL.

Comment 1

11 years ago
Created attachment 271920 [details]
full log of build, would probably help.

full log of build
(Assignee)

Comment 2

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

wrong bug?

Updated

11 years ago
Assignee: nobody → cyen
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.
Created attachment 272147 [details]
Mockup of the proposed organize menu

Here is the proposed menu under the organize button
Attachment #271920 - Attachment is obsolete: true

Comment 5

11 years ago
(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)?
Status: NEW → ASSIGNED

Comment 6

11 years ago
Rather - I think a better question is, what is left in the menu bar? Just the items marked [menu bar only]..?
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.

Updated

11 years ago
Depends on: 387746, 387749

Comment 8

11 years ago
Created attachment 272665 [details] [diff] [review]
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)

Updated

11 years ago
Attachment #272665 - Attachment is patch: true
Attachment #272665 - Attachment mime type: application/octet-stream → text/plain

Updated

11 years ago
Whiteboard: [swag:1d]
Target Milestone: --- → Firefox 3 M7

Comment 9

11 years ago
Created attachment 273623 [details] [diff] [review]
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?
Attachment #272665 - Attachment is obsolete: true
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.
Created attachment 273762 [details]
Menubar in Vista's explorer

Here is a mockup of how the classic menubar appears in Vista's explorer.
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.
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.
(Assignee)

Updated

11 years ago
Target Milestone: Firefox 3 M7 → Firefox 3 M8

Updated

11 years ago
Whiteboard: [swag:1d] → [swag:1d] [places-ui]
Regarding the last comment, we've decided to use this interface across all platforms, changing only the visual styling.
(Assignee)

Comment 16

11 years ago
christine, are you going to be able to work on this for M8?
(Assignee)

Updated

11 years ago
Blocks: 392648
(Assignee)

Updated

11 years ago
Blocks: 387742
(Assignee)

Comment 17

11 years ago
Created attachment 277470 [details] [diff] [review]
fix v1

- up to date w/ latest mockup
- removed the detail pane code
- removed the grouping UI (for now)
- some whitespace cleanup
Assignee: christineyen+bugs → dietrich
Attachment #273623 - Attachment is obsolete: true
Attachment #277470 - Flags: review?(mano)
Hrm, this is really a vista-thing, and unlikely something we're willing to do on mac.
>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.
(Assignee)

Comment 20

11 years ago
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.
Whiteboard: [swag:1d] [places-ui] → [places-ui]
(Assignee)

Comment 21

11 years ago
Created attachment 278628 [details] [diff] [review]
fix v2

per mconnor and faaborg, remove the old menus altogether.
Attachment #277470 - Attachment is obsolete: true
Attachment #278628 - Flags: review?(mano)
Attachment #277470 - Flags: review?(mano)
(Assignee)

Comment 22

11 years ago
Created attachment 278630 [details] [diff] [review]
fix v2

this time with strings
Attachment #278628 - Attachment is obsolete: true
Attachment #278630 - Flags: review?(mano)
Attachment #278628 - Flags: review?(mano)
+      <toolbarbutton id="organizeButton"

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

?

+                    type="menu" label="Organize"

?
On OS X, you probably want to overlay macBrowserOverlay on this window.
(Assignee)

Comment 25

11 years ago
Created attachment 279065 [details] [diff] [review]
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? :/
Attachment #278630 - Attachment is obsolete: true
Attachment #279065 - Flags: review?(mano)
(Assignee)

Comment 26

11 years ago
Created attachment 279106 [details]
screenshot - organize, view and i/b menus
Attachment #279065 - Attachment is patch: true
Attachment #279065 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Updated

11 years ago
Blocks: 387744
(Assignee)

Updated

11 years ago
Blocks: 392664
(Assignee)

Updated

11 years ago
No longer blocks: 387744
(Assignee)

Comment 27

11 years ago
Created attachment 279343 [details]
screenshot mac - browser menus, b/f nav, icons and buttons, no statusbar
Attachment #279106 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Attachment #279343 - Attachment description: screenshot - browser menus, b/f nav, icons and buttons, no statusbar → screenshot mac - browser menus, b/f nav, icons and buttons, no statusbar
(Assignee)

Comment 28

11 years ago
Created attachment 279369 [details]
screenshot win - browser menus, b/f nav, icons and buttons, no statusbar
(Assignee)

Comment 29

11 years ago
Created attachment 279417 [details] [diff] [review]
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
Attachment #279065 - Attachment is obsolete: true
Attachment #279065 - Flags: review?(mano)
(Assignee)

Comment 30

11 years ago
Created attachment 279612 [details] [diff] [review]
combined menu patch v2

still has shortcut problem.
Attachment #279417 - Attachment is obsolete: true
Attachment #279612 - Flags: review?(mano)
(Assignee)

Comment 31

11 years ago
Created attachment 279645 [details] [diff] [review]
combined menu patch v3

per mano:

- re-enabled title area
- removed pluggable views support
Attachment #279612 - Attachment is obsolete: true
(Assignee)

Comment 32

11 years ago
Created attachment 279751 [details] [diff] [review]
combined menu patch v4

re-enabled titlebar styles

Mano: was that what you meant, or something else?
Attachment #279645 - Attachment is obsolete: true
Attachment #279751 - Flags: review?(mano)
(Assignee)

Comment 33

11 years ago
Created attachment 279828 [details] [diff] [review]
combined menu patch v5

mano, not sure if you're finished scanning yet, but here's the patch with the commandset back in place.
Attachment #279751 - Attachment is obsolete: true
Attachment #279751 - Flags: review?(mano)
(Assignee)

Comment 34

11 years ago
Created attachment 279856 [details] [diff] [review]
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
Attachment #279828 - Attachment is obsolete: true
Attachment #279856 - Flags: review?(mano)
(Assignee)

Comment 35

11 years ago
note: keyboard shortcuts in the organizer are still broken on mac with this patch. they work ok on windows.
(Assignee)

Updated

11 years ago
Target Milestone: Firefox 3 M8 → Firefox 3 M9
(Assignee)

Comment 36

11 years ago
Created attachment 280402 [details] [diff] [review]
combined menu patch v7 (unrotted, moved the backup/restore menuitems)
Attachment #279856 - Attachment is obsolete: true
Attachment #279856 - Flags: review?(mano)
(Assignee)

Comment 37

11 years ago
Created attachment 281258 [details] [diff] [review]
combined menu patch v8 (unrotted again)
Attachment #280402 - Attachment is obsolete: true
Attachment #281258 - Flags: review?(mano)
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...
Attachment #281258 - Flags: review?(mano) → review-
(Assignee)

Comment 39

11 years ago
Created attachment 281353 [details] [diff] [review]
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.
Attachment #281258 - Attachment is obsolete: true
Attachment #281353 - Flags: review?(mano)
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.
Attachment #281353 - Flags: review?(mano) → review+
+      <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.
(Assignee)

Comment 42

11 years ago
Created attachment 281355 [details] [diff] [review]
for checkin
Attachment #281353 - Attachment is obsolete: true
Attachment #281355 - Flags: approval1.9?
Comment on attachment 281355 [details] [diff] [review]
for checkin

a=beltzner
Attachment #281355 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 44

11 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Updated

11 years ago
Blocks: 396630

Comment 45

11 years ago
There are two entities with the same name in places.dtd : view.columns.label , the second occurrence should be renamed.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
File a bug.
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED

Updated

11 years ago
Blocks: 396721
(Assignee)

Updated

11 years ago
Blocks: 396689
No longer blocks: 396721
> File a bug.

see bug #396689
(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.
(Assignee)

Comment 49

11 years ago
(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.

Updated

11 years ago
Blocks: 398597
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
Status: RESOLVED → VERIFIED
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
You need to log in before you can comment on or make changes to this bug.