marklieberman / downloadstar

Download all items in a webpage that match a pattern
GNU General Public License v3.0
90 stars 16 forks source link

Download Star reacts badly to a bogged down browser #49

Closed ssokolow closed 6 years ago

ssokolow commented 6 years ago

About half the time I open Download Star, it lingers on this state for several seconds before displaying properly.

screenshot11

I know this is mostly Firefox's fault for allowing unfocused content to starve focused content for CPU time, but couldn't you make the template start out looking a little more like its final state so the problem is less glaring?

marklieberman commented 6 years ago

That's just what it looks like before angular is done compiling template. I can't really do anything about it if angular is being slow. All the scraping and stuff is asynchronous so it technically shouldn't delay angular unless the CPU is just being starved. I'll try adding some setTimeout stuff to delay the scraping 100ms or so and see if it helps. I generally don't encounter the slowness on my machine so it would be difficult for me to test it.

ssokolow commented 6 years ago

I'm pretty sure it's CPU starvation. Yours isn't the only extension that bogs down like that sometimes. (eg. if I middle-click a bunch of tabs in rapid succession, Tree Style Tab's sidebar will start taking several seconds to respond to clicks while the browser's native tab bar remains responsive.)

I've also just filed a bug on Firefox asking for user-triggered WebExtension event handlers and the contents of browser/page action panels to get prioritized over other demands on the CPU.

What about having the completion of Angular template compilation trigger the beginning of scraping? (Aside from any additional HTTP requests which you might perform before scraping starts)

Also, regarding the appearance, I don't have time to look into whether the version and configuration of Angular that you're using has a more specialized solution (eg. something akin to ng-cloak in Angular 1.x) but the general solution is:

  1. Use traditional CSS to hide the contents of the Naming and Saving tabs and the template at the bottom.
  2. If Angular doesn't automatically unset the CSS in question, use an Angular init hook to unset it after Angular has finished.

Essentially, make the "no JS" look as close to the "JS initialized" case as is feasible so that, when it bogs down, it's not as eye-catching and the panel has as little need to resize itself as possible.

marklieberman commented 6 years ago

The more I look at the screenshot the more I'm thinking its actually just bogged down compiling the ng-repeat of the media items. Roughly how many items are getting scraped when it bogs down for you? It may be best to change the ng-repeat to some sort of virtualized list view. I don't really want to because I usually end up fighting the virtualized list view, but if a few hundred items is bogging it down that much it might be necessary.

ssokolow commented 6 years ago

I'd estimate about a hundred.

A quick test on a page that gave me trouble had roughly 6 entries per swipe of the scroll wheel, multiplied by roughly 12 swipes of the scroll wheel to go from top to bottom, for a total of roughly 96 items. It wasn't the slowest one, but it did exhibit problems.

ssokolow commented 6 years ago

Also, on that front, couldn't you initialize the UI and then inject the data?

Being able to see the initial state of filter controls and click them, even if the UI doesn't react until the entries are populated, would help to reduce the perceived wasted time.

marklieberman commented 6 years ago

I get what you're trying to say, but its more of a question of how to implement properly. The controller doesn't really have a concept of "UI is ready." It just puts the data on the scope because its a controller, and angular compiles everything eventually.

It's late so I haven't actually tested anything, but I'm theorizing that even though the scraping is async its resolving fast enough that angular hasn't compiled everything yet. So, it ends up doing the rest of the UI and all the ng-repeat items together. Scraping needs to start later. Possibly there's a suitable callback on the controller but I can't think of any beside $onInit and from docs I think its still too early. Maybe I just defer scraping 100ms with setTimeout and angular can get a few digest cycles in.

I'll figure it out, its just gonna be tough to test on my box because I don't get the slowdown. It's pretty much instantaneous for me even with a few hundred items.

ssokolow commented 6 years ago

I have no experience with recent versions of Angular, but, from the page I linked you to, I get the impression that OnAfterViewInit combined with the tick_then method should do the trick.

If I'm understanding the descriptions correctly, OnAfterViewInit is the last of the init hooks to fire and even OnInit should firmly put you after the {{...}} tokens have been substituted.

As for tick_then, I haven't checked where in the class hierarchy it comes from, but the page describes the use in their example as "The LoggerService.tick_then() postpones the log update for one turn of the browser's JavaScript cycle and that's just long enough."

marklieberman commented 6 years ago

DS is using angular 1.7, the docs you linked are like angular 2+. I use 1.x because I'm familiar with it from work and I haven't yet setup the whole transpiled build process used by 2+. Most of the hooks are similar but I don't think onAfterViewInit exists.

ssokolow commented 6 years ago

Ahh. Angular 1.x is the one I poked at briefly. In that case, there's a simple starting point to make things look better: Apply ng-cloak attributes to the portions of the template which shouldn't be visible prior to the {{..}} placeholders being filled, like the bodies of the tabs which aren't selected by default.

https://docs.angularjs.org/api/ng/directive/ngCloak

Note that it also recommends that, if the angular.js script isn't loaded in the <head> of the document, this should be added to your stylesheet:

`[ng\:cloak], [ng-cloak], [data-ng-cloak], [x-ng-cloak], .ng-cloak, .x-ng-cloak {
  display: none !important;
}

I'd add it anyway, so you're not relying on JavaScript at all to do the initial temporary hiding.