Skip to content
This repository has been archived by the owner on Dec 18, 2022. It is now read-only.

WIP: Input/Output toolbars #543

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

akleja
Copy link
Contributor

@akleja akleja commented Aug 27, 2021

Resolves: #435

Merges functionality of Meter Toolbars, Mixer toolbar and Device toolbar

  • Removes device toolbar.
  • Adds menu buttons next to the meters for selecting device/channel count.
  • Defines new default positions for the toolbars.
  • Adds volume sliders below the meters.

Todo:

  • Verify that I/O assignment works as it should
  • Add small down arrows to buttons to indicate menu
  • Sort out translatable strings / tooltips
  • Improve min-sizes
  • Acessiblity
    • Add device selection functionality to Extra menu
    • Test accessibility on windows for meter
    • Add Accessibility class for buttons / menus on windows?
  • Reduce reliance of hardcoded numbers for sizing
  • Cleanup of code / comments / commits
  • Rename toolbars from Record Toolbar/Playback Toolbar to Input Toolbar/Output Toolbar
  • Resolve startup crash on windows

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:

  • I can't get keyboard focus to work with widgets/AButton, but that seems to be an issue with AButton, and should probably be solved separately since I have the same problem with the other instances of AButton.
  • Combined toolbar is enabled temporarily, but will be disabled again eventually.
  • This is a work in progress. Some things might behave badly.
  • Some solutions aren't the best since I'm a novice. Suggestions for improvements are welcome.

Current appearance:
io-toolbar_current

Checklist
  • I have signed off my commits using -s or Signed-off-by* (See: Contributing § DCO)
  • I made sure the code compiles on my machine
  • I made sure there are no unnecessary changes in the code*
  • I made sure the title of the PR reflects the core meaning of the issue you are solving*
  • I made sure the commit message(s) contain a description and answer the question "Why do those changes fix that particular issue?" or "Why are those changes really necessary as improvements?"*

* indicates required

@akleja
Copy link
Contributor Author

akleja commented Aug 27, 2021

The windows build seems broken,
Since I don't have a windows build environment this will be tricky to investigate further for now.

@Be-ing
Copy link
Contributor

Be-ing commented Aug 27, 2021

What do you mean? The Windows build passed.

@Be-ing
Copy link
Contributor

Be-ing commented Aug 27, 2021

I edited your todo list so it shows actual checkboxes.

@akleja
Copy link
Contributor Author

akleja commented Aug 27, 2021

Yeah I didn't word it properly. What I meant is that the windows version crashes on launch. Sorry for lack of clarity

@fitojb
Copy link
Contributor

fitojb commented Aug 28, 2021

Since it’s a drop-down button, it should have a ▼ next to it

@akleja
Copy link
Contributor Author

akleja commented Aug 28, 2021

@fitojb Yeah I agree! It's on the to do list :)

@Be-ing
Copy link
Contributor

Be-ing commented Aug 28, 2021

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.

@akleja
Copy link
Contributor Author

akleja commented Aug 28, 2021

I guess splitting some things off would work, and might make sense.
Removal of Device/Mixer toolbars could fairly easily be split off.
But the changes to meter toolbars and widget/Meter might be hard to split up in a way that makes 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.

@Be-ing
Copy link
Contributor

Be-ing commented Aug 28, 2021

I think just separating the commits a bit more strictly might be the better option.

Sure.

Also I'm unsure about keep supporting the combined toolbar.

When in doubt, I say remove code.

@Be-ing Be-ing mentioned this pull request Aug 29, 2021
5 tasks
@Be-ing
Copy link
Contributor

Be-ing commented Aug 29, 2021

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.

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.

@akleja
Copy link
Contributor Author

akleja commented Aug 29, 2021

When in doubt, I say remove code.

I've begun removed the combined meter. It's a relief to get rid of it tbh.

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.

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.
But if that is the way forward I think the rest of the PortMixer stuff should be stripped out in it's own PR along with the removal of the mixer toolbar. That I can easily break of from this PR.

@Be-ing
Copy link
Contributor

Be-ing commented Aug 29, 2021

Maybe we should put this PR on hold and remove all the PortMixer stuff first.

@akleja
Copy link
Contributor Author

akleja commented Aug 29, 2021

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.
Then strip out the slider implementation in this PR and move forward.
I'll start looking into it tomorrow. At a first glance it doesn't seem too troublesome.

@akleja
Copy link
Contributor Author

akleja commented Aug 29, 2021

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.

@n0toose
Copy link
Member

n0toose commented Aug 30, 2021

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.

@n0toose
Copy link
Member

n0toose commented Aug 30, 2021

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.

@akleja
Copy link
Contributor Author

akleja commented Aug 30, 2021

@panos Oh sorry, I should've posted here when I opened the PR, my bad

@akleja akleja force-pushed the io-toolbar-wip-rebase branch 2 times, most recently from 0b85e1f to 07bb880 Compare September 2, 2021 13:08
@n0toose
Copy link
Member

n0toose commented Sep 16, 2021

Not sure if this is to be reviewed.

@akleja
Copy link
Contributor Author

akleja commented Oct 1, 2021

Sorry for the lack of activity here.
I'm not sure how to move forward with this PR since there's a problem on windows which I don't know how to debug. I suspect it is linked to the accessibility features in widgets/Meter.cpp, but this is purely speculative. Any help on this would be much appreciated.

@n0toose
Copy link
Member

n0toose commented Oct 5, 2021

Sorry, kind of missed something here. What kind of problem are we talking about?

CC: @emabrey

@akleja
Copy link
Contributor Author

akleja commented Oct 5, 2021

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.

@n0toose
Copy link
Member

n0toose commented Oct 6, 2021

@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.

@akleja
Copy link
Contributor Author

akleja commented Oct 6, 2021

@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 <storspov@gmail.com>
Signed-off-by: akleja <storspov@gmail.com>
Signed-off-by: akleja <storspov@gmail.com>
Removes icon from meter

Signed-off-by: akleja <storspov@gmail.com>
Adds Buttons to Input/Output toolbars

Signed-off-by: akleja <storspov@gmail.com>

Bind only used events to buttons

Update MeterToolBar.cpp
Signed-off-by: akleja <storspov@gmail.com>
Signed-off-by: akleja <storspov@gmail.com>
Signed-off-by: akleja <storspov@gmail.com>
Updates toolbars default position
Signed-off-by: akleja <storspov@gmail.com>
@akleja
Copy link
Contributor Author

akleja commented Oct 8, 2021

Just a small update. I have a windows build environment set up and I'm making some progress.
Startup crash on windows is resolved.
Crash on windows when changing host by accessing device settings from Input/Output menus detected. Investigating.

On prefs close toolbars are redrawn.
Signed-off-by: akleja <storspov@gmail.com>
@peepo5
Copy link

peepo5 commented Oct 11, 2021

If it is causing crashing, I suggest we have quite a few testers to mess around with this version.

@akleja
Copy link
Contributor Author

akleja commented Oct 11, 2021

@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.

@n0toose
Copy link
Member

n0toose commented Oct 12, 2021

Is this ready to be tested? (Should I change the name and then share this as much as possible?)

@akleja
Copy link
Contributor Author

akleja commented Oct 12, 2021

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.
Also I'll have to add arrows to the buttons.
And tidy up code.
I'm working on it, but I'm having a busy week. Should be able to make more progress during the weekend

@n0toose
Copy link
Member

n0toose commented Oct 12, 2021

Okay, got it, take your time :)

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

Successfully merging this pull request may close these issues.

On Linux, "audio device selector" toolbars is too small and dropdowns are cut off
5 participants