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

Android: settings lost on GUI state change #3144

Merged
merged 1 commit into from
Mar 28, 2024
Merged

Android: settings lost on GUI state change #3144

merged 1 commit into from
Mar 28, 2024

Conversation

pljones
Copy link
Collaborator

@pljones pljones commented Aug 12, 2023 •
edited
Loading

Short description of changes

This change implements applicationStateChanged and saveStateRequest signals to save settings.

This mostly affects mobile platforms - it was identified on Android but may also affect iOS (untested).

CHANGELOG: Android: save settings on app state change

Context: Fixes an issue?

Fixes: #2395

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

This behaviour hasn't been documented, so no change needed.

Status of this Pull Request

Tested on Android 11 (Moto G9 Power retgb) (Qt5) - fixes the problem which existed in 3.10.0rc1
Tested on Linux (Qt6) - no noticable effect

What is missing until this pull request can be merged?

  • Testing on earlier and later supported versions of Android
  • Testing on supported iOS versions (pre and post fix)
  • Testing on other supported platforms

Checklist

  • I've verified that this Pull Request follows the general code principles
  • I tested my code and it does what I want
  • 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

I did see this but it appears unrelated...

Build for MacOS (artifacts)
...
Installing create-dmg@1.1.0
~/Library/Cache/jamulus-homebrew-bottles ~/work/jamulus/jamulus
Building fresh create-dmg@1.1.0 package
Error: Cannot extract formula to homebrew/cask!
Error: Process completed with exit code 1.

and for the Windows Jack build:

Project ERROR: Error: jack.h was not found in the expected location (C:/Program Files (x86)). Ensure that the right JACK2 variant is installed (32 Bit vs. 64 Bit).

AUTOBUILD: Please build all targets

@pljones pljones added this to the Release 3.11.0 milestone Aug 12, 2023
@pljones pljones self-assigned this Aug 12, 2023
@pljones pljones force-pushed the bugfix/2395-save-settings-on-state-change branch from 0fa5dfc to ce91ced Compare August 15, 2023 16:28
@pljones pljones force-pushed the bugfix/2395-save-settings-on-state-change branch 4 times, most recently from ebbbb32 to c3d913f Compare August 28, 2023 12:28
@pljones pljones force-pushed the bugfix/2395-save-settings-on-state-change branch 2 times, most recently from c9a93cb to 106b2e1 Compare September 12, 2023 16:22
@ann0see
Copy link
Member

ann0see commented Sep 14, 2023

iOS didn't experience that, I think.

@pljones pljones force-pushed the bugfix/2395-save-settings-on-state-change branch from 106b2e1 to a35ba64 Compare September 18, 2023 18:13
ann0see
ann0see reviewed Sep 26, 2023
View reviewed changes
src/settings.h Outdated Show resolved Hide resolved
@pljones pljones force-pushed the bugfix/2395-save-settings-on-state-change branch from a35ba64 to fc1d401 Compare September 27, 2023 08:29
@ann0see
Copy link
Member

ann0see commented Sep 30, 2023

iOS name is still saved; mute myself seems to be lost, but that could be another bug. I believe iOS was never affected.

@pljones pljones force-pushed the bugfix/2395-save-settings-on-state-change branch 2 times, most recently from 8c4a3a6 to 51ec4e6 Compare October 5, 2023 17:46
ann0see
ann0see reviewed Oct 14, 2023
View reviewed changes
Copy link
Member

@ann0see ann0see left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hmm. I'd like to have someone else review this - I mainly don't understand why this bug is platform specific.

src/settings.h Show resolved Hide resolved
@pljones pljones force-pushed the bugfix/2395-save-settings-on-state-change branch 3 times, most recently from 6a0177f to bb097c2 Compare October 15, 2023 12:56
@pljones pljones force-pushed the bugfix/2395-save-settings-on-state-change branch from bb097c2 to 48eb7f1 Compare November 2, 2023 18:28
@pljones pljones force-pushed the bugfix/2395-save-settings-on-state-change branch from 48eb7f1 to 1e21d43 Compare December 9, 2023 11:07
@pljones

This comment was marked as outdated.

Sign in to view
@hoffie
Copy link
Member

hoffie commented Dec 10, 2023

Windows JACK build failing

see #3206

@pljones pljones force-pushed the bugfix/2395-save-settings-on-state-change branch from 1e21d43 to f3cc155 Compare December 14, 2023 17:57
@pljones pljones force-pushed the bugfix/2395-save-settings-on-state-change branch from f3cc155 to ab0c5ae Compare January 14, 2024 10:23
@pljones pljones force-pushed the bugfix/2395-save-settings-on-state-change branch 2 times, most recently from 5304008 to 4d87fbf Compare February 18, 2024 11:41
@pljones pljones force-pushed the bugfix/2395-save-settings-on-state-change branch from 4d87fbf to a3f976e Compare March 5, 2024 16:57
@pljones pljones force-pushed the bugfix/2395-save-settings-on-state-change branch from a3f976e to 0a031b0 Compare March 15, 2024 17:07
softins
softins reviewed Mar 26, 2024
View reviewed changes
Copy link
Member

@softins softins left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Code looks good. Just one question given below. Will be happy to approve after that.

src/settings.h Outdated Show resolved Hide resolved
@pljones pljones force-pushed the bugfix/2395-save-settings-on-state-change branch from 0a031b0 to 9f876e0 Compare March 26, 2024 17:16
@pljones pljones force-pushed the bugfix/2395-save-settings-on-state-change branch from 9f876e0 to 1ebb55f Compare March 26, 2024 21:12
softins
softins approved these changes Mar 28, 2024
View reviewed changes
Copy link
Member

@softins softins left a comment

Choose a reason for hiding this comment

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

LGTM

@ann0see
Copy link
Member

ann0see commented Mar 28, 2024

As I said, I can't test on a real device. So I can't reliably test this.
If it's ok you can merge it without my approval.

@softins
Copy link
Member

softins commented Mar 28, 2024

Just built this on RPi and tried it out. Works fine. I noticed that it now saves Jamulus.ini on state changes, such as moving the main window, or exiting from the settings dialog. This verifies that the new code is getting invoked. In comparison, 3.10.0 only updates Jamulus.ini on exiting the program.

@pljones pljones merged commit 6f28c7c into jamulussoftware:main Mar 28, 2024
15 checks passed
@pljones pljones deleted the bugfix/2395-save-settings-on-state-change branch March 28, 2024 16:44
@softins softins mentioned this pull request Jun 28, 2024
Closed
@softins
Copy link
Member

softins commented Jun 28, 2024

This change seems to break registration with a directory in GUI servers. See #3287 (comment)

@pljones pljones added feature request Feature request and removed feature request Feature request labels Jul 13, 2024
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ann0see ann0see ann0see left review comments

@softins softins softins approved these changes

Assignees

@pljones pljones

Labels
None yet
Projects
Tracking
Status: Done
Milestone
Release 3.11.0
Development

Successfully merging this pull request may close these issues.

Android/LineageOS: Jamulus settings don't get saved on closing app
4 participants
@pljones @ann0see @hoffie @softins