Greatly improve "Override Changes" button and UX (fixes #1769) #2063
Conversation
|
Can you change the look and wording to match what is used in the Web GUI?
Ideally, the strings should match 1:1, so that the same translation could be re-used.
Please don't rename. I agree that the better naming would be "Override Remote Changes" (similarly to "Revert Local Changes"), but this kind of a rename should happen in Syncthing proper first, not here. The usual concern is that if the wording is changed, all translations will have to be re-done. |
ZacharyACoon
force-pushed
the
improve_override_warnings
branch
from
21d500b to
b69f0f5
Compare
|
Glanced at the build failure in the github actions, that's fixed/overridden in these improved changes. Changes
New Screenshots
|
| Util.dismissDialogSafe(mDeleteDialog, this); | ||
|
|
||
| outState.putBoolean(IS_SHOW_OVERRIDE_REMOTES_DIALOG, mOverrideChangesDialog != null && mOverrideChangesDialog.isShowing()); |
There was a problem hiding this comment.
| outState.putBoolean(IS_SHOW_OVERRIDE_REMOTES_DIALOG, mOverrideChangesDialog != null && mOverrideChangesDialog.isShowing()); | |
| outState.putBoolean(IS_SHOW_OVERRIDE_CHANGES_DIALOG, mOverrideChangesDialog != null && mOverrideChangesDialog.isShowing()); |
Please change in all places. If unclear, it could also be OVERRIDE_REMOTE_CHANGES (but not just REMOTES).
There was a problem hiding this comment.
Please change in all places. If unclear, it could also be
OVERRIDE_REMOTE_CHANGES(but not justREMOTES).
I missed these, reverted them as well in the latest push.
| @@ -74,7 +74,7 @@ | |||
| <item quantity="many">%1$d / %2$d Fichiers</item> | |||
| <item quantity="other">%1$d / %2$d Fichiers</item> | |||
| </plurals> | |||
| <string name="override_changes">Ignorer les modifications</string> | |||
| <string name="override_changes">Écraser les changements distants</string> | |||
There was a problem hiding this comment.
Translations are handled at https://hosted.weblate.org/projects/syncthing/android, so I would suggest to remove the change from here and fix the text there.
app/src/main/res/values/strings.xml
Outdated
| @@ -116,7 +116,9 @@ | |||
| <item quantity="other">%1$d / %2$d Files</item> | |||
| </plurals> | |||
|
|
|||
| <string name="override_changes">Override changes</string> | |||
| <string name="override_changes">Override Changes</string> | |||
| <string name="override_changes_description">When configured as \"send only\" and changes have been made remotely, you may click here to overwrite the folder content on other devices.</string> | |||
There was a problem hiding this comment.
| <string name="override_changes_description">When configured as \"send only\" and changes have been made remotely, you may click here to overwrite the folder content on other devices.</string> | |
| <string name="override_changes_description">When configured as \"Send Only\" and changes have been made remotely, you may click here to overwrite the folder content on other devices.</string> |
app/src/main/res/values/strings.xml
Outdated
| <string name="override_changes">Override changes</string> | ||
| <string name="override_changes">Override Changes</string> | ||
| <string name="override_changes_description">When configured as \"send only\" and changes have been made remotely, you may click here to overwrite the folder content on other devices.</string> | ||
| <string name="override_changes_are_you_sure"><b>Warning!</b>\n\nThe folder content on other devices will be overwritten to become identical with this device. Files not present here will be deleted on other devices.\n\nAre you sure you want to continue?</string> |
There was a problem hiding this comment.
| <string name="override_changes_are_you_sure"><b>Warning!</b>\n\nThe folder content on other devices will be overwritten to become identical with this device. Files not present here will be deleted on other devices.\n\nAre you sure you want to continue?</string> | |
| <string name="override_changes_are_you_sure"><b>Warning!</b>\n\nThe folder content on other devices will be overwritten to become identical with this device. Files not present here will be deleted on other devices.\n\nAre you sure you want to continue?</string> |
There was a problem hiding this comment.
I made these changes in the latest force push to this PR. :)
ZacharyACoon
force-pushed
the
improve_override_warnings
branch
from
b69f0f5 to
32b8cbc
Compare
Thank you for working on this. I'm going do to some testing on a real device soon. |
ZacharyACoon
force-pushed
the
improve_override_warnings
branch
2 times, most recently
from
ae1a93e to
fcfd57f
Compare
Thank you for responding so quickly! |
Bump, is there anything I can do to help further? I'm hoping to see this! |
No need to bump 😅. When it comes to Syncthing, PR reviews can take a while. I promised to give this a try on a real device, and I will do this, but "soon" for me is more like in 1-2 weeks when I manage to find some free time. Also, I can't really review the Java code itself. Someone more knowledgeable would need to have a look at that part. |
|
Thanks a lot for tackling this - it's easily the most important thing to be improved in this app! Some high-level points first: The confirmation dialog is definitely needed and great! My initial reaction to moving the button into the edit dialog isn't entirely positive. Feels a bit surprising as it's not an operation to edit the folder settings. And in the syncthing web UI the button is on the folder overview (which admittedly is more extensive). So not sure a user would find the button? Then again the dialog opens by just tapping the folder, there's no indication of that being only for settings, so maybe it's fine. I also first wanted to ask you to make it look more like a button. Then I realized there's a bunch of "buttons"/menu opening entries there and they all look flat. I guess that's just what's considered normal/good under material design... If you have a good idea how to improve it, that'd be great, but if not it's fine as is clearly. Regarding review "speed": Sorry for the long wait. And after review it might be something between a long while and never until this reaches the majority of our users due to google, see #2064 |
|
I actually started by keeping the original button and just adding a
warning, it was certainly "easier".
I thought a little while about how I feel general user experience might
best be represented, and I came to the conclusion that the existing folder
actions interface in the web gui and android UI feels a bit out of place.
I do strongly prefer to consider what I am labeling "folder actions" a
subset of modifying the folder settings.
If that feels confusing, we might relabel "Folder Settings" to folder
details.
We could easily add a switch/checkbox to keep/reenable the original
override button.
Also. I'm happy to pull request this change in the Web UI as well. I
don't like the button being so prominent there either.
I've installed Android Studio now, so I'm happy to make more modifications.
…On Sun, Mar 10, 2024, 7:40 AM Simon Frei ***@***.***> wrote:
Thanks a lot for tackling this - it's easily the most important thing to
be improved in this app!
Some high-level points first:
The confirmation dialog is definitely needed and great!
My initial reaction to moving the button into the edit dialog isn't
entirely positive. Feels a bit surprising as it's not an operation to edit
the folder settings. And in the syncthing web UI the button is on the
folder overview (which admittedly is more extensive). So not sure a user
would find the button? Then again the dialog opens by just tapping the
folder, there's no indication of that being only for settings, so maybe
it's fine.
I assume you clearly prefer it inside the dialog. @tomasz1986
<https://github.com/tomasz1986> any input/opinion on that?
Just to be clear, I am not asking you to change it at this point, just
bringing it up.
I also first wanted to ask you to make it look more like a button. Then I
realized there's a bunch of "buttons"/menu opening entries there and they
all look flat. I guess that's just what's considered normal/good under
material design... If you have a good idea how to improve it, that'd be
great, but if not it's fine as is clearly.
Regarding review "speed": Sorry for the long wait. And after review it
might be something between a long while and never until this reaches the
majority of our users due to google, see #2064
<#2064>
—
Reply to this email directly, view it on GitHub
<#2063 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AQVQTH7IJ5SLCXNOJ4SMXD3YXRWG3AVCNFSM6AAAAABD2CAPPGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOBXGI2TKNJVGA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
| getIntent().hasExtra(EXTRA_FOLDER_OVERRIDABLE_CHANGES) | ||
| && getIntent().getBooleanExtra(EXTRA_FOLDER_OVERRIDABLE_CHANGES, false) |
There was a problem hiding this comment.
Isn't the hasExtra part redundant, as getBooleanExtra returns false if no value is present?
There was a problem hiding this comment.
I agree, was simply following the existing code... will remove
There was a problem hiding this comment.
Ah didn't notice that. Just in case: Following existing code is a good argument to me. As in if in the future you intentionally dont change something and don't want to change it, and I miss it's preexisting, feel free to just tell me that without changing anything.
|
|
||
| outState.putBoolean(IS_SHOW_OVERRIDE_CHANGES_DIALOG, mOverrideChangesDialog != null && mOverrideChangesDialog.isShowing()); | ||
| Util.dismissDialogSafe(mOverrideChangesDialog, this); |
There was a problem hiding this comment.
Why is this needed again within this condition? As far as I see it's the exact same function call that already happens uncoditionally before.
There was a problem hiding this comment.
I agree, was simply following the existing code... will remove, again.
There was a problem hiding this comment.
Just in case you forgot, as you already removed the code in the other case: It's not removed yet here.
(Also not that it matters, but here I don't see what the existing code is that is doing the same/something similar.)
| LinearLayout container = (LinearLayout) findViewById(R.id.overrideChangesContainer); | ||
| binding.overrideChangesContainer.setEnabled(state); |
There was a problem hiding this comment.
Isn't binding.overrideChangesContainer the same thing as what you assign to container, i.e. you can use only that?
(Struggling with android-studio, cant see type infos to check right now.)
| boolean overridableChanges = folder.type.equals(Constants.FOLDER_TYPE_SEND_ONLY) | ||
| && folderStatus.isNeedsItems() | ||
| && folderStatus.isOutOfSync(); |
There was a problem hiding this comment.
The previous condition checked for folder idleness. That's because you can't revert if a scan or sync operation is in progress. This is a bit of a problem with the menu, as the time when you open the menu is not necessarily the time the you click override. So the enabledness of the button might not match reality. Which means the operation will not work (likely hang and time out eventually).
There was a problem hiding this comment.
Except I just saw the condition for isOutOfSync further down - it checks both isNeedsItems and for idleness. So the isNeedsItems check here is redundant.
The timing problem in the menu still is a thing. Ideally that would somehow be hooked to folder status, but that's probably not super simple? I may be able to be convinced to just close my eyes and pretend I didn't know about this.
There was a problem hiding this comment.
I removed the isNeedsItems as redundant.
app/src/main/java/com/nutomic/syncthingandroid/views/FoldersAdapter.java
Outdated
Show resolved
Hide resolved
That kinda makes sense. Or even just "Folder", or no title at all. I am ok with leaving as is as well though.
Please not - I don't want more moving pieces for something like this.
You could do that, clearly not required for this PR. And I am not sure in the web UI that would go through. There's a lot more activity there, and opinions involved. So at least I'd start by explaining and proposing to get feedback/consensus, before investing much time into it there. |
ZacharyACoon
force-pushed
the
improve_override_warnings
branch
from
fcfd57f to
7a6ec4c
Compare
|
Sorry for updating the branch and potentially interfering with your branch... I only wanted to approve the CI run and misclicked (need to find a way to make that permanent for you, and potentially also find a way to make it stick in general per PR). |
imsodin
changed the title
| if (savedInstanceState != null){ | ||
| if (savedInstanceState.getBoolean(IS_SHOWING_DELETE_DIALOG)){ | ||
| if (savedInstanceState.getBoolean(IS_SHOW_DELETE_DIALOG)){ | ||
| showDeleteDialog(); | ||
| } | ||
| } | ||
|
|
||
| if (savedInstanceState != null){ | ||
| if (savedInstanceState.getBoolean(IS_SHOWING_DELETE_DIALOG)){ | ||
| if (savedInstanceState.getBoolean(IS_SHOW_DELETE_DIALOG)){ | ||
| showDeleteDialog(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Entirely optional, just suggesting it as you are renaming the constant which is also a somewhat unrelated cleanup: You could delete one of the two redundant blobs of code.
| public boolean isNeedsItems() { | ||
| return needFiles + needDirectories + needSymlinks + needDeletes > 0; | ||
| } | ||
|
|
||
| public boolean isOutOfSync() { | ||
| return state.equals("idle") && isNeedsItems(); | ||
| } |
There was a problem hiding this comment.
Given you aren't using isNeedsItems anymore, I'd prefer if you removed that and folded it into isOutOfSync.
|
Few minor points/leftover from laster review, otherwise this looks good to merge to me. |
|
@ZacharyACoon Do you have time/inclination to address the small remaining review comments above? Otherwise I can also do that myself and then merge - I'd really like to get this in (thanks again). |
Fixes #1769, #2062
Description
The override changes button does not clearly indicate its function, and performs a very dangerous action that by design wipes files across the entire cluster. It has caused numerous users to lose files unexpectedly and scared the hell out of me the other day, so I'm fixing it so that no user can be confused as to what it will do.
Changes
Screenshots