search

syncthing / syncthing-android

Wrapper of syncthing for Android.
https://syncthing.net/
Mozilla Public License 2.0
3.16k stars 362 forks source link

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

Open ZacharyACoon opened 4 months ago

ZacharyACoon commented 4 months ago

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

Before After
(Not enabled) Screenshot_20240226-032518
Screenshot_20240226-034947 Screenshot_20240226-032540
(Enabled) Screenshot_20240226-032545
Screenshot_20240226-032549
tomasz1986 commented 4 months ago

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 commented 4 months ago

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

Changes

New Screenshots

Screenshot_20240226-051036

Screenshot_20240226-051117

Screenshot_20240226-051126

tomasz1986 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 https://github.com/syncthing/syncthing-android/issues/2064

ZacharyACoon commented 4 months ago

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 https://github.com/syncthing/syncthing-android/issues/2064

— Reply to this email directly, view it on GitHub https://github.com/syncthing/syncthing-android/pull/2063#issuecomment-1987255550, or unsubscribe https://github.com/notifications/unsubscribe-auth/AQVQTH7IJ5SLCXNOJ4SMXD3YXRWG3AVCNFSM6AAAAABD2CAPPGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOBXGI2TKNJVGA . You are receiving this because you authored the thread.Message ID: @.***>

imsodin commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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

imsodin commented 1 day ago

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