facelessuser / ColorHelper

Sublime plugin that provides helpful color previews and tooltips
https://facelessuser.github.io/ColorHelper/
MIT License
254 stars 30 forks source link

Speed up first render for color previews #249

Closed alexkuz closed 1 year ago

alexkuz commented 1 year ago

Currently, color previews show up with a small lag, so here's a couple improvement suggestions to mitigate this. I won't make a PR as there's most probably a better way to implement this, but I'll show my code changes to give you a better idea.

  1. Force run color_helper_preview in on_activate

    ch_preview.py

    image right now on_activate doesn't run color_helper_preview immediately, but rather waits for a thread to run it (which can take up to 250ms due to sleep)

  2. Use wider visible_region so there's fewer layout jumps on scroll

    ch_preview.py

    image this is a lazy solution, I guess a better way would be to count N lines before and after visible region.

facelessuser commented 1 year ago

I do not recall if there was a reason I avoided on_activate, but if it doesn't create any bad side effects or race conditions, it is certainly something that could be considered.

As far as region size, I guess we could make this configurable. The window is defined simply to keep the payload manageable on scroll and modifications. We do not want to process the entire file each time, so we need to set bounds to restrict processing. The window must be constrained by both width and height as you could be viewing a minimized file that wraps one giant line. With that said, performance is relative to the system on which it runs, and I am sure larger windows can be handled without any impact on various systems. The default was just a conservative, initial approach.

If we provide a configurable window size, we may extend the initial size, but I imagine, by default, we will still keep it relatively conservative. In addition, we would provide a warning that using extremely large windows could cause lag during certain operations, like typing, scrolling, etc.

@gir-bot remove S: triage @gir-bot add T: feature

facelessuser commented 1 year ago

It's looking like this is doable. I haven't tested the on_activated much, but it seems to work. We can try to target the next release.

facelessuser commented 1 year ago

I'm just merging it. We'll see if people complain about odd race conditions. In my testing, I didn't see any, but getting this out to more people is probably the only way to know for sure.

alexkuz commented 1 year ago

@facelessuser thank you! Although I think it's better to use on_load_async instead of on_activated for two reasons:

alexkuz commented 1 year ago

With on_activated it looks like this on file open:

image

facelessuser commented 1 year ago

When testing, I was focused on activation, so I hadn't tested "on load".

We may have to add a first time check, and if the first time, queue it up in the traditional way as we have to wait anyways.

If you can file a new bug, I can look into this.

facelessuser commented 1 year ago

I think I know what happened. You did a combination of using on_activated and extending the buffer to a great deal. You were relying on the fact that you had extended the buffer to accommodate your screen size, so you didn't see an issue when this was suggested. When I extended the default to 20 to give a little buffer, not the 2000 you used, you started to see the issue. My tests were generally using a smaller file, so I did not see the issue in testing either.

If I added a "first time" check, you will only see the speed increase on reloads, not first loads. on_load is run after on_activated, but during on_activated, view.is_loading() already reports True even though visible region reports (0, 0).

I may end up reverting this change (just the immediate run portion). I'm not convinced that on_load_async is going to not introduce its own set of issues. I may explore it, but I'm okay with a little lag on the first load, but extending the buffer to reduce "pop in" on scroll.

facelessuser commented 1 year ago

Because I don't actually have a lot of time to spend on this, I'm attempting a fix by using async in a new 6.2.1 release: https://github.com/facelessuser/ColorHelper/releases/tag/st3-6.2.1.

If this causes race conditions, my next step will be to revert the change back to legacy behavior. Hopefully, that is not required.

facelessuser commented 11 months ago

We will likely set padding to zero by default, but we will leave the option in so people can increase it. The reason is the linked issue: https://github.com/facelessuser/ColorHelper/issues/257. By default, we want to keep performance acceptable to reduce incoming issues. People are free to push those boundaries as they like in personal settings based on how they use the tool, but I want to reduce complaints with default behavior.