Closed Bug 387740 Opened 17 years ago Closed 17 years ago

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

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 3 beta1

People

(Reporter: dietrich, Assigned: dietrich)

References

()

Details

(Whiteboard: [places-ui])

Attachments

(5 files, 17 obsolete files)

(deleted), image/png
Details
(deleted), image/png
Details
(deleted), image/png
Details
(deleted), image/jpeg
Details
(deleted), patch
beltzner
: approval1.9+
Details | Diff | Splinter Review
see the mockup in the bug URL.
Attached file full log of build, would probably help. (obsolete) (deleted) —
full log of build
(In reply to comment #1) > Created an attachment (id=271920) [details] > full log of build, would probably help. > > full log of build > wrong bug?
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.
Attached image Mockup of the proposed organize menu (deleted) —
Here is the proposed menu under the organize button
Attachment #271920 - Attachment is obsolete: true
(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
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.
Depends on: 387746, 387749
Attached patch checkpt 1 (obsolete) (deleted) — Splinter Review
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)
Attachment #272665 - Attachment is patch: true
Attachment #272665 - Attachment mime type: application/octet-stream → text/plain
Whiteboard: [swag:1d]
Target Milestone: --- → Firefox 3 M7
Attached patch checkpt 2 (obsolete) (deleted) — Splinter Review
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.
Attached image Menubar in Vista's explorer (deleted) —
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.
Target Milestone: Firefox 3 M7 → Firefox 3 M8
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.
christine, are you going to be able to work on this for M8?
Blocks: 392648
Blocks: 387742
Attached patch fix v1 (obsolete) (deleted) — Splinter Review
- 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.
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]
Attached patch fix v2 (obsolete) (deleted) — Splinter Review
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)
Attached patch fix v2 (obsolete) (deleted) — Splinter Review
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.
Attachment #278630 - Flags: review?(mano) → review-
Attached patch fix v3 (obsolete) (deleted) — Splinter Review
(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)
Attached image screenshot - organize, view and i/b menus (obsolete) (deleted) —
Attachment #279065 - Attachment is patch: true
Attachment #279065 - Attachment mime type: application/octet-stream → text/plain
Blocks: 387744
Blocks: 392664
No longer blocks: 387744
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
Attached patch combined menu patch v1 (obsolete) (deleted) — Splinter Review
- 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)
Attached patch combined menu patch v2 (obsolete) (deleted) — Splinter Review
still has shortcut problem.
Attachment #279417 - Attachment is obsolete: true
Attachment #279612 - Flags: review?(mano)
Attachment #279612 - Flags: review?(mano) → review-
Attached patch combined menu patch v3 (obsolete) (deleted) — Splinter Review
per mano: - re-enabled title area - removed pluggable views support
Attachment #279612 - Attachment is obsolete: true
Attached patch combined menu patch v4 (obsolete) (deleted) — Splinter Review
re-enabled titlebar styles Mano: was that what you meant, or something else?
Attachment #279645 - Attachment is obsolete: true
Attachment #279751 - Flags: review?(mano)
Attached patch combined menu patch v5 (obsolete) (deleted) — Splinter Review
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)
Attached patch combined menu patch v6 (obsolete) (deleted) — Splinter Review
[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)
note: keyboard shortcuts in the organizer are still broken on mac with this patch. they work ok on windows.
Target Milestone: Firefox 3 M8 → Firefox 3 M9
Attachment #279856 - Attachment is obsolete: true
Attachment #279856 - Flags: review?(mano)
Attached patch combined menu patch v8 (unrotted again) (obsolete) (deleted) — Splinter Review
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-
Blocks: 387746, 387749
No longer depends on: 387746, 387749
Attached patch combined menu patch v9 - addresses comments (obsolete) (deleted) — Splinter Review
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.
Attached patch for checkin (deleted) — Splinter Review
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+
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
Closed: 17 years ago
Resolution: --- → FIXED
Blocks: 396630
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
Closed: 17 years ago17 years ago
Resolution: --- → FIXED
Blocks: 396721
Blocks: 396689
No longer blocks: 396721
No longer blocks: 396630
Depends on: 396630
(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.
(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.
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.

Attachment

General

Creator:
Created:
Updated:
Size: