Closed Bug 230349 Opened 21 years ago Closed 21 years ago

Adding multiple history entries to bookmarks fails and context menu reads "Bookmark this Page(L)"

Categories

(Core Graveyard :: History: Global, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: durbacher, Assigned: durbacher)

Details

Attachments

(1 file, 4 obsolete files)

User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6b) Gecko/20031209 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6b) Gecko/20031209 As summary says... Reproducible: Always Steps to Reproduce: 1. Open bookmarks window (e.g. Ctrl-H) 2. Select two or more history entries (but no 'folder'!) 3. Right-click 4. Read label of context menu Actual Results: "Bookmark this Page(L)" Expected Results: "Add Links to Bookmarks" (or similar) The label is taken from http://lxr.mozilla.org/seamonkey/source/xpfe/communicator/resources/locale/en-US/contentAreaCommands.dtd#18 which earlier said "Add to Bookmarks", but was changed: http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/xpfe/communicator/resources/locale/en-US&command=DIFF_FRAMESET&file=contentAreaCommands.dtd&rev2=1.15&rev1=1.14 probably without this usage in mind... Shortcut letter is "L" which makes it look even more strange... therefore I suggested something with "Link" in it. This should probably go into history's own files (I cannot think of any other place where you can bookmark two URLs at once...).
Attached patch proposed patch (obsolete) — Splinter Review
This is my proposal how to fix this bug: Put a new label/accesskey for bookmarking more than one selected history entries into the file historyTreeOverlay.dtd. That file only contains tree labels until now, but I guess it's the best place for the new label because it is used in historyTreeOverlay.xul. And only there because only in history there can be several URLs bookmarked at once. The other possibility would have been to put it where it's "brother" entry for bookmarking _single_ history entries is: http://lxr.mozilla.org/seamonkey/source/xpfe/communicator/resources/locale/en-US/contentAreaCommands.dtd#53 If this is considered better, I would change the patch to do so. This would allow for other users of that string if there ever are some... This patch also fixed the accesskey: previously only the label was changed, the accesskey remained the same: "L" (this is why there is a "L" in brackets after the context menu entry). With my patch it is still "L", but now there is an "L" in the entry so it looks normal. So it is now switched from "L" to "L" all the time. This is obviously not necessary at the moment but allows for changes in the future without glitches as seen in this bug.
Assignee: nobody → durbacher
Status: NEW → ASSIGNED
Comment on attachment 138658 [details] [diff] [review] proposed patch Requesting r= from Neil and sr= from alecf. If you test this and encounter problems in the context menu it's probably covered by my patch in bug 133590 or bug 131182 or several other ones (there are lots of problems there...), so this patch is really most probably not the culprit. Don't be reluctant to say it if you'd like me to change my patch in some way.
Attachment #138658 - Flags: superreview?(alecf)
Attachment #138658 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 138658 [details] [diff] [review] proposed patch sr=alecf
Attachment #138658 - Flags: superreview?(alecf) → superreview+
Comment on attachment 138658 [details] [diff] [review] proposed patch Unfortunately adding multiple bookmarks doesn't actually work :-/
Right, but with this patch it looks nicer... ;-) (Did I mention that I also don't think I will ever use that feature?)
Adding multiple bookmarks fails in http://lxr.mozilla.org/seamonkey/source/xpfe/components/bookmarks/resources/bookmarks.js#1565 because BMSVC is not defined. The containing function addBookmark is called from history.js with aShowDialog=false so the bookmarks are added directly. When only adding one bookmark, aShowDialog is true and that dialog manages the adding, so this error does not occur. pch said on IRC that initServices() and initBMService() have to be used to init BMSVC properly. Neil mentioned that this might be overkill, but adding a single bookmark also does this before the "Add Bookmark" dialog appears. The solutions I currently see are: 1) use the "add Bookmark" dialog also for multiple bookmarks. Easiest solution, but hell for sombody who adds more than a dozen bookmarks at once. 2) init Bookmark Service with initServices() and initBMService() 3) try to find a smaller set of instructions to init BMSVC Extending bug summary.
Summary: History context menu reads "Bookmark this Page(L)" when multiple entries selected → Adding multiple history entries to bookmarks fails and context menu reads "Bookmark this Page(L)"
Severity: trivial → normal
Calling addBookmark with "undefined" as charset makes the called function use the charset of the currently open document: var fw = document.commandDispatcher.focusedWindow; aCharset = fw.document.characterSet; (http://lxr.mozilla.org/seamonkey/source/xpfe/components/bookmarks/resources/bookmarks.js#1553) - does this make any sense??? Wouldn't it make more sense to use a "mostly" working charset instead of one chosen by random? But I'm not sure what the charset of a bookmark is for, so this might all be nonsense...
Comment on attachment 138658 [details] [diff] [review] proposed patch First patch is obsolete. New patch(es) coming...
Attachment #138658 - Attachment is obsolete: true
Attachment #138658 - Flags: review?(neil.parkwaycc.co.uk)
v2 A: do the same as the first patch plus init the Bookmarks Service
v2 A: do the same as the last one plus several other changes: - as mentioned in comment #7, the charset of the currently open document - which happens to be "UTF-8" for the history window - is used for the added bookmarks. (Don't know about the sidebar case, but it certainly doesn't produce a better result) So I guess using that charset UTF-8 directly cannot hurt. I'm not sure of any possible problems when a bookmark has a wrong charset attached, but IF there are any, they already existed for"ever". http://lxr.mozilla.org/seamonkey/source/xpfe/components/bookmarks/src/nsBookmarksService.cpp#3226 is where that charset is finally being used, just in case anybody wants to have a look. - don't call the bookmarks.js init functions because they init a lot of stuff we don't need. Instead only init BMSVC and do the adding call to it directly. - when only one entry is selected to be added, now bookmarks.js cannot do anything for us, all it does is calling openDialog, so we can do this ourselves.
Comment on attachment 138753 [details] [diff] [review] Patch v2 B: do the same as the first one plus do several other things... Requesting Neil's opinion on the two possible patches. Please say what looks better to you. Combinations of these patches are also possible. E.g. I might leave alone the "one bookmark" case (maybe changing to UTF-8) but use the "multiple bookmarks" solution from the second patch. Or...
Attachment #138753 - Flags: review?(neil.parkwaycc.co.uk)
I think I'd like pch to comment on the use of BookmarkUtils.addBookmark (especially given the comment about aShowDialog) and also the charset issue.
As a note: firebird also uses "undefined" for charset: http://lxr.mozilla.org/mozilla/source/browser/components/history/content/history.js#230 But pch probably knows best what to do, so...
Sorry for the delay. - about the aShowDialog flag: addBookmarkImmediately is obsolete, because the modifications to the bookmarks datasource are not processed through the transaction manager. - the behavior of createBookmarks when the charset is obviously bogus, it was written for the case in which the page is bookmarked from the browser window, not from another window a sidebar panel. However, we need to infer a charset... We may look at the cache (if it has this info) but I don't know what to do (except relying on the automatic charset detection) by not asserting the charset in the bookmarks datasource. Another solution comes to my mind. Do we really need the ability to add a bookmark from the history??? I think that loading the page then bookmarking it is a good workaround, in the improbable case that the user hasn't already done that.
s/don't know what to do/don't know what to do when cache of the page is expired
OK, so two more questions... 1. Does Bookmarks/Bookmark This Page add a bookmark immediately correctly? 2. What charset does Bookmark Manager's File/New/Bookmark use?
1) adding a bookmark via Bookmarks > Bookmarks This Page go through BookmarksUtils.addBookmarkForBrowser that picks the charset of the doc shell but it's not correctly handled by the transaction manager. 2) adding a new bookmark exhibits the same pb that when adding a bookmark from the history. The null or "" charset is sent to BookmarksUtils.createBookmark that wrongly changes it to the add bookmark window one that turns out to be "UTF-8" for chrome windows. We should remove... if (!aCharSet) { var fw = document.commandDispatcher.focusedWindow; if (fw) aCharSet = fw.document.characterSet; } ... that is really bogus and check copy/paste and drag and drop (see bug 174197, that relies on the snipet) operations.
Comment on attachment 138753 [details] [diff] [review] Patch v2 B: do the same as the first one plus do several other things... I think I would stick to using initBMService and addBookmark for now, but with a third parameter of null rather than undefined as I think that any implicit or explicit UTF-8 charset would be wrong. I also don't see why you need both init calls, and definitely not inside the loop!
Attachment #138753 - Flags: review?(neil.parkwaycc.co.uk) → review-
Attached patch revised patch (obsolete) — Splinter Review
Both init calls are necessary because the first one inits some variables used by the second (namely e.g. RDF). And the second inits the service.
Attachment #138752 - Attachment is obsolete: true
Attachment #138753 - Attachment is obsolete: true
Comment on attachment 139381 [details] [diff] [review] revised patch Requesting r= from Neil.
Attachment #139381 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 139381 [details] [diff] [review] revised patch >- BookmarksUtils.addBookmark(url, title, undefined, true); >+ BookmarksUtils.addBookmark(url, title, null, true); You need to do this for the multiple bookmark case too. >+ initServices(); >+ initBMService(); Might be worth wrapping this in an if (!BMSVC)
Attachment #139381 - Flags: review?(neil.parkwaycc.co.uk) → review-
Attached patch again revisedSplinter Review
Changes those two things. BMSVC is only _not_ null if you already have bookmarked several history items during the lifetime of the currently open history window. No matter what other bookmarks activities are going on. But yes, it's probably still worth it.
Attachment #139381 - Attachment is obsolete: true
Comment on attachment 139419 [details] [diff] [review] again revised Requesting r= from Neil.
Attachment #139419 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #139419 - Flags: review?(neil.parkwaycc.co.uk) → review+
Comment on attachment 139419 [details] [diff] [review] again revised Requesting sr= from alecf.
Attachment #139419 - Flags: superreview?(alecf)
Comment on attachment 139419 [details] [diff] [review] again revised sr=alecf
Attachment #139419 - Flags: superreview?(alecf) → superreview+
checked in
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: