Refactoring: Move sound sources into src/sound folder #2575

ann0see · 2022-04-01T21:06:34Z

Short description of changes

Moves the sound.cpp / sound.h / sound.mm files to src/sound/

CHANGELOG: Refactoring: Moved sound API files into src/sound folder. Contributors will now find the sound.cpp/sound.h/sound.mm in the src/sound folder for consistency

Context: Fixes an issue?

Related to: #2573

@pgScorpio, I assume you should also have a look at this PR.

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

Status of this Pull Request

Waiting on second review

What is missing until this pull request can be merged?

Second review

Checklist

I've verified that this Pull Request follows the general code principles
I tested my code and it does what I want (works on Linux and Windows. Since the CI is clean it should be 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

pgScorpio · 2022-04-02T00:14:41Z

@ann0see
Yes, I will keep an eye on this PR too...

And if we indeed are going to change the src tree we maybe should have a look if it wouldn't be useful to split up some more at the same time... i.e. Common, Client, Server, Protocol,....

EDIT: Also the generated ui_*.h files should not go into root but into a src/generated folder

And would it be possible to get the same structure in the editors (Qt Creator, VS, XCode) too? That would be a great improvement too.

hoffie

Didn't have a deeper look yet, just some thoughts. As said elsewhere, I like this idea.

Did you check translations? It looks like each .ts file contains references to the source path, so I guess they must/should be updated to match the new location to avoid all translations showing up as new? I haven't checked though if this is really the case. Maybe Qt tooling is intelligent enough to update it itself, but it should be checked nevertheless.
Various checks or scripts contain explicit references to our code directories. As everything is moved to the existing src/, this should be fine already, but I'm mentioning it nevertheless.
I didn't think of the fact that windows/ etc. are not gone automatically, as they contain build scripts, platform-specific resources (icons, installers). Some (at least mac/) continues to ship source files. Would be nice to get rid of that as well, but maybe in a follow-up PR...
I think it would be logical to move the windows/ASIOSDK2 extraction target. Either, it should be moved along with the code which is using it or it could be moved to libs/ where all other third-party stuff lives. As this is closely related to the move of the ASIO sound implementation, I think this should be done in this PR as well. It will likely affect docs and build scripts though.

ann0see · 2022-04-02T20:05:01Z

Hmm. Moving ASIO around involves some bigger changes, I think?

I'd like to have it in libs.

ann0see · 2022-04-02T20:14:37Z

@hoffie just updated the .ts files

ann0see · 2022-04-02T20:15:14Z

I didn't think of the fact that windows/ etc. are not gone automatically, as they contain build scripts, platform-specific resources (icons, installers). Some (at least mac/) continues to ship source files. Would be nice to get rid of that as well, but maybe in a follow-up PR...

Yes. But this needs a bit of restructuring other places too...

Jamulus.pro

pgScorpio · 2022-04-02T20:57:55Z

Hmm. Moving ASIO around involves some bigger changes, I think?

I'd like to have it in libs.

I agree that that seems more consistent. oboe is also in libs.
Though I don't see yet why that would involve bigger changes. Changing base folder is changing base folder, whether it is src/sound/asio/ or src/libs/ doesn't make that much of a difference or do I mis something here ?

ann0see · 2022-04-02T20:58:59Z

No. I did that already in this PR - and it seems to build fine.

Jamulus.pro

COMPILING.md

Jamulus.pro

pgScorpio · 2022-04-07T19:51:04Z

move sound API files into src/sound folder

To keep related things together please move soundbase.h and soundbase.cpp to ./Sound too !

ann0see · 2022-04-07T20:55:37Z

@pgScorpio moved the soundbase files too.

Also added a README stub which you can use for documenting your new design + ideas.

ann0see · 2022-04-08T19:15:47Z

What's missing here?

pgScorpio

Looks good to me

hoffie

Looks good to me. Thanks! :)

ann0see · 2022-04-09T19:48:35Z

@pljones is this ok to be merged? @pgScorpio depends on this being merged such that he can continue

ann0see · 2022-04-16T20:00:08Z

@jamulussoftware/maindevelopers any review here? I think we should merge this.

ann0see · 2022-04-17T17:12:16Z

Rebased. I'll open another PR based on this branch moving the other files soon (as requested by @hoffie)

pljones · 2022-04-17T17:21:10Z

One thing I noticed when I was doing my various changes... linux/sound.h depends on src/server.h. It shouldn't - not sure if you want to go fixing this here, though. (It means moving the high resolution timer from src/server.* to src/util.* - was a simple cut-n-paste.)

ann0see · 2022-04-17T18:02:08Z

I wouldn't want to include it here. This should just move the files without many changes

pgScorpio · 2022-04-17T18:10:41Z

One thing I noticed when I was doing my various changes... linux/sound.h depends on src/server.h. It shouldn't - not sure if you want to go fixing this here, though.

Should be another PR I think

(It means moving the high resolution timer from src/server.* to src/util.* - was a simple cut-n-paste.)

To keep things clean I would create a new .h/.cpp i.e. 'timers'.
I personally don't call a timer a 'util' and in my opinion util is already becoming a receptacle.

EDIT: This will be solved for linux/sound in the sound-redesign anyway...

ann0see · 2022-04-20T17:43:21Z

@pljones could you please review this PR?

The sound.h server.h dependency should be part of another PR, so this isn't blocking.

ann0see · 2022-04-30T19:33:16Z

@hoffie @npostavs since you're both familiar with the code could you please review? I assume this is safe to merge.

Jamulus.pro

npostavs · 2022-04-30T20:36:50Z

Jamulus.pro

-            !exists(windows/ASIOSDK2) {
-                error("Error: ASIOSDK2 must be placed in Jamulus windows folder.")
+            !exists(libs/ASIOSDK2/common) {
+                error("Error: ASIOSDK2 must be placed in Jamulus \\libs folder such that e.g. \\libs\ASIOSDK2\common exists.")


Ah, you added two more unescaped backslashes.

I double checked it, and it seems that qmake treats \\x and \x the same (with exception of x = "), so I guess you could just leave them unescaped, but it still seems cleaner to have it escaped, IMO.

ann0see · 2022-05-09T21:37:52Z

Note to maintainers: Should be squash-merged.

ann0see · 2022-05-09T21:45:46Z

Since @pgScorpio depends on this and I think it's save, I will merge this in the following days if nobody objects

ann0see · 2022-05-15T06:08:29Z

@jamulussoftware/maindevelopers I will merge this today

ann0see force-pushed the patch/moveSound branch from 972b4bc to d0abc00 Compare April 1, 2022 21:15

ann0see marked this pull request as ready for review April 2, 2022 19:15

ann0see mentioned this pull request Apr 2, 2022

improve maintainability Jamulus.pro #2573

Closed

ann0see force-pushed the patch/moveSound branch from d0abc00 to c5a4670 Compare April 2, 2022 19:32

hoffie reviewed Apr 2, 2022

View reviewed changes

hoffie added this to the Release 3.9.0 milestone Apr 2, 2022

ann0see force-pushed the patch/moveSound branch from 6f72bdc to 8a93a22 Compare April 2, 2022 20:13

ann0see commented Apr 2, 2022

View reviewed changes

Jamulus.pro Outdated Show resolved Hide resolved

ann0see force-pushed the patch/moveSound branch from 1ae014d to 3425b83 Compare April 2, 2022 20:34

pgScorpio reviewed Apr 2, 2022

View reviewed changes

Jamulus.pro Show resolved Hide resolved

COMPILING.md Outdated Show resolved Hide resolved

Jamulus.pro Outdated Show resolved Hide resolved

ann0see force-pushed the patch/moveSound branch 3 times, most recently from 133e7fc to 1901792 Compare April 7, 2022 20:53

ann0see requested a review from hoffie April 8, 2022 19:13

ann0see requested a review from pljones April 8, 2022 19:17

pgScorpio approved these changes Apr 8, 2022

View reviewed changes

hoffie approved these changes Apr 8, 2022

View reviewed changes

ann0see added the refactoring Non-behavioural changes, Code cleanup label Apr 16, 2022

ann0see force-pushed the patch/moveSound branch from 1901792 to 64c04a5 Compare April 17, 2022 17:07

npostavs reviewed Apr 30, 2022

View reviewed changes

Jamulus.pro Outdated Show resolved Hide resolved

ann0see force-pushed the patch/moveSound branch from 64c04a5 to d8d00df Compare April 30, 2022 20:01

npostavs reviewed Apr 30, 2022

View reviewed changes

ann0see force-pushed the patch/moveSound branch from d8d00df to 6101576 Compare April 30, 2022 20:37

Refactoring: Move sound sources into src/sound folder

8314b2d

ann0see force-pushed the patch/moveSound branch from 2b7871b to 8314b2d Compare May 15, 2022 06:06

ann0see merged commit e875d60 into jamulussoftware:master May 15, 2022

ann0see mentioned this pull request May 15, 2022

linux/sound.h should not depend on src/server.h #2628

Closed

ann0see deleted the patch/moveSound branch May 15, 2022 06:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring: Move sound sources into src/sound folder #2575

Refactoring: Move sound sources into src/sound folder #2575

ann0see commented Apr 1, 2022 •

edited

Loading

pgScorpio commented Apr 2, 2022 •

edited

Loading

hoffie left a comment •

edited by ann0see

Loading

ann0see commented Apr 2, 2022

ann0see commented Apr 2, 2022

ann0see commented Apr 2, 2022

pgScorpio commented Apr 2, 2022

ann0see commented Apr 2, 2022

pgScorpio commented Apr 7, 2022

ann0see commented Apr 7, 2022

ann0see commented Apr 8, 2022

pgScorpio left a comment

hoffie left a comment

ann0see commented Apr 9, 2022 •

edited

Loading

ann0see commented Apr 16, 2022

ann0see commented Apr 17, 2022 •

edited

Loading

pljones commented Apr 17, 2022

ann0see commented Apr 17, 2022

pgScorpio commented Apr 17, 2022 •

edited

Loading

ann0see commented Apr 20, 2022 •

edited

Loading

ann0see commented Apr 30, 2022

npostavs Apr 30, 2022

ann0see May 9, 2022

ann0see commented May 9, 2022

ann0see commented May 9, 2022

ann0see commented May 15, 2022

Refactoring: Move sound sources into src/sound folder #2575

Refactoring: Move sound sources into src/sound folder #2575

Conversation

ann0see commented Apr 1, 2022 • edited Loading

Checklist

pgScorpio commented Apr 2, 2022 • edited Loading

hoffie left a comment • edited by ann0see Loading

Choose a reason for hiding this comment

ann0see commented Apr 2, 2022

ann0see commented Apr 2, 2022

ann0see commented Apr 2, 2022

pgScorpio commented Apr 2, 2022

ann0see commented Apr 2, 2022

pgScorpio commented Apr 7, 2022

ann0see commented Apr 7, 2022

ann0see commented Apr 8, 2022

pgScorpio left a comment

Choose a reason for hiding this comment

hoffie left a comment

Choose a reason for hiding this comment

ann0see commented Apr 9, 2022 • edited Loading

ann0see commented Apr 16, 2022

ann0see commented Apr 17, 2022 • edited Loading

pljones commented Apr 17, 2022

ann0see commented Apr 17, 2022

pgScorpio commented Apr 17, 2022 • edited Loading

ann0see commented Apr 20, 2022 • edited Loading

ann0see commented Apr 30, 2022

npostavs Apr 30, 2022

Choose a reason for hiding this comment

ann0see May 9, 2022

Choose a reason for hiding this comment

ann0see commented May 9, 2022

ann0see commented May 9, 2022

ann0see commented May 15, 2022

ann0see commented Apr 1, 2022 •

edited

Loading

pgScorpio commented Apr 2, 2022 •

edited

Loading

hoffie left a comment •

edited by ann0see

Loading

ann0see commented Apr 9, 2022 •

edited

Loading

ann0see commented Apr 17, 2022 •

edited

Loading

pgScorpio commented Apr 17, 2022 •

edited

Loading

ann0see commented Apr 20, 2022 •

edited

Loading