-
-
Notifications
You must be signed in to change notification settings - Fork 253
WIP: Input/Output toolbars #543
base: master
Are you sure you want to change the base?
Conversation
27f7351
to
c68f1be
Compare
The windows build seems broken, |
What do you mean? The Windows build passed. |
I edited your todo list so it shows actual checkboxes. |
Yeah I didn't word it properly. What I meant is that the windows version crashes on launch. Sorry for lack of clarity |
Since it’s a drop-down button, it should have a ▼ next to it |
@fitojb Yeah I agree! It's on the to do list :) |
This pull request is quite large. Would it be feasible to split off small independent changes? I suspect the answer is no in this case because the design only works well when considered all together. |
I guess splitting some things off would work, and might make sense. Another thought is wether to keep the sliders for PortMixer or not. Not keeping them would simplify things a lot, not to mention reduce the size of the PR significantly. But not keeping them would kinda mean dropping PortMixer support which might be a bit hasty. I could perhaps split the menu stuff and the sliders stuff into separate PRs. But since they wouldn't be entirely independent as to the order they would need to be applied I think just separating the commits a bit more strictly might be the better option. Also I'm unsure about keep supporting the combined toolbar. The implementation is a bit strange and I'm not sure if it'll ever be used again. I've kept it around for now since I thought it might be useful, but dropping it might make more sense, and might be good to do while we have the chance to make possibly breaking changes to the cfg file, which I suspect is the main reason why it's still there. |
Sure.
When in doubt, I say remove code. |
If it is significant extra work to maintain, I am okay with removing it. As we discussed on Matrix before, that feature didn't even work reliably before and it's probably impossible to make it work reliably without the magic power to force all audio interface manufacturers and operating systems to follow a consistent standard. |
I've begun removed the combined meter. It's a relief to get rid of it tbh.
Yeah I mean right now it's fine but for the future it'll surely be quite the hassle maintaining an old conan branch just for making sure the PortMixer stuff works. At least on my system PortMixer never worked reliably in the first place. And apart from maintainability, dropping in would make this PR so much smaller and simpler. |
Maybe we should put this PR on hold and remove all the PortMixer stuff first. |
Yeah I could split of the commit that removes the mixer toolbar and then go through the rest of the PortMixer stuff and make a separate PR. |
There's also EXPERIMENTAL_AUTOMATED_INPUT_LEVEL_ADJUSTMENT that should be scrapped as well. It uses PortMixer to try to adjust the input level automatically, which is just silly imo. |
On it. |
I have good news and bad news. The good news is that I fixed my compiler setup because I haven't been at it for a bit. The bad news is (for me, this is actually good news lol) that the issue @akleja brought up already being dealt with here: https://github.com/tenacityteam/tenacity/pull/559/ Just leaving it here for posterity. |
@panos Oh sorry, I should've posted here when I opened the PR, my bad |
0b85e1f
to
07bb880
Compare
Not sure if this is to be reviewed. |
Sorry for the lack of activity here. |
Sorry, kind of missed something here. What kind of problem are we talking about? CC: @emabrey |
It just segfaults on launch. The first step in this PR was to remove the old buttons from Meter.cpp and I think I messed up MeterAx when I did so. But it could be caused by something completely different as well, I'm mostly guessing here. I don't have the possibility of setting up a win development VM right now due to having an old machine and very limited hard drive space, as well as a lack of familiarity with development on windows. Which leaves me with the option of pushing possible fixes and check the artifacts, which seems like a tedious approach and seems like it would clog the build queue. If anybody can provide some insight, or at least a backtrace I'd be really thankful. |
@akleja Would it help if I provided you with a Windows VM? It'd be very clunky and running on my laptop because I was planning to install Windows on that thing for development purposes myself, but I guess that's one way to go around this. |
@panos Thanks, but I'm not sure, it sounds like a last resort type of solution, but if everything else falls through it might be an option. Just realized I do have an ancient laptop, so I'll try using it. |
An additional border in the floating toolbar is not necessary. The OS draws a border already Signed-off-by: akleja <[email protected]>
Signed-off-by: akleja <[email protected]>
Signed-off-by: akleja <[email protected]>
Removes icon from meter Signed-off-by: akleja <[email protected]>
Adds Buttons to Input/Output toolbars Signed-off-by: akleja <[email protected]> Bind only used events to buttons Update MeterToolBar.cpp
Signed-off-by: akleja <[email protected]>
Signed-off-by: akleja <[email protected]>
Signed-off-by: akleja <[email protected]>
Updates toolbars default position Signed-off-by: akleja <[email protected]>
07bb880
to
34cf8f3
Compare
Just a small update. I have a windows build environment set up and I'm making some progress. |
On prefs close toolbars are redrawn. Signed-off-by: akleja <[email protected]>
If it is causing crashing, I suggest we have quite a few testers to mess around with this version. |
@peepopoggers Absolutely, especially when it's finished for review it'll need extensive testing since it's quite a big change. There's still a lot to do though. I have at least resolved the crashing issues for now it seems. |
Is this ready to be tested? (Should I change the name and then share this as much as possible?) |
No not yet, I still have to figure out some of the accessibility issues, like a good way to include the dropdowns in the extra menu, and improve screen reader behaviour. |
Okay, got it, take your time :) |
Resolves: #435
Merges functionality of Meter Toolbars,
Mixer toolbarand Device toolbarAdds volume sliders below the meters.Todo:
Background:
At first I thought the best way to go about this was to just rely on the Prefs>Device. But for users having multiple audio devices that are switched between frequently, like say a bluetooth headset and internal soundcard it seems reasonable to make changing devices easy.
Additional notes:
Combined toolbar is enabled temporarily, but will be disabled again eventually.Current appearance:
Checklist
-s
orSigned-off-by
* (See: Contributing § DCO)* indicates required