Closed Bug 730829 Opened 12 years ago Closed 9 years ago

Restore "Open all in tabs" to the bottom of livemarks popups (was: "Open all in tabs" missing after bug 613588)

Categories

(Firefox :: Bookmarks & History, defect)

Product:
Firefox
Component:
Bookmarks & History
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 42
Tracking Flags:
Tracking Status
firefox41 --- fixed
firefox42 --- fixed

People

(Reporter: pablo, Assigned: jorgk-bmo, Mentored)

References

Details

(Whiteboard: [lang=js])

Attachments

(3 files, 6 obsolete files)

Screenshot to calrify what we're talking about
40.22 KB, image/png
Details
console output
4.29 KB, text/plain
Details
Proposed patch with more nits fixed
4.41 KB, patch
mak
: review+
Gijs
: feedback+
ritu
: approval-mozilla-aurora+
Details | Diff | Splinter Review 
Since bug 613588 "Open all in tabs" is missing from livemarks menu. Also middle click is not working.
Blocks: livemarksIO
when you use this functionality, do you actually need to open ALL articles or just some of them?
Since I'm under the impression that fixing bug 260611 would bring much better functionality to livemarks than opening each single article.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Given the new implementation, I suggest "wontfix". Having 30 tabs or so getting opened "some time later" isn't great at all, so is having it working "sometimes" (after first time the menu is opened...).  The menuitem should be hidden though.
(In reply to Marco Bonardo [:mak] from comment #1)
> when you use this functionality, do you actually need to open ALL articles
> or just some of them?
> Since I'm under the impression that fixing bug 260611 would bring much
> better functionality to livemarks than opening each single article.

That depends on the time of the day (at least in my case).
Imagine a news feed, in the morning I'd prefer opening all news you missed during the night (middle click, open all), but during the day, you just want to see the updates so bug 260611 would be great.
Also, menu "Open unread in tabs" would be the best for me.
(In reply to Pablo Greco from comment #3)
> Also, menu "Open unread in tabs" would be the best for me.

This would indeed be handy, though I wonder if it wouldn't better suited for an add-on.
I also read all news in the morning, though when I see there is a lot of news, I end up just clicking on the "Open <site>" link, since it's easier to handle than 20 tabs. While for updates I do the same as you (so as well I would appreciate bug 260611).
I absolutely want ALL tabs to be opened. And I want them to start loading right away (sidestepping the new defer-load feature of Firefox). So PLEASE restore this functionality. 

I point a bunch of tabs at my own Pinboard.im feeds (formerly my Delicious feeds). They only rarely change. I have a bunch of things I want to read in the morning, and I want them all to preload and be ready for me to read. 

I've been doing it this way for years, and the new Firefox breaks this use case. There aren't "30 tabs" opening when I do this; just the dozen or so I want to open. And no, I don't want to go through and click a dozen times when I really just want them all to open quickly.

And no, I don't want to create a set of bookmark folders that only Firefox knows about. I can use Firefox sync to sync that with other Firefox instances, but I can't sync with Chrome or get access to the links from the web. Having a set of folders in Firefox and another set in Chrome and another on my mobile device is a violation of DRY (Don't Repeat Yourself). Having them all in one place on a cross-browser service is much cleaner for me. So please fix.
By some mystery I somehow failed to update form FF 10 until yesterday. Now I'm on FF13 without option to open a bunch of bookmarks or RSS feeds in tabs.  :-( I use this functionality for browsing new replays from several forums and also opening a bunch of links that I daily use for translating. I have created one folder for all the stuff I need when translating - links to thesaurus, dictionary, list of common translations and links on launchpad. Now I have to click a bunch of bookmarks manually and they are not easy to get to..  Hope this bug gets higher priority than normal. Or changes to browser preference will have to be made..... which sux for me..
Do you think that an "Open unread entries in tabs" could be more useful that the old plain "Open All In Tabs" or would it still not cover your needs?
"Open unread entries in tabs" would be broken for my use case. In my case, the entries are all the same, every day (front page of news sites, comics, etc.) and I want to open them all, every day (even though their RSS entries have all be "read" before), because the pages themselves change every day.

If you're talking about "unread" meaning "the page hasn't changed" (and you'd ping the page for its current state)...that COULD work, but honestly it sounds hard to get right for all possible pages, so it would probably be important to have an "Open All In Tabs" option anyway in case it didn't work for a particular user.
(In reply to Marco Bonardo [:mak] from comment #8)
> Do you think that an "Open unread entries in tabs" could be more useful that
> the old plain "Open All In Tabs" or would it still not cover your needs?

If you were referring to Primoz's post, then it wouldn't work for him at all. He says:

>I have created one folder for all the stuff I need when translating - links to thesaurus, dictionary, list of common translations and links on launchpad.

Many of these would be completely unchanged each day, so "Open Unread Entries In Tabs" would fail for him after the first time.

Please just restore the functionality that we've been relying on for years.
Open unread entries would be perfect for my personal purposes BUT the inconvenience to the poster's above situations outweigh the inconvenience I would have just by having 'open all tabs' added back to FF. I think it should just be added back as was, though if there were an about:config option to change the behaviour to 'open only unread tabs' that would be perfect. Why please half the people if you can please them all :-)
Yes, I do want to be able to open all in tabs. I use live bookmarks in coordination with google bookmarks to provide grouped synced bookmarks without any additional services or extensions, and I use particular tags to give me groupings I need at one time. You can do similar things with other bookmarking services - they provide RSS per tag/folder for a reason. RSS is multifaceted and not only used for articles, so to assume people only want to open new items is presumptuous in regards to how they are utilizing the tool. Please put this feature back, it wasn\\\'t hurting anything. Live bookmarks are just a synced folder, and that is how it should be treated.
Hello, this is also a big letdown for me. I need to open all in tabs daily for the blogs I follow. The option to open all unread could also work, but my suggestion would rather be to have both options. That way you can cater for those who need to open everything and those who just need to view unread articles.

Right now I have an RSS of about 20 articles. Rather than 2-3 clicks, I now have to click each individual article to read them.
Not nice. Not nice at all.
Fix it, Please.

Most of the RSS feeds that I use churn often enough that if I miss a day (or so) I absolutely want the "open all" functionality.

and, sorry - I want it both ways - I use the "keep open after clicking to open tab" as well, for feeds where pick-n-choose is the order of the day. 

Both are useful. One doesn't trump the other
Hello. It seems I am similar to other folks it my behaviours:

a) In the morning I tend to open all tabs to catch up on news, updates, etc... from BBC, Register, FT, Y!, etc...
b) Throughout the day I just want to open new stories.

I think the best solution here is to:

a) Restore the functionality to Open All In Tabs for the RSS feeds.
b) Add a second option Open All Unread In Tabs for the RSS feeds.

I think this will meet everyones needs. Just from a crash prevention persepective you might want to bury a setting somewhere to restrict the number of tabs that can opened by this feature.

Happy weekend to all!
Also, I forgot to mention a couple of other issues with RSS feeds:

a) In FF13 the feeds have all lost their little logo (favicon.ico?) that used to appear when a page was read.
b) I also see inconsitent behaviour in a page being marked as 'read' if it exists in multiple feeds, e.g. two different FT feeds, but story only marked as 'read' in one feed. But BBC and Y! mark the story as 'read' in all feeds, thus saving time in identifying which stories to open.

Should I open separate defects for these?
(In reply to Chunky Monkey from comment #19)
> a) In FF13 the feeds have all lost their little logo (favicon.ico?) that
> used to appear when a page was read.

it's expected, we use generic rss icons now.

> b) I also see inconsitent behaviour in a page being marked as 'read' if it
> exists in multiple feeds, e.g. two different FT feeds, but story only marked
> as 'read' in one feed. But BBC and Y! mark the story as 'read' in all feeds,
> thus saving time in identifying which stories to open.

Are you sure the URI of the page is exactly the same identical ones? it's possible each feed adds some tracking info to the uri making it "different".
Please re-enable the Open All in Tabs option for RSS feeds. I maintain a set of bookmarks to web comics as an RDF file to make it convenient to get to from wherever I am. On a daily basis I want to open all of those web comics and read them. There is no concept of which are read or unread, since it's a static RDF file sitting on my personal web server.
The removal of the "Open all in tabs" functionality for Live Bookmarks in Firefox 13 destroys one of my main reasons for using Firefox over other browsers. The way this was handled was the most elegant I had come across, and I have been successfully using it this way for many years.

I, too, would welcome the additional feature of "Open Unread in Tabs" which would be very useful, but doesn't match all of my use cases.

Returning this functionality would take away the pain I am currently experiencing with RSS feeds. Adding the "open unread" feature would just make Firefox AWESOME.

Thanks
Troy
I'd like to add my vote to the 'fix it' column.  I've just upgraded, and suddenly this functionality was removed.  I read a collection of RSS news feeds every morning, and with the rate of updating, they're almost always completely new information.  Now, instead of one click to open & read all the articles, it's a painful process.

I can understand the desire to improve the functionality for other cases... but doing that should not remove the functionality that other people use.
I'd also like to add my vote to the 'fix it' column.

I also can understand the desire to improve the functionality for other cases... but doing that should not remove the functionality that other people use.

PLEASE FIX This Bug it's not a feature.

Thanx
I vehemently agree. I've been relying on this functionality for years. An "open unread" version will not work for my purposes either. I need the "open all in tabs" as it was.

Put in an option to disable it if you must, but please fix this. There are obviously many who use livefeeds like this.
Can someone please bump this to critical importance? There are way more people complaining about it than there are "votes". It's obvious from the comments it's a showshopper bug for a lot of people. I also know several people IRL who haven't posted here but who also want a fix -- and we're stuck using Chrome for reading all of our bookmarked sites until this is fixed. I've switched to Chrome before when Firefox memory management was killing my system, and it may happen again if there isn't a fix soon.

I'd happily revert to Firefox 12 if I could, but I can't (AFAIK -- if it's possible, the information is well concealed). IMO, version 13 shouldn't have shipped without this feature, and yet I'm stuck with the new version -- if I wanted to take chances on new versions I'd be signed up as a beta tester!

No one has even accepted the bug to work on it. Please, please someone fix it!
+1 on the open in all tabs -> The removal of this functionality is a bug

As is the removal of this:
(In reply to Chunky Monkey from comment #19)
> a) In FF13 the feeds have all lost their little logo (favicon.ico?) that
> used to appear when a page was read.

it's expected, we use generic rss icons now.

--- Now it is very hard reading news feeds with Firefox as they all look the same.

With the favicons the feed articles did not appear uniform and it was more easy to see quickly what has not yet been read...

Can the favicon functionality also return pls (Is there a bug thread for it yet?) And then, why is functionality disappearing from Firefox? There is a lot going missing with each update?
"Open unread in tabs" please. I don't need the old "Open all in tabs".
Just to echo comments above, this feature is the main reason I use firefox.  Really really missing the 'open all in tabs' functionality.  Please fix this bug!
I also used this feature daily and strongly miss it. I also disagree with the UI choice of generic RSS icons rather than the visually distinct color cues of site favicons. With longer newsfeeds it's much faster and easier to select one feed by its favicon then to read the text of the whole list to find it.
Mozilla guys & girls, please, repair this bug OR prevent my FF to keeping update from the last correct version 12. Now, I am about to remove version 13 or 14 sixth times in six days and downgrade back to FF 12. Despite disabling updates (Options -> Advanced -> Updates) the FF updates. Is it another bug?. I really need "Open all in tabs" functionality. After several years of excellent service and my complete satisfaction I am very angry against Mozilla right now. Grrrrrrrr!
"Open all in tabs" or "Open unread in tabs", whatever. I used to use this option often, because sometimes I travel with laptop where I don't have possibility to connect to internet, so I cache news or important pages in tabs and for example I put my laptop into standby mode for later use. Also it is in my opinion convenient way of faster reading new articles, since rss channel provides too few information to tell what articles are important.
(In reply to Astinus from comment #32)
> "Open all in tabs" or "Open unread in tabs", whatever. I used to use this
> option often, because sometimes I travel with laptop where I don't have
> possibility to connect to internet, so I cache news or important pages in
> tabs and for example I put my laptop into standby mode for later use. Also
> it is in my opinion convenient way of faster reading new articles, since rss
> channel provides too few information to tell what articles are important.

Have a look at extension called Pocket (formerly Read it Later), it is much more convenient way to cache pages for offline reading.
Does not solve missing Open in tabs option though.
From what I gather, RSS Feeds aren't keept as being marked as read any more because of whatever change. That's why the icons are all the time.
On top of that, they put the "Open <website>" on top, and as many others in this thread have said, removed the "Open all in tabs".

I upgraded to FF13 soon after it came out and almost immediately restored 12 from my daily backup. I'm stuck at 12 right now. It's absolutely ridiculous how often things that could EASILY be an option (even merely via about:config) are instead just removed. With so many people complaining on this thread, it's clear that at the least "open all in tabs" is a wanted feature, so why not return it?
(and please stick it back on the bottom. Having it and the other thing on top is an eyesore).

I too am tempted to go to Chrome soon, but only hold off because of a couple of needed extensions.
(In reply to TerraEpon from comment #34)
> From what I gather, RSS Feeds aren't keept as being marked as read any more
> because of whatever change.

Likely due to some theme or add-on, unless it's a feed doing some strange redirect (there's a bug filed on that).

> With so many people complaining on
> this thread, it's clear that at the least "open all in tabs" is a wanted
> feature, so why not return it?

No reasons to rage, I'm fine with reintroducing the option that was actually cut mostly for time/technical reasons.  Feedback is valuable and noted.
This is an OS project, anyone can provide patches.  We'll do when there's resources for it, but this feature is low priority so takes a bit of time unless someone contributes a patch externally.
Found temporary workaround for open all tabs (which is really useful functionality that I use for RSS reading).

Using Firefox 14.0.1, Select Bookmarks, Show All Bookmarks, Select appropriate RSS Bookmark Folder on left side, do a "Select All" on list of individual items on right side.  After all items selected, right click and select Open All In Tabs.

This workaround is an extra few steps, but provides same functionality.  Would love to have original functionality back.
The workaround suggested by Chris Meenan appears to bring back the old functionality.
I wonder if somebody could write an add-on that implements that, unless of course Mozilla brings the feature back.
I'd be happy to see the Open All in Tabs functionality brought back as well.  But better still, I've always wanted the Windows List Box style capability to Ctrl-Left Click to highlight multiple items/Shift-Left Click to highlight a range of items in an RSS feed list (or, really, any other bookmark folder as well), then Right-Click and "Open All Selected in Tabs."
Agreed. I will not be upgrading from Firefox 12b until this issue is resolved. This is critical functionality for me as I use it every day. Why did they remove this feature?
If someone wants to work on this bug I will be happy to mentor, the problem is that the original method to open in tabs only accept synchronous places nodes, the nodes used in livemarks are added asynchronously.
Basically the _mayAddCommandsItems call here http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/browserPlacesViews.js#853 needs to be revised to work on asynchronous populated popups, and the command PlacesUIUtils.openContainerNodeInTabs should be fixed as well to support livemarks.
There's quite some work to do, unfortunately this is low priority and we lack internal resources to work on it atm.
Whiteboard: [mentor=mak]
The question in my mind is, who authorized making the change that killed this feature without allocating enough resources to actually restore this feature when the other feature was complete?

If it's not faster for you to just re-add Open in Tabs than it would be to mentor someone who's never seen the codebase (or any non-trivial JavaScript), then the change that broke it needs to be REVERTED, because it SHOULDN'T BE THAT HARD.
This is a killer feature for Trac / Redmine users also. The ability to open all issues which mach certain criteria with a single click saves lots of time. These and other bugtracking systems publish their issue queries on rss/atom feeds.
(In reply to Marco Bonardo [:mak] from comment #40)
> If someone wants to work on this bug I will be happy to mentor, the problem
> is that the original method to open in tabs only accept synchronous places
> nodes, the nodes used in livemarks are added asynchronously.
> Basically the _mayAddCommandsItems call here
> http://mxr.mozilla.org/mozilla-central/source/browser/components/places/
> content/browserPlacesViews.js#853 needs to be revised to work on
> asynchronous populated popups, and the command
> PlacesUIUtils.openContainerNodeInTabs should be fixed as well to support
> livemarks.
> There's quite some work to do, unfortunately this is low priority and we
> lack internal resources to work on it atm.

Any update on this?
Whiteboard: [mentor=mak] → [mentor=mak][lang=js]
Monthly bump or something. After updating because I'm sick of a few things from having such an "ancient" browser as FF12, this is a really annoying loss of a feature. As a great example of where one wants all to open in tabs -- I have a feed of Allrecipe's daily recipes, and they update each early morning. So the entire feed is new each day. 

Also, the thing for opening the website on top, as I mentioned, is extremely annoying. And god those favicons are ugly (yet they are normal in when looking at the bookmark library, though I wonder if that's related to the theme I have installed)
Entire new feed each day? I have one feed that has all new entries every few hours! This bug is a major PITA.
Summary: "Open all in tabs" missing after bug 613588 → Restore "Open all in tabs" and "open <site>" to the bottom of livemarks popus (was: "Open all in tabs" missing after bug 613588)
Summary: Restore "Open all in tabs" and "open <site>" to the bottom of livemarks popus (was: "Open all in tabs" missing after bug 613588) → Restore "Open all in tabs" and "open <site>" to the bottom of livemarks popups (was: "Open all in tabs" missing after bug 613588)
What is the status of this??? It's not worked for almost 2 years now!
Hi,

I am interested in working on this bug. So please can u assign this bug to me.

Thanks in Advance,

Regards,
A. Anup
Assignee: nobody → allamsetty.anup
I did some stuff with this on the train to the summit, but I never got around to finishing this. Maybe it's helpful. Maybe not.
Good luck Anup, this has been a sorely missed feature for far too long now
Status: NEW → ASSIGNED
comment 40 has still a decent description of the involved steps, off-hand I'd make _mayAddCommandsItems always add the entry for Livemarks and openContainerNodeInTabs walk siblings. Gijs patch is doing a couple things that are not that good, first synchronous use of annotations (may rather use hasCachedLivemarkInfo, while not perfect it should do it for this use-case), second the registerForUpdates hack, not sure what's it for. Ideally it should just open whatever was in the view at the time of the click, there may not be the need to query the service.
(In reply to Marco Bonardo [:mak] from comment #51)
> second the registerForUpdates hack, not sure what's it for. Ideally it
> should just open whatever was in the view at the time of the click, there
> may not be the need to query the service.

How would this work if there is nothing in the view because the livemark hasn't been opened yet? It wouldn't open anything. Hence the registerForUpdates check. I don't see why it's a "hack", either... is there a better API to use? :-\
Status: ASSIGNED → NEW
Ugh, bugzilla, you suck.
Status: NEW → ASSIGNED
(In reply to :Gijs Kruitbosch from comment #52)
> How would this work if there is nothing in the view because the livemark
> hasn't been opened yet? It wouldn't open anything.

I think it should do nothing indeed, bonus points if the command is disabled.
(In reply to Marco Bonardo [:mak] from comment #54)
> (In reply to :Gijs Kruitbosch from comment #52)
> > How would this work if there is nothing in the view because the livemark
> > hasn't been opened yet? It wouldn't open anything.
> 
> I think it should do nothing indeed, bonus points if the command is disabled.

This doesn't make sense to me. If I open Firefox, and immediately middle-click a livemark in my bookmarks toolbar, surely the expected behaviour should be that it loads and then opens whatever's in there, rather than doing nothing until I've first manually opened it and waited for it to load? :-\
Bummer. I just swapped back from Chrome as my primary browser, partially due to missing this functionality, and I discover it is now gone. My timing is terrible. :)

Ah well, here is my +1 for getting it back. My use cases are all covered in the preceding comments so I don't have anything new to add on that front.

I don't have the time to be able to offer to work on it for now, if I suddenly do I will swing by offer what help I can.
Anup, are you still working on this?

Marco, can you clarify re: comment #55? :-)

(In reply to :Gijs Kruitbosch from comment #55)
> (In reply to Marco Bonardo [:mak] from comment #54)
> > (In reply to :Gijs Kruitbosch from comment #52)
> > > How would this work if there is nothing in the view because the livemark
> > > hasn't been opened yet? It wouldn't open anything.
> > 
> > I think it should do nothing indeed, bonus points if the command is disabled.
> 
> This doesn't make sense to me. If I open Firefox, and immediately
> middle-click a livemark in my bookmarks toolbar, surely the expected
> behaviour should be that it loads and then opens whatever's in there, rather
> than doing nothing until I've first manually opened it and waited for it to
> load? :-\
Flags: needinfo?(mak77)
Flags: needinfo?(allamsetty.anup)
Sorry to say this, I am finding it really hard to fix this. So I am reassigning it to nobody@mozilla.com
Flags: needinfo?(allamsetty.anup)
Assignee: allamsetty.anup → nobody
(In reply to :Gijs Kruitbosch from comment #57)
> Marco, can you clarify re: comment #55? :-)
> 
> (In reply to :Gijs Kruitbosch from comment #55)
> > (In reply to Marco Bonardo [:mak] from comment #54)
> > > (In reply to :Gijs Kruitbosch from comment #52)
> > > > How would this work if there is nothing in the view because the livemark
> > > > hasn't been opened yet? It wouldn't open anything.
> > > 
> > > I think it should do nothing indeed, bonus points if the command is disabled.
> > 
> > This doesn't make sense to me. If I open Firefox, and immediately
> > middle-click a livemark in my bookmarks toolbar, surely the expected
> > behaviour should be that it loads and then opens whatever's in there, rather
> > than doing nothing until I've first manually opened it and waited for it to
> > load? :-\

What should I clarify? I already answered before the question.
I think it should do nothing becayse doesn't make sense that I click on an empty list and later some tabs will open to stuff I didn't know about. You should wait for it to load so you can see what you are opening, imo.
Flags: needinfo?(mak77)
Mentor: mak77
Whiteboard: [mentor=mak][lang=js] → [lang=js]
Why isn't there anyone willing to take this on?
Comment on attachment 826414 [details] [diff] [review]
WIP patch, possibly bitrotted, possibly not the right approach, definitely not finished

I think the patch is adding confusion.

We need to use hasCachedLivemarkInfo in _mayAddCommandsItems to figure if this is a livemark, if it is we just set hasMultipleURIs to true.

Then openContainerNodeInTabs can be modified so that if the view is a popup and it's open, we collect urls from existing menuitems, rather than using getURLsForContainerNode.
Attachment #826414 - Attachment is obsolete: true
(In reply to Marco Bonardo [::mak] from comment #61)
> Comment on attachment 826414 [details] [diff] [review]
> WIP patch, possibly bitrotted, possibly not the right approach, definitely
> not finished
> 
> I think the patch is adding confusion.
> 
> We need to use hasCachedLivemarkInfo in _mayAddCommandsItems to figure if
> this is a livemark, if it is we just set hasMultipleURIs to true.
> 
> Then openContainerNodeInTabs can be modified so that if the view is a popup
> and it's open, we collect urls from existing menuitems, rather than using
> getURLsForContainerNode.

That sounds like a reasonable approach. I do think you are missing a couple of possible use paths in your thinking, assuming I understand your proposed approach correctly, that might require some extra work.

There are several paths via the UI to trigger this functionality:
First from the Bookmarks button pop-up:
1) Click on the LiveMarks folder, Click on the Open all in Tabs button at the bottom of the list.
     a) This sounds like it will work with your approach.
2) Right click on the LiveMarks folder, then click on Open all in Tabs from the context menu.
     a) This sounds like it will work with your approach, though there is probably a microscopic window in which one of collections will not be populated since the LiveMarks windows has not popped up yet.
3) Middle click on the LiveMarks folder.
     a) This may be an issue, since this can be triggered before the LiveMarks list has rendered on the screen.
Next from a LiveMarks folder that is accessed via the Bookmarks Toolbar:
1) Right click on the LiveMarks folder and choose Open all in Tabs.
     a) This one never opens the actual LiveMarks drop-down, so I assume it has not populated at least some of the collections you mention.
2) Left click on LiveMarks folder, then right click on the header of the resulting drop-down and choose "Open All in Tabs"
     a) This sounds like it will work with your approach.
3) Middle click on the LiveMarks folder.
     a) This also never opens the actual LiveMarks drop-down like Item 1 in the list.

I am not saying we need to make all of these work, we could certainly remove some of them and only make the remaining ones work. I am just trying to provide a list of all of the ways that were there before for consideration.
(In reply to Itto from comment #62)
> 2) Right click on the LiveMarks folder, then click on Open all in Tabs from
> the context menu.
>      a) This sounds like it will work with your approach, though there is
> probably a microscopic window in which one of collections will not be
> populated since the LiveMarks windows has not popped up yet.

Right, the context menu could be a problem.

> 3) Middle click on the LiveMarks folder.
>      a) This may be an issue, since this can be triggered before the
> LiveMarks list has rendered on the screen.

We will likely not support this.

> Next from a LiveMarks folder that is accessed via the Bookmarks Toolbar:
> 1) Right click on the LiveMarks folder and choose Open all in Tabs.
>      a) This one never opens the actual LiveMarks drop-down, so I assume it
> has not populated at least some of the collections you mention.

it's basically the same contextual menu code as above.

> 3) Middle click on the LiveMarks folder.
>      a) This also never opens the actual LiveMarks drop-down like Item 1 in
> the list.

again, I suspect we won't support this.

> I am not saying we need to make all of these work, we could certainly remove
> some of them and only make the remaining ones work. I am just trying to
> provide a list of all of the ways that were there before for consideration.

Right, the context menu option is something we can't fix with my approach, but I'm not even sure it's so useful for the same reason I expressed above.
Basically you can't predict what is inside these containers since the content is dinamically generated, so asking to open in tabs unknown contents doesn't sound very useful.

So, personally I'd only fix the Open All In Tabs menuitem.
I think middle click / context menu "open everything" without opening the item is a usable thing for e.g. my checking the news feed of the BBC in the morning. Certainly if the only option was to open the submenu first, I wouldn't actually bother reading all the article titles before clicking "open all in tabs". Clearly it could be a followup bug, but if the approach makes that more difficult, maybe we should re-examine the approach?
(In reply to Marco Bonardo [::mak] from comment #63)
> (In reply to Itto from comment #62)
> > 2) Right click on the LiveMarks folder, then click on Open all in Tabs from
> > the context menu.
> >      a) This sounds like it will work with your approach, though there is
> > probably a microscopic window in which one of collections will not be
> > populated since the LiveMarks windows has not popped up yet.
> 
> Right, the context menu could be a problem.
> 
> > 3) Middle click on the LiveMarks folder.
> >      a) This may be an issue, since this can be triggered before the
> > LiveMarks list has rendered on the screen.
> 
> We will likely not support this.
> 
> > Next from a LiveMarks folder that is accessed via the Bookmarks Toolbar:
> > 1) Right click on the LiveMarks folder and choose Open all in Tabs.
> >      a) This one never opens the actual LiveMarks drop-down, so I assume it
> > has not populated at least some of the collections you mention.
> 
> it's basically the same contextual menu code as above.
> 
> > 3) Middle click on the LiveMarks folder.
> >      a) This also never opens the actual LiveMarks drop-down like Item 1 in
> > the list.
> 
> again, I suspect we won't support this.
> 
> > I am not saying we need to make all of these work, we could certainly remove
> > some of them and only make the remaining ones work. I am just trying to
> > provide a list of all of the ways that were there before for consideration.
> 
> Right, the context menu option is something we can't fix with my approach,
> but I'm not even sure it's so useful for the same reason I expressed above.
> Basically you can't predict what is inside these containers since the
> content is dinamically generated, so asking to open in tabs unknown contents
> doesn't sound very useful.
> 
> So, personally I'd only fix the Open All In Tabs menuitem.

It used to be that just hovering over the a single Livemark would show the titles of the various articles on that feed.  I often used the open all tabs within a single feed (which if I remember had gotten limited to 20 or 25 total articles at some point).  when you've built a specific RSS feed for your particular interest then you almost always want to open all the underlying articles.  You're not always opening someone elses premade RSS feed.  So please don't dismiss the very reason that many of us have created bug reports over the removal of this functionality.
(In reply to Marco Bonardo [::mak] (Away June 17-22 - then ping me in Whistler) from comment #63)
> So, personally I'd only fix the Open All In Tabs menuitem.

Great, when can we expect this? I have to start FF 12 (!!) every once in a while to use this function.
Flags: needinfo?(mak77)
Ditto the above - I really miss this functionality.
I hope to not offend anyone, but this is a mentored bug in an open source project, pretty sure someone can do this and that is not me, also because I really don't have any time to...
Flags: needinfo?(mak77)
I hope to not offend anyone, but as far as I can see, this functionality was removed by M-C core staff, so I don't really understand why this is now passed on to the community. But be that as it may.

What does the mentoring involve? A rather short patch was submitted in attachment 826414 [details] [diff] [review], but obviously the comment "I think the patch is adding confusion" scared off the submitter.

So is there a chance to give exact instructions to fix this patch and then land it?

Personally I'm confused by comment #62, comment #63 and comment #65. I thought all we want to do it enable the "Open All in Tab" option on the context (right-click) menu.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Jorg K from comment #69)
> A rather short patch was submitted in
> attachment 826414 [details] [diff] [review], but obviously the comment "I
> think the patch is adding confusion" scared off the submitter.

It is a dangerous thing to make assumptions. I am not "scared off" by Marco, who I know personally, or his comment, and I am m-c staff myself. But I have other things to work on, the patch has bitrotted, and I am unlikely to come back to this in the short term. Someone else, should they wish to, should have a go.

Regarding instructions, comment #61 is a fair summary of what to do, I think.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Marco Bonardo [::mak] (Away June 17-22 - then ping me in Whistler) from comment #61)
> We need to use hasCachedLivemarkInfo in _mayAddCommandsItems to figure if
> this is a livemark, if it is we just set hasMultipleURIs to true.
> 
> Then openContainerNodeInTabs can be modified so that if the view is a popup
> and it's open, we collect urls from existing menuitems, rather than using
> getURLsForContainerNode.

So if I understand these instructions correctly, this is the first step, right?

I'd like to receive some mentoring on the second step. openContainerNodeInTabs is a 10 line function,
http://mxr.mozilla.org/mozilla-central/source/browser/components/places/PlacesUIUtils.jsm#794
so I guess, it's modification is rather trivial to someone who knows what they are doing (which I sadly am not). How would I check for a popup and collect the urls from the menu items. Is there an example to look at?

BTW: Gijs, sorry, making assumptions is indeed always dangerous.
Flags: needinfo?(mak77)
Assignee: nobody → mozilla
(In reply to Jorg K from comment #71)
> WIP: Step 1, bring "Open all in tabs" back to the menu.
> 
> (In reply to Marco Bonardo [::mak] (Away June 17-22 - then ping me in
> Whistler) from comment #61)
> > We need to use hasCachedLivemarkInfo in _mayAddCommandsItems to figure if
> > this is a livemark, if it is we just set hasMultipleURIs to true.

> So if I understand these instructions correctly, this is the first step,
> right?

right.

> I'd like to receive some mentoring on the second step.
> openContainerNodeInTabs is a 10 line function,
> http://mxr.mozilla.org/mozilla-central/source/browser/components/places/
> PlacesUIUtils.jsm#794
> so I guess, it's modification is rather trivial to someone who knows what
> they are doing (which I sadly am not). How would I check for a popup and
> collect the urls from the menu items. Is there an example to look at?

I think from aView, you can check if .viewElt is defined and check its .localName (don't recall if the view is the popup or its parent menu, btw both would be fine for this simple check).
If that is true you'll need again to check hasCachedLivemarkInfo on the controller (should be reachable from aView) and if it's a livemark, just use common DOM methods to walk the popup children and check if they have a ._placesNode.uri (Collect them)

Note, I made these staps just from a quick skim at the code, they might not be 100% perfect, but we can look at roadblocks along the way.
Flags: needinfo?(mak77)
Please take a look at the debug from the enclosed patch.
Clicking on the "Open all in tabs" on a livemark menu gives this:
Name of parent: toolbaritem
Name view element: hbox
Name of child: hbox
Name of grandchild: hbox
Name of grandchild: scrollbox
Name of grandchild: toolbarbutton
Name node element: undefined
Type node element: 6

When clicking on "Open all in tabs" in a normal bookmarks folder, I basically get a reflection of my bookmark structure:
Name of parent: menubar
Name view element: menu
Name of child: menupopup
Name of grandchild: menuitem
Name of grandchild: menuseparator
Name of grandchild: menuitem
Name of grandchild: menuseparator
Name of grandchild: menuitem
Name of grandchild: menuitem
Name of grandchild: menu
[stuff deleted]
Name node element: undefined
Type node element: 6

I'm still lost in understanding the arguments passed in. Also, are we're looking at a unified approach for livemarks folder and booksmarks folder or just two separate cases?
Flags: needinfo?(mak77)
(In reply to Jorg K from comment #73)
> Created attachment 8625869 [details] [diff] [review]
> WIP 2: Trying to understand what is passed to openContainerNodeInTabs
> 
> Please take a look at the debug from the enclosed patch.
> Clicking on the "Open all in tabs" on a livemark menu gives this:

Oh this is when the menu is closed right? so clicking in the contextual menu.
In this case we cannot really use the popup children, cause it might be closed... So instead we need to directly ask for children to the livemarks service.

in Gijs patch there is some interesting code for that using getNodesForContainer and we should likely do that.
Flags: needinfo?(mak77)
I think there is some confusion about what we're talking about. "Open all in tabs" shows up in two places:
1) Step 1 add "Open all in tabs" back to the bottom of the dropdown menu as can be seen in the attachment. The livemarks are loaded. This is the case I'm looking at.
2) Context menu (right click onto the feed folder).
I'm *NOT* looking at that, since at that stage, the livemarks are possibly not loaded.

So to answer your question:
> Oh this is when the menu is closed right? so clicking in the contextual menu.
No.

This is the output I get when I click on "Open all in tabs" as shown in the screenshot:
JK: setting hasMultipleURIs = true
Name of parent: toolbaritem
Name view element: hbox
Name of child: hbox
Name of grandchild: hbox
Name of grandchild: scrollbox
Name of grandchild: toolbarbutton
Name node element: undefined
Type node element: 6
Flags: needinfo?(mak77)
BTW, I don't intend to open all the livemarks on the BBC feed in tabs, they're just too many. But there are smaller feeds where this makes a lot of sense.

For completeness, also dumping out the grandparent and great grandchildren of the view element, so we get:

Name of grandparent: toolbar
Name of parent: toolbaritem
Name view element: hbox
--Name of child: hbox
----Name of grandchild: hbox
------Name of great grandchild: image
----Name of grandchild: scrollbox
------Name of great grandchild: toolbarbutton
[stuff deleted]
------Name of great grandchild: toolbarbutton
------Name of great grandchild: toolbarbutton
----Name of grandchild: toolbarbutton
------Name of great grandchild: menupopup <----
Actually, this is because I have the feed folder on the bookmarks *toolbar*.

Accessing the feed folder from the bookmarks *menu* gives a different hierarchy. I'd have to dig though the trees a little more to find the menuitems that carry the URLs we need to collect.

Or is this the wrong approach and you prefer something more along the lines of Gijs patch?
you can do both ways, either collect the menuitems or take Gijs approach, the thing I didn't like about his approach was that he was waiting for the livemark to be loaded and using annotations instead of the cached info. I think we must take the easy path and just open the current children.
Flags: needinfo?(mak77)
(In reply to Marco Bonardo [::mak] from comment #77)
> you can do both ways, either collect the menuitems or take Gijs approach,
> the thing I didn't like about his approach was that he was waiting for the
> livemark to be loaded and using annotations instead of the cached info. I
> think we must take the easy path and just open the current children.

From my experiments I've seen that digging about the aView.viewElt tree is very messy. Also, in comment #74 you said that Gijs' code around "getNodesForContainer" could possibly be used. So I'd like to take further look at Gijs' approach. I think that the key processing happens here:
+  _openLiveMarkNodesInTabs:
+  function PUIU_openLiveMarkNodesInTabs(aNode, aEvent, aView) {
...
+    PlacesUtils.livemarks.getLivemark({id: aNode.itemId},
+      function(aStatus, aLivemark) {
+        if (Components.isSuccessCode(aStatus)) {
+          if (aLivemark.isUpdated) {
+            openChildrenInTab(aLivemark.getNodesForContainer(aNode));
+          } else {
+            aLivemark.registerForUpdates(aNode, loadObserver);
+          }
+        }
+      }
+    );
+  },

First of all, I cannot find the definition of getLivemark that would take two arguments. Looks like the second argument is some sort of iterator function, that should be called for the livemark corresponding to aNode.itemId.
I only found:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsLivemarkService.js#289

In the attached patch, the openLiveMarkNodesInTabs is called, but the inner function is not.
Flags: needinfo?(mak77)
originally getLivemark was taking a callback, now it returns a Promise, that's why you can't find it. just use the promise .then() method
Flags: needinfo?(mak77)
Attached patch This works for me ;-) (obsolete) — — Splinter Review 
Please let me know what you think. I'm not really a JS programmer ;-(
Attachment #8625488 - Attachment is obsolete: true
Attachment #8625869 - Attachment is obsolete: true
Attachment #8626339 - Attachment is obsolete: true
Attachment #8626651 - Flags: feedback?(mak77)
Hmm, I tested this some more and saw this debug:

A coding exception was thrown and uncaught in a Task.
Full message: TypeError: Filter is null
[stuff deleted]
A coding exception was thrown in a Promise rejection callback.
See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise
Full message: TypeError: Cu is null

Maybe that's got something to do with the "undefined".

+    PlacesUtils.livemarks.getLivemark({id: aNode.itemId})
+      .then(aLivemark => {
...
+      }, () => undefined);
+  },

I've seen other places where Components.utils.reportError is used.
Comment on attachment 8626651 [details] [diff] [review]
This works for me ;-)

Review of attachment 8626651 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/places/PlacesUIUtils.jsm
@@ +799,5 @@
> +      .then(aLivemark => {
> +        urlsToOpen = [];
> +
> +        let nodes = aLivemark.getNodesForContainer(aNode);
> +        for (let i = 0; i < nodes.length; i++) {

for (let node of nodes) {

@@ +806,5 @@
> +
> +        if (!this._confirmOpenInTabs(urlsToOpen.length, window))
> +          return;
> +
> +        this._openTabset(urlsToOpen, aEvent, window);

I'd invert the if condition and call _openTabset in the if body.

@@ +807,5 @@
> +        if (!this._confirmOpenInTabs(urlsToOpen.length, window))
> +          return;
> +
> +        this._openTabset(urlsToOpen, aEvent, window);
> +      }, () => undefined);

It's ok to , Cu.reportError) here

I don't think the error you see in the console has anything to do with this patch, since you didn't use Cu anywhere, looks like due to some other code, try checking the promise stack and see if anything jumps to your eyes, it's likely someone used Cu in a context where it's not defined and he should have used Components.utils.

::: browser/components/places/content/browserPlacesViews.js
@@ +852,5 @@
> +      } else {
> +        aPopup._endOptOpenAllInTabs.setAttribute("oncommand",
> +          "PlacesUIUtils.openContainerNodeInTabs(this.parentNode._placesNode, event, " +
> +                                                 "PlacesUIUtils.getViewForNode(this));");
> +      }

would it be possible to modify openContainerNodeInTabs, so that it checks aView.controller.hasCachedLivemarkInfo(aNode) (or aView.view.controller... I don't recall off-hand) and just do the right thing for livemarks?
Then you could avoid the temp isLivemark here and the special openLiveMarkNodesInTabs in PlacesUIUtils.
Attachment #8626651 - Flags: feedback?(mak77) → feedback+
Attached patch Proposed patch with nits fixed (obsolete) — — Splinter Review 
There were three nits:
1) for (let node of nodes) - done
2) I'd invert the if condition and call _openTabset in the if body - done
   (also in the existing code in openContainerNodeInTabs)
3) Cu.reportError: done, but not sure what the message should be.

The other critique you had was to change openContainerNodeInTabs so it can handle livemarks too.
I deliberately didn't do that. The caller already checks and then calls the right function based on whether its livemarks or not. Why would I call a generic function and then do the same processing again, only because the caller discarded the information it already had? That doesn't seem like a good idea to me ;-)
Attachment #8626651 - Attachment is obsolete: true
Attachment #8627909 - Flags: feedback?(mak77)
Attached file console output — 
This is the console output I get when I close Firefox while it's still busily opening the livemarks in tabs. Obviously I'm bored with watching it, so once I can see that it opened all the tabs, I clicked to [x] to close the window and don't wait until they are all loaded.

Some part of the system doesn't seem to like that. Perhaps you can make sense of the output.
Attachment #8627910 - Attachment mime type: text/x-log → text/plain
The errors are caused by adblock plus.
Comment on attachment 8627909 [details] [diff] [review]
Proposed patch with nits fixed

Review of attachment 8627909 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/places/PlacesUIUtils.jsm
@@ +806,5 @@
> +
> +        if (this._confirmOpenInTabs(urlsToOpen.length, window)) {
> +          this._openTabset(urlsToOpen, aEvent, window);
> +        }
> +      }, Cu.reportError("Error getting livemark"));

What Marco meant was just using Cu.reportError with no arguments, ie passing a reference to the function itself. It will then get called with any errors / issues, you don't need to provide a message.

Right now, you're always reporting an error (even when there isn't one) and the return value of the function call (probably undefined, but I'm not sure) will be used as the error handler for the promise. That's not what you want. :-)

So the code would be:

      .then(aLivemark => {
<snip lots of handling>
      }, Cu.reportError("Error getting livemark"));
(In reply to :Gijs Kruitbosch from comment #86)
> So the code would be:
> 
>       .then(aLivemark => {
> <snip lots of handling>
>       }, Cu.reportError);

Egh. Corrected, sorry.
Thanks, Gijs!!
Attachment #8627909 - Attachment is obsolete: true
Attachment #8627909 - Flags: feedback?(mak77)
Attachment #8628211 - Flags: feedback?(mak77)
I forgot to ask. Do we need a try run for this? If so, which tests?
(In reply to Jorg K from comment #89)
> I forgot to ask. Do we need a try run for this? If so, which tests?

Here's some try syntax. Can you push to try yourself or do you need someone else to do it?

try: -b do -p linux,linux64,linux64-asan,macosx64,win32,win64 -u mochitest-bc[Ubuntu,10.6,10.10,Windows XP,Windows 7,Windows 8],mochitest-e10s-browser-chrome-1[Ubuntu,10.6,10.10,Windows XP,Windows 7,Windows 8],mochitest-e10s-browser-chrome-2[Ubuntu,10.6,10.10,Windows XP,Windows 7,Windows 8],mochitest-e10s-browser-chrome-3[Ubuntu,10.6,10.10,Windows XP,Windows 7,Windows 8] -t none

(this is verbose because I want to force OSX testing, which is disabled by default and I've seen that biting people too much the last few weeks)
Thanks, I've got level 1 rights. Here it is:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca0445911127
I'll be away from the screen for a few hours now.
Looks mostly green ;-)
Comment on attachment 8628211 [details] [diff] [review]
Proposed patch with more nits fixed

Review of attachment 8628211 [details] [diff] [review]:
-----------------------------------------------------------------

This looks OK to me, let's make sure you get a review rather than just feedback from Marco (he might still have suggestions and/or disagree with my assessment, as he knows the code much better than me).
Attachment #8628211 - Flags: review?(mak77)
Attachment #8628211 - Flags: feedback?(mak77)
Attachment #8628211 - Flags: feedback+
Comment on attachment 8628211 [details] [diff] [review]
Proposed patch with more nits fixed

Review of attachment 8628211 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM!
Thank you for going through this adventure with us.
Attachment #8628211 - Flags: review?(mak77) → review+
Summary: Restore "Open all in tabs" and "open <site>" to the bottom of livemarks popups (was: "Open all in tabs" missing after bug 613588) → Restore "Open all in tabs" to the bottom of livemarks popups (was: "Open all in tabs" missing after bug 613588)
(In reply to Marco Bonardo [::mak] from comment #94)
> Thank you for going through this adventure with us.

I don't know whether you remember me: I did the debugging for bug 1042561, the auto-complete problem.

I've also worked a lot with Ehsan on editor bugs (bug 1100966, bug 1154791, bug 1140105, bug 1141017, bug 756984, bug 1140617 to name the more important ones). Otherwise I'm working on Thunderbird.

But this was certainly an adventure of a different kind.

How do you feel about uplifting this to Aurora? It has 19 votes and the Aurora cycle started only a few days ago.
Keywords: checkin-needed
I don't feel like it's critical enough, and moreover is longstanding, so doesn't seem to require rushing at this point. On the other side it's a very simple patch, so it might be approved from that point of view.
Comment on attachment 8628211 [details] [diff] [review]
Proposed patch with more nits fixed

Approval Request Comment
[Feature/regressing bug #]: 613588
[User impact if declined]: Long standing problem will be fixed even later.
[Risks and why]: Simple patch in livemark handling.
[String/UUID change made/needed]: None.
Attachment #8628211 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/663108a1d580
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Comment on attachment 8628211 [details] [diff] [review]
Proposed patch with more nits fixed

Approving for uplift. I was concerned about possible e10s impact but Marco mentioned that this is chrome only code. So we are good for uplift!
Attachment #8628211 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: qe-verify+
Flags: qe-verify+
Is there a way to disable this ? I don't need it and found it disturbing but I can't find a way to disable it.

Thanks !
Not that I know. This was standard behaviour until FF 12. Then it got lost and later re-established in TB 41.
Is it possible to make it optional please with an option or a setting in about:config ?
I opened a bug report to make this optionnal

https://bugzilla.mozilla.org/show_bug.cgi?id=1285805

Thanks !
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: