Add Git Synchronization #173
Conversation
|
This is just a work in progress for the time being, but I wanted to put this up just in case anyone wants to take a look at the approach I am taking. |
colonelpanic8
force-pushed
the
gitSynchronization
branch
from
3d60470
to
bb82b36
Compare
|
@nevenz This is getting pretty close, although it's not quite there. Do you want to take a look at what I did with the UI and see if it's in line with what you would like? I had to make some changes to the existing code to make file browsing work that you might want to take a look at. |
|
I tried it and for now just took a quick look a the code...
Should cloning work already? I'm getting Might be what you mean by "not quite there", but since I tried it, I might as well report. I get the same when I try to open the fragment to edit the repo (from the list of repositories). I don't think anything serious (like cloning) should happen at that point. BTW, all that code should probably be moved from Apart from few smaller UI stuff which I'm not going to mention now - when checkmark is clicked, repos are being created (looking at the logs), but fragment is not closed. Also, it would be helpful displaying some error messages (even in this early stage) if repo is not created due to missing data or whatever.
I like it, I can't select the key though. I had to select a directory, then type the file name. |
Oh oops, I guess maybe we should make it so that the browser can select a file?
No, not quite yet, I need to fiddle with where it actually puts the stuff, and how it provides credentials
Yeah, actually I plan on doing the clone when the repo is created, to validate that all of the settings work as well, so I want to share the code somehow. I agree that it should be moved out of factory though.
Yeah agreed, thats another thing I need to get to. I mostly just wanted to get feedback on whether the changes to the starting of the browser okay. My main concern (and the reason I wanted to have you look at the UI stuff) is actually with the BrowserResultHandler browserResultHandler instance variable. I haven't really done any android development before this, but my understanding is that fragments can be destroyed at any time by the runtime if they aren't displayed. Could something weird happen here where that variable points to a dead fragment or something like that? I'm not 100% clear on the conditions under which fragments/activity can be destroyed/created. |
My idea with browser is to be reused for something like notes browsing too. For example for refiling when you have to choose the target note. So there could be implementation of Now, we either rename It's probably easier to just create a new implementation which would inherit the existing one, for now?
It should be OK. Activity can killed by the system if it's not displayed, but as long as there is a fragment displayed and others are in the backstack, they should be fine. BTW, looking at all those interfaces/listeners (and more coming in the future), I'm thinking we should switch to using activities per repo at one point (probably something I should have done from the start). It's getting confusing now who calls what etc. Maybe after this is released, I'll take a shot at that and see if it's worth it. |
Yeah that seems like the most reasonable approach to me.
Yes, definitely. The current setup is way too complicated. @nevenz It would really make the rest of the work for this pull request a lot easier if we could move to storing repository type in the database instead of relying on uris to determine repo type. What is the advantage of doing things that way? |
Sounds good to me. Are you suggesting we do this before this PR, or as a part of it? Which places besides |
|
@nevenz git synchronization should be able to handle the case where there are both local and remote modifications. How would you refactor things to allow this? |
|
@nevenz @tulth I would like to pursue a much more aggresive refactor of syncing that gives a bit more control to the particular sync types implementation. The current implementation makes quite a few assumptions that seem tightly coupled to current implementations. The idea would be to have the current code in Shelf.java to be one of multiple sync "strategies" that could be used. I think that at the very least, it should be up to the repo implementation to decide what the status of the sync is. The fact that this is not abstract causes a lot of issues. The ideal interface for git synchronization would combine book storage and retrieval: VersionedRook syncBook(Uri uri, File source, File destination, VersionedRook current) throws IOException; |
| Repo repo = getRepo(rook.getRepoUri()); | ||
| Repo.TwoWaySync sync = repo.getSync(); | ||
| if (sync != null) { | ||
| handleTwoWaySync(sync, namesake); |
There was a problem hiding this comment.
@nevenz I expect that this is probably not the way you would like me to handle this, but I need an interface like this to handle git synchronization properly.
Do you have ideas/feelings about how this should be done?
| BookAction.Type.INFO, | ||
| namesake.getStatus().msg(UriUtils.friendlyUri(repo.getUri().toString()))); | ||
| } | ||
|
|
||
| switch (namesake.getStatus()) { | ||
| case NO_CHANGE: |
There was a problem hiding this comment.
I feel like all of this should not be required of all synchronization strategies i.e. it should belong in its own class that CAN be used for some of the repos, but not others.
| @@ -42,4 +42,12 @@ | |||
| void delete(Uri uri) throws IOException; | |||
|
|
|||
| String toString(); | |||
|
|
|||
| TwoWaySync getSync(); | |||
There was a problem hiding this comment.
This is obviously hideous, I have a few ideas about how to get around it, but they require a lot of changes in Shelf.java
|
Synchronization sort of works now if anyone would like to try this. Some things (ssh key selection, directory naming, overlapping filenames across repositories) might be weird. |
|
Hi! I'm trying to test this out, and I'm having a bit of a problem building it, and I'm not exactly sure whats going on. Could someone take a look at my build? The full output is here. Otherwise, can someone provide me with an APK so I can try this out! I'm super excited for this, I've been waiting for it for a long time! |
|
@jgkamat Not sure what is going on there. Here's an apk: https://nofile.io/f/tdSBOAeOzS1/orgzly-git-sync.apk not that only ssh key support is added |
|
Im getting a crash on startup with the version you gave, the version from fdroid seems fine. |
|
@jgkamat Ahh oops, i know why thats happening. Try https://drive.google.com/file/d/0B4o0sMkRk8C1OU9Bc0VsTU1PbjA/view?usp=sharing |
|
What about: leaving |
@nevenz I'm definitely fine with that if it makes you happy. |
|
I wasn't able to get past the repository creation screen, the checkbox didn't seem to let me beyond the settings page. In the logs, there were a bunch of errors relating to 'could not delete' the directory where the git repo was to be cloned. after deleting it manually, the error still persisted. Also, once trying out the "Directory" sync after trying that out, I got a bunch of errors, and the file didn't import properly. Switching to the release version worked fine, so I think that this PR might need some more work 😢 |
|
@nevenz Random question -- Did you ever consider letting the actual files be the canonical source of data for orgzly? Given how quickly orgzly was able to sync my 100+ file repository, it seems like parsing etc. is quick enough that everything could be done in memory. I have a hard time believing that peformance is a huge concern in this case -- this is, after all, a TODO application, and we are not dealing with millions of pieces of DATA. |
Briefly in the beginning. But I didn't see that working very well or fast, without re-inventing a lot of things that you get when you use DB. Some of the reasons that come to mind... You'd have to rewrite the whole file after every note modification. Doing a lot of small modifications to just a single large notebook (which sounds like something that it's done often, at least I do) could be a battery killer. It's a similar issue to the one auto-sync now has. Search across all files would depend too much on number of files and their size and it could be very slow. In addition, it's way too easy to do a search with a complicated query now using a DB. Also, org files are just one of the possible file formats Orgzly supports for exporting and importing data. Something like BookName.Format is there from day 1 and is used throughout the code. I did get lazy over time, so there are quite a few Org-specific things in the code now. But it's one of the reasons I didn't consider using an org file as a way to store notes. |
colonelpanic8
force-pushed
the
gitSynchronization
branch
from
a3f9ab4
to
ff14666
Compare
|
@nevenz This is pretty much ready to go. There are several serious bug when the uri of a git repo is edited, but it is not because of any new code. |
|
@jgkamat Sorry about that, I've fixed a bunch of the issues. There might be a few rough spots with this version, but it should at least pull in your repository. |
|
Issues or missing features to be resolved before enabling this in the app
Minor stuff
|
|
Is this available to play with? |
Yeah I think this is pretty crucial. |
colonelpanic8
force-pushed
the
gitSynchronization
branch
from
b290d0a
to
566c20a
Compare
|
@nevenz Did my best to resolve conflicts, but I haven't touched android studio in a while and can't seem to get things working right now. The original history can be found here: |
Thanks for taking the time to do this. I was wondering what to do with it as I started moving repository fragments to activities, which would make merging even more difficult. It's merged to master now, so it doesn't fall behind again. It's hidden under There's no browser fragment anymore, so currently nothing happens on "Browse". I'll see to fix that and make remaining fragments (directory and now git) into activities. I'll keep the above comment updated.
You might need the latest version if haven't tried it already. |
It's |
|
Can I please send someone money to make this happen? If there were a kickstarter (or similar), I'd donate and help promote it. |
|
@bpiel you can always use the link I posted above to get a really old version that has working git synchronization: https://keybase.pub/imalison/orgzly-git-sync.apk I looked at trying to get stuff working again briefly when I did the merge a while back (Guess it was May 20th) and it seemed like a bit too much for me to take on at that moment. |
|
What is the status of this? What needs to get done? I would be willing to pick this up, if I can find some free time. I'm an android dev and would be comfortable building a diff gui of some sort, or whatever needs doing. Mostly because I would really like this feature and Orgzly doesn't offer me much value until it's in. |
|
@caleb-allen see the first comment on #543 (comment) . Would love it if you could finish this off. I'm not an android dev and it just became too painful for me to do the UI work. |
|
Thanks @IvanMalison to work on that, i see you use keybase, is there any chance that this plugin work with encrypted keybase:// git capabilities ? |
|
@reyman no, because it uses jgit |
|
@IvanMalison Seems some people use (with success, i need to test) another strategy with git-remote-gcrypt on termux : keybase/kbfs#1231 and https://md.hecke.rs/s/SyWx8-hBX# |
Fixes #24