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.

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

Define JACK headers and sources globally and add some documentation #3046

Merged
merged 2 commits into from
May 3, 2023

Conversation

ann0see
Copy link
Member

@ann0see ann0see commented Apr 15, 2023

Short description of changes
Instead of using hardcoded paths to the sound files @pgScorpio suggested to use variables. This PR uses global definitions for JACK sources and headers and adds some small comments.

CHANGELOG: Internal: Improved maintainability of Jamulus.pro by using global definitions.

Context: Fixes an issue?
Fixes: #2573

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

Status of this Pull Request
Ready for testing

What is missing until this pull request can be merged?
Needs whole CI to pass

Checklist

  • I've verified that this Pull Request follows the general code principles
  • I tested my code and it does what I want (Linux (JACK gives no warning and sound is OK), Windows too. I can't test macOS at the moment - but build is OK.)
  • 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

AUTOBUILD: Please build all targets

@ann0see ann0see added this to the Release 3.10.0 milestone Apr 15, 2023
@ann0see ann0see added this to Triage in Tracking (old) via automation Apr 15, 2023
@ann0see ann0see requested a review from pljones April 15, 2023 18:30
@ann0see ann0see moved this from Triage to Waiting on Team in Tracking (old) Apr 15, 2023
Jamulus.pro Outdated
@@ -68,6 +68,35 @@ INCLUDEPATH_OPUS = libs/opus/include \
libs/opus/silk/fixed \
libs/opus

# Add include paths and headers for new audio APIs here

# ASIO
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd split this into two parts:

  • provided by the SDK
  • "part of Jamulus"

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd also make all the variable definitions be either

VARNAME =

or

VARNAME = value

depending on the config that makes them relevant.

Copy link
Member Author

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

Done

@pljones
Copy link
Collaborator

pljones commented Apr 20, 2023

Done

image

I'm not sure if you misunderstood or forgot a commit here but this doesn't look to have done what I suggested.

@ann0see
Copy link
Member Author

ann0see commented Apr 20, 2023

Hmm. Then I misunderstood you. I thought you were referring to splitting the ASIO part out to another variable? Can you please provide a suggestion what you'd like to see?

Jamulus.pro Outdated
# Oboe
HEADERS_OBOE = src/sound/oboe/sound.h
SOURCES_OBOE = src/sound/oboe/sound.cpp \
src/android/androiddebug.cpp # TODO: Remove debugging code if no longer needed
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should line 97 be done more like

SOURCES_OBOE = src/sound/oboe/sound.cpp
# TODO: Remove debugging code if no longer needed else move to src/sound if part of sound
SOURCES_OBOE += src/android/androiddebug.cpp

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok. Yes. Done.

@pljones
Copy link
Collaborator

pljones commented Apr 21, 2023

I'm starting to think this is creating a lot of variables for no benefit.

The variables should only be added to the overall HEADERS, etc, variables conditionally - so wouldn't it be better to do this (addding the headers, etc, separately) where that conditional adding of the variables is done?

@ann0see
Copy link
Member Author

ann0see commented Apr 26, 2023

@pljones so you mean basically that we should revert this? I think @pgScorpio mainly wanted to get rid of the duplication introduced by the JACK headers.

@pljones
Copy link
Collaborator

pljones commented Apr 27, 2023

@pljones so you mean basically that we should revert this? I think @pgScorpio mainly wanted to get rid of the duplication introduced by the JACK headers.

Are the variables used in multiple places? Otherwise there's no duplication being removed.

@ann0see
Copy link
Member Author

ann0see commented Apr 27, 2023

The JACK headers are used for all three main OS.

@pljones
Copy link
Collaborator

pljones commented Apr 28, 2023

The JACK headers are used for all three main OS.

OK, so it's probably best done just for the JACK values, right?

@ann0see ann0see changed the title Use variables for sound headers Define JACK headers and sources globally and add some documentation Apr 29, 2023
@ann0see
Copy link
Member Author

ann0see commented Apr 29, 2023

@pljones I've now updated my PR and split it into two commits. I hope that's ok for all.

@pljones
Copy link
Collaborator

pljones commented May 1, 2023

Looks good.

@ann0see ann0see merged commit 5034975 into jamulussoftware:main May 3, 2023
Tracking (old) automation moved this from Waiting on Team to Done May 3, 2023
@ann0see ann0see deleted the addJamulusProVars branch May 3, 2023 20:58
@pljones pljones added the refactoring Non-behavioural changes, Code cleanup label Jun 4, 2023
@pljones pljones added the good first issue Things which should be doable without lots of context label Jun 4, 2023
@pljones pljones removed this from Done in Tracking (old) Jul 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Things which should be doable without lots of context refactoring Non-behavioural changes, Code cleanup
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

improve maintainability Jamulus.pro
2 participants