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

Do not export alarm properties on D-Bus #104

Closed

Conversation

tari01
Copy link
Member

@tari01 tari01 commented Feb 4, 2023

fixes #103

@JamiKettunen: Can I have your feedback on this, please?

@tari01 tari01 requested a review from sunweaver February 4, 2023 13:38
@sunweaver
Copy link
Member

@tari01: can you please improve the commit message "Do not export/test alarm properties on D-Bus" and explain why you think this to be the approach to go? In 6 months from now, that one-liner will let us frown and scratch our bald heads. Thanks!

This commit should resolve the alarm volume/vibration issue reported in Lomiri:
https://gitlab.com/ubports/development/apps/lomiri-clock-app/-/issues/180
and upstream:
AyatanaIndicators#103
@tari01
Copy link
Member Author

tari01 commented Feb 12, 2023

@tari01: can you please improve the commit message "Do not export/test alarm properties on D-Bus" and explain why you think this to be the approach to go? In 6 months from now, that one-liner will let us frown and scratch our bald heads. Thanks!

Amended. Review, but do not merge until @JamiKettunen confirms this actually solves the problem.

@JamiKettunen
Copy link

JamiKettunen commented Feb 12, 2023

Hm I've not tested anything but wouldn't it be better to wait until UT 22.04 dev starts before merging this? Also does it always read the GSettings already every time when sounding alarm etc since after this it won't be possible to change them via D-Bus anymore like the Clock app currently does when messing with the settings?

Or I suppose rather is the internal state of the indicator for those props updated when changes to the GSettings happen? Based on the issue of D-Bus props being unaccessible (due to AppArmor) that doesn't look to be the case currently

@tari01
Copy link
Member Author

tari01 commented Feb 12, 2023

Or I suppose rather is the internal state of the indicator for those props updated when changes to the GSettings happen?

Yes. The indicator tracks the GSettings internally and updates the variables accordingly. Nothing else is needed there.

This change (supposedly) only prevents the indicator from exporting those values to D-Bus to avoid the clash resulting from Lomiri already doing the same.

This should definitely be tested before merging.

@sunweaver
Copy link
Member

@JamiKettunen please test this, if possible.

@sunweaver
Copy link
Member

@JamiKettunen Is this PR still required / needed / valid? @tari01 Feedback?

I will close this PR now, please reopen if you want to pick up the thread again.

@sunweaver sunweaver closed this Oct 10, 2023
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.

Lomiri: Read alarm volume/vibration enabled state settings from GSettings
3 participants