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/Wizard: Init repo and connect to repo wizard pages and communication to daemon #297

Closed
binwiederhier opened this Issue Dec 11, 2014 · 28 comments

Comments

Projects
None yet
3 participants
@binwiederhier
Member

binwiederhier commented Dec 11, 2014

The wizard is missing sy connect and sy init wizard pages. Implementing will require:

  • 1. Init/Connect: Wizard page for plugin configuration (dynamically generated on basis of plugin settings vs. XML-description of wizard page)
  • 2. Init/Connect: Wizard page for "testing repository" with test results Not necessary.
  • 3. Init: Wizard page for entering a new password (+ confirmation password)
  • 4. Connect: Wizard page for connecting via syncany:// link
  • 5. Connect: Wizard page for entering the existing password
  • 6. New request message + request handler: InitWatchManagementRequest
  • 7. New request message + request handler: ConnectWatchManagementRequest
  • 8. Figure out a concept how to implement UserInteractionListener via WebSocket (used during InitOperation and ConnectOperation!)
  • 9. Figure out how to handle plugin option callbacks. Only allow before/after-plugin callbacks? Force order? Discuss with others.
  • 10. Centralize duplicate code in InitCommand <-> InitPanelController
  • 11. PluginSettingsPanel: Validate fields individually and alltogether. Highlight fields as invalid and make nicer error messages.
  • 13. PluginSettingsPanel: Allow non-string types in plugin text fields (File, int)
  • 14. Allow boolean types in plugin settings (= checkbox) No use case yet.
  • 15. File: Differ between folder and file, allow file masks (e.g. via @Setup(fileType = FileType.FILE, fileMask = "*.txt")); Discuss with others.
  • 16. Centralize warningImageLabel, warningMessageLabel, showWarning() and hideWarning() We can do this later.
  • 17. Disable GUI for plugins with nested settings (for now)
  • 18. Implement @OAuth annotation (explanation see below)
  • 19. Implement artificial OAuth token field (explanation see below)
  • 20. Implement invisible fields a la @Setup(visible = false) (explanation see below)
  • 21. Feedback on current code (see below)
  • 22. Eliminate onShowMessage and onUserNewPassword in UserInteractionListener (see below)
  • 23. Discuss naming of the web socket messages (see below)
  • 24. Implement 4 "connect" screen flows (see below)
  • 25. Remove "retry passwords" logic in ConnectOperation (see below)
  • 26. Test negative flows (wrong credentials, wrong password, rejecting a certificate, etc.)
  • 27. Focus first field per panel

This issue is flagged as prio:high because it's one of the few things separating us from having a usable GUI. The above TODO items are updated with new items and checked when done.

@binwiederhier

This comment has been minimized.

Show comment
Hide comment
@binwiederhier

binwiederhier Dec 15, 2014

Member

So I realized that before I start developing stuff like crazy, it makes sense to make a few drawings and discuss them here. Things are not as straight forward as I thought they would be. Here are my screen flows for the three use cases of the wizard so far.

wizard use cases

Thoughts?

Member

binwiederhier commented Dec 15, 2014

So I realized that before I start developing stuff like crazy, it makes sense to make a few drawings and discuss them here. Things are not as straight forward as I thought they would be. Here are my screen flows for the three use cases of the wizard so far.

wizard use cases

Thoughts?

@binwiederhier

This comment has been minimized.

Show comment
Hide comment
@binwiederhier

binwiederhier Dec 16, 2014

Member

Feedback on IRC. Too many steps. How's this?

wizard use cases

Member

binwiederhier commented Dec 16, 2014

Feedback on IRC. Too many steps. How's this?

wizard use cases

@pimotte

This comment has been minimized.

Show comment
Hide comment
@pimotte

pimotte Dec 16, 2014

Member

Looks sensible to me. I would recommend against showing details by default, since the vast majority of people are scared by text and entertained by (neverending) progress bars.

Member

pimotte commented Dec 16, 2014

Looks sensible to me. I would recommend against showing details by default, since the vast majority of people are scared by text and entertained by (neverending) progress bars.

@binwiederhier

This comment has been minimized.

Show comment
Hide comment
@binwiederhier

binwiederhier Dec 16, 2014

Member

@pimotte The details only pop out if something goes wrong.

Doebi also suggested some designs here -- content-wise they are pretty similar to what we've got above, so I'll continue with that approach. Redesigns can always be done at a later stage. I'm just not good with UI stuff ...

Member

binwiederhier commented Dec 16, 2014

@pimotte The details only pop out if something goes wrong.

Doebi also suggested some designs here -- content-wise they are pretty similar to what we've got above, so I'll continue with that approach. Redesigns can always be done at a later stage. I'm just not good with UI stuff ...

@binwiederhier

This comment has been minimized.

Show comment
Hide comment
@binwiederhier

binwiederhier Dec 17, 2014

Member

Some screenshots of the current branch implementation.

2014121701 33 09_selection_001
2014121701 33 18_selection_001
2014121701 33 27_selection_001
2014121701 33 40_selection_001
2014121701 33 48_selection_001
2014121701 33 58_selection_001
2014121701 34 11_selection_001
2014121701 44 39_selection_001
2014121701 44 59_selection_001

Member

binwiederhier commented Dec 17, 2014

Some screenshots of the current branch implementation.

2014121701 33 09_selection_001
2014121701 33 18_selection_001
2014121701 33 27_selection_001
2014121701 33 40_selection_001
2014121701 33 48_selection_001
2014121701 33 58_selection_001
2014121701 34 11_selection_001
2014121701 44 39_selection_001
2014121701 44 59_selection_001

@binwiederhier

This comment has been minimized.

Show comment
Hide comment
@binwiederhier

binwiederhier Dec 17, 2014

Member

The PluginSettingsPanel is created dynamically from the TransferPluginOptions that @cr0 invented (good stuff!). What's not clear for me here is how we should handle the interactive parts that we've successfully implemented in the CLI. Stuff like OAuth for the Dropbox/Flickr plugin.

@cr0 Ideas? Some help?

Branches:

Member

binwiederhier commented Dec 17, 2014

The PluginSettingsPanel is created dynamically from the TransferPluginOptions that @cr0 invented (good stuff!). What's not clear for me here is how we should handle the interactive parts that we've successfully implemented in the CLI. Stuff like OAuth for the Dropbox/Flickr plugin.

@cr0 Ideas? Some help?

Branches:

@cr0

This comment has been minimized.

Show comment
Hide comment
@cr0

cr0 Dec 17, 2014

Member

I suggest that we either

  1. use a push button to open the token request url and use a textfield to enter the token (basically the same like in the CLI)
  2. or use an embedded webview which loads the token request page (suggested method by Dropbox)

I would like to go with (1) since its more likely that a user is already logged in his Dropbox account which is obviously more comfortable (especially for users that use 2fa). It should be very easy to implement. Perhaps you can replace the button with a textfield for the token once it's clicked.

I think I got your question all wrong. You're asking how we could handle Callbacks? Is this mechanism already implemented?

Member

cr0 commented Dec 17, 2014

I suggest that we either

  1. use a push button to open the token request url and use a textfield to enter the token (basically the same like in the CLI)
  2. or use an embedded webview which loads the token request page (suggested method by Dropbox)

I would like to go with (1) since its more likely that a user is already logged in his Dropbox account which is obviously more comfortable (especially for users that use 2fa). It should be very easy to implement. Perhaps you can replace the button with a textfield for the token once it's clicked.

I think I got your question all wrong. You're asking how we could handle Callbacks? Is this mechanism already implemented?

@binwiederhier

This comment has been minimized.

Show comment
Hide comment
@binwiederhier

binwiederhier Dec 17, 2014

Member

Yes, I'm asking "how" :) The problem is that since we display all fields at the same time, we cannot force the user to enter the details in a specific order. So it's pretty difficult to react on "before this field" and "after this field" (like the callbacks do).

For Dropbox and Flickr (basically, for OAuth-powered backends), it might be sufficient to have a callback that displays a text "before the settings are set" (= display OAuth link); so maybe the per-field callbacks are not necessary. To be honest, I am just babbling along; I have no clue how to properly implement this...

Member

binwiederhier commented Dec 17, 2014

Yes, I'm asking "how" :) The problem is that since we display all fields at the same time, we cannot force the user to enter the details in a specific order. So it's pretty difficult to react on "before this field" and "after this field" (like the callbacks do).

For Dropbox and Flickr (basically, for OAuth-powered backends), it might be sufficient to have a callback that displays a text "before the settings are set" (= display OAuth link); so maybe the per-field callbacks are not necessary. To be honest, I am just babbling along; I have no clue how to properly implement this...

@binwiederhier

This comment has been minimized.

Show comment
Hide comment
@binwiederhier

binwiederhier Dec 17, 2014

Member

So the current branch (see above) implements the full 'init' process. It works alright, but it feels like it could blow up any time. What we basically do is the same as with the CLI: We gather all the data needed t call the InitOperation (via the wizard pages), and then we send a InitManagementRequest containing a InitOperationOptions object to the daemon. The daemon executes the operation and returns a InitManagementResponse containing an InitOperationResult object.

I had to duplicate lots of code from the InitCommand to create the InitOperationOptions object. That's ugly and needs to be cleaned up.

Also: Looking at the InitManagementRequest (or InitOperationOptions), we can see that it might need some cleanup too (in particular RepoTO and <cipherSpecs..>. That's a task for a different time though ...

Request:

<initManagementRequest xmlns="http://syncany.org/ws/1">
   <id>1181308374</id>
   <options>
      <createTarget>true</createTarget>
      <localDir>/home/pheckel/Syncany/NewFolder</localDir>
      <configTO xmlns="http://syncany.org/config/1">
         <machineName>xUNTdaHIVTCnXtdraVTx</machineName>
         <displayName>pheckel</displayName>
         <connection class="org.syncany.plugins.local.LocalTransferSettings" type="local">
            <path>/tmp/10</path>
         </connection>
      </configTO>
      <repoTO>
         <repoid>f2e5486a53f8ad4194bd14143a051e8929f4046d9519fb3baba806d2fdc7cde4</repoid>
         <chunker type="fixed">
            <property name="size">16</property>
         </chunker>
         <multichunker type="zip">
            <property name="size">4096</property>
         </multichunker>
         <transformers>
            <transformer type="gzip"/>
            <transformer type="cipher">
               <property name="cipherspecs">1,2</property>
            </transformer>
         </transformers>
      </repoTO>
      <encryptionEnabled>true</encryptionEnabled>
      <cipherSpecs class="java.util.ArrayList">
         <cipherSpec class="org.syncany.crypto.specs.AesGcm128CipherSpec">
            <id>1</id>
            <algorithm>AES/GCM/NoPadding</algorithm>
            <keySize>128</keySize>
            <ivSize>128</ivSize>
            <needsUnlimitedStrength>false</needsUnlimitedStrength>
         </cipherSpec>
         <cipherSpec class="org.syncany.crypto.specs.TwofishGcm128CipherSpec">
            <id>2</id>
            <algorithm>Twofish/GCM/NoPadding</algorithm>
            <keySize>128</keySize>
            <ivSize>128</ivSize>
            <needsUnlimitedStrength>false</needsUnlimitedStrength>
         </cipherSpec>
      </cipherSpecs>
      <password>qwertzuiop</password>
      <daemon>true</daemon>
      <genlinkOptions>
         <shortUrl>false</shortUrl>
      </genlinkOptions>
   </options>
</initManagementRequest>

Response:

<initManagementResponse xmlns="http://syncany.org/ws/1">
   <code>200</code>
   <requestId>1181308374</requestId>
   <result>
      <resultCode>OK</resultCode>
      <genLinkResult>
         <shareLink>syncany://2/32hqNN6woQm8bFqt7WyYbhTuAYjrdTpHkEQ57h3qMEudFGEHcxibatD6uTrXgMMiqwCe7pQXo4uK8ZQVTH1kHHzc/DzGFcfKcSuzBS3PBZ2...</shareLink>
         <shareLinkEncrypted>true</shareLinkEncrypted>
      </genLinkResult>
      <addedToDaemon>true</addedToDaemon>
   </result>
</initManagementResponse>
Member

binwiederhier commented Dec 17, 2014

So the current branch (see above) implements the full 'init' process. It works alright, but it feels like it could blow up any time. What we basically do is the same as with the CLI: We gather all the data needed t call the InitOperation (via the wizard pages), and then we send a InitManagementRequest containing a InitOperationOptions object to the daemon. The daemon executes the operation and returns a InitManagementResponse containing an InitOperationResult object.

I had to duplicate lots of code from the InitCommand to create the InitOperationOptions object. That's ugly and needs to be cleaned up.

Also: Looking at the InitManagementRequest (or InitOperationOptions), we can see that it might need some cleanup too (in particular RepoTO and <cipherSpecs..>. That's a task for a different time though ...

Request:

<initManagementRequest xmlns="http://syncany.org/ws/1">
   <id>1181308374</id>
   <options>
      <createTarget>true</createTarget>
      <localDir>/home/pheckel/Syncany/NewFolder</localDir>
      <configTO xmlns="http://syncany.org/config/1">
         <machineName>xUNTdaHIVTCnXtdraVTx</machineName>
         <displayName>pheckel</displayName>
         <connection class="org.syncany.plugins.local.LocalTransferSettings" type="local">
            <path>/tmp/10</path>
         </connection>
      </configTO>
      <repoTO>
         <repoid>f2e5486a53f8ad4194bd14143a051e8929f4046d9519fb3baba806d2fdc7cde4</repoid>
         <chunker type="fixed">
            <property name="size">16</property>
         </chunker>
         <multichunker type="zip">
            <property name="size">4096</property>
         </multichunker>
         <transformers>
            <transformer type="gzip"/>
            <transformer type="cipher">
               <property name="cipherspecs">1,2</property>
            </transformer>
         </transformers>
      </repoTO>
      <encryptionEnabled>true</encryptionEnabled>
      <cipherSpecs class="java.util.ArrayList">
         <cipherSpec class="org.syncany.crypto.specs.AesGcm128CipherSpec">
            <id>1</id>
            <algorithm>AES/GCM/NoPadding</algorithm>
            <keySize>128</keySize>
            <ivSize>128</ivSize>
            <needsUnlimitedStrength>false</needsUnlimitedStrength>
         </cipherSpec>
         <cipherSpec class="org.syncany.crypto.specs.TwofishGcm128CipherSpec">
            <id>2</id>
            <algorithm>Twofish/GCM/NoPadding</algorithm>
            <keySize>128</keySize>
            <ivSize>128</ivSize>
            <needsUnlimitedStrength>false</needsUnlimitedStrength>
         </cipherSpec>
      </cipherSpecs>
      <password>qwertzuiop</password>
      <daemon>true</daemon>
      <genlinkOptions>
         <shortUrl>false</shortUrl>
      </genlinkOptions>
   </options>
</initManagementRequest>

Response:

<initManagementResponse xmlns="http://syncany.org/ws/1">
   <code>200</code>
   <requestId>1181308374</requestId>
   <result>
      <resultCode>OK</resultCode>
      <genLinkResult>
         <shareLink>syncany://2/32hqNN6woQm8bFqt7WyYbhTuAYjrdTpHkEQ57h3qMEudFGEHcxibatD6uTrXgMMiqwCe7pQXo4uK8ZQVTH1kHHzc/DzGFcfKcSuzBS3PBZ2...</shareLink>
         <shareLinkEncrypted>true</shareLinkEncrypted>
      </genLinkResult>
      <addedToDaemon>true</addedToDaemon>
   </result>
</initManagementResponse>

binwiederhier added a commit to syncany/syncany-plugin-gui that referenced this issue Dec 18, 2014

@binwiederhier

This comment has been minimized.

Show comment
Hide comment
@binwiederhier

binwiederhier Dec 18, 2014

Member

@cr0
Feedback is very welcome on this :)

Member

binwiederhier commented Dec 18, 2014

@cr0
Feedback is very welcome on this :)

binwiederhier added a commit to syncany/syncany-plugin-gui that referenced this issue Dec 18, 2014

@binwiederhier

This comment has been minimized.

Show comment
Hide comment
@binwiederhier
Member

binwiederhier commented Dec 18, 2014

@binwiederhier

This comment has been minimized.

Show comment
Hide comment
@binwiederhier

binwiederhier Dec 19, 2014

Member

Here are the two additional panels necessary for the 'connect' screen flow:

2014121909 11 19_selection_001
2014121909 11 40_selection_001

Member

binwiederhier commented Dec 19, 2014

Here are the two additional panels necessary for the 'connect' screen flow:

2014121909 11 19_selection_001
2014121909 11 40_selection_001

binwiederhier added a commit to syncany/syncany-plugin-gui that referenced this issue Dec 19, 2014

binwiederhier added a commit to syncany/syncany-plugin-gui that referenced this issue Dec 19, 2014

@binwiederhier

This comment has been minimized.

Show comment
Hide comment
@binwiederhier

binwiederhier Dec 20, 2014

Member

@cr0 and I discussed some things on IRC. Here are the results:

ad "Oauth in GUI" (see item 9 above):

@Oauth(DropboxOauthGenerator.class)
public class DropboxTransferSettings extends TransferSettings {
  ...
}

public interface OauthGenerator {
  public URI generateAuthUrl();
  public void parseToken(String token);
}

public DropboxOauthGenerator implements OauthGenerator {
  ...
} 

ad "Oauth token field": Remove actual field and make "artificial field". Basically: If @Oauth annotation is present, generate a field "Token"

ad "Invisible fields": Some fields need to be invisible to the user, because we want to store information that is not relevant to the user (access/requestToken in Oauth). Solution: Implement @Setup(visible = false)

ad "UserInteractionListener via WebSocket" (see item 8 above): Continue approach with WebSocketUserInteractionListener and see where it leads.

Member

binwiederhier commented Dec 20, 2014

@cr0 and I discussed some things on IRC. Here are the results:

ad "Oauth in GUI" (see item 9 above):

@Oauth(DropboxOauthGenerator.class)
public class DropboxTransferSettings extends TransferSettings {
  ...
}

public interface OauthGenerator {
  public URI generateAuthUrl();
  public void parseToken(String token);
}

public DropboxOauthGenerator implements OauthGenerator {
  ...
} 

ad "Oauth token field": Remove actual field and make "artificial field". Basically: If @Oauth annotation is present, generate a field "Token"

ad "Invisible fields": Some fields need to be invisible to the user, because we want to store information that is not relevant to the user (access/requestToken in Oauth). Solution: Implement @Setup(visible = false)

ad "UserInteractionListener via WebSocket" (see item 8 above): Continue approach with WebSocketUserInteractionListener and see where it leads.

@binwiederhier

This comment has been minimized.

Show comment
Hide comment
@binwiederhier

binwiederhier Dec 20, 2014

Member

Target layout for OAuth plugins:

2014122015 17 37_selection_001

Member

binwiederhier commented Dec 20, 2014

Target layout for OAuth plugins:

2014122015 17 37_selection_001

@cr0

This comment has been minimized.

Show comment
Hide comment
@cr0

cr0 Dec 20, 2014

Member

👍

Member

cr0 commented Dec 20, 2014

👍

@binwiederhier

This comment has been minimized.

Show comment
Hide comment
@binwiederhier
Member

binwiederhier commented Dec 20, 2014

2014122017 26 49_selection_001

2014122017 28 06_selection_001

@binwiederhier

This comment has been minimized.

Show comment
Hide comment
@binwiederhier

binwiederhier Dec 20, 2014

Member

Works like a charm for the Flickr and Dropbox plugin.

Member

binwiederhier commented Dec 20, 2014

Works like a charm for the Flickr and Dropbox plugin.

@binwiederhier

This comment has been minimized.

Show comment
Hide comment
@binwiederhier

binwiederhier Dec 21, 2014

Member

Sooo. From my point of view, all positive use cases have been implemented. All that's left to do is to test the negative use cases and enhance the implementation. Some parts of the implementation still feel a bit wonky, because of all the web socket messages, user interaction listeners and so on (I'll outline them in another post). But all in all, it feels good enough to be merge-ready in a few days.

The newest screen is this onUserConfirm message box that plugins can use if they have questions for the user or need to show a message. Right now, only the WebDAV plugin is using this functionality (to reproduce this, delete your truststore.jks and keystore.jks in the daemon config dir and do initialize a WebDAV folder with https://...):

2014122123 58 22_selection_001

Member

binwiederhier commented Dec 21, 2014

Sooo. From my point of view, all positive use cases have been implemented. All that's left to do is to test the negative use cases and enhance the implementation. Some parts of the implementation still feel a bit wonky, because of all the web socket messages, user interaction listeners and so on (I'll outline them in another post). But all in all, it feels good enough to be merge-ready in a few days.

The newest screen is this onUserConfirm message box that plugins can use if they have questions for the user or need to show a message. Right now, only the WebDAV plugin is using this functionality (to reproduce this, delete your truststore.jks and keystore.jks in the daemon config dir and do initialize a WebDAV folder with https://...):

2014122123 58 22_selection_001

@binwiederhier

This comment has been minimized.

Show comment
Hide comment
@binwiederhier

binwiederhier Dec 22, 2014

Member

Here's the more detailed information on the remaining TODOs, as promised. I apologize that it's so long. It kinda got out of hand... I'd love some feedback, though!

ad (10): Duplicate code in InitCommand and InitPanelController:
Even though this seems like a tiny issue, it is part of a bigger issue we wanted to tackle way, way back in #29 (@gtrefs, cough). We should definitely centralize this code for now, but revisit the ugly structure of the RepoTO (see above) at a later point in time --especially because all the "chunking information" here is unused. Opinions?

(21) Feedback on code
... is very welcome!

(22) The UserInteractionListener has 4 methods:

  • (a) onShowMessage is supposed to show a message to the user. This is currently used to display "Creating master key .." message in the AbstractInitOperation; and to display "ERROR: Invalid password or corrupt ciphertext. X tries left." in the ConnectOperation. IMHO, this can be eliminated with the help of a ShowMessageInternalEvent or something.
  • (b) onUserConfirm can ask the user a yes/no question during Init/Connect. This is currently used by the WebDAV plugin to ask if a certificate should be trusted; and (mis-)used by the AbstractInitCommand to ask to retry the connection.
  • (c) onUserPassword is used to ask the user for a password during the ConnectOperation. This is needed because if we enter the details to the repo manually, we don't know if the repo needs a password before we run the ConnectOperation. The onUserPassword is a callback to ask for a password if none has been provided to the operation, but it is needed (relates to (24), see below!).
  • (d) onUserNewPassword is used to ask the user for a password and confirmation during the InitOperation. As of today, this is needed to first test the repo and then ask for a repo password (if the repo test was successful). IMHO, this is not really necessary if we change the screen flow of the CLI: We could ask for a password/confirmation before we test the repo. That way InitOperation would always have a password.

(f) As stated above: In my opinion, at least onShowMessage and onUserNewPassword can be eliminated. For the GUI<->Daemon user interaction stuff, I have only implemented onUserConfirm as part of the WebSocketUserInteractionListener. Messages (no onShowMessage) are simply ignored, and all passwords are asked before we run the operation (no onUserPassword/onUserNewPassword) --> we just supply the password to the operation --> it is never null --> the operation will not ask for it.

(g) As you can see the WebSocketUserInteractionListener implementation is quite a hackish solution. To fix that, we either need to implement the missing callbacks or eliminate them in the UserInteractionListener. Thoughts?

(22a) Side note / crazy idea TestRepoDetailsOperation:
Would extracting the repo test to its own operation and running it before (Init|Connect)Operation help? Maybe a TestRepoDetailsOperation? This would also allow the GUI to show more detailed results...

(23) Naming of the web socket messages:
By convention, all incoming messages (= to the daemon) are called *Request, and all outgoing messages (= sent by the daemon) are either called *Event or *Response. This makes sense for 95% of the messages so far. For the "user interaction" stuff, however, it doesn't:

Any ideas how to fix this? Not fix it at all? Just leave a nice comment at the class?

(24) Screen flow in "connect":
We have 4 "connect" use cases:

  • (a) Encrypted syncany://-link
  • (b) Unencrypted syncany://-link
  • (c) Manually entering connection details + encrypted repo
  • (d) Manually entering connection details + unencrypted repo

All of these cases need a different screen flow. As of today's implementation of the GUI, we only support (a) and (c), because we always ask for a password. So we assume that every repo is encrypted.

Detecting if a password is needed in use case (a) and (b) is trivial, because the ApplicationLink has an isEncrypted() method.

Detecting if a password is needed in use case (c) and (d) is not trivial, because we'd need to connect to the repo and download the syncany/master file first. This would imply a different screen flow for each use case ([ and ] indicate the start/stop of the ConnectOperation):

  • For (a): EnterPasswordPanel -> [ ProgressPanel (connect) ]
  • For (b): [ ProgressPanel (connect) ]
  • For (c): [ ProgressPanel (dwnld. 'syncany') -> EnterPasswordPanel -> ProgressPanel (conn.) ]
  • For (d): [ ProgressPanel (dwnld. 'syncany') -> ProgressPanel (conn.) ]

Not really a question here.
Maybe this: Okay like this?

(25) "Retry password" in ConnectOperation
For some reason, the ConnectOperation and the ConnectCommand implement a retry mechanism. The one in ConnectCommand is okay because it's UI logic-related, but the one in ConnectOperation sort of misses the point a bit I think: If the repo file is encrypted and the password is not present, it asks for the password and gives the user 3 tries to get it right. this logic should be present only in the UIs (GUI/CLI), but it is also present in the operation. With regards to the GUI also using this operation, this is quite odd. Opinions?

Member

binwiederhier commented Dec 22, 2014

Here's the more detailed information on the remaining TODOs, as promised. I apologize that it's so long. It kinda got out of hand... I'd love some feedback, though!

ad (10): Duplicate code in InitCommand and InitPanelController:
Even though this seems like a tiny issue, it is part of a bigger issue we wanted to tackle way, way back in #29 (@gtrefs, cough). We should definitely centralize this code for now, but revisit the ugly structure of the RepoTO (see above) at a later point in time --especially because all the "chunking information" here is unused. Opinions?

(21) Feedback on code
... is very welcome!

(22) The UserInteractionListener has 4 methods:

  • (a) onShowMessage is supposed to show a message to the user. This is currently used to display "Creating master key .." message in the AbstractInitOperation; and to display "ERROR: Invalid password or corrupt ciphertext. X tries left." in the ConnectOperation. IMHO, this can be eliminated with the help of a ShowMessageInternalEvent or something.
  • (b) onUserConfirm can ask the user a yes/no question during Init/Connect. This is currently used by the WebDAV plugin to ask if a certificate should be trusted; and (mis-)used by the AbstractInitCommand to ask to retry the connection.
  • (c) onUserPassword is used to ask the user for a password during the ConnectOperation. This is needed because if we enter the details to the repo manually, we don't know if the repo needs a password before we run the ConnectOperation. The onUserPassword is a callback to ask for a password if none has been provided to the operation, but it is needed (relates to (24), see below!).
  • (d) onUserNewPassword is used to ask the user for a password and confirmation during the InitOperation. As of today, this is needed to first test the repo and then ask for a repo password (if the repo test was successful). IMHO, this is not really necessary if we change the screen flow of the CLI: We could ask for a password/confirmation before we test the repo. That way InitOperation would always have a password.

(f) As stated above: In my opinion, at least onShowMessage and onUserNewPassword can be eliminated. For the GUI<->Daemon user interaction stuff, I have only implemented onUserConfirm as part of the WebSocketUserInteractionListener. Messages (no onShowMessage) are simply ignored, and all passwords are asked before we run the operation (no onUserPassword/onUserNewPassword) --> we just supply the password to the operation --> it is never null --> the operation will not ask for it.

(g) As you can see the WebSocketUserInteractionListener implementation is quite a hackish solution. To fix that, we either need to implement the missing callbacks or eliminate them in the UserInteractionListener. Thoughts?

(22a) Side note / crazy idea TestRepoDetailsOperation:
Would extracting the repo test to its own operation and running it before (Init|Connect)Operation help? Maybe a TestRepoDetailsOperation? This would also allow the GUI to show more detailed results...

(23) Naming of the web socket messages:
By convention, all incoming messages (= to the daemon) are called *Request, and all outgoing messages (= sent by the daemon) are either called *Event or *Response. This makes sense for 95% of the messages so far. For the "user interaction" stuff, however, it doesn't:

Any ideas how to fix this? Not fix it at all? Just leave a nice comment at the class?

(24) Screen flow in "connect":
We have 4 "connect" use cases:

  • (a) Encrypted syncany://-link
  • (b) Unencrypted syncany://-link
  • (c) Manually entering connection details + encrypted repo
  • (d) Manually entering connection details + unencrypted repo

All of these cases need a different screen flow. As of today's implementation of the GUI, we only support (a) and (c), because we always ask for a password. So we assume that every repo is encrypted.

Detecting if a password is needed in use case (a) and (b) is trivial, because the ApplicationLink has an isEncrypted() method.

Detecting if a password is needed in use case (c) and (d) is not trivial, because we'd need to connect to the repo and download the syncany/master file first. This would imply a different screen flow for each use case ([ and ] indicate the start/stop of the ConnectOperation):

  • For (a): EnterPasswordPanel -> [ ProgressPanel (connect) ]
  • For (b): [ ProgressPanel (connect) ]
  • For (c): [ ProgressPanel (dwnld. 'syncany') -> EnterPasswordPanel -> ProgressPanel (conn.) ]
  • For (d): [ ProgressPanel (dwnld. 'syncany') -> ProgressPanel (conn.) ]

Not really a question here.
Maybe this: Okay like this?

(25) "Retry password" in ConnectOperation
For some reason, the ConnectOperation and the ConnectCommand implement a retry mechanism. The one in ConnectCommand is okay because it's UI logic-related, but the one in ConnectOperation sort of misses the point a bit I think: If the repo file is encrypted and the password is not present, it asks for the password and gives the user 3 tries to get it right. this logic should be present only in the UIs (GUI/CLI), but it is also present in the operation. With regards to the GUI also using this operation, this is quite odd. Opinions?

@binwiederhier

This comment has been minimized.

Show comment
Hide comment
@binwiederhier

binwiederhier Dec 23, 2014

Member

ad (24): I implemented all of these cases. The WebSocketUserInteractionListener works better than expected. You can see case (c) in this video: http://youtu.be/uURhxVxr08c

Member

binwiederhier commented Dec 23, 2014

ad (24): I implemented all of these cases. The WebSocketUserInteractionListener works better than expected. You can see case (c) in this video: http://youtu.be/uURhxVxr08c

@binwiederhier

This comment has been minimized.

Show comment
Hide comment
@binwiederhier

binwiederhier Dec 23, 2014

Member

ad (24): What you can see in the video is this:

  1. After entering the WebDAV details, the click on 'Next' sends a ConnectInitManagementRequest and thereby starts the ConnectOperation on the daemon
  2. The WebDAV plugin in the ConnectOperation then uses the UserInteractionListener to confirm the SSL certificate (ConfirmUserInteractionExternalEvent).
  3. The user confirms: The GUI sends a ConfirmUserInteractionExternalManagementRequest.
  4. The ConnectOperation continues: It downloads the syncany file and realizes that it is encrypted. It then triggers the user interaction listener again: The daemon sends GetPasswordUserInteractionExternalEvent.
  5. The wizard reacts by showing the password panel
  6. The user enters the password and clicks 'Next'. The GUI sends GetPasswordUserInteractionExternalManagementRequest (containing the password) to the daemon.
  7. The ConnectOperation continues and eventually finishes and sends a ConnectInitManagementResponse.
Member

binwiederhier commented Dec 23, 2014

ad (24): What you can see in the video is this:

  1. After entering the WebDAV details, the click on 'Next' sends a ConnectInitManagementRequest and thereby starts the ConnectOperation on the daemon
  2. The WebDAV plugin in the ConnectOperation then uses the UserInteractionListener to confirm the SSL certificate (ConfirmUserInteractionExternalEvent).
  3. The user confirms: The GUI sends a ConfirmUserInteractionExternalManagementRequest.
  4. The ConnectOperation continues: It downloads the syncany file and realizes that it is encrypted. It then triggers the user interaction listener again: The daemon sends GetPasswordUserInteractionExternalEvent.
  5. The wizard reacts by showing the password panel
  6. The user enters the password and clicks 'Next'. The GUI sends GetPasswordUserInteractionExternalManagementRequest (containing the password) to the daemon.
  7. The ConnectOperation continues and eventually finishes and sends a ConnectInitManagementResponse.
@pimotte

This comment has been minimized.

Show comment
Hide comment
@pimotte

pimotte Dec 23, 2014

Member

Comments from dry-review:

(P1) From video: While "magic passphrase" is a nice computery-joke, it will confuse the hell out of people: "I just have a password, not a passphrase? Why is it asking for a passphrase? Also, I'M A MUGGLE I WANT NOTHING TO DO WITH MAGIC."

(P2) https://github.com/syncany/syncany-plugin-gui/compare/feature/issue297-wizard?w=1#diff-663ad6a54d5b3dcebc035ae023ba1834R117 claims an IllegalArgumentException, but also catches all of them and throws StorageExceptions instead?

(P3) https://github.com/syncany/syncany-plugin-gui/compare/feature/issue297-wizard?w=1#diff-dff4ec577167b005f8c067f01e57bb3bR63 Why did the internationalization get cut here?

(P4) Random thought: Should we support PluginOperations (most importantly, installing) through the GUI?

I scrolled through the GUI code, to me those things always feel convoluted, because of all the asynchronous events and such, but I have no actual suggestions how to do it simpler.

ad (22): Everything you say seems sensible. It seems most efficient if you do this, since you seem to have a good grasp on this and this is central to the whole thing.

ad(22a): Not crazy, but maybe do this later to prevent overcrowding of this branch?

ad (23): Maybe calling it a Negotiation? Why are all responses broadcasted to all socket listeners anyway? (I don't really have a problem with just commenting it, but the fact that this is so unnatural makes me kind of uneasy and it might be a sign of a deeper problem.)

ad (24): 👍

ad (25): Yes, this should be fixed. If it causes problems, we do it now, otherwise I would like to do it in a new branch, after this is merged.

ad (10): I'll give this a shot.

Member

pimotte commented Dec 23, 2014

Comments from dry-review:

(P1) From video: While "magic passphrase" is a nice computery-joke, it will confuse the hell out of people: "I just have a password, not a passphrase? Why is it asking for a passphrase? Also, I'M A MUGGLE I WANT NOTHING TO DO WITH MAGIC."

(P2) https://github.com/syncany/syncany-plugin-gui/compare/feature/issue297-wizard?w=1#diff-663ad6a54d5b3dcebc035ae023ba1834R117 claims an IllegalArgumentException, but also catches all of them and throws StorageExceptions instead?

(P3) https://github.com/syncany/syncany-plugin-gui/compare/feature/issue297-wizard?w=1#diff-dff4ec577167b005f8c067f01e57bb3bR63 Why did the internationalization get cut here?

(P4) Random thought: Should we support PluginOperations (most importantly, installing) through the GUI?

I scrolled through the GUI code, to me those things always feel convoluted, because of all the asynchronous events and such, but I have no actual suggestions how to do it simpler.

ad (22): Everything you say seems sensible. It seems most efficient if you do this, since you seem to have a good grasp on this and this is central to the whole thing.

ad(22a): Not crazy, but maybe do this later to prevent overcrowding of this branch?

ad (23): Maybe calling it a Negotiation? Why are all responses broadcasted to all socket listeners anyway? (I don't really have a problem with just commenting it, but the fact that this is so unnatural makes me kind of uneasy and it might be a sign of a deeper problem.)

ad (24): 👍

ad (25): Yes, this should be fixed. If it causes problems, we do it now, otherwise I would like to do it in a new branch, after this is merged.

ad (10): I'll give this a shot.

@binwiederhier

This comment has been minimized.

Show comment
Hide comment
@binwiederhier
Member

binwiederhier commented Dec 26, 2014

2014122604 10 39_selection_001

binwiederhier added a commit to syncany/syncany-plugin-gui that referenced this issue Dec 27, 2014

@binwiederhier

This comment has been minimized.

Show comment
Hide comment
@binwiederhier

binwiederhier Dec 27, 2014

Member

@pimotte Thanks for the comments, and sorry for the late reply -- Christmas and all ... 🎅

ad (P1)-(P3): Done.
ad (P4): Yes we should. Not in this issue though :)
ad (22): I removed onShowMessage, but I left onUserNewPassword in there for now. We can remove it later. I don't want to touch the CLI logic in this branch too much.
ad (22a): Agreed.

ad (23): Convention is convention; and it's only unnatural because we have that convention. But you are right, we only have that problem because of that. Essentially, we use the class type to route messages differently. Requests are sent to the event bus, Responses are forwarded to the originator (via WS or REST), and Events are broadcasted to all WS listeners. In my opinion it makes great sense; but in this special case, the naming is just off because the direction is reversed.

ad (25): The retry is actually quite nice even in the GUI, because the enter-password panel pops up up to 3 times before the operation fails completely.

I think this is merge ready. Objections?
If not: We'll keep it in 'develop' for a few days and then I'll release it as 0.4.0.

Member

binwiederhier commented Dec 27, 2014

@pimotte Thanks for the comments, and sorry for the late reply -- Christmas and all ... 🎅

ad (P1)-(P3): Done.
ad (P4): Yes we should. Not in this issue though :)
ad (22): I removed onShowMessage, but I left onUserNewPassword in there for now. We can remove it later. I don't want to touch the CLI logic in this branch too much.
ad (22a): Agreed.

ad (23): Convention is convention; and it's only unnatural because we have that convention. But you are right, we only have that problem because of that. Essentially, we use the class type to route messages differently. Requests are sent to the event bus, Responses are forwarded to the originator (via WS or REST), and Events are broadcasted to all WS listeners. In my opinion it makes great sense; but in this special case, the naming is just off because the direction is reversed.

ad (25): The retry is actually quite nice even in the GUI, because the enter-password panel pops up up to 3 times before the operation fails completely.

I think this is merge ready. Objections?
If not: We'll keep it in 'develop' for a few days and then I'll release it as 0.4.0.

@pimotte

This comment has been minimized.

Show comment
Hide comment
@pimotte

pimotte Dec 27, 2014

Member

ad (23): Question without looking at the code: can't we have the daemon be
the originator, such that it is handled as all responses and "forwarded" to
the daemon (either actually or virtually)?

If you think this is merge-ready, I'm good with merging, as long as you
make separate issues for the stuff we have identified above to not be
handled in this branch.

Member

pimotte commented Dec 27, 2014

ad (23): Question without looking at the code: can't we have the daemon be
the originator, such that it is handled as all responses and "forwarded" to
the daemon (either actually or virtually)?

If you think this is merge-ready, I'm good with merging, as long as you
make separate issues for the stuff we have identified above to not be
handled in this branch.

@binwiederhier

This comment has been minimized.

Show comment
Hide comment
@binwiederhier

binwiederhier Dec 27, 2014

Member

ad (merging): I'll merge soon and make new issues for the remaining, well, issues.

ad (23): No that unfortunately doesn't work -- or maybe I don't understand correctly. However, what do you think about introducing an ExternalEventResponse type and handle it similar to a request (just without expecting a response)? Edit: I implemented this. Looks nice :)

Just so you understand better:

  • Arriving Requests via REST are handled here, those arriving via WS are handled here.
  • ExternalEvents and Responses arriving on the event bus are handled here.
Member

binwiederhier commented Dec 27, 2014

ad (merging): I'll merge soon and make new issues for the remaining, well, issues.

ad (23): No that unfortunately doesn't work -- or maybe I don't understand correctly. However, what do you think about introducing an ExternalEventResponse type and handle it similar to a request (just without expecting a response)? Edit: I implemented this. Looks nice :)

Just so you understand better:

  • Arriving Requests via REST are handled here, those arriving via WS are handled here.
  • ExternalEvents and Responses arriving on the event bus are handled here.
@binwiederhier

This comment has been minimized.

Show comment
Hide comment
@binwiederhier

binwiederhier Dec 27, 2014

Member

Merged. 👍

Member

binwiederhier commented Dec 27, 2014

Merged. 👍

@binwiederhier

This comment has been minimized.

Show comment
Hide comment
@binwiederhier

binwiederhier Dec 27, 2014

Member

(25) is now #320
(22) is now #319
(P4) is now #321

Closing this.

Member

binwiederhier commented Dec 27, 2014

(25) is now #320
(22) is now #319
(P4) is now #321

Closing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment