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.

Sign up for GitHub

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

Jump to bottom

Add "Sort by Channel" to mixer view #3418

Merged
merged 1 commit into from
Oct 29, 2024
Merged

Add "Sort by Channel" to mixer view #3418

merged 1 commit into from
Oct 29, 2024

Conversation

pljones
Copy link
Collaborator

@pljones pljones commented Oct 25, 2024

Short description of changes

Adds "Sort by Channel" to mixer view

CHANGELOG: Client: Add "Sort by Channel" to mixer view

Context: Fixes an issue?

Fixes #3417

Does this change need documentation? What needs to be documented and how?

Yes. The new option needs documenting. The entry should explain its relevance to --ctrlmidich.

Status of this Pull Request

Test working on Ubuntu, Qt 6.8.0

What is missing until this pull request can be merged?

Review, testing on other platforms.

Checklist

  • I've verified that this Pull Request follows the general code principles
  • I tested my code and it does what I want
  • My code follows the style guide
  • I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
  • I've filled all the content above

@pljones pljones force-pushed the sort-faders-by-channel branch from 7400201 to 5bda36c Compare October 25, 2024 18:53
@pljones pljones self-assigned this Oct 25, 2024
@pljones pljones added the feature request Feature request label Oct 25, 2024
@pljones pljones added this to the Release 3.12.0 milestone Oct 25, 2024
@pljones pljones force-pushed the sort-faders-by-channel branch 2 times, most recently from 9b91d75 to 7f56cb7 Compare October 25, 2024 19:03
ann0see
ann0see reviewed Oct 25, 2024
View reviewed changes
src/audiomixerboard.cpp Show resolved Hide resolved
src/audiomixerboard.cpp Show resolved Hide resolved
src/clientdlg.cpp Outdated Show resolved Hide resolved
@pljones pljones force-pushed the sort-faders-by-channel branch from 7f56cb7 to 9e569fa Compare October 26, 2024 10:37
@pljones pljones force-pushed the sort-faders-by-channel branch from 9e569fa to 9cc1d32 Compare October 26, 2024 10:47
@pljones pljones force-pushed the sort-faders-by-channel branch from 9cc1d32 to 01944f9 Compare October 26, 2024 10:56
ann0see
ann0see reviewed Oct 26, 2024
View reviewed changes
Copy link
Member

@ann0see ann0see left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll test it on my device soon.

src/util.h
@@ -558,11 +558,12 @@ enum ERecorderState
enum EChSortType
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not part of this PR, but having this enum in utils looks unorganized.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. I'll look to move it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK it's in util.h because it's used in CSettings as well as the mixer board, I think -- all of the static_cast<ENUM> enums that CSettings does get shared with somewhere else and CSettings doesn't want to be fishing through lots of headers, so they get dumped into util.h.

(Ideally, CSettings wouldn't work like that: each component would have its own settings and read or write them to the XML that CSettings simply read from a file or wrote to a file... But that is definitely a different PR!)

ann0see
ann0see approved these changes Oct 26, 2024
View reviewed changes
Copy link
Member

@ann0see ann0see left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works ok on Debian

@ann0see
Copy link
Member

ann0see commented Oct 26, 2024 •
edited
Loading

Should still have a quick look by @softins, but otherwise I think it's good to be merged.

@ann0see ann0see added the needs documentation PRs requiring documentation changes or additions label Oct 26, 2024
@softins
Copy link
Member

softins commented Oct 27, 2024

Should still have a quick look by @softins, but otherwise I think it's good to be merged.

Will look soon!

softins
softins approved these changes Oct 29, 2024
View reviewed changes
Copy link
Member

@softins softins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me - nice code!

Tested on a Pi with Qt 5.15.8

@ann0see ann0see merged commit aecefc7 into jamulussoftware:main Oct 29, 2024
11 checks passed
@pljones pljones mentioned this pull request Oct 29, 2024
Closed
@pljones
Copy link
Collaborator Author

pljones commented Oct 29, 2024

jamulussoftware/jamuluswebsite#1059 raised to get the documentation change done.

@pljones pljones deleted the sort-faders-by-channel branch October 29, 2024 18:12
@softins softins mentioned this pull request Oct 30, 2024
Closed
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ann0see ann0see ann0see approved these changes

@gilgongo gilgongo gilgongo approved these changes

@softins softins softins approved these changes

Assignees

@pljones pljones

Labels
feature request Feature request needs documentation PRs requiring documentation changes or additions
Projects
Tracking
Status: Done
Milestone
Release 3.12.0
Development

Successfully merging this pull request may close these issues.

Add "Sort by Channel" to mixer view
4 participants
@pljones @ann0see @softins @gilgongo