feat(backup): add SettingsLine in SyncingView to change backup path #18209
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jrainville
requested review from
a team,
micieslak,
caybro,
alexjba and
noeliaSD
as code owners
Jenkins BuildsClick to see older builds (11)
|
jrainville
force-pushed
the
feat/local-backup-popup
branch
from
ca91432 to
033f0fe
Compare
jrainville
force-pushed
the
feat/backup-setting-change
branch
from
7265e0e to
b2bec06
Compare
jrainville
force-pushed
the
feat/local-backup-popup
branch
from
033f0fe to
8123023
Compare
jrainville
force-pushed
the
feat/backup-setting-change
branch
from
b2bec06 to
c88fd91
Compare
caybro
approved these changes
Jun 23, 2025
jrainville
force-pushed
the
feat/local-backup-popup
branch
from
8123023 to
330ecf1
Compare
jrainville
force-pushed
the
feat/backup-setting-change
branch
from
c88fd91 to
6e49a3d
Compare
noeliaSD
reviewed
Jul 1, 2025
| @@ -21,6 +21,14 @@ QtObject { | |||
| // Backup import properties | |||
| readonly property int backupImportState: syncModule ? syncModule.backupImportState : 0 | |||
| readonly property string backupImportError: syncModule ? syncModule.backupImportError : "" | |||
| readonly property string backupPath: appSettings.backupPath | |||
| function setBackupPath(path) { | |||
There was a problem hiding this comment.
I know we have this pattern used in lots of stores right now but I would opt to start thinking on treating the Context Properties this way: "Public Read, Private Write", enforcing unidirectional data flow via:
- Stores expose read-only state (readonly property)
- Explicit mutation methods (setSomething())
| @@ -21,6 +21,14 @@ QtObject { | |||
| // Backup import properties | |||
| readonly property int backupImportState: syncModule ? syncModule.backupImportState : 0 | |||
| readonly property string backupImportError: syncModule ? syncModule.backupImportError : "" | |||
| readonly property string backupPath: appSettings.backupPath | |||
There was a problem hiding this comment.
Also another comment here regarding this pattern: I would avoid accessing directly to a Context Property since it can have performance penalties bc of needed lookups when accessing it. Instead, it's better to create a new and "private / internal" property and access it via a reference. Here my suggestion (indeed we use this pattern in other stores but still not consistently):
QtObject {
id: d
readonly property var appSettingsInst: appSettings
}
readonly property string backupPath: d.appSettingsInst.backupPathThere was a problem hiding this comment.
Indeed, this pattern gives us clearer ownership and cleaner boundaries too! :)
| } | ||
|
|
||
| function toFileUri(path) { | ||
| return globalUtils.toFileUri(path) |
There was a problem hiding this comment.
Same reasoning than with appSettings applies here.
| anchors.leftMargin: 0 | ||
| anchors.rightMargin: 0 | ||
| text: qsTr("Directory of the local backup files") | ||
| currentValue: root.devicesStore.backupPath |
There was a problem hiding this comment.
I would opt for declaring a new property backupPath and avoid accessing directly from the store here.
The idea is moving up the stores as much as possible and explicitly pass the needed data to the needed components, making an explicit api for each component and reducing passing unecessary data up / down.
| id: backupPathDialog | ||
|
|
||
| title: qsTr("Select your backup directory") | ||
| currentFolder:root.devicesStore.toFileUri(root.devicesStore.backupPath) |
There was a problem hiding this comment.
Same reasoning here related to the deviceStore access.
|
|
||
| title: qsTr("Select your backup directory") | ||
| currentFolder:root.devicesStore.toFileUri(root.devicesStore.backupPath) | ||
| onAccepted: root.devicesStore.setBackupPath(backupPathDialog.selectedFolder) |
jrainville
force-pushed
the
feat/local-backup-popup
branch
from
330ecf1 to
f79fdf2
Compare
Fixes #18128 Adds a SettingsLine above the "Backup Data" and "Import Backup" buttons that shows what is the current path for the backup and let,s the user change that path.
jrainville
force-pushed
the
feat/backup-setting-change
branch
from
6e49a3d to
f7d465f
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does the PR do
Fixes #18128
Adds a SettingsLine above the "Backup Data" and "Import Backup" buttons that shows what is the current path for the backup and let's the user change that path.
Status-go PR: status-im/status-go#6699
Affected areas
formatImagePathtofromPathUribecause it's not just useful for imagesArchitecture compliance
My PR is consistent with this document: Status Desktop Architecture Guide
Screenshot of functionality
change-setting-path.webm
Impact on end user
Let's the user know what the path is and change it if needed
How to test
service.extand in Desktopexport FLAG_LOCAL_BACKUP_ENABLED=1)Risk
Tick one:
-->