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

gui: Allow to switch between folders/devices from inside Edit modal (fixes #7737) #8558

Draft
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

tomasz1986
Copy link
Contributor

@tomasz1986 tomasz1986 commented Sep 27, 2022

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)

image

Folder (changes)

image

Device (no changes)

image

Device (changes)

image

…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>
@tomasz1986 tomasz1986 force-pushed the tomasz86/gui/switch-between-folders-and-devices branch from 3e53632 to 7e5ff45 Compare September 27, 2022 14:03
@calmh
Copy link
Member

calmh commented Sep 27, 2022

Does switching mean saving or not saving or somehow queueing the decision for later save/close pressing?

@tomasz1986
Copy link
Contributor Author

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?

@calmh
Copy link
Member

calmh commented Sep 27, 2022

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.

@tomasz1986
Copy link
Contributor Author

tomasz1986 commented Sep 27, 2022

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

tomasz1986 commented Jan 5, 2023

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.

image

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>
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
@tomasz1986
Copy link
Contributor Author

tomasz1986 commented Jan 6, 2023

The last commits 5862f53 (#8558) and 304c815 (#8558) disable the Save button when there are no changes. When it comes to the standard Save, this provides the user with information whether the settings have been modified or not, and the modal can still be closed either with the Close button or simply by clicking somewhere outside of it.

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

It seems that the Windows build keeps failing. It failed yesterday, and today again. Is something wrong?

@tomasz1986
Copy link
Contributor Author

The Windows build has now completed successfully, however the others are failing. Doesn't seem related to this particular PR though. Any ideas?

@st-review
Copy link

🤖 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 slow-pr label.

@st-review st-review closed this May 11, 2023
@tomasz1986
Copy link
Contributor Author

Please reopen 🙂.

What should we do with this one? The code does work, I've been using it myself since February.

@calmh calmh reopened this May 12, 2023
@calmh
Copy link
Member

calmh commented May 12, 2023

It looks like I just forgot to continue looking at it after your changes. I vow to do so again at some point soonish! 😅

@tomasz1986 tomasz1986 marked this pull request as draft July 20, 2023 17:41
@tomasz1986
Copy link
Contributor Author

tomasz1986 commented Jul 20, 2023

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!

@tomasz1986 tomasz1986 marked this pull request as ready for review July 20, 2023 19:53
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
@acolomb
Copy link
Member

acolomb commented Jul 25, 2023

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.

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

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.

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

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?

@tomasz1986
Copy link
Contributor Author

tomasz1986 commented Jul 25, 2023

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.

I don't think many users will make sense of buttons changing colors etc.

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.

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?

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 config.xml file (which is limited in terms of functionality as the changes aren't applied live), but this sounds like a whole new and ambitious project in comparison to what I'm doing here (which is just about enhancing the current GUI a little) 😉.

@acolomb
Copy link
Member

acolomb commented Jul 25, 2023

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.

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?

This is especially true if you want to edit a very large number of folders/device. 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).

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.

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.

Not really, just add a "Revert" / "Forget changes" button which applies to the currently displayed folder only.

... but this sounds like a whole new and ambitious project in comparison to what I'm doing here (which is just about enhancing the current GUI a little)

Yes definitely, some more engineering required. But the common requirement is that you need to keep not only $scope.currentFolder (IIRC) but instead an array like $scope.foldersConfigTemp. Whether you use that to step through a single form or format the whole thing as a table is then a smaller enhancement.

@tomasz1986
Copy link
Contributor Author

tomasz1986 commented Jul 25, 2023

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?

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.

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.

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

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.

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.

Not really, just add a "Revert" / "Forget changes" button which applies to the currently displayed folder only.

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

acolomb commented Jul 30, 2023

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.

@tomasz1986
Copy link
Contributor Author

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?

The racy problem has now been fixed by 95b3c26! I'm going to work at the apply/discard changes confirmation pop-up next.

@tomasz1986 tomasz1986 marked this pull request as draft September 5, 2023 23:43
@tomasz1986 tomasz1986 added the slow-pr Pull requests that are worked on infrequently label Dec 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
slow-pr Pull requests that are worked on infrequently
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants