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

Remove plugin decoration rendering order config #759

Open
aminya opened this issue Jan 15, 2021 · 9 comments · May be fixed by #761
Open

Remove plugin decoration rendering order config #759

aminya opened this issue Jan 15, 2021 · 9 comments · May be fixed by #761
Labels

Comments

@aminya
Copy link
Collaborator

aminya commented Jan 15, 2021

There is unnecessary complexity in the decoration management/rendering code to render the decorations based on an order. However, this seems unnecessary to me as decorations can perfectly be shown on top of each other and they do not have conflicts. The default for all of them is 0 which means until today by default no ordering has been applied. But the code is still there slowing down the rendering.

This config does not make sense to me and just adds complexity and slow-down.

Instead, I am considering drawing the decorations in this order instead of calculating the order dynamically every time. Rendering in this order does not result in any conflict.

  • line
  • highlight-under
  • highlight-over
  • highlight-outline
  • gutter
  • background-custom
  • foreground-custom
@aminya
Copy link
Collaborator Author

aminya commented Jan 15, 2021

@UziTech Any thoughts?

@aminya aminya added the discuss label Jan 15, 2021
@aminya aminya linked a pull request Jan 15, 2021 that will close this issue
@aminya
Copy link
Collaborator Author

aminya commented Jan 15, 2021

If there is no objection, I want to merge #761

@UziTech
Copy link
Collaborator

UziTech commented Jan 15, 2021

I would need some time to do more research. I can see why someone would like to have a specific order of plugins so more important ones show on top. I don't know if anyone actually uses it or which plugins would actually benefit from it.

@aminya
Copy link
Collaborator Author

aminya commented Jan 15, 2021

I would need some time to do more research. I can see why someone would like to have a specific order of plugins so more important ones show on top. I don't know if anyone actually uses it or which plugins would actually benefit from it.

I doubt that anyone uses it. This config is so minor probably no one will notice a difference.

In the future, if we wanted to add support for a brand-new decoration type that absolutely needs to be placed in a certain place, we should add the code inside the dispatcher. The customizability for the users (not developers) has no point here, and so I think the config should be removed.

Prototype for the dispatcher implementation (WIP):

switch (decType) {

@UziTech
Copy link
Collaborator

UziTech commented Jan 15, 2021

I agree it probably isn't used by many users (I didn't even know it existed) but I could see it being used by a few users. This actually solves a problem I was having seeing find-and-replace decorations even when the cursor-line decoration is on the same line. Without this ordering the cursor-line decoration covers the fin-and-replace decoration even though find-and-replace is more specific.

I feel like we need to figure out a way to keep user defined ordering for power users, even if it isn't this setting.

Is there some way we can look at which decoration is smaller and place it on top?

@aminya
Copy link
Collaborator Author

aminya commented Jan 15, 2021

I feel like we need to figure out a way to keep user defined ordering for power users, even if it isn't this setting.

I think if anyone really wants to modify an internal behavior of Minimap, they should modify the code. Making things dynamic is not good all the time, and here, for example, it results in a slow-down in each scroll movement. I prefer that most users get a better experience over a few users wanting to change the Z index of elements.

Is there some way we can look at which decoration is smaller and place it on top?

The current order is buggy and that's why I want to change it so the specific decorations to be always on the top. Hopefully, my prototype will fix this.

@UziTech
Copy link
Collaborator

UziTech commented Jan 15, 2021

Is this something where we can move the sorting to when the decorations get added instead of on each draw? Then the scroll speed is not affected correct?

@aminya
Copy link
Collaborator Author

aminya commented Jun 26, 2021

I am bumping the major version in #798 by removing some of the legacy functions. It might be a good thing to include #761 in this major bump too.

@UziTech
Copy link
Collaborator

UziTech commented Jun 26, 2021

I actually think the plugin order is necessary. how else is it possible to always show find-and-replace above cursor-line? (Which I think it obviously should). I'm fine with it not being user defined but we need some way for plugins to have a well defined priority.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants