-
Notifications
You must be signed in to change notification settings - Fork 101
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
Adds Input to Volume Module #246
Conversation
painerp
commented
Sep 11, 2024
- Introduced a toggleable option to show the input in the volume module, allowing it to be displayed alongside or independently of the output volume.
- Added a toggle feature for the output volume.
- Implemented an option to hide the label for both output and input when muted.
- Updated themes to reflect these changes.
We should try to minimize nesting bindings as much as possible. This is because for every top level binding change, the child binding is executed as a multiple of the parent. So for instance, imagine we have a top level bind called parent (ex: parent.hook(...))... then inside that hook we have a child hook that's nested. For every parent change N, the child changes M times. So if the parent hook is executed 100 times, the child is executed 100 times as well, then the total execution will be 100 * 100 - which combines for a 1000 executions. Essentially is O(n^2) and causes runaway memory consumption which will inevitably crash AGS. You can see this if you put total += 1;
console.log(total); The more and more you start changing the volume, because there is so much hook/binding nesting, the number grows out of control to a point where every single change in volume is executing the hook - and calling the function - 2000 times. And this number will continue to grow. What we can do here is move all the bindings to the top level and simple pass the value down to the children and dependencies. Also it seems that the split styling is broken with the double module enabled. There's a divider and a rounded border even when the icon is not on the outer edge. We'll have to determine the best UI approach for this. |
Oh my bad I forgot to fix the hooks. Regarding the split style, I tried some different ways but couldn't settle on one so I left it looking like the others for now. Specifically making the right icon squared instead of rounded or moving it all the way to the right. |
I will get back to you next week with some possible design options. So that we can brainstorm a suitable design for the split style. Do you think the current version is adequate for you function wise or does anything else need changing? Thanks for your thorough explanation btw! |
Overall, this is much much nicer! After this we should discuss how we want to handle the split style and it should be good to merge. Though one thing we need to change is that we shouldn't be able to disable both playback and microphone sections and hide the entire module. Maybe we add a side-effect to enable the other if one is disabled. |
# Conflicts: # modules/bar/volume/index.ts
# Conflicts: # modules/bar/volume/index.ts
I've removed the hooks and moved everything to the main function. I assumed using the hooks takes less processing since they only change the text/icon instead of redrawing the whole box but it seems to make a negligible difference and hiding them is more consistent as well. So Thanks for the great suggestion! I've also refactored some of the more complex checks to improve readability. Regarding the option side effect: I've looked into it and couldn't find an existing implementation. I created a quick POC which works but is pretty rough around the edges. Could you give me a hint on how you would implement this? |
Ooo I like the second one. Side-effects just mean a secondary action that occurs as a consequence of a primary action. To give you an example, if we had 3 switches in which only one could be enabled, the side-effect of enabling SwitchA would be to disable SwitchB and SwitchC. Now, in such a case we should probably go with a enumerator instead of a switch and just switch between the different styles rather than having 3 different switches. For a simple example you can look at |
I've implemented the toggle side effects and the right icon for the split style. Do you think we should make the right icon optional for the other styles as well? Thanks for the example I must have missed the file when I was looking for it. Feel free to let me know if you spot anything else! |
We may actually end up going with no icon tbh. The reason being is that while it looks cool, the more that I think about it, the more I realize that it goes against the standard design of icon@left label@right. We can keep the input label with a divider which looks great, but leave the icon out. A better approach may be to later on implement the ability to switch the icon between input or playback. |
I agree that was also my initial feeling with the right icon style. Do you dislike the default/wave style (output icon -> label -> separator -> input icon -> label) as well or just the right icon in split style? If it's the broader concern, I think separating the input into its own module could be a good solution. This would alleviate our consistency concerns and would also provide a better experience especially with your recent changes (#278), which would make it possible to use the scroll wheel to control the volume of both output and input. I also feel like otherwise it would break the hide muted label option since spotting which label is shown would make it harder without the icon next to it. Alternatively, we could keep the current style and make the input icon toggleable. I'm down to implement either approach. Let me know which one you'd prefer to move forward with |
My bigger concern is with the double icon. I think having a single icon is the way to go but we can still have split style. Just let the user pick what icon they want to show (either mic or speaker). So it would be [Icon of choice][vol][split][input] |
Alright, I will work on this in the coming days! |
# Conflicts: # modules/bar/volume/index.ts # options.ts # scss/style/bar/audio.scss # themes/catppuccin_frappe.json # themes/catppuccin_frappe_split.json # themes/catppuccin_latte.json # themes/catppuccin_latte_split.json # themes/catppuccin_macchiato.json # themes/catppuccin_macchiato_split.json # themes/catppuccin_mocha.json # themes/catppuccin_mocha_split.json # themes/cyberpunk.json # themes/cyberpunk_split.json # themes/dracula.json # themes/dracula_split.json # themes/everforest.json # themes/everforest_split.json # themes/gruvbox.json # themes/gruvbox_split.json # themes/monochrome.json # themes/monochrome_split.json # themes/nord.json # themes/nord_split.json # themes/one_dark.json # themes/one_dark_split.json # themes/rose_pine.json # themes/rose_pine_moon.json # themes/rose_pine_moon_split.json # themes/rose_pine_split.json # themes/tokyo_night.json # themes/tokyo_night_split.json # widget/settings/pages/config/bar/index.ts # widget/settings/pages/theme/bar/index.ts
Okay finally got to it. Sorry for the wait. |
No worries, I'll review it this weekend. Thank you! |