iOS sound support 2 by ngocdh · Pull Request #1865 · jamulussoftware/jamulus

ngocdh · 2021-06-12T21:44:29Z

I messed #1512 up. So this is a brand new branch for iOS sound support. I hope this is not creating much trouble for the team.

~~TODO: clang-format~~

…"Objective C++" option when compiling for iOS.

move mRecordingStream to public for DEBUGING (find devid)

pljones · 2021-06-13T12:26:24Z

android/sound.cpp

             << "frames_in: " << frames_in << ",frames_out: " << frames_out << ",frames_filled_out: " << frames_filled_out
             << ",in_callback_calls: " << in_callback_calls << ",out_callback_calls: " << out_callback_calls << ",ring_overrun: " << ring_overrun;
 }
+


Why is this android file changing in an iOS change?

pljones · 2021-06-13T12:27:44Z

ios/sound.h

 *
 * Author(s):
- *  ann0see based on code from Volker Fischer
+ *  ann0see and ngocdh based on code from Volker Fischer


and the copyright date could do with updating.

ios/sound.mm

pljones · 2021-06-13T12:29:41Z

ios/sound.mm

-  //vecsTmpAudioSndCrdStereo.Init ( iCoreAudioBufferSizeStereo );
-
-  return iCoreAudioBufferSizeMono;
+void checkStatus(int status){


Why are these global?

Also style is wrong here. I think clang-format is ignoring the .mm files, which it should treat like .cpp files.

pljones · 2021-06-13T12:31:04Z

src/chatdlg.cpp

    connect ( action, SIGNAL ( triggered() ), this, SLOT ( close() ) );
 #endif

+#if defined( ANDROID ) || defined( Q_OS_ANDROID )


Again, this change is for iOS, so why this android change?

pljones · 2021-06-13T12:33:41Z

src/clientdlg.cpp

    pMenu->addMenu ( pEditMenu );
    pMenu->addMenu ( new CHelpMenu ( true, this ) );

+#if defined( Q_OS_IOS ) or defined( Q_OS_ANDROID ) or defined( ANDROID )


I'd suggest this whole change - switching input device - be split out from the main pull request. It will make more sense as a separate change to have both iOS and android support then. Here the android bit looks lost.

pljones · 2021-06-13T12:38:01Z

OK, now I can see what's going on, I'd like the introduction of the new feature of selecting input device split into a separate change, dependent upon this one.

There's a lot happening here already. Having that in as an "different thing" just makes this PR too big, in my opinion.

Also, there are too many commits to simply merge this - we'd have to squash and that's a bit messy. Or someone would have to tidy it up a bit.

ngocdh · 2021-06-13T13:10:24Z

Thank you @pljones for the thorough review. I’ll check the .mm file style.

About the input selection feature, it would be complicated for me to split, now that the iOS PR is not yet merged.

And yeah I’d love to have my commits tidied up.

@pljones

Review of jamulussoftware#1865 by @pljones revealed .mm extension was not checked.

ann0see · 2021-06-13T19:24:46Z

About the input selection feature, it would be complicated for me to split, now that the iOS PR is not yet merged.

Yes, but the other problem is we can't merge this as is (#1865 (comment)). Can you have a look at related commits and describe which features this PR exactly adds?

Add iOS support
Allow switching between intenal/external microphones

If we exactly know what you changed, we can try to get this into smaller commits.

See solution 3 here: https://about.gitlab.com/blog/2018/06/07/keeping-git-commit-history-clean/

ngocdh · 2021-06-13T20:08:46Z

Ok so here comes some explanation for this PR:

My 1st commit (d9abe62) was for iOS sound support, meaning iOS sound fully worked in this commit
While testing on my iPhone, I felt the need to implement a feature: "switching between intenal/external microphones/devices". However, the feature depends on the first unmerged commit, so I couldn't just create another unrelated branch. I had to continue to develop on the same branch. 2 next commits did this task.
While that feature worked great on iPhone, I thought maybe Android could benefit the same, so I wrote the Android part. That's when things got messy: I could have written the Android part in a separated branch, but since the same feature was written for iOS, I figured maybe I could just add code in the same branch.
I also didn't have a proper dev environment for Android, so I wrote directly on github.com and wait for the auto-build to finish to test the apk (yeah I know you can laugh at me). The following long list of commits were because of this. (Actually it's a bit more complicated, this feature is not yet fully complete in Android: it's working, but you have to know how to make it work, i.e. it's not a few clicks away. For it to be complete: java code, and thus a bridge must be involved).
Then some more commits to reduce the launch time in iOS, some to fix a socket crash in socket.cpp, specific to iOS environment, some to fix clang style.

The key here is: that additional feature could only have been implemented in a separate PR if the iOS sound support code had already been merged.

ann0see · 2021-06-13T20:16:25Z

Ok. Thanks for this explanation. This means that we can probably reduce the number of commits dramatically:

Commit 1: Add iOS sound support

This should also include the code which fixes the crash, formatting and the removal of the libraries to reduce the launch time.

Commit 2: Allow automatic switching between internal/external audio devices (iOS)
You can branch off an unmerged branch by using git checkout -b myNewFeature while being on the branch with the changes you made.

Commit 3: Add initial auto switching of audio device to Android

You'd squash all the related commits into one or even better use fixup in an interactive rebase.

ngocdh · 2021-06-13T21:24:38Z

I'm not sure what to do.
I tried git rebase -i to squash commits related to ios sound only and already got conflicts...

The best thing I can do now, I think is to create a new branch from jamulus master, manually apply changes from this branch, and then remove all android related changes. That way I have a new iOS-only PR. Let's just say the feature comes with the sound support in iOS. Android can come later.

ngocdh · 2021-06-14T08:02:18Z

Closing this as I've created #1875 for iOS, and a clean commit history.
Will create other PRs for Android.

@pljones

Review of jamulussoftware#1865 by @pljones revealed .mm extension was not checked.

@pljones

* Add .mm extension to coding style check Review of #1865 by @pljones revealed .mm extension was not checked. * .clang-format for ObjC * Apply clang-format to sound.mm * Apply clang format on all .mm files Co-authored-by: ann0see <[email protected]>

ann0see · 2022-03-09T21:05:12Z

unrelated: @hoffie I just ran your changelog-helper script and it listed this PR as being added to the next release?

ann0see · 2022-03-09T21:12:33Z

I'd just add CHANGELOG: SKIP

hoffie · 2022-03-09T23:02:09Z

unrelated: @hoffie I just ran your changelog-helper script and it listed this PR as being added to the next release?

Yes, as it was referenced in fac5d9d (which will be part of 3.9.0).
Meta: Maybe we should work on this in a new Issue if that's considered a generic problem.

ann0see · 2022-03-09T23:06:45Z

Yes. It’s at least counter intuitive to have a closed PR in the changelog. On the other hand I think feature branch merges are not in the changelog (basically also an edge-case)

@pljones

* Add .mm extension to coding style check Review of jamulussoftware#1865 by @pljones revealed .mm extension was not checked. * .clang-format for ObjC * Apply clang-format to sound.mm * Apply clang format on all .mm files Co-authored-by: ann0see <[email protected]>

ngocdh added 30 commits April 13, 2021 19:30

iOS sound support. opus lib modified because of "Compile sources as" …

d9abe62

…"Objective C++" option when compiling for iOS.

resolving merge

1c90af9

BOOL -> bool

60fcdef

Android built-in mic / external device

9b9474f

Update sound.h

ed1e28e

Update sound.cpp

4c4eb56

Update clientdlg.cpp

444a820

Update sound.cpp

36e7232

Update sound.cpp

4eaf3de

Update clientdlg.cpp

7660feb

Update clientsettingsdlg.cpp

6e4d23e

Update client.cpp

e88bf3b

Update client.cpp

5d5b757

Update clientdlg.cpp

c7b9cac

Update clientsettingsdlg.cpp

84c49dd

Update chatdlg.cpp

6fc18e2

Update clientdlg.cpp

99c5e58

Update chatdlg.cpp

f36b806

Update clientsettingsdlg.cpp

9ab1fa7

Update clientdlg.h

d7d0611

Update client.cpp

6e436c4

Update sound.cpp

4d16da0

Update sound.cpp

133feb8

Update sound.h

4ad5680

move mRecordingStream to public for DEBUGING (find devid)

Update sound.cpp

16f2284

Update clientdlg.cpp

ef748a7

Update client.h

4d189e7

Update sound.cpp

f853767

Update sound.h

68f7881

Update clientdlg.cpp

e5f4372

ngocdh reopened this Jun 13, 2021

pljones reviewed Jun 13, 2021

View reviewed changes

ios/sound.mm Show resolved Hide resolved

pljones reviewed Jun 13, 2021

View reviewed changes

gilgongo mentioned this pull request Jun 13, 2021

iOS sound support #1512

Closed

ngocdh added a commit to ngocdh/jamulus that referenced this pull request Jun 13, 2021

Add .mm extension to coding style check

65c7aa4

Review of jamulussoftware#1865 by @pljones revealed .mm extension was not checked.

ngocdh mentioned this pull request Jun 13, 2021

Add .mm extension to coding style check #1871

Merged

ngocdh added 2 commits June 13, 2021 18:42

Merge remote-tracking branch 'refs/remotes/origin/ngocdhmaster2'

45d854c

clang-format -style=file -i *

4de56d2

ngocdh mentioned this pull request Jun 14, 2021

iOS sound support #1875

Merged

5 tasks

ngocdh closed this Jun 14, 2021

ngocdh deleted the ngocdhmaster2 branch June 16, 2021 04:54

ann0see pushed a commit to ngocdh/jamulus that referenced this pull request Feb 27, 2022

Add .mm extension to coding style check

01bba29

Review of jamulussoftware#1865 by @pljones revealed .mm extension was not checked.

hoffie mentioned this pull request Mar 9, 2022

tools/changelog-helper: Ignore non-MERGED PRs & ignore already listed PRs faster #2485

Merged

5 tasks

Comments

Conversation

ngocdh commented Jun 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pljones Jun 13, 2021

Choose a reason for hiding this comment

Uh oh!

pljones Jun 13, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pljones Jun 13, 2021

Choose a reason for hiding this comment

Uh oh!

pljones Jun 13, 2021

Choose a reason for hiding this comment

Uh oh!

pljones Jun 13, 2021

Choose a reason for hiding this comment

Uh oh!

pljones commented Jun 13, 2021

Uh oh!

ngocdh commented Jun 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ann0see commented Jun 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ngocdh commented Jun 13, 2021

Uh oh!

ann0see commented Jun 13, 2021

Uh oh!

ngocdh commented Jun 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ngocdh commented Jun 14, 2021

Uh oh!

ann0see commented Mar 9, 2022

Uh oh!

ann0see commented Mar 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hoffie commented Mar 9, 2022

Uh oh!

ann0see commented Mar 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ngocdh commented Jun 12, 2021 •

edited

Loading

ngocdh commented Jun 13, 2021 •

edited

Loading

ann0see commented Jun 13, 2021 •

edited

Loading

ngocdh commented Jun 13, 2021 •

edited

Loading

ann0see commented Mar 9, 2022 •

edited

Loading

ann0see commented Mar 9, 2022 •

edited

Loading