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)
Firefox
Bookmarks & History
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.
Assignee | ||
Comment 2•17 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•17 years ago
|
Assignee: nobody → cyen
Comment 3•17 years ago
|
||
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•17 years ago
|
||
Here is the proposed menu under the organize button
Updated•17 years ago
|
Attachment #271920 -
Attachment is obsolete: true
Comment 5•17 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•17 years ago
|
||
Rather - I think a better question is, what is left in the menu bar? Just the items marked [menu bar only]..?
Comment 7•17 years ago
|
||
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•17 years ago
|
Comment 8•17 years ago
|
||
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•17 years ago
|
Attachment #272665 -
Attachment is patch: true
Attachment #272665 -
Attachment mime type: application/octet-stream → text/plain
Updated•17 years ago
|
Whiteboard: [swag:1d]
Target Milestone: --- → Firefox 3 M7
Comment 9•17 years ago
|
||
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
Comment 10•17 years ago
|
||
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•17 years ago
|
||
Here is a mockup of how the classic menubar appears in Vista's explorer.
Comment 12•17 years ago
|
||
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•17 years ago
|
||
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•17 years ago
|
Target Milestone: Firefox 3 M7 → Firefox 3 M8
Updated•17 years ago
|
Whiteboard: [swag:1d] → [swag:1d] [places-ui]
Comment 14•17 years ago
|
||
Regarding the last comment, we've decided to use this interface across all platforms, changing only the visual styling.
Comment 15•17 years ago
|
||
Here is the latest iteration of the Places Organizer:
http://people.mozilla.com/~faaborg/files/granParadisoUI/placesOrganizer_i5WindowLayout.png
Assignee | ||
Comment 16•17 years ago
|
||
christine, are you going to be able to work on this for M8?
Assignee | ||
Comment 17•17 years ago
|
||
- 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)
Comment 18•17 years ago
|
||
Hrm, this is really a vista-thing, and unlikely something we're willing to do on mac.
Comment 19•17 years ago
|
||
>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•17 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•17 years ago
|
||
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•17 years ago
|
||
this time with strings
Attachment #278628 -
Attachment is obsolete: true
Attachment #278630 -
Flags: review?(mano)
Attachment #278628 -
Flags: review?(mano)
Comment 23•17 years ago
|
||
+ <toolbarbutton id="organizeButton"
style="min-width:0px !important;"
?
+ type="menu" label="Organize"
?
Comment 24•17 years ago
|
||
On OS X, you probably want to overlay macBrowserOverlay on this window.
Updated•17 years ago
|
Attachment #278630 -
Flags: review?(mano) → review-
Assignee | ||
Comment 25•17 years ago
|
||
(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•17 years ago
|
||
Updated•17 years ago
|
Attachment #279065 -
Attachment is patch: true
Attachment #279065 -
Attachment mime type: application/octet-stream → text/plain
Assignee | ||
Comment 27•17 years ago
|
||
Attachment #279106 -
Attachment is obsolete: true
Assignee | ||
Updated•17 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•17 years ago
|
||
Assignee | ||
Comment 29•17 years ago
|
||
- 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•17 years ago
|
||
still has shortcut problem.
Attachment #279417 -
Attachment is obsolete: true
Attachment #279612 -
Flags: review?(mano)
Updated•17 years ago
|
Attachment #279612 -
Flags: review?(mano) → review-
Assignee | ||
Comment 31•17 years ago
|
||
per mano:
- re-enabled title area
- removed pluggable views support
Attachment #279612 -
Attachment is obsolete: true
Assignee | ||
Comment 32•17 years ago
|
||
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•17 years ago
|
||
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•17 years ago
|
||
[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•17 years ago
|
||
note: keyboard shortcuts in the organizer are still broken on mac with this patch. they work ok on windows.
Assignee | ||
Updated•17 years ago
|
Target Milestone: Firefox 3 M8 → Firefox 3 M9
Assignee | ||
Comment 36•17 years ago
|
||
Attachment #279856 -
Attachment is obsolete: true
Attachment #279856 -
Flags: review?(mano)
Assignee | ||
Comment 37•17 years ago
|
||
Attachment #280402 -
Attachment is obsolete: true
Attachment #281258 -
Flags: review?(mano)
Comment 38•17 years ago
|
||
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-
Updated•17 years ago
|
Assignee | ||
Comment 39•17 years ago
|
||
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 40•17 years ago
|
||
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+
Comment 41•17 years ago
|
||
+ <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•17 years ago
|
||
Attachment #281353 -
Attachment is obsolete: true
Attachment #281355 -
Flags: approval1.9?
Comment 43•17 years ago
|
||
Comment on attachment 281355 [details] [diff] [review]
for checkin
a=beltzner
Attachment #281355 -
Flags: approval1.9? → approval1.9+
Assignee | ||
Comment 44•17 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
Closed: 17 years ago
Resolution: --- → FIXED
Comment 45•17 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 → ---
Comment 46•17 years ago
|
||
File a bug.
Status: REOPENED → RESOLVED
Closed: 17 years ago → 17 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•17 years ago
|
Comment 47•17 years ago
|
||
> File a bug.
see bug #396689
Updated•17 years ago
|
Comment 48•17 years ago
|
||
(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•17 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.
Comment 50•16 years ago
|
||
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
Comment 51•15 years ago
|
||
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.
Description
•