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

Draft: New toolbar layout #58

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

generic-pers0n
Copy link
Member

Draft PR for a possible new toolbar layout. This is in its early stages and is not guaranteed to make it through.

Related to: #57 (this is a discussion, not an issue; I will allow PRs to link to discussions instead of issues, but should say "Related to" instead of "Resolves")

  • I made sure the code compiles on my machine*
  • I made sure that my contributes are licensed under GPLv2 or later.*
  • 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.

(an optional longer description of the changes)

@generic-pers0n
Copy link
Member Author

generic-pers0n commented Sep 21, 2022

https://github.com/tenacityteam/tenacity/pull/543

New plan: cherry pick commits from this PR onto this one, adding on to our work. Maybe merge some advanced functionality into a new toolbar, much like Audacity's audio setup toolbar (starting in 3.2).

@generic-pers0n generic-pers0n added the enhancement New feature or request label Sep 21, 2022
@generic-pers0n
Copy link
Member Author

@akleja If you'd like to help draft a new toolbar layout, you can start work on this PR bringing over your ideas from Tenacity or even bringing new ideas to the table. I can assign this PR to you so you can make most of the shots here. Just try not to rewrite everything because of our impending toolkit transition 😆. (Unless it's to clean up and refactor existing code, but please try to be thoughtful of the impending toolkit transition).

@akleja
Copy link
Collaborator

akleja commented Oct 2, 2022

@generic-pers0n sorry for not replying earlier, the notification drowned out in the vast sea of notifications, plus I've been a bit extra busy with work this past week.
But I'd be glad to work on this!

I was having a few minor problems I never got around to solving though.

One is related to accessibility and how wxWidgets work. I wanted the device selection to be available under the extras menu as well, since it exist for accessibility reasons. But if a menu is assigned as a sub-menu, you can't display it outside of the menu as a context menu as well. So I need to store the menu data separately rather than in the menu object and create both menus by getting that data. The menu items is also what is used as actions in the scripting stuff, and by removing the device toolbar you loose some of that functionality.

Another thing that was less than ideal is that I was querying the device handler on opening the menu to make sure to get an updated list of audio devices, which made the menu not that responsive. I'm not sure about the best method to solve it. I was thinking of just adding a menu item in the context menus, or a toolbar button for refreshing the list of devices. Maybe it would be okay. But then it would be possible to select a device that might not exist any more, so then those cases must be handled.

@generic-pers0n
Copy link
Member Author

generic-pers0n commented Oct 2, 2022

Another thing that was less than ideal is that I was querying the device handler on opening the menu to make sure to get an updated list of audio devices, which made the menu not that responsive. I'm not sure about the best method to solve it. I was thinking of just adding a menu item in the context menus, or a toolbar button for refreshing the list of devices. Maybe it would be okay. But then it would be possible to select a device that might not exist any more, so then those cases must be handled.

I think I have an idea for this: rather than querying the device handler when the menu is clicked, query it on a rescanned device event. You can look at how the device toolbar does this (DeviceToolBar::OnRescannedDevices is the staring point, but that just calls RefillCombos next). That way, you can update the menu data when a rescanned device event happens.

A word of caution: I'm looking at migrating these events away from wxWidgets to something Audacity's Observer system, which might be itself migrated over to Boost.Signals2. I haven't done anything yet, but please keep that in mind.

As for the first issue, I'm not sure and will have to read about that further to make sure I'm understanding everything.


TL;DR: Look at how DeviceToolBar works and maybe try to merge that functionality into what you're doing.

@akleja
Copy link
Collaborator

akleja commented Oct 2, 2022

I didn't realize the device manager sent an event, that's really handy, thanks!

Maybe I didn't explain the first part that good. But it basically means I need to restructure the handling of the data a bit, but it'll be a good thing since it will make it easier when moving away from wxWidgets.

So far I've just rebased my old patches and started looking through the mess of code I wrote. At least it compiles and the functionality is intact, but I'll have to sort out the above mentioned issues, and generally tidy up things a bit.

@generic-pers0n
Copy link
Member Author

I didn't realize the device manager sent an event, that's really handy, thanks!

Maybe I didn't explain the first part that good. But it basically means I need to restructure the handling of the data a bit, but it'll be a good thing since it will make it easier when moving away from wxWidgets.

So far I've just rebased my old patches and started looking through the mess of code I wrote. At least it compiles and the functionality is intact, but I'll have to sort out the above mentioned issues, and generally tidy up things a bit.

Nice to know things still work! That saves us some time :D

You also explained things well and clear. It's just how we're going to reorganize this data now. Either way, since we're talking about restructuring this data and moving away from wxWidgets, it'll be a good thing like you said.

If you need assistance with anything or have questions, feel free to ask them here and we'll figure it out together. :D

@akleja
Copy link
Collaborator

akleja commented Oct 2, 2022

Yeah I didn't expect it. But yeah I'm rewriting a lot of it.

It was way simpler than I thought. I just made a struct for the data and put it in a vector. Then I can clear it when the event triggers and store device id, name, status etc. So then I'll be able to create the menus I want with the same data. Plus I can have a less awful way of passing device id from menu event to the device handler.

Thanks! I'll might have some questions when I'm further along :)

Saucedacity's default settings now place the time toolbar next to the
transport toolbar. This isn't a big change in itself, but it could mean
the start of a toolbar rearrangement.

Signed-off-by: Avery King <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants