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

Dialog -> Widget, Use signals instead of timer, and code cleanup (#1) #11

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

Conversation

suniahk
Copy link

@suniahk suniahk commented Oct 6, 2024

This originally started out as trying to translate the Dialog UI to a dockable Widget. That turned into "hey, I can replace the timers with signals for a cleaner approach", which turned into "I should fix the UI bug where labels paint over each other". And now here we are. So, without further ado...

  • Changed the UI from a dialog to a dockable widget. This also means that the layout is now wrapped in a QScrollArea, so that the UI size never grows too large.
  • Removed all timers; code now triggers from source signals instead. QTextEdit boxes update variablesAndValues using the textChanged signal.
  • Added checks to make sure UI isn't double-rendered whenever a dependent property changes.
  • Stopped triggering a scene collection save on every close; values are now saved to a json file in the module_config_path.
  • Finally, the catch-all collective of assorted code-golf and performance optimizations

@marktheminer
Copy link
Owner

Hi @suniahk I'm on vacation this coming week and I'll try to get to this then. Wondering if these json changes would cover:

#12

@suniahk
Copy link
Author

suniahk commented Oct 11, 2024

Right now the JSON file is loaded to/from the directory returned from obs_module_config_path, and it's only used to store values on close. It could definitely be adapted to work as an import mechanism, but I think that the obs_data code only supports saving/loading and not watching. Not that we couldn't just do it ourselves, but at that point we're using an api helper function to parse a file into a data format, only to then repeat the overhead every time something changes. It just seems a bit redundant to me.

A better approach could be to adapt the text source approach as seen here. We'd always have the most recent values from the file, we can easily write back using os_quick_write_utf8_file, and as an extra bonus, we're not tied to JSON to structure the data. For user-structured data, I'm definitely more in favor of the .ini format like what was proposed in the issue you linked for the simplicity of it.

@suniahk
Copy link
Author

suniahk commented Oct 20, 2024

I've updated the code with your suggestions, definitely good catches!

@marktheminer
Copy link
Owner

Looks like it needs formatting @suniahk

@marktheminer
Copy link
Owner

@suniahk can you run the formatter?

@marktheminer
Copy link
Owner

Alternatively @suniahk , if you can tell me the right way to pull your branch and update your PR I can do it. I've not worked with github much prior to these OBS plugins.

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.

2 participants