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

[WIP] Feat/issue 2581/list of variables are used #2784

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

tgadler
Copy link

@tgadler tgadler commented Mar 4, 2024

Fixes issue 2581 for CustomVariables. Left to do is to extend it to also include other Variables. Furthermore I would appreciate some feedback on the layout.

@CLAassistant
Copy link

CLAassistant commented Mar 4, 2024

CLA assistant check
All committers have signed the CLA.

@tgadler tgadler changed the title Feat/issue 2581/list of variables are used [WIP] Feat/issue 2581/list of variables are used Mar 4, 2024
@Julusian
Copy link
Member

I have some concerns about the approach here

  1. It is doing a poll for each custom-variable listed, so for each custom-variable it is searching through all controls for usages of that variable. This means the cost of showing this info will grow exponentially as configs get more complex. It would be better to do one search to find all the usages of all of the custom-variables, and save some cpu work (and try to get it back to being a linear growth).
  2. It would be nice if this could use a visitor pattern similar to https://github.com/bitfocus/companion/blob/main/companion/lib/Util/Visitors/ReferencesCollector.js . That will help ensure that as changes are made to the system, this will be updated to match how other areas of the system interpret things. For example, this will currently miss some references from internal actions, where the custom variable is stored as internal:custom_my_var instead of as $(internal:custom_my_var). Although even this will miss some references from internal actions where they store just the my_var part, so these visitors will need to be extended to support visiting those too.
    Additionally, some refactoring will need to be done to be able to use the visitor in a more general way, it is currently tied into the import/export logic a bit too much.

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.

3 participants