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

improve maintainability Jamulus.pro #2573

Closed
pgScorpio opened this issue Apr 1, 2022 · 4 comments · Fixed by #3046
Closed

improve maintainability Jamulus.pro #2573

pgScorpio opened this issue Apr 1, 2022 · 4 comments · Fixed by #3046
Assignees
@ann0see
Labels
good first issue Things which should be doable without lots of context refactoring Non-behavioural changes, Code cleanup
Milestone
Release 3.10.0

Comments

@pgScorpio
Copy link
Contributor

Describe the bug

In the past month there where a lot of changes to Jamulus.pro, leading to a lot of rebase actions with a high chance on mistakes.
Especially for me since I have a sound-redesign branch with added files for Sound in the OS folders.
(PS: there are also several headers missing in the definitions for ASIO)

The headers and sources for sound (especially linux) are used in multiple places in the Jamulus.pro file and therefore have to be changed in multiple places too.
I suggest we use separate defines for the sound files which can be used at many places, but have to be edited in just one place.

To Reproduce

n.a.

Expected behavior

Easier maintenance, less prone to mistakes/errors.

Screenshots

n.a.

Operating system

all

Version of Jamulus

n.a.

Additional context
General idea:

HEADERS_ASIO= ...
SOURCES_ASIO= ...

HEADERS_JACK= ...
SOURCES_JACK= ...

HEADERS_COREAUDIO= ...
SOURCES_COREAUDIO= ...

HEADERS_COREAUDIOIOS= ...
SOURCES_COREAUDIOIOS= ...

HEADERS_OBOE= ...
SOURCES_OBOE= ...

....

HEADERS+=$$HEADERS_JACK
SOURCES+=$$SOURCES_JACK

...

HEADERS+=$$HEADERS_COREAUDIO
SOURCES+=$$SOURCES_COREAUDIO

....

HEADERS+=$$HEADERS_JACK
SOURCES+=$$SOURCES_JACK

Another point is that it looks very strange when including "linux" files in a Windows build etc.
So it would be more clear when using separate folders for the sound implementation files like sound/jack, sound/asio, sound/coreaudio, sound/oboe, ...
@pgScorpio pgScorpio added the bug Something isn't working label Apr 1, 2022
@pgScorpio
Copy link
Contributor Author

See my implementation in my sound-redesign branch.
(which has several added files, so not valid for current master.)

@hoffie
Copy link
Member

hoffie commented Apr 1, 2022 •
edited
Loading

  1. I like the idea of having the includes grouped together (although I don't have a strong opinion here)
  2. I totally agree regarding the backend-specific directory structure. I don't know why those directories should be at the root of the repo either. src/sound/{jack,asio,oboe,coreaudio,...} would seem logical to me. I would certainly support this change.

Edit: When submitting a PR, the renames should not contain any other changes to the contents of the renamed files (except changes which are relevant to the rename, of course ;)).

@ann0see ann0see mentioned this issue Apr 1, 2022
Merged
5 tasks
@ann0see
Copy link
Member

ann0see commented Apr 2, 2022

@hoffie #2575 is ready for review now. I'm not 100% sure about the src/sound/{...} names, but feel free to suggest alternatives

@ann0see ann0see added refactoring Non-behavioural changes, Code cleanup and removed bug Something isn't working labels Apr 3, 2022
@ann0see
Copy link
Member

ann0see commented Apr 8, 2022

Missing here:
Change the files to variables

@ann0see ann0see added the good first issue Things which should be doable without lots of context label Oct 14, 2022
@ann0see ann0see added this to the Release 3.10.0 milestone Apr 13, 2023
@ann0see ann0see self-assigned this Apr 13, 2023
ann0see added a commit to ann0see/jamulus that referenced this issue Apr 15, 2023
@ann0see @pgScorpio
Use variables for sound headers
730be46 
Fixes: jamulussoftware#2573

Also adds some comments and small changes

Co-authored-by: pgScorpio <[email protected]>
@ann0see ann0see mentioned this issue Apr 15, 2023
Merged
5 tasks
ann0see added a commit to ann0see/jamulus that referenced this issue Apr 19, 2023
@ann0see @pgScorpio
Use variables for sound headers
3fd0f3a 
Fixes: jamulussoftware#2573

Also adds some comments and small changes

Co-authored-by: pgScorpio <[email protected]>
ann0see added a commit to ann0see/jamulus that referenced this issue Apr 19, 2023
@ann0see @pgScorpio
Use variables for sound headers
cb4867f 
Fixes: jamulussoftware#2573

Also adds some comments and small changes

Co-authored-by: pgScorpio <[email protected]>
ann0see added a commit to ann0see/jamulus that referenced this issue Apr 26, 2023
@ann0see @pgScorpio
Use variables for sound headers
5b6fdbc 
Fixes: jamulussoftware#2573

Also adds some comments and small changes

Co-authored-by: pgScorpio <[email protected]>
ann0see added a commit to ann0see/jamulus that referenced this issue Apr 29, 2023
@ann0see @pgScorpio
Use variables for sound headers
a5c7074 
Fixes: jamulussoftware#2573

Also adds some comments and small changes

Co-authored-by: pgScorpio <[email protected]>
@pljones pljones added this to Tracking Jul 28, 2023
@github-project-automation github-project-automation bot moved this to Triage in Tracking Jul 28, 2023
@pljones pljones moved this from Triage to Done in Tracking Jul 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ann0see ann0see

Labels
good first issue Things which should be doable without lots of context refactoring Non-behavioural changes, Code cleanup
Projects
Tracking
Status: Done
Milestone
Release 3.10.0
Development

Successfully merging a pull request may close this issue.

Define JACK headers and sources globally and add some documentation ann0see/jamulus
3 participants
@hoffie @pgScorpio @ann0see