On Tue, Dec 24, 2013 at 3:57 PM, Philipp Heckel <[email protected]>wrote:
> Hello Vincent, > > First of all: *Merry Christmas!!* > Merry christmas too !! > > > I'm answering here so everyone can take part in this discussion. I'll give > a lot of feedback and I'm hoping that this will help you/us for the GUI. > It's not a criticism, it's a way to make a good product :-) You're doing a > great job and I love what you've done so far. Also, I think the > websocket+SWT stuff really was a good decision! > thx, I also think SWT is a good swing alternative > > @Everyone else: I'm referring to Vincent's work on the GUI here: > https://github.com/vwiencek/syncany > > I actually fiddled a bit with the code and put some of my suggestions into > code changes. You don't have to take all of these changes, but its always > easier (for me) to have a little code to express yourself :-) -- Here's my > branch: https://github.com/binwiederhier/syncany/tree/gui - To give you > an impression what this looks like on Linux right now, here are some > screenshots: imgur.com/fUH6h1g,AcSuezQ,V2cJxHV,wEbdnT7,hzxqRYt -- I made > the window a little bigger, that's why it looks a bit weird right now... > still struggling with SWT layouts .... but no reasons I can't fix it > > Now to my feedback! > > > *1. Executing 1.1.Executing with Gradle*: Running gradle runGui fails > with the following message. Any ideas? Running the printed command from > ("--info") from the command line works perfectly, just running it from > gradle doesnt work ... > > :syncany-plugins:syncany-plugin-rest:processResources UP-TO-DATE > :syncany-plugins:syncany-plugin-rest:classes > :syncany-plugins:syncany-plugin-rest:jar > :syncany-gui:runGui > Error: Could not find or load main class > :syncany-gui:runGui FAILED > > FAILURE: Build failed with an exception. > > * What went wrong: > Execution failed for task ':syncany-gui:runGui'. > > Process 'command '/usr/lib/jvm/java-7-openjdk-amd64/bin/java'' > finished with non-zero exit value 1 > ... might be linked to SWT gui thread. Just like swing, SWT uses a dedicated ui thread to update UI independently of your business logic. But this works differently between osx/windows/linux. I've managed to make it work under mac osx through specific jam arg. I've installed virtual box with ubuntu to test it under linux. > > *1.2. Eclipse execution*: It works from Eclipse, but I sometimes get the > following error in the "new Shell()" line: "Exception in thread "main" > org.eclipse.swt.SWTException: Invalid thread access" > It sometimes happens under windows when an other instance of the application is still running. More globally I need to figure out best practices to work with Shell and Dialog ..... I'm not as familiar with SWT than I am with swing, I still need to discover best practices. > > *3. GUI + Daemon* > *3.1. **GUI<->daemon interaction:* When the GUI is started, do you think > it should try to start the daemon if it is not running? What if the GUI is > exited -- the daemon shouldn't stop then, right? Spitballing: Maybe, if > the GUI has started the daemon (it was not running), it should also stop > it; but if the daemon was running before, it should leave it running. Does > that make sense? > *remarks :* - isn't it dangerous to run daemon on pure background ? we could imagine an other tray icon just indicating "sync any daemon running" + menu command to launch gui. - but sure if GUI starts dameon, then it should stop it / if daemon is already launched, gui shouldn't terminate it, or at least ask user to ? > *3.3. Storing settings: *"properties.txt": You're storing the watched > folders in ~/.syncany/properties.txt; it's probably easier to do it like > the the lib is storing the config (ConfigTO, RepoTO, etc.) > *remark :* - aren't gui application parameters linked to the application and not to the repository ? example : proxy settings if client is behind a proxy ? - instead of property files, we can imagine dedicated h2 database with cyphered parameters (in order to hide proxy passwords for example). > *3.4. SWT dialogs: *You always do a shell.dispose(); when switching to a > new dialog. Is this best practice? Because it creates a flickering on Linux > when showing the new dialog. Also, if the old dialog was moved, the new one > always appears in the middle of the screen. I know that's kind of a big > change, but is it possible to switch the panels inside a dialog instead of > the dialog itself? Or is that not best practice with SWT? > Yes, flickering to on windows :) Need to find best practices to do it. I'll probably be ending replacing "items" in already built dialogs. > *3.5. PluginGui -> Plugin panels:* I don't quite understand the rationale > behind the PluginGui class? It's never used, right? If you want to fill the > drop down for the available plugins, you could do that with "Reflections" > class and the method getSubTypesOf(PluginPanel) or something like that. > Ghost class ==> to be deleted > *3.6. Gradle module "syncany-plugins-gui"*: Is there a reason for this > module? Couldn't you just move it to the main gui package? Anything I'm not > seeing? > Yes sadly, the idea is the following to get clean design : - syncany-gui doesn't down anything about syncany-plugin-xxx and syncany-plugin-gui-xxx - but to be able to query plugin panels, and get dedicated arguments/parameters, we need to have plugin panels implement a specific interface, contained in syncany-plugin-qui. - then syncany-gui and syncany-plugin-xxx-gui only depend on syncany-plugin-gui It produces many projects with few files.... we could rethink this design of course. > *3.7. E-mail dialog and e-mail in general: *The mockup I sent you > included the e-mail dialog and you implemented it like that. However, I > don't see that this should be part of the "default" selection. In fact, I > don't think there is a good default storage selection right now. Any ideas? > I'd say the only option right now is to simply show the drop down list of > options. That's why I think the mockups are helpful, because we can discuss > before implementing... > ok > *3.8. "NewLocalFolders" dialog:* Right now we only support one folder per > repository (much like Dropbox), so there is no need for this dialog, right? > ok > *3.9. Panel graphic (wizard left side):* If you want, you can use the > panel graphic I used back in 2011: > https://github.com/binwiederhier/syncany/tree/d8fa30f57e37355af8f70f0c2eda510ec43cbe74/syncany-assets/artwork/rendered/wizard > ok > > *3.10. Connect panel: *The "connect" panel should/must also allow users > to enter the full connection details, because you don't always have the > same conneciton details and want to share them with others, e.g. an FTP > syncany://-link includes the username and password of a user; if two users > have different usernames/passwords, it'd be stupid if they couldn't connect > to a repo. > ok great, should then reuse plugin panels > > *4. Tray Icon* > *4.1. Tray on Linux Mint: *The tray icon (yes, I'm obsessed!) on Linux > looks good only if it's not resized, so getImage() not getResizedImage() > true. Linuw specific item: - idea would be to have already resized clean png > *4.2. Tray menu updates: *I can see that there is a method updateTray(), > but its never used. I tried to call updateTray(update.getData()); and it > worked. I fixed it in my branch. > just trying "live update" of tray menu while displayed > > *5. Packages and Files* > *5.1. Package dependencies: *There are many packages with just 1-2 > classes; As I am not a fan of package dependencies, I think you should > minimize them by putting things together that belong together. > ok, see above, I will rethink projects organisation > *5.2. syncany-util*: The syncany-util gradle sub project was a good > idea. However, I think it should only contain stuff that's actually needed > by more than just one module. An example would be the e-mail validation. > That's clearly just a GUI thing. As soon as it's needed in more than one > module, we can put it in syncany-util. Or is there a particular reason you > were putting it there? > ok great. Idea : we should have a "passwordStrenghChecker" with all password constraints in util project no ? > *5.3. OS.java: *we both have operating system detection classes. We > should consolidate them. Mine is called EnvironmentUtil > ok sure, > *5.4. Leftover resources, classes and methods: * You have a lot of files > checked in that are referenced nowhere. Images, classes, methods. These > should be cleaned up. > ok > *5.5. Code style: *Our coding styles are quite different. You'll see that > I moved stuff around a lot without changing the actual code. If possible, > it'd be great if you could try to adjust your style a bit to the rest of > the project. I know that's probably the hardest part, but it makes the > project look cleaner if the code looks similar in all classes. For some > things, you can use Ctrl+F. > ok > > I have more comments, but I think that's already a lot :) > And I should be getting to my parents' house anyway -- it's Christmas Eve > after all ... :-D > > All the best, > Philipp > > <http://pgp.mit.edu> > > -- > Mailing list: https://launchpad.net/~syncany-team > Post to : [email protected] > Unsubscribe : https://launchpad.net/~syncany-team > More help : https://help.launchpad.net/ListHelp > > -- Vincent Wiencek [email protected]
-- Mailing list: https://launchpad.net/~syncany-team Post to : [email protected] Unsubscribe : https://launchpad.net/~syncany-team More help : https://help.launchpad.net/ListHelp
