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

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ZacharyACoon
Copy link

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) Screenshot_20240226-032518
Screenshot_20240226-034947 Screenshot_20240226-032540
(Enabled) Screenshot_20240226-032545
Screenshot_20240226-032549

@tomasz1986
Copy link
Contributor

tomasz1986 commented Feb 26, 2024

Can you change the look and wording to match what is used in the Web GUI?

image

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
Copy link
Author

ZacharyACoon commented Feb 26, 2024

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

Screenshot_20240226-051036

Screenshot_20240226-051117

Screenshot_20240226-051126

Util.dismissDialogSafe(mDeleteDialog, this);

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

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

@@ -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>
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

@@ -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>
Copy link
Contributor

Choose a reason for hiding this comment

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

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>

<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>
Copy link
Contributor

Choose a reason for hiding this comment

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

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>

Copy link
Author

Choose a reason for hiding this comment

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

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

@tomasz1986
Copy link
Contributor

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 ZacharyACoon force-pushed the improve_override_warnings branch 2 times, most recently from ae1a93e to fcfd57f Compare February 26, 2024 13:34
@ZacharyACoon
Copy link
Author

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Member

imsodin commented Mar 10, 2024

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
Copy link
Author

ZacharyACoon commented Mar 10, 2024 via email

Copy link
Member

@imsodin imsodin left a comment

Choose a reason for hiding this comment

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

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

Comment on lines 182 to 183
getIntent().hasExtra(EXTRA_FOLDER_OVERRIDABLE_CHANGES)
&& getIntent().getBooleanExtra(EXTRA_FOLDER_OVERRIDABLE_CHANGES, false)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Comment on lines +336 to +337

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

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@imsodin imsodin Apr 1, 2024

Choose a reason for hiding this comment

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

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

Comment on lines 809 to 808
LinearLayout container = (LinearLayout) findViewById(R.id.overrideChangesContainer);
binding.overrideChangesContainer.setEnabled(state);
Copy link
Member

Choose a reason for hiding this comment

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

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

Comment on lines 107 to 108
boolean overridableChanges = folder.type.equals(Constants.FOLDER_TYPE_SEND_ONLY)
&& folderStatus.isNeedsItems()
&& folderStatus.isOutOfSync();
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

I removed the isNeedsItems as redundant.

@imsodin
Copy link
Member

imsodin commented Mar 22, 2024

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
Copy link
Member

imsodin commented Apr 1, 2024

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 imsodin changed the title Greatly improve "Override Changes" button and UX. Greatly improve "Override Changes" button and UX (fixes #1769) Apr 1, 2024
Comment on lines 186 to 196
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();
}
}
Copy link
Member

Choose a reason for hiding this comment

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

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.

Comment on lines +31 to +37
public boolean isNeedsItems() {
return needFiles + needDirectories + needSymlinks + needDeletes > 0;
}

public boolean isOutOfSync() {
return state.equals("idle") && isNeedsItems();
}
Copy link
Member

Choose a reason for hiding this comment

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

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

@imsodin
Copy link
Member

imsodin commented Apr 1, 2024

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

@imsodin
Copy link
Member

imsodin commented Jul 7, 2024

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

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.

Improve Override/Revert Changes UX (wording, warning/explanation dialog, ...)
3 participants