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

Add functionality to repeat last callout upon shaking device + RCOS Documentation #94

Merged
merged 12 commits into from
Sep 15, 2024

Conversation

connorhakan8
Copy link
Contributor

No description provided.

Information about my progress on Soundscape so far.
LaunchHelper.swift contains the code for detecting motion

POICallout.swift contains the sound that I could get to run upon a POI callout.
Using a boolean variable to detect motion, a previous sound variable, and reusing existing code, I attempt to play the previous sound when the device is shaken.
Variable called previousSounds initalized, though not done properly. Currently, previousSound is set to the current sound every time the code runs, but I only want this to happen when it hasn't been initialized, and I can't figure how to do this without getting errors. Daniel has directed me to another variable that does show the callout history, though this is an array of type POI and not a single variable of sounds. I'm not 100% sure how to utilize this.
Forgot to add the updated lab notebook detailing my progress in the previous commit, so I'm adding it to this commit.
The code half-accomplishes the goal - shaking the device runs code originally obtained from a function called handleRepeat in HomeViewController+RemoteControl.swift to restart the existing callout. However, when the restarted callout is complete, it will not continue to state other callouts if there are more in the queue.
Made a line of code appear as a code block.
@RDMurray
Copy link
Contributor

Hi. Thanks for working on this! It still needs quite a bit of clean up though; there are remnants of previous attempts, commented sections of code, whitespace changes in unrelated files etc.

I also think a setting to optionally enable this is essential. It can be very easy to trigger a shake action by mistake especially when moving around.

Please don't be discouraged by this comment - we really appreciate your work on this. Your notes are an interesting read and clarify some things which aren't well explained in the existing documentation.

Shake toggle at its core works but presents two bugs
1. Callout animation does not stop after all callouts have been played
2. Disabling all callouts causes Soundscape to crash stating that "Invalid batch updates detected: the number of sections and/or rows returned by the data source before and after performing the batch updates are inconsistent with the updates."
Copy link
Member

@steinbro steinbro left a comment

Choose a reason for hiding this comment

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

Thanks for the cleanup! I haven't tested it, but I did note a few files that don't appear to have meaningful changes so should probably be reverted before merging. I also left a couple questions for you.

Copy link
Member

Choose a reason for hiding this comment

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

All changes in this file are extraneous.

Copy link
Member

Choose a reason for hiding this comment

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

All changes in this file are extraneous.

Copy link
Member

Choose a reason for hiding this comment

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

All changes in this file are extraneous.

guard success else {
GDLogVerbose(.stateMachine, "Callout did not finish playing successfully. Terminating state machine...")
calloutGroup.onComplete?(false)
strongSelf.stateMachine.fireEvent(.failed)
return
}

strongSelf.stateMachine.fireEvent(.delayCalloutAnnounced)
Copy link
Member

Choose a reason for hiding this comment

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

Except maybe this deletion is meaningful?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Based on the comment below) I don't know how this ended up getting deleted. I experimented a lot with the code when I was just getting started, so maybe I deleted this in that process. I have re-added it, so you should see the line added back on the next push of this branch.

@@ -505,7 +505,7 @@ class AuthoredActivityLoader {

for (wptIndex, waypoint) in content.waypoints.enumerated() {
for (clipIndex, image) in waypoint.images.enumerated() {
guard let key = manager.cacheKey(for: image.url), await manager.imageCache.containsImage(forKey: key, cacheType: .all) == .none else {
guard let key = manager.cacheKey(for: image.url), await manager.imageCache.containsImage!(forKey: key, cacheType: .all) == .none else {
Copy link
Member

Choose a reason for hiding this comment

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

Why was the exclamation point added here? Is it necessary?

@@ -318,7 +318,7 @@ public enum GeoJsonGeometry: Equatable, Codable {

extension GeoJsonGeometry {
private static func into_coord_pair(_ coord: CLLocationCoordinate2D) -> [CLLocationDegrees] {
return [coord.longitude, coord.latitude]
return [coord.latitude, coord.longitude]
Copy link
Member

Choose a reason for hiding this comment

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

@RDMurray is this the latitude/longitude swap in JSON parsing that you caught? Which one was correct?

@connorhakan8
Copy link
Contributor Author

For many of the changes, I don't know how they came to fruition (things like the changes in Packages.resolved, Map.storyboard, and exclamation marks added to a few of the files). Maybe they somehow got added when I pulled the branch (I remember way back when starting this project that there were a few compilation errors due to running this on a newer version of XCode, so maybe they got added there?). I wonder if these changes should be reverted or not before pulling this. And I'll work on removing the extraneous information - they generally seem to be things I missed in my first run-through of cleaning up the program.

Fixed bugs with shake device callouts listed in the previous commit. Now, the callout animation properly stops and there is no crash when disabling all callouts.

However, when doing a shake callout on "my location", I noticed that shaking the device will state the callout but also the coordinates, which seems like unnecessary information to the user. I wonder if this is a thing running it on the simulator? Using media controls on my phone to call the previous callout that way doesn't list the coordinates.

I also cleaned up a few more files that I missed the first time around and moved my documentation to the appropriate folder.
@connorhakan8
Copy link
Contributor Author

Here's the new commit. It should be virtually ready to be pulled (after resolving the issues listed above). Another thing: I question how the "allow callouts" toggle actually works. It seems like all the buttons below are functional when callouts are disabled. My code that supports shaking the device to repeat the last callout also still works. If you know how this toggle is supposed to work, please reply down here and I can make changes accordingly.

@steinbro steinbro merged commit bdb0898 into soundscape-community:main Sep 15, 2024
1 check passed
@steinbro
Copy link
Member

Finally merged! Thanks for tackling this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants