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

[sailfish-utilities] Add a Restart Bluetooth button. JB#58019 #54

Merged
merged 1 commit into from May 4, 2022

Conversation

nephros
Copy link
Contributor

@nephros nephros commented Mar 14, 2021

This adds a new option in the Utilities Menu to restart the Bluetooth related services, dealing with this feature request

Please review and comment:

  • Are the calls to systemctl in restart_bluetooth.sh correct?
  • There are two translation strings, one for the name "Bluetooth" and the description of the option itself.

Resolves: #61

@nephros nephros force-pushed the bt-restart branch 2 times, most recently from 876dbf8 to 6af7c4b Compare March 15, 2021 07:46
@pvuorela
Copy link
Contributor

Hm, I'd be a little cautious on adding new options to the utilities. As was coming up in the forum discussion, ideally we'd fix the underlying bugs so the utilities wouldn't be needed. Maybe even some day getting rid of the whole thing. Then again if there are adaptation side bugs related it could warrant for adding this.

@jusa what's your view?

@Thaodan
Copy link
Contributor

Thaodan commented Mar 15, 2021

I'm not Jusa but some options help when the underlying vendor-software/hardware is buggy.

@nephros
Copy link
Contributor Author

nephros commented May 28, 2021

As was coming up in the forum discussion, ideally we'd fix the underlying bugs so the utilities wouldn't be needed. Maybe even some day getting rid of the whole thing.

Agreed that ideally this whole package needn't exist at all.

While it does though, and while users are still reporting problems about two releases later, and are reporting that something like this thing here helps in practice, is there any interest in this PR?

Copy link
Member

@llewelld llewelld left a comment

Choose a reason for hiding this comment

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

I tested the changes out and they worked nicely. I've made a few minor suggestions.

From reading the forums, I'm not sure whether this is still an issue for users. If it is, then I think we should consider accepting this PR. @nephors, are you still seeing reports of bluetooth problems that restarting the Bluetooth stack seems to fix?

Is this still a work-in-progress?

plugin/utiltools.h Outdated Show resolved Hide resolved
qml/ActionList.qml Outdated Show resolved Hide resolved
qml/plugins/RestartBluetooth.qml Outdated Show resolved Hide resolved
qml/plugins/RestartBluetooth.qml Show resolved Hide resolved
qml/plugins/RestartBluetooth.qml Outdated Show resolved Hide resolved
qml/plugins/RestartBluetooth.qml Outdated Show resolved Hide resolved
@nephros
Copy link
Contributor Author

nephros commented Apr 26, 2022

I tested the changes out and they worked nicely. I've made a few minor suggestions.

From reading the forums, I'm not sure whether this is still an issue for users. If it is, then I think we should consider accepting this PR. @nephors, are you still seeing reports of bluetooth problems that restarting the Bluetooth stack seems to fix?

I have not seen any reports of it in recent times.
But I have published a packaged version of these changes on OBS, and a patch which also allows BT restart (lumped together with the existing Restart Networking feature). That one has 317 downloads.
So it's possible people are just working around any problems by using those.

Myself I use and need this from time to time on SFOS4.2/Xperia10-one as BT devices sometimes just won't connect.

Is this still a work-in-progress?

It is in the sense I am ready to make any changes required.

However I do not have a great amount of devices and SFOS Versions to test on, and there are differences in the actions required to make the restart work depending on at least OS version, maybe even device.

@nephros
Copy link
Contributor Author

nephros commented Apr 26, 2022

Once I have guidance on the two points:

  • order of features in the UI
  • convention on how to name the translation IDs

I can rebase, squash and update this branch.

Copy link
Member

@llewelld llewelld left a comment

Choose a reason for hiding this comment

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

Concerning the ordering, I personally would prefer to see items added at the end, to avoid messing with users' muscle memory.

The alphabetic ordering and categorisation is a bit nebulous anyway, since in the UI they're translated differently (e.g. "Network Restart", "Home screen Restart" aren't alphabetical).

I could see an argument for keeping the Restart and Clear items together. In that case, please rebase onto the latest changes and fix the ordering of the Fingerprint Restart button too.

This adds a new option in the Utilities Menu to restart Bluetooth
related services

Co-authored-by: David Llewellyn-Jones <david@flypig.co.uk>
@nephros nephros changed the title WIP: Add option to restart Bluetooth subsystem [sailfish-utilities] Add a Restart Bluetooth button Apr 27, 2022
@nephros nephros requested a review from llewelld April 28, 2022 07:39
@llewelld llewelld changed the title [sailfish-utilities] Add a Restart Bluetooth button [sailfish-utilities] Add a Restart Bluetooth button. JB#58019 Apr 28, 2022
Copy link
Member

@llewelld llewelld left a comment

Choose a reason for hiding this comment

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

Many thanks for a nice addition @nephros, and for making the changes. This looks good to me now.

@llewelld llewelld merged commit 899bd7c into sailfishos:master May 4, 2022
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.

Introduce Bluetooth service restart
5 participants