when warning on "opening too many tabs", only count bookmarks (and not folders) c.ascheberg@gmx.de writes in bug #333734: "The shown number of tabs that are about to be opened seems to be wrong. When I middle-click click a folder that has subfolders, these subfolders are counted as pages. So it happens that I am warned that 16 tabs will be opened although there are only 10 or so pages." thanks c.ascheberg@gmx.de for catching this.

I've also noticed that the count is wrong when I delete some bookmarks from a folder, and then in the same session try to open the folder in tabs. I'm guessing (forgive my lack of knowledge about the bookmarks code) that this is because the items are marked as "hidden", so that the undo feature works, and the "open in tabs" code doesn't take this "hidden" attribute into account when counting the number of bookmarks to be opened.

> the count is wrong when I delete some bookmarks thanks for reporting this gavin. I've updated the summary and I'll check out this scenario.

I'd like to fix this for 2.0, since it's a bug on something new to 2.0. seeking blocking-firefox2.

I have a patch for this for bon echo (but I still need to reproduce the "deleted" case from gavin.) working on one for the trunk too.

oops, I forgot another place in the bon echo version that calls _confirm..()

Comment on attachment 230215 [details] [diff] [review] trunk fix for places r=ben@mozilla.org, but use preincrement in both places instead of postincrement

this has r=ben

fixed on the trunk. I still have to finish porting this back to branch (non-places)

fixed comment: s/multiple tags/mutliple bookmarks

Comment on attachment 230239 [details] [diff] [review] branch fix for RDF bookmarks code >Index: components/bookmarks/content/bookmarks.js >+ // we can't just use |RDFC.GetCount()| as that might include >+ // folders, separators, deleted bookmarks, etc. >+ while (containerChildren.hasMoreElements()) { >+ var res = containerChildren.getNext().QueryInterface(kRDFRSCIID); >+ if (BMDS.GetTarget(res, urlArc, true)) >+ numTabsToOpen++; >+ } I don't know that you want to use the "URL" arc as an identifier of bookmarkiness. There's a type arc too, you should use that.

(Reason is: in the RDF bookmarks code, any resource can have a URL property, even folders and notes that become folders thanks to composite datasources, so this is not a reliable check.)

ben, thanks for the info. I'll look into it. FWIW: I used urlArc because of this code: var target = BMDS.GetTarget(res, urlArc, true); if (target) { var uri = target.QueryInterface(kRDFLITIID).Value; if (index < tabCount) tabPanels[index].loadURI(uri); upon further testing, something is wrong in my patch, as I appear to have broken "Open in Tabs"! (YIKES)

> upon further testing, something is wrong in my patch, as I appear to have > broken "Open in Tabs"! (YIKES) I just need to reset the containerChildren enumerator, and that fixes it. on to ben's comment...

> (Reason is: in the RDF bookmarks code, any resource can have a URL property, > even folders and notes that become folders thanks to composite datasources, so > this is not a reliable check.) per Ben, I've logged a spin off bug on doing some code cleanup on some existing code. see bug #345894

Comment on attachment 230511 [details] [diff] [review] branch fix for RDF bookmarks code, v2 r=ben@mozilla.org Sorry for not marking this sooner!

Comment on attachment 230511 [details] [diff] [review] branch fix for RDF bookmarks code, v2 as the trunk uses places, this would be a branch only fix. seeking approval.

I've landed the branch fix on the branch and the trunk (to keep the two in sync). gavin: "I've also noticed that the count is wrong when I delete some bookmarks" I was not able to reproduce that. Can you try again (with a fresh trunk / branch) and let me know if you are still seeing it?

(In reply to comment #20) > gavin: "I've also noticed that the count is wrong when I delete some > bookmarks" > > I was not able to reproduce that. > > Can you try again (with a fresh trunk / branch) and let me know if you are > still seeing it? Yeah, I still see it with today's branch build. I filed bug 346193.

> Yeah, I still see it with today's branch build. I filed bug 346193. thanks gavin, I'll investigate. morphing the subject of this bug to just cover separators and folders, and not deleted bookmarks.