syncany-team team mailing list archive

[Vincent Wiencek <vwiencek@xxxxxxxxx>]

On Tue, Dec 24, 2013 at 3:57 PM, Philipp Heckel <philipp.heckel@xxxxxxxxx>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     : syncany-team@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~syncany-team
> More help   : https://help.launchpad.net/ListHelp
>
>


-- 
Vincent Wiencek
vwiencek@xxxxxxxxx