Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008010705 Minefield/3.0b3pre ID:2008010705 When you want to modify the bookmark properties within the bookmarks sidebar you are not able to add/remove tags because no UI exists yet. Fields (like the details deck inside the Library) should be added to be able to modify the tags for that bookmark.

This also applies for bookmarks on the bookmarks toolbar.

As far as I know this has been implemented in Firefox 3.0 Beta 2. I don't know what version of Firefox you are using but in mine I am able to tag bookmarks and then search using those tags. This works with the toolbar too. RESOLVED?

This bug isn't solved yet. The Bookmark Properties dialog still lacks the tagging UI. No idea about what specific bookmarks properties you are talking about but doing the steps from my initially comment should show this for you.

Not blocking on this bug for final ship. Would take a safe enough patch if one comes through.

I think there are 3 options to solve this. When clicking on Properties in the context menu in the Bookmarks sidebar: 1. Open the Library with the bookmark in question selected. 2. Open the star/"Edit this bookmark" dialog that drops down from the AwsomeBar. 3. Just include tags in the bookmark Properties modal. Considering that the main menu Bookmarks > Bookmark This Page does 2 (above) that seems like a nice solution. But since the Bookmarks sidebar is in a sense a light version of the Library, 1 (above) makes more sense. 3 (above), I think is the worst solution since there really is no need for a third way to edit a bookmark.

I wrote a mall extension which open "Edit This Bookmark" solution of No.2(Comment #12) https://addons.mozilla.org/ja/firefox/addon/7396

s/mall/small/

mconnor, which plans are out there for the old bookmarks properties dialog? Should it be replaced by the page bookmarked panel like it was done by Alice's extension?

(In reply to comment #16) > mconnor, which plans are out there for the old bookmarks properties dialog? > Should it be replaced by the page bookmarked panel like it was done by Alice's > extension? > Yes, it should. Making this bug target 3.1.

Fixed by the Ex Bookmark Properties addon. See also bug 426674

oops, forgot link: Ex Bookmark Properties Addon: https://addons.mozilla.org/en-US/firefox/addon/7396

I've made a patch to add a tag field into the Bookmark Properties dialog. The functionality was taken from the new add/edit bookmark overlay attached to the Location Bar.

Andrew, i tested your patch with Minefield/3.1b1pre ID:20080902033133 and it seems to be okay (tag creation, deletion). you may want to ask for (ui-)review ?!?

Comment on attachment 325882 [details] [diff] [review] Adds tagging to Bookmark Properties dialog Thanks, XtC4Uall. I've added the reviewers based on mconnor's suggestion from #developers.

Thanks for the patch Andrew, but I don't think we should be extending that dialog. Instead, we should be standardizing on the new bookmark properties panel, and removing the old one. I think that the dialog should embed the new panel, like the library does. This will reduce code and provide a consistent experience for users. I also don't like that there are two versions of bookmark properties UI available via primary UI that are so radically different in appearance, but that's an issue for another bug.

(In reply to comment #25) > ... we should be standardizing on the new bookmark properties > panel, and removing the old one. > I think that the dialog should embed the new panel, like the library does. anyone mind enlighten me about the "new" bookmark properties panel (mockup, bugreport, etc.)? > I also don't like that there are two versions of bookmark properties UI > available via primary UI that are so radically different in appearance, but > that's an issue for another bug. are there any bulletproof plans in 3.1 timeframe for this? if not, i'd say get this in before there's nothing at all.

a first take on this, really wip, edit works, add is correct but does not work (code missing for now), and this still needs A LOT of cleanup. The biggest issue is that editBookmarkOverlay does not support creation, but with some fix here and there it should work, so we would have tag autocomplete and multiedit for free.

(In reply to comment #26) > (In reply to comment #25) > > ... we should be standardizing on the new bookmark properties > > panel, and removing the old one. > > > I think that the dialog should embed the new panel, like the library does. > > anyone mind enlighten me about the "new" bookmark properties panel (mockup, > bugreport, etc.)? i think he's talking about the editItemOverlay that is used in the star panel and in the Library... we can embed it into the dialog, so we have only one code to manage for all the panels... My patch tries to make that, solving the problem with the "auto apply" behaviour that is not correct for an OK,CANCEL dialog, and the creation case. It takes away a lot of duplicated code but it's still quite dirt, since that code has to manage many different cases. Ideally we should try to unify all panels under a common code, so adding a feature to that code would add to all panels (see tag autocomplete, multiple edit and so on). > > I also don't like that there are two versions of bookmark properties UI > > available via primary UI that are so radically different in appearance, but > > that's an issue for another bug. Dietrich this is something we should do later imho, before we should try to use the overlayCode in all dialogs, and then try to unify the "styling", hwv i think that having a dialog style panel is not so bad, especially on Windows, the star panel as it is (borderless and opened under the locationbar) is good until is used from the star of from Bookmark this page menuitem.

i've tried to reduce a bit the patch moving functions to their original position and restoring some cleanup change that is unneeded actually, can be done later. this works for both edit and add, and should provide dialogs quite similar to previous ones (so this will mostly not need an UI review since the only visible change will be the added tags field that is wanted and a more consistent disposition of elements like in other panels). The only issue i'm aware of actually is the tags selector that is not in sync when adding a new element, will fix next wip. taking for now since this appear working

and "new folder" does not work correctly for now.

fixed previously reported bugs. todo: fill up hidden elements, support last used folder, global testing, cleanup reminder: followup to support tag multiediting when doing bookmark all tabs

Marco, that's great. When your patch will be ready for testing just create a try server build and we will have a test-run.

a panels list for future testing, this is where we are using the bookmark properties panel: 1. history sidebar, right click and choose Bookmark this link, should show name (filled) and folder picker 2. places views context menu New Bookmark, should show name, location, tags, keyword, description, loadinsidebar 3. places views context menu New Folder, should show name(filled), description 4. go to a livemark, subscribe using live bookmarks, should show name (filled) and folder picker 5. right click on a search field, Add keyword for this search, should show name, keyword, folder picker 6. add a sidebar panel on http://www.google.com/mozilla/google-search.html, should show name (filled) and folder picker 7. click on a panel add link with rel="sidebar" (tested on http://tntluoma.com/sidebars/ Add links) should show name (filled) and folder picker 8. add bookmarks button through customize toolbars, drag & drop a link upon it, should show name (filled) and folder picker 9. right click a bookmark/folder/livemark and choose properties, should show correct editable fields based on item type, with correct content clearly all panels should work saving correct data

10. open 2 tabs, from bookmarks menu choose Bookmark all tabs, should show name (filled) and folder picker

filed enh request bug 458698 - when doing Bookmark All Tabs allow for tags multiediting

for tests 6. and 7. the bookmark should open in sidebar when clicked and when editing load in sidebar should be checked

asking a first-pass review to start cleaning-up this thing. I've done the above tests and they appear correct, however after this review i'll generate a trybuild for QA testing.

Comment on attachment 341937 [details] [diff] [review] patch v1 > * item. > * @ defaultInsertionPoint (InsertionPoint JS object) - optional, the > * default insertion point for the new item. > * @ keyword (String) - optional, the default keyword for the new item. > * @ postData (String) - optional, POST data to accompany the keyword. >+* @ charSet (String) - optional, character-set to accompany the keyword. nit: indent > if ("title" in dialogInfo) >- this._itemTitle = dialogInfo.title; >+ this._title = dialogInfo.title; > if ("defaultInsertionPoint" in dialogInfo) > this._defaultInsertionPoint = dialogInfo.defaultInsertionPoint; > else { > // default to the bookmarks root nit: please update this comment while you're there > this._defaultInsertionPoint = >- new InsertionPoint(PlacesUtils.bookmarksMenuFolderId, -1); >+ new InsertionPoint(PlacesUtils.unfiledBookmarksFolderId, -1, >+ Ci.nsITreeView.DROP_ON); > } why this change? this would silently change behavior, acting against user expectations. >+ // Listen on uri fields to enable accept button if input is valid >+ var inputListener = { >+ _self: this, >+ handleEvent: function(event) { >+ document.documentElement.getButton("accept").disabled = >+ !this._self._inputIsValid(); >+ } >+ }; >+ if (this._itemType == BOOKMARK_ITEM) { >+ this._element("locationField") >+ .addEventListener("input", inputListener, false); >+ } >+ if (this._itemType == LIVEMARK_CONTAINER) { nit: else? > _showHideRows: function EIO__showHideRows() { > var isBookmark = this._itemId != -1 && > this._itemType == Ci.nsINavBookmarksService.TYPE_BOOKMARK; > var isQuery = false; > if (this._uri) > isQuery = this._uri.schemeIs("place"); > >- this._element("nameRow").collapsed = this._hiddenRows.indexOf("name") != -1; >+ // in insertMode hiddenRows is the only valid condition >+ this._element("nameRow").collapsed = >+ this._hiddenRows.indexOf("name") != -1; > this._element("folderRow").collapsed = >+ this._insertMode ? this._hiddenRows.indexOf("folderPicker") != -1 : > this._hiddenRows.indexOf("folderPicker") != -1 || this._readOnly; >- >- this._element("tagsRow").collapsed = !this._uri || >- this._hiddenRows.indexOf("tags") != -1 || isQuery; >+ this._element("tagsRow").collapsed = >+ this._insertMode ? this._hiddenRows.indexOf("tags") != -1 : >+ this._hiddenRows.indexOf("tags") != -1 || !this._uri || isQuery; > this._element("descriptionRow").collapsed = >+ this._insertMode ? this._hiddenRows.indexOf("description") != -1 : > this._hiddenRows.indexOf("description") != -1 || this._readOnly; >- this._element("keywordRow").collapsed = !isBookmark || this._readOnly || >- this._hiddenRows.indexOf("keyword") != -1 || isQuery; >- this._element("locationRow").collapsed = !isBookmark || isQuery || >- this._hiddenRows.indexOf("location") != -1; >- this._element("loadInSidebarCheckbox").collapsed = !isBookmark || isQuery || >- this._readOnly || this._hiddenRows.indexOf("loadInSidebar") != -1; >- this._element("feedLocationRow").collapsed = !this._isLivemark || >- this._hiddenRows.indexOf("feedLocation") != -1; >- this._element("siteLocationRow").collapsed = !this._isLivemark || >- this._hiddenRows.indexOf("siteLocation") != -1; >+ this._element("keywordRow").collapsed = >+ this._insertMode ? this._hiddenRows.indexOf("keyword") != -1 : >+ this._hiddenRows.indexOf("keyword") != -1 || !isBookmark || >+ this._readOnly || isQuery; >+ this._element("locationRow").collapsed = >+ this._insertMode ? this._hiddenRows.indexOf("location") != -1 : >+ this._hiddenRows.indexOf("location") != -1 || !isBookmark || isQuery; >+ this._element("loadInSidebarCheckbox").collapsed = >+ this._insertMode ? this._hiddenRows.indexOf("loadInSidebar") != -1 : >+ this._hiddenRows.indexOf("loadInSidebar") != -1 || !isBookmark || >+ isQuery || this._readOnly; >+ this._element("feedLocationRow").collapsed = >+ this._insertMode ? this._hiddenRows.indexOf("feedLocation") != -1 : >+ this._hiddenRows.indexOf("feedLocation") != -1 || !this._isLivemark; >+ this._element("siteLocationRow").collapsed = >+ this._insertMode ? this._hiddenRows.indexOf("siteLocation") != -1 : >+ this._hiddenRows.indexOf("siteLocation") != -1 || !this._isLivemark; > this._element("selectionCount").hidden = !this._multiEdit; madness. please just cache isHidden for each row or something, instead of the ternary-to-OR-to-OR. > var itemToSelect = userEnteredNameField; > try { >- if (this._itemId != -1 && >- this._itemType == Ci.nsINavBookmarksService.TYPE_BOOKMARK && >- !this._readOnly) >+ if ((this._insertMode && this._uri) || >+ (this._itemId != -1 && >+ this._itemType == Ci.nsINavBookmarksService.TYPE_BOOKMARK && >+ !this._readOnly)) > this._microsummaries = PlacesUIUtils.microsummaries > .getMicrosummaries(this._uri, -1); make a local var for part of that |if| clause, this is a readability mess. >+ /** >+ * Save actual changes. >+ * This method is valid only if instantApply is false. >+ */ >+ saveChanges: function EIO_saveChanges() { >+ if (this._instantApply) >+ return false; >+ if (this._insertMode) { // new item >+ this._createNewItem(); >+ } >+ else { // edit >+ this._updateElementIfVisible("namePicker"); >+ this._updateElementIfVisible("tagsField"); >+ this._updateElementIfVisible("descriptionField"); >+ this._updateElementIfVisible("keywordField"); >+ this._updateElementIfVisible("locationField"); >+ this._updateElementIfVisible("feedLocationField"); >+ this._updateElementIfVisible("siteLocationField"); >+ this._updateElementIfVisible("loadInSidebarCheckbox"); >+ } >+ return true; >+ }, >+ >+ /** >+ * Helper to save changes in edit mode. >+ * This method is valid only if we are not in insertMode. >+ */ >+ _updateElementIfVisible: function EIO__updateElementIfVisible(aElementId) { >+ if (this._insertMode) >+ return; >+ this._instantApply = true; why not just set this once in that block in saveChanges()? >@@ -491,32 +491,32 @@ var PlacesUIUtils = { > showMinimalAddBookmarkUI: > function PU_showMinimalAddBookmarkUI(aURI, aTitle, aDescription, > aDefaultInsertionPoint, aShowPicker, > aLoadInSidebar, aKeyword, aPostData, > aCharSet) { > var info = { > action: "add", > type: "bookmark", >- hiddenRows: ["location", "description", "loadInSidebar"] >+ hiddenRows: ["location", "tags", "description", "loadInSidebar"] > }; hm, i'd certainly like to be able to add tags there... r=me for first-pass. ask mano for final review.

(In reply to comment #38) > (From update of attachment 341937 [details] [diff] [review]) > >@@ -491,32 +491,32 @@ var PlacesUIUtils = { > > showMinimalAddBookmarkUI: > > function PU_showMinimalAddBookmarkUI(aURI, aTitle, aDescription, > > aDefaultInsertionPoint, aShowPicker, > > aLoadInSidebar, aKeyword, aPostData, > > aCharSet) { > > var info = { > > action: "add", > > type: "bookmark", > >- hiddenRows: ["location", "description", "loadInSidebar"] > >+ hiddenRows: ["location", "tags", "description", "loadInSidebar"] > > }; > > hm, i'd certainly like to be able to add tags there... for now i want to obtain dialog parity with actual version, adding tags field to additional dialogs (for exaple minimal UI ones) will be easy to do in tiny followups.

addressed dietrich's comments, passing to Mano since editBookmarkOverlay is mostly his work.

>I also don't like that there are two versions of bookmark properties UI >available via primary UI that are so radically different in appearance, but >that's an issue for another bug. I've filed follow up bug 459958 to determine what we should do in all of the different instances of the bookmarks property dialog.

first take on the new implementation, instant apply inside a dialog. As requested by Mano i've tried to touch editItemOverlay at a minimum: - i changed order of the roots to be consistent with the order we have all around in the app (toolbar, menu, unsorted) - moved icon styling to editBookmarkOverlay.css to be used in all dialogs (and in the menulist). - overriden method _showHideRows(). - added a string for New Folder to editBookmarkOverlay.dtd fixes also: bug 433484, Bug 432848 this requires string changes so it would be better landing before the string freeze on 30th. Due to this maybe we could take this as a first step and fix styling and fields later in bug 459958, unless i can get final specifications from Alex in a couple days. Mano if you agree with the new approach used in this patch but have too many reviews in your queue, you could do a first pass and then i could ask final review to Dietrich (if he has some time slot).

Comment on attachment 344262 [details] [diff] [review] patch mh i've fixed another couple things, the override is useless, new version coming

new in this patch: - removed useless overridde - don't show folderpicker when editing - don't ask for tags when adding a keyword We should practically be at the same visual point as actual version, plus the tags field and the dialog when choosing Bookmark this link in the content.

trybuilds here: https://build.mozilla.org/tryserver-builds/2008-10-22_04:55-mak77@bonardo.net-try-37e064e858a/

when this will land it will need some testing from QA to ensure all dialogs are sanely converted to the new method

Yep, I'll review the completeness of litmus coverage here. Update and/or add test cases as need then plus this when it's complete. thanks Marco

This implements all requests from Alex in bug 459958, final work could require some polish on Linux or Mac, i've actually setup a linux build env to test and eventually fix styling, however this polish can be done in bug 459958 before the code freeze, i would prefer taking this before the string freeze instead. Since Mano's reviews queue is quite long and i don't touch anymore his work in editBookmarkOverlay, i'm moving review request to Dietrich.

fixes a couple styling issues on Linux and Windows

The windows zip try server build in comment 45 does not appear to fix bug 432848 as suggested in comment 42. On XP I am seeing the bookmarks menu icon used even when the unsorted bookmarks or bookmarks toolbar option is selected.

(In reply to comment #50) > The windows zip try server build in comment 45 does not appear to fix bug > 432848 > as suggested in comment 42. On XP I am seeing the bookmarks menu icon used even > when the unsorted bookmarks or bookmarks toolbar option is selected. thanks for reporting, will look into it, i was seeing this working

fixes icons in menulist (for real this time)

Comment on attachment 345419 [details] [diff] [review] patch 1.4 > bookmarkLink: function PCH_bookmarkLink(aParent, aURL, aTitle) { > var linkURI = makeURI(aURL); > var itemId = PlacesUtils.getMostRecentBookmarkForURI(linkURI); >- if (itemId == -1) { >- StarUI.beginBatch(); >- var txn = PlacesUIUtils.ptm.createItem(linkURI, aParent, -1, aTitle); >- PlacesUIUtils.ptm.doTransaction(txn); >- itemId = PlacesUtils.getMostRecentBookmarkForURI(linkURI); >+ if (itemId == -1) >+ PlacesUIUtils.showMinimalAddBookmarkUI(linkURI, aTitle); >+ else { >+ PlacesUIUtils.showItemProperties(itemId, >+ PlacesUtils.bookmarks.TYPE_BOOKMARK); > } can move linkURI into the |if| block >+ // Load In Sidebar >+ this._loadInSidebar = PlacesUtils.annotations >+ .itemHasAnnotation(this._itemId, >+ LOAD_IN_SIDEBAR_ANNO); >+ break; hrm, would be nice to someday centralize all internal anno usage like this in get/set* methods in PlacesUtils. >+ // When collapsable elements change their collapsed attribute we must spelling nit: collapsible > _containsValidURI: function BPP__containsValidURI(aTextboxID) { > try { >- var value = this._element(aTextboxID).value; >+ let value = this._element(aTextboxID).value; > if (value) { >- var uri = PlacesUIUtils.createFixedURI(value); >+ let uri = PlacesUIUtils.createFixedURI(value); > return true; > } is there a practical benefit to this? otherwise it muddies blame unnecessarily. > _getDescriptionAnnotation: > function BPP__getDescriptionAnnotation(aDescription) { >- var anno = { name: DESCRIPTION_ANNO, >+ let anno = { name: DESCRIPTION_ANNO, ditto, and all others >+ //XXX TODO: this should be in a transaction! > if (this._charSet) > PlacesUtils.history.setCharsetForURI(this._uri, this._charSet); file followup? >+ >+ // Set a selectedIndex attribute to show special icons >+ this._folderMenuList.setAttribute("selectedIndex", >+ this._folderMenuList.selectedIndex); this updates all the special icons for the static items? >diff --git a/browser/locales/en-US/chrome/browser/places/bookmarkProperties.properties b/browser/locales/en-US/chrome/browser/places/bookmarkProperties.properties >--- a/browser/locales/en-US/chrome/browser/places/bookmarkProperties.properties >+++ b/browser/locales/en-US/chrome/browser/places/bookmarkProperties.properties >@@ -1,13 +1,15 @@ dialogAcceptLabelAddItem=Add > dialogAcceptLabelAddItem=Add >+dialogAcceptLabelSaveItem=Save >+dialogAcceptLabelAddLivemark=Subscribe > dialogAcceptLabelAddMulti=Add Bookmarks >-dialogAcceptLabelEdit=Save Changes >-dialogTitleAddBookmark=Add Bookmark >-dialogTitleAddLivemark=Add Live Bookmark >-dialogTitleAddFolder=Add Folder >-dialogTitleAddMulti=Bookmark All Tabs >+dialogAcceptLabelEdit=Save >+dialogTitleAddBookmark=New Bookmark >+dialogTitleAddLivemark=Subscribe to Live Bookmark >+dialogTitleAddFolder=New Folder >+dialogTitleAddMulti=New Bookmarks Hm, you "subscribe" to a feed, not to a Live Bookmark. Maybe "New Live Bookmark" for consistency w/ the others? Also, ask Faaborg or Beltzner for ui-r for these string changes. r=me w/ these changes

(In reply to comment #53) > >- var uri = PlacesUIUtils.createFixedURI(value); > >+ let uri = PlacesUIUtils.createFixedURI(value); > > return true; > > } > > is there a practical benefit to this? otherwise it muddies blame unnecessarily. i don't want to mixup styles, so i've preferred moving all to one common, could be let or var. So maybe you're right and i'll move all to var to not pollute blame. > >+ //XXX TODO: this should be in a transaction! > > if (this._charSet) > > PlacesUtils.history.setCharsetForURI(this._uri, this._charSet); > > file followup? filed bug 462310 > > >+ > >+ // Set a selectedIndex attribute to show special icons > >+ this._folderMenuList.setAttribute("selectedIndex", > >+ this._folderMenuList.selectedIndex); > > this updates all the special icons for the static items? this allows to recognize different icons in the stylesheet, since we know the position of special items (menu/toolbar/unfiled we can style based on the position. This is actually used to style those 3 items > > >diff --git a/browser/locales/en-US/chrome/browser/places/bookmarkProperties.properties b/browser/locales/en-US/chrome/browser/places/bookmarkProperties.properties > >--- a/browser/locales/en-US/chrome/browser/places/bookmarkProperties.properties > >+++ b/browser/locales/en-US/chrome/browser/places/bookmarkProperties.properties > >@@ -1,13 +1,15 @@ dialogAcceptLabelAddItem=Add > > dialogAcceptLabelAddItem=Add > >+dialogAcceptLabelSaveItem=Save > >+dialogAcceptLabelAddLivemark=Subscribe > > dialogAcceptLabelAddMulti=Add Bookmarks > >-dialogAcceptLabelEdit=Save Changes > >-dialogTitleAddBookmark=Add Bookmark > >-dialogTitleAddLivemark=Add Live Bookmark > >-dialogTitleAddFolder=Add Folder > >-dialogTitleAddMulti=Bookmark All Tabs > >+dialogAcceptLabelEdit=Save > >+dialogTitleAddBookmark=New Bookmark > >+dialogTitleAddLivemark=Subscribe to Live Bookmark > >+dialogTitleAddFolder=New Folder > >+dialogTitleAddMulti=New Bookmarks > > Hm, you "subscribe" to a feed, not to a Live Bookmark. Maybe "New Live > Bookmark" for consistency w/ the others? > > Also, ask Faaborg or Beltzner for ui-r for these string changes. These string changes have been required by Faaborg himself in https://bugzilla.mozilla.org/show_bug.cgi?id=459958#c24, the idea is to make buttons and titles closer to the actual action, since in the dialog you see the feed name Subscribe does not sound badly, so the user knows he is continuing the subscribe action he has started some second before.

(In reply to comment #53) > >+ // Load In Sidebar > >+ this._loadInSidebar = PlacesUtils.annotations > >+ .itemHasAnnotation(this._itemId, > >+ LOAD_IN_SIDEBAR_ANNO); > >+ break; > > hrm, would be nice to someday centralize all internal anno usage like this in > get/set* methods in PlacesUtils. do you mean having PlacesUtils.getLoadInSidebar() so all const can be defined in one common place? that could be nice yes

When expanding the tag box into a list view to show all tags it is not aligned with the other boxes in the dialog. Will this be covered by Bug 413053 or is it a separate piece of work?

(In reply to comment #56) > When expanding the tag box into a list view to show all tags it is not aligned > with the other boxes in the dialog. Will this be covered by Bug 413053 or is it > a separate piece of work? Thanks for following this work, Bug 413053 is for align those views, won't be done here.

So to be clear will the alignment of the tree & list views in this bookmark properties dialog be covered in this bug or should a new one be filed?

(In reply to comment #58) > So to be clear will the alignment of the tree & list views in this bookmark > properties dialog be covered in this bug or should a new one be filed? to be clear, will be done IN Bug 413053, so no need to file a new one.

(In reply to comment #53) > (From update of attachment 345419 [details] [diff] [review]) > > > bookmarkLink: function PCH_bookmarkLink(aParent, aURL, aTitle) { > > var linkURI = makeURI(aURL); > > var itemId = PlacesUtils.getMostRecentBookmarkForURI(linkURI); > >- if (itemId == -1) { > >- StarUI.beginBatch(); > >- var txn = PlacesUIUtils.ptm.createItem(linkURI, aParent, -1, aTitle); > >- PlacesUIUtils.ptm.doTransaction(txn); > >- itemId = PlacesUtils.getMostRecentBookmarkForURI(linkURI); > >+ if (itemId == -1) > >+ PlacesUIUtils.showMinimalAddBookmarkUI(linkURI, aTitle); > >+ else { > >+ PlacesUIUtils.showItemProperties(itemId, > >+ PlacesUtils.bookmarks.TYPE_BOOKMARK); > > } > > can move linkURI into the |if| block this confused me too at a first read, but i can't since getMostRecentBookmarkForURI is using it

addressed comments

* editBookmarkOverlay.newFolderButton.accesskey and editBookmarkOverlay.name.accesskey share the same value: is it ok (different scope)? * "Subscribe to Live Bookmark": AFAIK in Firefox you can subscribe to a feed using Live Bookmarks (see for example subscribeFeedUsing="Subscribe to this feed using" in subscribe.properties), the action of subscribing to a Live Bookmark seems a bit "strained"

(In reply to comment #63) > * editBookmarkOverlay.newFolderButton.accesskey and > editBookmarkOverlay.name.accesskey share the same value: is it ok (different > scope)? thanks for this, has to be fixed, Faaborg, what do you think would be better to assign here? > * "Subscribe to Live Bookmark": AFAIK in Firefox you can subscribe to a feed > using Live Bookmarks (see for example subscribeFeedUsing="Subscribe to this > feed using" in subscribe.properties), the action of subscribing to a Live > Bookmark seems a bit "strained" This was expressely asked to both Faaborg and Beltzner, they think it's ok, hwv it's their choice to change it.

Comment on attachment 345470 [details] [diff] [review] patch 1.5 ui-r+ with the following nit/change as per localizer comments: +dialogAcceptLabelAddLivemark=Subscribe +dialogTitleAddLivemark=Subscribe with Live Bookmark Quite right that you don't subscribe to the Live Bookmark; you subscribe to the feed WITH a Live Bookmark. Alex's design was right, though, to carry the verb that we use elsewhere in the UI forward into this dialog, though.

Followup to fix locale as required in previous comment by Beltzner, accesskey for New Folder is o per IRC.

followup pushed: http://hg.mozilla.org/mozilla-central/rev/1ed0ef198437

verified in nightly build of 2008103102 on Mac also updated litmus test case: https://litmus.mozilla.org/show_test.cgi?id=5952

Doing the string change without a key change is borderline. You may read the original string as obviously broken or not, and either localizers would catch that magically or not. Mind doing a quick post to mozilla.dev.l10n to make sure that localizers see that there is an update to an existing string, -dialogTitleAddLivemark=Subscribe to Live Bookmark +dialogTitleAddLivemark=Subscribe with Live Bookmark in browser/places/bookmarkProperties.properties. CCing our l10n watcher to get some bugmail noise out, too.

... and earlier changes to that file, too. Dietrich, patches like that should get an r-.

i posted in mozilla.dev.l10n, really sorry for that Axel, was my fault :(

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