Greatly improve "Override Changes" button and UX (fixes #1769) #2063

ZacharyACoon · 2024-02-26T12:04:59Z

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

Renames "Override Changes" to "Override Remotes"
Moves button to folder settings to avoid annoying users and accidental presses.
Provides detailed description of function before pressing.
Provides serious warning of danger and requires confirmation when pressed.
Button fits into Syncthing's theme nicely rather than looking weird in the main screen.

Screenshots

Before	After
	(Not enabled)

	(Enabled)

tomasz1986 · 2024-02-26T12:16:45Z

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.

Renames "Override Changes" to "Override Remotes"

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 · 2024-02-26T13:18:41Z

Glanced at the build failure in the github actions, that's fixed/overridden in these improved changes.

Changes

Removed renaming
chose an icon similar to web ui's up arrow in circle
used the exact same wording in the confirmation dialog
used similarly themed wording in the button description

New Screenshots

tomasz1986 · 2024-02-26T13:23:41Z

app/src/main/java/com/nutomic/syncthingandroid/activities/FolderActivity.java

        Util.dismissDialogSafe(mDeleteDialog, this);

+        outState.putBoolean(IS_SHOW_OVERRIDE_REMOTES_DIALOG, mOverrideChangesDialog != null && mOverrideChangesDialog.isShowing());


Suggested change

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).

Please change in all places. If unclear, it could also be OVERRIDE_REMOTE_CHANGES (but not just REMOTES).

I missed these, reverted them as well in the latest push.

tomasz1986 · 2024-02-26T13:25:58Z

app/src/main/res/values-fr/strings.xml

@@ -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>


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-fr/strings.xml

tomasz1986 · 2024-02-26T13:27:09Z

app/src/main/res/values/strings.xml

@@ -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>


Suggested change

<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>

tomasz1986 · 2024-02-26T13:27:20Z

app/src/main/res/values/strings.xml

-    <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>


Suggested change

<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>

I made these changes in the latest force push to this PR. :)

tomasz1986 · 2024-02-26T13:29:18Z

Glanced at the build failure in the github actions, that's fixed/overridden in these improved changes.

Thank you for working on this. I'm going do to some testing on a real device soon.

ZacharyACoon · 2024-02-26T13:35:23Z

Thank you for working on this. I'm going do to some testing on a real device soon.

Thank you for responding so quickly!
I believe I've made all of the changes/improvements you've requested so far.

ZacharyACoon · 2024-02-29T00:26:37Z

Thank you for working on this. I'm going do to some testing on a real device soon.

Bump, is there anything I can do to help further? I'm hoping to see this!

tomasz1986 · 2024-02-29T19:08:25Z

Thank you for working on this. I'm going do to some testing on a real device soon.

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.

imsodin · 2024-03-10T14:40:23Z

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 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

ZacharyACoon · 2024-03-10T17:50:14Z

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: ***@***.***>

imsodin

Finally got around to an actual review, sorry for the long wait. Overall looks good, some smallish comments.

imsodin · 2024-03-10T14:42:53Z

app/src/main/java/com/nutomic/syncthingandroid/activities/FolderActivity.java

+                getIntent().hasExtra(EXTRA_FOLDER_OVERRIDABLE_CHANGES)
+                && getIntent().getBooleanExtra(EXTRA_FOLDER_OVERRIDABLE_CHANGES, false)


Isn't the hasExtra part redundant, as getBooleanExtra returns false if no value is present?

I agree, was simply following the existing code... will remove

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.

imsodin · 2024-03-22T21:26:45Z

app/src/main/java/com/nutomic/syncthingandroid/activities/FolderActivity.java

+
+            outState.putBoolean(IS_SHOW_OVERRIDE_CHANGES_DIALOG, mOverrideChangesDialog != null && mOverrideChangesDialog.isShowing());
+            Util.dismissDialogSafe(mOverrideChangesDialog, this);


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.

I agree, was simply following the existing code... will remove, again.

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.)

imsodin · 2024-03-22T21:45:34Z

app/src/main/java/com/nutomic/syncthingandroid/activities/FolderActivity.java

+        LinearLayout container = (LinearLayout) findViewById(R.id.overrideChangesContainer);
+        binding.overrideChangesContainer.setEnabled(state);


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.)

imsodin · 2024-03-22T22:17:07Z

app/src/main/java/com/nutomic/syncthingandroid/fragments/FolderListFragment.java

+        boolean overridableChanges = folder.type.equals(Constants.FOLDER_TYPE_SEND_ONLY)
+            && folderStatus.isNeedsItems()
+            && folderStatus.isOutOfSync();


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).

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.

I removed the isNeedsItems as redundant.

app/src/main/java/com/nutomic/syncthingandroid/views/FoldersAdapter.java

imsodin · 2024-03-22T22:29:43Z

If that feels confusing, we might relabel "Folder Settings" to folder
details.

That kinda makes sense. Or even just "Folder", or no title at all. I am ok with leaving as is as well though.

We could easily add a switch/checkbox to keep/reenable the original
override button.

Please not - I don't want more moving pieces for something like this.

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.

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.

imsodin · 2024-04-01T14:40:23Z

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 · 2024-04-01T14:49:16Z

app/src/main/java/com/nutomic/syncthingandroid/activities/FolderActivity.java

        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();
            }
        }


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.

imsodin · 2024-04-01T14:55:38Z

app/src/main/java/com/nutomic/syncthingandroid/model/FolderStatus.java

+    public boolean isNeedsItems() {
+        return needFiles + needDirectories + needSymlinks + needDeletes > 0;
+    }
+
+    public boolean isOutOfSync() {
+        return state.equals("idle") && isNeedsItems();
+    }


Given you aren't using isNeedsItems anymore, I'd prefer if you removed that and folded it into isOutOfSync.

imsodin · 2024-04-01T14:56:15Z

Few minor points/leftover from laster review, otherwise this looks good to merge to me.

ZacharyACoon force-pushed the improve_override_warnings branch from 21d500b to b69f0f5 Compare February 26, 2024 13:13

tomasz1986 reviewed Feb 26, 2024

View reviewed changes

app/src/main/res/values-fr/strings.xml Outdated Show resolved Hide resolved

tomasz1986 reviewed Feb 26, 2024

View reviewed changes

ZacharyACoon force-pushed the improve_override_warnings branch from b69f0f5 to 32b8cbc Compare February 26, 2024 13:29

ZacharyACoon force-pushed the improve_override_warnings branch 2 times, most recently from ae1a93e to fcfd57f Compare February 26, 2024 13:34

imsodin reviewed Mar 22, 2024

View reviewed changes

greatly improve override_changes ux/ui

7a6ec4c

ZacharyACoon force-pushed the improve_override_warnings branch from fcfd57f to 7a6ec4c Compare April 1, 2024 02:54

Merge branch 'main' into improve_override_warnings

951a16c

imsodin changed the title ~~Greatly improve "Override Changes" button and UX.~~ Greatly improve "Override Changes" button and UX (fixes #1769) Apr 1, 2024

imsodin reviewed Apr 1, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Greatly improve "Override Changes" button and UX (fixes #1769) #2063

Greatly improve "Override Changes" button and UX (fixes #1769) #2063

ZacharyACoon commented Feb 26, 2024

tomasz1986 commented Feb 26, 2024 •

edited

ZacharyACoon commented Feb 26, 2024 •

edited

tomasz1986 Feb 26, 2024

ZacharyACoon Feb 26, 2024

tomasz1986 Feb 26, 2024

tomasz1986 Feb 26, 2024

tomasz1986 Feb 26, 2024

ZacharyACoon Feb 26, 2024

tomasz1986 commented Feb 26, 2024

ZacharyACoon commented Feb 26, 2024

ZacharyACoon commented Feb 29, 2024

tomasz1986 commented Feb 29, 2024

imsodin commented Mar 10, 2024

ZacharyACoon commented Mar 10, 2024 via email •

edited by imsodin

imsodin left a comment •

edited

imsodin Mar 10, 2024

ZacharyACoon Apr 1, 2024

imsodin Apr 1, 2024

imsodin Mar 22, 2024

ZacharyACoon Apr 1, 2024

imsodin Apr 1, 2024 •

edited

imsodin Mar 22, 2024

imsodin Mar 22, 2024

imsodin Mar 22, 2024

ZacharyACoon Apr 1, 2024

imsodin commented Mar 22, 2024

imsodin commented Apr 1, 2024

imsodin Apr 1, 2024

imsodin Apr 1, 2024

imsodin commented Apr 1, 2024

		Util.dismissDialogSafe(mDeleteDialog, this);

		outState.putBoolean(IS_SHOW_OVERRIDE_REMOTES_DIALOG, mOverrideChangesDialog != null && mOverrideChangesDialog.isShowing());

	outState.putBoolean(IS_SHOW_OVERRIDE_REMOTES_DIALOG, mOverrideChangesDialog != null && mOverrideChangesDialog.isShowing());
	outState.putBoolean(IS_SHOW_OVERRIDE_CHANGES_DIALOG, mOverrideChangesDialog != null && mOverrideChangesDialog.isShowing());

	<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>

	<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>

		getIntent().hasExtra(EXTRA_FOLDER_OVERRIDABLE_CHANGES)
		&& getIntent().getBooleanExtra(EXTRA_FOLDER_OVERRIDABLE_CHANGES, false)


		outState.putBoolean(IS_SHOW_OVERRIDE_CHANGES_DIALOG, mOverrideChangesDialog != null && mOverrideChangesDialog.isShowing());
		Util.dismissDialogSafe(mOverrideChangesDialog, this);

		LinearLayout container = (LinearLayout) findViewById(R.id.overrideChangesContainer);
		binding.overrideChangesContainer.setEnabled(state);

Greatly improve "Override Changes" button and UX (fixes #1769) #2063

Are you sure you want to change the base?

Greatly improve "Override Changes" button and UX (fixes #1769) #2063

Conversation

ZacharyACoon commented Feb 26, 2024

Description

Changes

Screenshots

tomasz1986 commented Feb 26, 2024 • edited

ZacharyACoon commented Feb 26, 2024 • edited

Changes

New Screenshots

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomasz1986 commented Feb 26, 2024

ZacharyACoon commented Feb 26, 2024

ZacharyACoon commented Feb 29, 2024

tomasz1986 commented Feb 29, 2024

imsodin commented Mar 10, 2024

ZacharyACoon commented Mar 10, 2024 via email • edited by imsodin

imsodin left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imsodin Apr 1, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imsodin commented Mar 22, 2024

imsodin commented Apr 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imsodin commented Apr 1, 2024

tomasz1986 commented Feb 26, 2024 •

edited

ZacharyACoon commented Feb 26, 2024 •

edited

ZacharyACoon commented Mar 10, 2024 via email •

edited by imsodin

imsodin left a comment •

edited

imsodin Apr 1, 2024 •

edited