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

Implement Task to quickly sync files #37

Closed
wants to merge 26 commits into from
Closed

Conversation

newhinton
Copy link

@newhinton newhinton commented Jan 1, 2020

Also see original pull request.

  • Remove GPL-File (Switch to MIT)
  • !Check general Codestyling
    • Use constants where constants are appropriate
    • !Indentation
    • Naming
    • Remove unnessessary comments
  • Fix the "ghost" notification required to start a task via intentservice
  • Find a Solution for the configurationexport which uses deprecated methods
  • Remove logging
  • Write some tests
  • Properly document the intentservice
  • Find out how the intentservice needs to be structured to allow it to act as a tasker plugin
  • !Clean git-history
  • !Add myself to the contributors file

Additional features:

  • Add persistent notification after rclone finishes when it was started by an intent (so that the user is informed whether it was a success or not)
  • Add 'Pin to Homescreen'-shortcut for Task
  • Add switch to surpress notification for task so that no notification shows up

Further bugs:

  • Only one sync task can be run at a time
  • !Add proper icon for tasks in navigation-drawer
  • !Verify Dialog for taskcreate

@x0b x0b added the ✨ Enhancement New feature or request label Jan 1, 2020
@x0b
Copy link
Owner

x0b commented Jan 23, 2020

You have made a lot of progress :)

Do like to completely finish this or should we merge the main parts earlier and then later add additional features?

Could you please either confirm the CLA and/or add yourself to the contributors file? Because you own the rights to all your changes (not just new files), and your PR would then be covered by different licenses, which is a legal nightmare.

@newhinton
Copy link
Author

I will add myself to the contributors file :D

Yes and no. There are some things that are fine if they are not fixed for a release, but some should be taken care of beforehand. (Especially codestyle and the ui)

I will mark what i deem nessessary, and if you want to add/change priorities you can tell me (or edit the message directly? Is that possible?)

@x0b
Copy link
Owner

x0b commented Jan 29, 2020

Yes and no. There are some things that are fine if they are not fixed for a release, but some should be taken care of beforehand. (Especially codestyle and the ui)

Sorry, I did not want to rush this. I was thinking of these items...

  • Write some tests
  • Find out how the intentservice needs to be structured to allow it to act as a tasker plugin
  • Only one sync task can be run at a time

...which imho aren't really merge blockers.

I will mark what i deem nessessary, and if you want to add/change priorities you can tell me (or edit the message directly? Is that possible?)

I don't think i can change the list It seems that is actually possible, but you seem to have everything covered.

Also looking through the commits, firebase seems to cause a bit of a struggle when building. It might be a good idea to wrap that in a noop implementation that is replaced in release builds.

@CLAassistant
Copy link

CLAassistant commented Mar 6, 2020

CLA assistant check
All committers have signed the CLA.

@newhinton
Copy link
Author

@x0b I think for now i am done. I have added some new icons, i tried my best :D if anyone wants to redo them, thats fine by me ^^

this is far from perfect, but i guess it will be useful. Should i squash this?

@x0b
Copy link
Owner

x0b commented Mar 7, 2020

I would rebase it on top of the latest release, fix/squash the commits and then merge it. If you want less work with merge conflicts, I'd wait until 1.11 is merged into master (currently only in preview).

I haven't yet had the time to try out the current version, but it looks great.

@x0b x0b closed this Apr 8, 2020
@x0b x0b reopened this Apr 8, 2020
@x0b
Copy link
Owner

x0b commented Apr 8, 2020

Sorry, I accidentally deleted the base branch because it was no longer protected. What do you think, how far along are you with the feature?

@x0b x0b changed the base branch from master-x0b to master April 8, 2020 11:47
@newhinton
Copy link
Author

I think this is more or less done. If you dont see any issues with the code, or with something else i dont think there is a reason to not merge it

@x0b
Copy link
Owner

x0b commented Apr 10, 2020

Okay, I've pulled your changes and started rebasing them onto the latest master. This was required because development has diverged significatly from the original app, e.g. Gradle, Java 8, AndroidX, etc. The changes will be documented in git history, if you are interested.
Your PR is going to release in 1.12 🎉

A few ideas for later versions:

  1. I'd prefer using JSON for tasks - it has a few advantages:

    • We can directly serialize/deserialize the (annotated) Task class without any custom serializer code
    • We aren't using any of the XML features
    • We already have the required dependencies, while XML support comes from javax, which will probably need to be jettified to jakarta in the future
  2. The task creation/edit dialog multi step sequence is unusual for Android, and breaking established UX patterns does not make happy users. The Material solution would be a full screen dialog.

    Also, source and target folder entry really need a Picker. Especially since display names of remotes are not enforced to be unique, meaning the technical name might not be resolvable.

  3. We should evaluate Android Room for DB access - again, similar thing as with JSON (avoid mapper code), and it probably scales better than Android Preferences.

return sw.toString();
}

public static void storeFile(Activity activity, String content) throws IOException {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See rclone config import/export (#1, #2) for the non-deprecated solution

String xml = Exporter.create(activity);
Exporter.storeFile(activity, xml);
} catch (ParserConfigurationException | TransformerException | IOException e) {
e.printStackTrace();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always use FLog with appropriate log level and log tag (create log tags with logt shortcut). E.g. FLog.e(TAG, "Error when exporting tasks", e)

@x0b
Copy link
Owner

x0b commented Apr 10, 2020

Note: These comments are examples of things I'm changing before merging this into master - don't worry, you don't have to do this yourself.

@newhinton
Copy link
Author

@x0b do you want me to fix the issues you mentioned? That way we could merge it, if you are busy atm

@x0b
Copy link
Owner

x0b commented Apr 29, 2020

Sorry, this was a bit of a onboarding/collaboration/communicaton failure on my part.

When I initially pulled your code, I thought It would be ready to merge with a simple rebase. Unfortunately, the rcx code base has deviated quite a bit from the original rcloneExplorer code base - AndroidX, Java 8, View Binding - just on the tooling side. And it also needs to integrate properly with new rcx features like renaming remotes.

I did that before this reply. And then I started actually reviewing your code, and noticed that it differed wildly in quality from the existing project code.

And here is where I failed. I noticed more and more things that needed to be changed, and it just seemed ridiculous to add a review comment for all of them, and I also knew that I lacked the time to work with you on those things - I thought it would be more efficient if I just quickly solved those things and then later explained why. However, I did not consider the time needed to actually implement those changes, so progress has been slow.

Unfortunately, I haven't yet pushed the current state back to GitHub, but I can do that tomorrow or at the latest on the week. That way, you can work on it without me blocking you.

@newhinton
Copy link
Author

Ah okay :D what do you want me to do? I'd still like this feature, should i reimplement this or are you on it?

@x0b
Copy link
Owner

x0b commented May 1, 2020

Unfortunately, I haven't yet pushed the current state back to GitHub, but I can do that tomorrow or at the latest on the week. That way, you can work on it without me blocking you.

E.g. I'll push the current state to this branch on the weekend (I don't have it with me unfortunately) - it's 80% where it should be, so it should be relatively easy for you to continue working on this.

@x0b
Copy link
Owner

x0b commented May 3, 2020

I don't seem to have access rights to your branch, so I've pushed it to rcx/pull/37 - It is rebased on top of 1.11.3, so you'll need to pull it into a new branch (git checkout origin/pull/37).

@newhinton
Copy link
Author

@x0b I didn't think you would actually have rebased my code onto the new rcx-base, it's quite impressive!

Two things: i tested syncing two small files (with the rclone binary from 1.11.3) and it is extremely slow (1 or 2 b/s). Is that a known issue or is it just me?

Second: you mentioned that my code lacks quality, which i will not deny. But could you point me to stuff that should be improved, so that i can do that?

From now on, all my work will be based of your pull/37 branch

@x0b
Copy link
Owner

x0b commented May 5, 2020

Two things: i tested syncing two small files (with the rclone binary from 1.11.3) and it is extremely slow (1 or 2 b/s). Is that a known issue or is it just me?

For small files the transaction itself probably takes a significant time. I have not experienced slow speeds though - can you cross-check against executing the same command in Termux + rclone? If Termux is faster, we might be using rclone inefficiently.

Second: you mentioned that my code lacks quality, which i will not deny. But could you point me to stuff that should be improved, so that i can do that?

There is a lot of commentary in 2a19150, and there are also a few TODOs I didn't manage to address. For specific feedback, update this PR by force-pushing the pull/37 to your newhinton:master - I can then leave comments which will appear here.

Some things that I might comment on might not be "wrong", rather than unnecessarily unusual. For a project with our line count, it is very important to keep the code as readable and maintainable as possible, which means using the same style and approaches as the rest of the code base.

@newhinton
Copy link
Author

For the slow speeds: I found out that it was the bruteforce protection of nextcloud that delayed everything. It seems that my initial builds with a wrong password triggered the protection.

For the rest: i will do that, and hopefully we can merge this soon!

On a sidenote, it's offtopic but i didnt want to open a new ticket for that, it would be cool if you could make the link to "https://x0b.github.io/dev/" a little more visible in the page, or maybe add an index. to make finding it a little bit easier :D

@x0b
Copy link
Owner

x0b commented May 6, 2020

For the slow speeds: I found out that it was the bruteforce protection of nextcloud that delayed everything. It seems that my initial builds with a wrong password triggered the protection.

Good that that's solved. I was afraid we were doing something stupid with rclone.

On a sidenote, it's offtopic but i didnt want to open a new ticket for that, it would be cool if you could make the link to "https://x0b.github.io/dev/" a little more visible in the page, or maybe add an index. to make finding it a little bit easier :D

The dev page is linked from the README, but the website is still a work in progress. I'll look into the index thing.

@x0b
Copy link
Owner

x0b commented Jun 10, 2020

For the rest: i will do that, and hopefully we can merge this soon!

Just checking in if you are still up for this? I am currently extremely busy and won't have the time to make further adjustments to release this. I'm planning a release in 2-3 weeks.

Also, GitHub support has informed me that they have successfully detached this repo from kaczmarkiewiczp'. They have further explained that you will have to create a new PR for that. Also, I'm planning on completing the move to the new app name and structure, which means we should merge your PR before that to avoid conflicts.

@newhinton
Copy link
Author

@x0b

I have updated the tasks to rclone. I manually moved the files to the latest master.

#84

@newhinton newhinton mentioned this pull request Jun 9, 2021
3 tasks
@newhinton
Copy link
Author

closed in favor of #84

@newhinton newhinton closed this Jun 11, 2021
@newhinton newhinton mentioned this pull request Jun 12, 2021
23 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants