-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
gui: Allow to switch between folders/devices from inside Edit modal (fixes #7737) #8558
base: main
Are you sure you want to change the base?
gui: Allow to switch between folders/devices from inside Edit modal (fixes #7737) #8558
Conversation
…ixes syncthing#7737) Currently, one needs to repeatedly open and close the edit modal per folder/device in order to check or modify settings of multiple folders or devices. This process is slow and cumbersome. Thus, add "previous" and "next" buttons directly inside the edit folder/device, so that the user can switch between them without having to exit the modal. When doing the switching, we also make sure to preserve the currently opened tab, so that the user can easily compare the same settings for different folders/devices. Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
3e53632
to
7e5ff45
Compare
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
Does switching mean saving or not saving or somehow queueing the decision for later save/close pressing? |
It actually doesn't save anything at the moment. "Switching" is essentially equal to closing the modal for one folder and re-opening it for another folder. It should probably save, right? Or maybe ask to save if there are changes? |
I guess something like that? I suppose the use case for this is to make one change to several folders (they should all be send only instead of send-receive for now, for example)? I think it would have been neat if the changes accumulated "in memory" and were all either applied or discarded at the end with a normal tap of save/close. |
I can think of two use cases actually - one is what you wrote above, the other is simply to verify that folders are configured properly (e.g. I have enabled specific file versioning with custom settings, and now I want to verify that all folders are set the same way). Switching between them inside the modal is faster than unfolding folders one by one in the Web GUI (or even faster than using the Advanced Configuration, as a matter of fact). I'm not sure if I will be able to implement in-memory accumulation and mass application. I can definitely try implementing saving changes on switch though. |
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
I've updated the code to simply save changes on switch. Everything seems to work fine regarding the functionality but I'm not sure about the UI side of things (see the updated screenshots in the OP). I've put the two buttons deliberately next to the "Save" button and changed the colour to match to indicate that the changes are saved but I'd like to hear others' opinion on this. I've also looked into utilising the "discard changes" confirmation pop-up as already used in settings, i.e. however I've checked the code and it's coded specifically for settings, checking each and every value (see https://github.com/syncthing/syncthing/blob/main/gui/default/syncthing/core/syncthingController.js#L1491). To adapt this to folder and device modals, I guess the same check for all values present there would need to be done which isn't something I can really come up with in a short amount of time. I'm still going to consider looking into making the "discard changes" pop-up more universal but just not right now. |
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
The last commits The important part is when it comes to the Switch buttons. With this, if no settings have been touched, the switch doesn't need to save the config, and thus it's instant. When the settings have been changed, the buttons also change their colour to blue and then they do save the changes on press. This allows to make switching between folders/devices super quick when only verifying the existing settings while still allowing to save actual changes when there are any. The latter is slower, as Syncthing needs to save changes to the config upon pressing the Switch buttons. I've updated the screenshots in the OP. |
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
It seems that the Windows build keeps failing. It failed yesterday, and today again. Is something wrong? |
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
The Windows build has now completed successfully, however the others are failing. Doesn't seem related to this particular PR though. Any ideas? |
🤖 beep boop I'm going to close this pull request as it has been idle for more than 90 days. This is not a rejection, merely a removal from the list of active pull requests that is periodically reviewed by humans. The pull request can be reopened when there is new activity, and merged once any remaining issues are resolved. To permanently exempt this PR from bot nagging, add the |
Please reopen 🙂. What should we do with this one? The code does work, I've been using it myself since February. |
It looks like I just forgot to continue looking at it after your changes. I vow to do so again at some point soonish! 😅 |
Just for information, I've switched the PR to draft because there is a bug in the device switching mechanism which should ignore the own device but current does not. I'm going to fix this shortly. Edit: Fixed! |
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
Sorry for not voicing this earlier, I didn't have time to look at stuff back then. I really think the "accumulating, then confirm saving or discard all" behavior is the only sensible way here. The current iteration (from what I've read) sounds really complicated and I don't think many users will make sense of buttons changing colors etc.
Well, both of these use-cases will be covered by a "save all" confirmation. It needs to track what has changed anyway, and if nothing did (because we're just browsing through), the confirmation can be skipped. If I did change something by accident, I'd rather have the chance to discard changes at the end, instead of them being committed immediately with a button press that I rather meant to just take me to the next folder.
As a matter of a different fact, the Advanced Configuration editor doesn't even support any versioning settings, nor specifying which devices share a folder (or vice versa). I know it's asking much, but maybe your coding skills can grow from the challenge of the accumulated changes implementation? 😉 What I still really miss as a productivity feature working with more than a dozen of devices and folders would be a status table, with columns for every state and every config setting (including a device / folder sharing matrix) and rows for each folder (or device). It could be a read-only overview as a first step (simple HTML table) to get the data structures implemented sanely. Then slowly add on top any editing features. Including, of course, a single "discard or save" confirmation for all changed cells, which would require the same temporary structures as the proper solution here. What do you think about that idea? |
Thank you for all the comments! Personally, I'm not really sure whether I want to have all changes accumulated first, and only then applied. There are at least two reasons for this. Firstly, the current approach seems more efficient, because the changes are applied immediately on folder/device switch, meaning that Syncthing is already doing the work in background for the previous folder/device while you're editing the next one. This is especially true if you want to edit a very large number of folders/devices. In such a case, with accumulation, no changes would be applied until you've finished going through all of them (which can take a lot of time). Secondly, if you make a mistake while doing the editing (which I often do), it's easy to cancel simply by closing the folder/device modal, and doing so will only undo the changes done to that one folder/device while all other changes done to the previous folders/devices will still prevail. If the changes were to be accumulated, I think it would require quite a lot of gymnastics and checks e.g. if you wanted to cancel them for the current folder/device and still apply to the rest.
The buttons have tooltips now which are very clear about what they do, or at least I think so, but of course the alternative way is to simply use text, e.g. "Switch" and "Save & Switch". The textless buttons (with tooltips) look better and take less space though.
I'm all for it and believe that it would be interesting and useful, as currently the only way to have a view of everything (except for per-folder ignore patterns) in one place is to use and edit the |
It's also racy, as in applying changes can take a bit of time depending on connection latency, and the user might have stepped two folders ahead in the meantime. Can you be sure this is correctly queued, if stuff happens simultaneously?
Actually it's just one JSON object being transmitted instead of several subobjects, which is more efficient. What takes time is restarting the folder in the model I suppose, but who cares when you've just closed the dialog because finished? They will probably start to show Scanning anyway for some time. My concern is mainly that without a confirmation, there is simply no way to NOT save when switching to the next one. And adding a confirmation between each defeats the purpose of efficiently stepping through.
Not really, just add a "Revert" / "Forget changes" button which applies to the currently displayed folder only.
Yes definitely, some more engineering required. But the common requirement is that you need to keep not only |
I haven't encountered any real problems myself, but I guess that you can't prevent something from happening if there are latency issues. However, the same happens if you try to do modifications the current way, meaning opening a folder/device modal, applying modifications, saving, doing the same for the next one, etc. If the GUI gets laggy and unresponsive in the process, some of the changes eventually get forgotten. Fixing this would require a lot of code re-engineering and I think is way beyond this PR (which simply utilises the mechanisms already present in the GUI code) and my code skills.
What I meant actually is that it just takes time for the user to do their modifications to all folders/devices, and with accumulation, then they only get applied once they've finished going through all of them even if the user prefers to apply them immediately (which I myself normally do).
Yeah, that is true, but having a confirmation pop-up asking whether to save changes on every switch would probably be too annoying, and adding a separate button for save and a separate one for switch would make it look rather Frankenstein-ish.
This sounds like a good candidate for separate PR 😉. It would be cool to have a new "revert" button that restores the folder/device to its current state (by actually restoring the values in the modal without having to close and re-open it). |
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
To summarize my main concern: I wouldn't want this functionality if it immediately, silently saves upon switching to the next item. Just too invisible and unexpected if there is a "Save" button and I didn't click it. |
The racy problem has now been fixed by 95b3c26! I'm going to work at the apply/discard changes confirmation pop-up next. |
Currently, one needs to repeatedly open and close the edit modal per
folder/device in order to check or modify settings of multiple folders
or devices. This process is slow and cumbersome. Thus, add "previous"
and "next" buttons directly inside the edit folder/device, so that the
user can switch between them without having to exit the modal.
Signed-off-by: Tomasz Wilczyński twilczynski@naver.com
Screenshots
Folder (no changes)
Folder (changes)
Device (no changes)
Device (changes)