bgregos / foreground

Simple Android personal task manager with Taskwarrior integration
Apache License 2.0
253 stars 15 forks source link

Work on local taskwarrior file #145

Open PMunch opened 2 years ago

PMunch commented 2 years ago

I have taskwarrior set up with a shared folder through Syncthing to my various devices, including my phone. I'm looking for an easy way to add tasks to taskwarrior when I'm out and about and found this app. It has taskwarrior sync, but only to a taskwarrior server. I'd like to simply point it to my local taskwarrior files and have it work on those instead and let Syncthing handle all the synchronization for me.

martin-braun commented 1 year ago

I have the very same problem. The app looks great, but the most simple thing is not possible: Working with local files.

shfattig commented 1 year ago

This would be very helpful! I'm not a kotlin/android developer by any means, but it looks like this is just a tweak to how the app stores tasks, as well as likely a configuration option to choose your location. Currently it's stored in SharedPrefs (which I think should mostly be used for small sets of data like app preferences anyway). It looks like the correct file storage API to use would be either getExternalFilesDir or the MediaStoreAPI (depending on whether it is desirable to remove tasks on uninstall)

https://stackoverflow.com/questions/17119144/saving-data-on-android-file-storage-vs-sqlite-database-vs-shared-preferences

shfattig commented 1 year ago

@bgregos first of all - thoughts? I'm game to attempt the update if you think it a worthy improvement. If so, can you help point me? I'm working to understand the codebase but there's also some Android development mechanisms I don't yet understand (it looks like I need a context object but the point at which files are saved may not have access to that object?)

bgregos commented 1 year ago

@shfattig I think this is a worthwhile feature for sure and I'd be happy to accept a PR that implements this. It may be a bit more work than expected though. The feature will need the following:

  1. A layer to read/write task data to a group of files (pending.data, backlog.data, completed.data, and maybe undo.data) that's compatible with Taskwarrior. You'll need to do this manually, since the existing SharedPrefs implementation stores tasks in a JSON format that Taskwarrior can't read. deleted.data and completed.data will probably require deeper changes since Foreground basically "forgets" tasks once they're completed/deleted and they've been sent to a task sever.
  2. A way to migrate users from the SharedPrefs implementation to the new one. You can extend the migration system already in place at TaskRepository.kt:runMigrationsIfRequired

Some additional thoughts:

  1. We'll need to decide which API to use to handle the storage. The main options for sharing files outside the app on an ongoing basis are getExternalFilesDir or the Storage Access framework (which accesses shared storage). Using getExternalFilesDir would be great but it means other apps can see your task data by default, which is bad. The Storage Access framework route would require the user to pick a storage directory at first app launch, also bad. My recommendation here is to initially save to context.filesDir (private app-specific storage) and then have an option in settings to allow saving to external storage with a message about visibility to other apps.
  2. context is your hook into the system and is exposed at the UI layer. We generally don't want to pass this around to other higher layers if we can avoid it since it makes unit testing very difficult. This app uses dependency injection to inject children of context where needed. The individual children (or children of children) of context are easier to mock in testing.
  3. If you go down the internal/external files route, I created a skeleton with the DI stuff taken care of for you, it's available as the taskFileStorage branch.

I realize this feature is pretty complex, maybe more so than expected, but I wish you good luck if you want to take this on! I'm available to answer any questions you have along the way.

shfattig commented 1 year ago

@bgregos AWESOME, let's do it. I was able to track down where SharedPrefs is coming from and mimic the implementation using getExternalFilesDir (and successfully build, run, and write to a file!). This also helped me to find how context is being used. I also discovered a little more about what Dagger is doing to generate code. I'll have to compare with your branch and see if I'm on the right track. I see the steps you're pointing out, and I have a few thoughts:

  1. With the current implementation, as you mentioned, completed and deleted tasks are essentially "forgotten". Judging by the success of this app it's probably ok to retain that limitation and simply ignore the completed.data and deleted.data files (at least as a first implementation). I know at least for my mobile taskwarrior usage I'm ok with that limitation.
  2. However, this also brings up another point. I'm not sure yet how you've set up synchronization with taskserver (I know you have some java client, but idk how it works). If this feature shifts the app to use the same file database as taskwarrior, how do we interact with these files without re-implementing taskwarrior? It would be great to somehow embed the taskwarrior binary such that interactions with the app just generate "queries", that way we get things like completed/deleted for free, as well as any updates/fixes. But as it currently stands it seems like we'd have to actually implement direct interactions with the files. Ex. on task creation: instead of modifying the file to add a new task the exact same way taskwarrior would, just send a "task add" command to taskwarrior.
  3. Since it's all json it may be easy enough to just change the items we care about and leave the rest alone (i.e. just treat it like read-only metadata) which is likely how you're already doing things, but again I'm not sure how your sync stuff works.

In regards to a storage location, I think handling this particular feature request requires visibility to other apps, so that Syncthing can track the file and read/write. I guess I assumed we would keep the default storage location to SharedPrefs unless otherwise specified, but your recommendation seems to indicate that this feature would actually change the default location as well. Is your recommendation also based on the fact that defaulting to external/shared/visible storage would be bad practice and dirty since it is insecure and requires the user to specify a location?

Do you want me to open a WIP PR to continue communication? Would be an easier place to discuss code changes!

bgregos commented 1 year ago

I'm glad to hear you're going to be doing this!

  1. I'm fine with ignoring completed.data and deleted.data as long as it doesn't cause an issue with Taskwarrior. I think you may need to implement them so the other Taskwarrior client knows that those tasks have been completed. I'm not sure what to do with undo.data since Foreground doesn't support undo - we'd need some more investigation to see if we need to handle this.
  2. The app basically implements a full taskwarrior sync client without using any existing Taskwarrior binaries or code. Changing the underlying storage mechanism shouldn't affect sync, since Forground loads your task data into memory, works on it, and handles syncing from the copy of the data in memory.
    1. About integrating the Taskwarrior binary - this would be a (very) big lift since Foreground itself implements its own Taskwarrior client and we'd be effectively rewriting Foreground's entire data layer. I'm not opposed to this in the long run but it's probably outside the scope of this issue. Feel free to file a new issue if you'd like to explore this deeper.
  3. The problem here is that Taskwarrior's internal data format isn't JSON, so you'll need to convert everything into Taskwarrior's data format, including all fields. Taking a shortcut and only supporting fields is likely to cause issues in my opinion.
  4. Yes, my issue with defaulting to external/shared storage is that user data would be visible to all apps by default, which is a major security issue in my eyes. We would want to keep it private by default, using either the existing SharedPreferences implementation or using internal app-specific storage. I'd be fine keeping the SharedPreferences as the default storage mechanism and using external/shared storage if the user requests it. Ditching SharedPreferences entirely might be a bit easier, since you wouldn't need to juggle the different formats of SharedPreferences (json) and external/shared (Taskwarrior format) and you could use Taskwarrior format for both internal (private) and external (shared) storage.

Opening a WIP PR sounds good to me!