Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial implementation of a minimap plugin. #39

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

andsve
Copy link

@andsve andsve commented May 22, 2020

  • Overloads DocView methods for scrollbar rendering and logic.
  • Exposes two global commands, toggling visibility of minimap and toggling syntax highlighting inside the minimap.
  • Reloading the minimap module has issues with above mentioned commands.
  • Haven't been able to test on any other platform than macOS (with a high dpi backbuffer), but using the SCALE global so should work.

image

- Overloads DocView methods for scrollbar rendering and logic.
- Exposes two global commands, toggling visibility of minimap and toggling syntax highlighting inside the minimap.
- Reloading the minimap module has issues with above mentioned commands.
- Haven't been able to test on any other platform than macOS (with a high dpi backbuffer), but using the SCALE global so _should work_.
@rxi
Copy link
Owner

rxi commented May 22, 2020

Very cool! Thanks for submitting this. Some initial thoughts after looking through the code and giving it a try

  • All seems to work well on my linux machine regarding the SCALE
  • I see what you mean about the per-character stuff; I didn't consider the optimisation used by the tokenizer that rolls whitespace into existing tokens would prevent only iterating the tokens
  • It seems to struggle on big files (>10k loc) as I don't believe it's culling to the currently displayed lines
  • the options at the top of the file: minimap_width etc. should be stored as values in the config table instead

A few places where performance could be improved:

  • If we avoid creating a new color table with each rectangle drawn
  • The resultant batched rectangles could be added to a weak table mapping tokens tables to tables-of-rectangles-to-draw, this means we'd typically only have to iterate the resultant rectangles for each line each frame. This would lead to a huge boost in performance

Regarding reloading the module, this isn't something that is strictly meant to be supported by plugins -- the reload module command was used more for development purposes and then changing color themes.

Also might be worth noting that the common.bench(label, function) function can be used for timing things if you're looking to make performance improvements

Additionally, as this is a larger plugin, if you're going to continue working on it would you be happy with creating a github repo for it and linking to that repo from the plugins table (similar to what the console and linter plugins do). Not a problem if you'd rather not; let me know if this is something you plan to continue expanding on, nice work so far!

@andsve
Copy link
Author

andsve commented May 23, 2020

  • It seems to struggle on big files (>10k loc) as I don't believe it's culling to the currently displayed lines

Good point, planned on doing this but simply forgot, I'll fix!

  • the options at the top of the file: minimap_width etc. should be stored as values in the config table instead

Ah, of course, I fix!

  • If we avoid creating a new color table with each rectangle drawn
  • The resultant batched rectangles could be added to a weak table mapping tokens tables to tables-of-rectangles-to-draw, this means we'd typically only have to iterate the resultant rectangles for each line each frame. This would lead to a huge boost in performance

Very good point, will explore this. 👍

Additionally, as this is a larger plugin, if you're going to continue working on it would you be happy with creating a github repo for it and linking to that repo from the plugins table (similar to what the console and linter plugins do). Not a problem if you'd rather not; let me know if this is something you plan to continue expanding on, nice work so far!

Also good point, I wasn't planning on the plugin ending up this big, but I think it's hard to do any other way. I'll do the above fixes on this branch, but move it to a separate repo once you have given a second feedback review, if you are fine with that?

Thanks for the feedback! :)

@rxi
Copy link
Owner

rxi commented May 23, 2020

That all sounds great! Once the culling is in, is the scrolling behaviour of the minimap something you plan to support too, eg. for cases where the document on the minimap is greater than the height of the minimap?

Regarding the rectangle caching, I was thinking something akin to this:

local cache = setmetatable({}, { __mode = "k" })

local function get_cached(tokens)
  local c = cache[tokens]
  if not c then
    c = {}
    cache[tokens] = c
    -- iterate tokens and push [x,width,color] to `c` table
  end
  return c
end

Although [x,width,color] could be stored as a table each, to save memory and possibly improve performance it might make more sense to store them flatly in the table (eg, push first the x-offset, then width then color, then the next x-offset etc. and then iterate the table with a step of 3)

Since we're using a weak table we never have to worry about cleaning things up, and since we're using the tokens tables as keys, which are never reused, we don't have to worry about invalidating the cache.

Otherwise great work -- glad to hear you're going to continue working on it!

@andsve
Copy link
Author

andsve commented May 24, 2020

Just wanted to give you a quick update!

That all sounds great! Once the culling is in, is the scrolling behaviour of the minimap something you plan to support too, eg. for cases where the document on the minimap is greater than the height of the minimap?

This is what I have been fixing today, seem to work much better with larger files already. (Added culling as well.)

large_scroll2
Maybe a bit hard to see in this compressed gif, but this is scrolling a ~10k lines C file, tried to keep the behaviour close to what Sublime does at least.

Regarding the rectangle caching, ...
I think that sounds good, haven't had time to look at better caching yet but this will come in handy. This is next on my list! 👍

@rxi
Copy link
Owner

rxi commented May 25, 2020

Looks great! Looking forward to seeing how it performs with the rectangle caching too, but otherwise it seems like it's basically there. Really nice work.

Did you plan on adding a background to it? I wasn't sure if this was something that just wasn't a priority yet or if you were going for a different aesthetic.

Thanks for the update!

@Anachron
Copy link

Hope you dont mind me chiming in, is the height adjustable? I would prefer being able to set a max height.

@andsve
Copy link
Author

andsve commented May 25, 2020

Did you plan on adding a background to it? I wasn't sure if this was something that just wasn't a priority yet or if you were going for a different aesthetic.

I'll add a background rect, but also an option to disable it. 👍

Hope you dont mind me chiming in, is the height adjustable? I would prefer being able to set a max height.

Should be possible to add this as a option, do you just mean that the minimap should only be visible up to a certain px height? Do you want a scrollbar along with it? Currently this replaces the scrollbar.

@Anachron
Copy link

Yes I would still want the scrollbar. The minimap only in the top X pixels. This way my view will stay sleek while still giving me some overview of the file.

- Added separate logic when scrolling using minimap in large files.
- Culls lines outside of minimap (both y and x).
- Moved config variables into the config table.
- Drawing background rect behind minimap, if config enabled (default: yes).
@rxi
Copy link
Owner

rxi commented May 26, 2020

@andsve two small things I noticed:

  • With how the override on on_mouse_moved is done — where the original function isn't called — that other plugins that patch into the same function stop working, for example this breaks the linter plugin
  • How the background color is set means that changing the theme doesn't update the background color. In other plugins that use fall-back colors I'd do: style.minimap_background_color or style.background_color at the usage point (the color would be expected in the style table too)

@andsve
Copy link
Author

andsve commented May 27, 2020

  • With how the override on on_mouse_moved is done — where the original function isn't called — that other plugins that patch into the same function stop working, for example this breaks the linter plugin
  • How the background color is set means that changing the theme doesn't update the background color. In other plugins that use fall-back colors I'd do: style.minimap_background_color or style.background_color at the usage point (the color would be expected in the style table too)

Good points, pushed a change that hopefully fixes this.

Haven't had time to look into caching due to work, but hopefully within a day or two.

edit: Checked with the linters plugin (specifically the one using luacheck), seems to work fine.
image

@Jipok
Copy link

Jipok commented Nov 3, 2020

Works well for me. Is there a chance of adding this plugin to the list of preinstalled ones?
It would be cool if all themes supported the background color of the minmap.

@Jipok
Copy link

Jipok commented Nov 4, 2020

With the minimap, when I open the command window, such a bar appears:
image

Also, the minimap does not drag with the mouse if you click and drag without releasing the left button:
tmp

@terencode
Copy link

For long lines I can't get to their very end. This is either using the 'end' key or trying to select a line up to its end:

Peek.2021-02-03.15-51.mp4

Jipok pushed a commit to Jipok/lite-plugins that referenced this pull request Dec 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants