[WIP] Modernize and new features

[dliw]

Hello!
After using your app for a while, I started to do some modifications here and there. In the meantime the changes are massive and I have a hard time to follow upstream (@WildOrangutan 😉). Unfortunately, I also missed the point where I still could separate individual changes. At the same time these changes might be useful for others as well. Therefore, I post my current status. If you are interested, I will do my best to get it up to date and in a mergeable state.

To build the app, the following dependency is needed in the main folder (should be a git submodule):
https://github.com/dliw/timezonepicker

Changes (no particular order)

• migrated to AndroidX
• ported away from deprecated functions (multiselect, preferences, ...)
• ported to java.time using ThreeTenABP (in the meantime could work natively), added support for DST and time zones
• ported to view binding
• new database scheme:
  • table event: store times as timestamps to avoid parsing strings
  • no more week table, instead cache with intermediate results at the beginning of every month and week
  • new table target to store per-day settings (target time overrides like holidays etc.)
• new navigation drawer
• restored top buttons (new preference, default off)

[mathisdt]

Hi @dliw, it's great to see that you made so many improvements (some of which I had also in mind, see #9 or #12)! I'd very much like to be able to merge your modifications (and possibly some of the TODOs also). I'm happy you shared this, thanks! 😃

One thought (as you already wrote about the preferences): Could you make the navigation buttons switchable by preference setting? Some might feel the buttons are redundant. But this is no requirement for your final PR to be merged...

[dliw]

Great, thanks for your interest. How do you suggest we proceed?

In terms of functionality, my code more or less has your state from the beginning of the year plus the new view pager for the week table. A merge would reset all other changes and that can't be the goal. I yet have to understand the new flexi option and figure out, how to migrate it to the modified time calculator.
Most of my code is well tested and in daily use. I verified, that the modified time calculator gets exactly the same results as the reference I started from (with some bugs fixed when on and off events were on different days).

Changes for the version I just pushed:

• InsertDefaultWorkTimes is implemented
• Buttons for navigation are off by default and can be activated in the settings

[mathisdt]

I suggest you try to migrate the changes from this repo into your fork and ask questions if there's something unclear. It took me some attempts at understanding the concept behind PR #59 but it's really not as complicated as it seems - the main point is that you can add a FLEX event at a specific time (e.g. 6 a.m.) and then your daily working time is set to this amount of time (6 hours in the above example). If the FLEX event was at 6 p.m. you would have to work 18 hours until your flexi time was at the same level as on the day before. And finally, if you set the FLEX event at midnight (0:00), the day doesn't have working time even if it's a regular work day, so the flexi time stays the same - and if you clock work on such a day, the flexi time increases from the first minute of work. Maybe @dirkmb can explain it better, but that's my take.

Do you mean you created unit tests when you wrote "well tested"? @WildOrangutan just wrote some new tests and migrated some older tests which originally were hidden in the app's production code - my bad, I didn't know better back then 😐. Maybe you can combine the tests with yours?

Again, if there's something unclear, I'll try to help. And thank you for taking this step and not just keeping your improvements for yourself!

[WildOrangutan]

If I can add, I would suggest merging multiple times from master into this branch.

I would merge from where you're at currently, to the master that is closest to your branch state. I would then do a merge every time feature branch (aka PR) was merged to master. Or something like that, depends on amount of changes - common sense applies.

Doing that, I think reviewer could easier compare apples to apples.

[dliw]

Thanks for explaining the FLEX event. It seems, it will be possible to migrate these events to the new target table.

Do you mean you created unit tests when you wrote "well tested"?

It means, that I rewrote the core time calculator more than a year ago and been using it since then without issues. Also, I have an app installation for testing purposes with a set of events including various corner cases. The latter could be turned into a unit test, but at the moment this is not my priority. As far as the existing tests are concerned, most of them can be dropped because they are no longer necessary when java.time is used.

If I can add, I would suggest merging multiple times from master into this branch.

The thing is, that I started playing around with the code without any version control (my bad, I should have known better). To make matters worse, my editor changed all tab indents to space indents. Eventually I started a local git repo, but completely separate from upstream (trackworktime looked rather dead at this time). Finally, to prepare this pull request, I imported upstream and tried to reduce the diff as far as possible. Actually, the only clean way would be to start from scratch...

My strategy is to port over all the missing features into this branch and once again try to reduce the diff as much as possible. I hope I can find the time and it works out. If not, feel free to use parts of my code as you see fit.

[dliw]

• Changes now on top of upstream master, resolved merge conflicts
• Ported flexi reset and related tests to java.time

[dliw]

• consolidated my intermediate database migration steps
• reworked auto pause (should work again)

[dliw]

• removed unnecessary changes, diff got smaller
• reworked calculation of remaining work time
• flexi reset fully integrated (needs testing)
• migration of flexi events to new target table (also needs testing)

The last missing piece (apart from more cleanup and testing) is backup and restore. With the new target table (or should I call it meta as it contains metadata?) there are now two tables that need to be saved. The reason I introduced this table is time zones. With time zones there is no such thing as a start of the day, so per-day settings didn't really fit into the event table or would have had to be rewritten on every change of the home time zone. With this table it is only a matter of recalculating the cache table. Also, this table could be used to store changes to the weekly target working time and the like.

However, for backup and restore this makes things more complicated. I see several possible solutions:

1. Export both tables to separate csv files. Probably the easiest and most transparent to external tools.
2. Join both parts into a pseudo csv with some separation marker.
3. Create a hybrid csv column scheme that fits for both event and target table.
4. Find a way to get along with only the event table (hard for the reasons mentioned above).

What would you prefer? I haven't had a closer look at the backup and restore code yet ...

[dliw]

Wrong button 🤦

[mathisdt]

@WildOrangutan Can you recommend one of the options? As far as I remember you implemented the backup...

[dliw]

@mathisdt Are you sure? The "blame" for backup (backupToWriter) and restore (restoreFromReader) is on you according to git:
https://github.com/mathisdt/trackworktime/blame/master/app/src/main/java/org/zephyrsoft/trackworktime/database/DAO.java

Edit: The commit message says contibuted by Peter Rosenberg...

[mathisdt]

Yes, that was Peter's work, but I pushed it. I don't know anymore why I did it that way, it's more than six years ago...

[WildOrangutan]

Sorry, but I didn't implement backup. I think you have me confused with the "other" Peter :)

[mathisdt]

I'm sorry, I really confused the two of you.

@PRosenb Can you recommend a course of action for the backup?

[WildOrangutan]

Does the backup really need to be in readable format tough? What if backup was changed, and would plainly copy database file?

At this point, we're basically maintaining two databases. Sure, csv format is more resistant to changes, but still needs to be migrated sometimes.

If anyone would need all events in readable format, it's still possible to export them with "reports".

[dliw]

I'm not sure, if a database file would be that much easier than one (or more) csv files. You will still want to verify the data on import and you wouldn't export the cache, so the backup file would be more of an export similar to what is needed for csv than a simple copy. The advantage of csv is that it is "transparent" and can easily be processed with other tools (and be it Excel).

The question of compatibility (forward or backward) IMO is independent of the export format. In any case it would be good if older exports could be imported into a newer app versions.

[PRosenb]

Since implementation of that backup behaviour, a lot changed on Android side and "Android Auto Backup" was introduced.
https://developer.android.com/guide/topics/data/backup

For backup purpose I suggest to solely use "Android Auto Backup" (from Android 6.0) what basically stores the database files (and other files) and restores them back on reinstallation of the app. So on dev side we only need to configure "Android Auto Backup" in Android-Manifest.xml and the related backup config file to include/exclude the file we want. And remove the code that did backup with "Android Backup Service".

To allow the user to export his/her data is a different story.
Would it be possible to recreate the database when the two files are exported and import again later?
What do you think? Should we try to still allow that or is it enough when the users rely on the backup behaviour by Android?

[dliw]

Would it be possible to recreate the database when the two files are exported and import again later?

If you refer to what I have called solution 1 the answer is yes. It would be one csv for the actual check in and check out events and one for the metadata. Also, the tables are only loosely linked, so it would be possible to import only either of both.

or is it enough when the users rely on the backup behaviour by Android?

This app is on F-Droid, so there is a chance, there are people with a more or less Google-free Android. In this case, I assume, Android Auto Backup does not work. Also, an own import and export functionality could be useful for backups to self hosted data storages (Nextcloud), for synchronizing multiple devices, advanced report generation etc.

[PRosenb]

Yes, I referred to your solution 1. I see about Google-free Android.

In that case I guess ideal would be Android Auto Backup for the users that have a Google Account (similar to how "Android Backup Service" was and almost free to implement) and the export/import of your solution 1 for Google Account free user and the once how want to selfhost their data.

[dliw]

Backup and restore is implemented now. For CSV two files are created, I extended the existing solution to also write the target table. I kept using the Android Backup Service. It could be migrated to Android Auto Backup as @PRosenb suggested, but I cannot really test.

Some minor things are still missing and code could be cleaned up a bit more. Nevertheless I think the code is now ready for (a first round of review) and testing.

[dliw]

I'm unable to reproduce the widget error. I tested the widget on two devices and it worked fine. Actually, I didn't change the original widget code apart from small adjustments to the time calculation. The other bugs (and a few more) should be fixed now.

[mathisdt]

Thanks for your new push! The widget works for me now, I don't know why it didn't in the first place. 🤷‍♂️

I tested again and still found some issues while testing on API level 19 (probably these issues are also present on other API levels):

• "Flex" events seem to be lost, I can't find a possibility to enter a flex event anymore. Or is there an equivalent concept now which I didn't find yet? I would really like to keep @dirkmb's work in the app as he (and probably other users) rely on it.
• The option "Show navigation buttons" doesn't show any effect until the app is force-closed and reopened.
• The option "Notification enabled" doesn't do anything, even after force-closing the app and clocking in afterwards. Normally the persistent notification should be shown all the time while (a) it's enabled and (b) the user is clocked in.

Did you test the location-based and WiFi-based tracking features? I didn't test it yet because I currently don't have a spare device.

Finally, I have a minor suggestion: Could all of the app's main menu be moved to the nice sidebar? I'm not sure whether there's a clear distinction between the items which are still in the app menu and the items you already moved to the sidebar.

[dliw]

The "show navigation button" setting is applied immediately now.

As for the other issues:

• Flex events as such do not exist any more. Instead there are per-day targets now, which should cover the use case of the flex events while being a more general approach. Per-day targets can be set by clicking on the date in the table. I'm aware that is is in no way obvious and probably there is a much better way to integrate this.
• I'm unable to reproduce an issue with the notification. Usually I test on two devices (API level 23 and 28). Now I also tried with an older device (API 16). If enabled, on all three devices a notification is show immediately, as soon as a clock-in happens. Also the notification preference is applied as soon as I leave the options activity.
• When I started using Track Work Time, I couldn't get location-based or WiFi-based tracking to work and didn't try later. So, no, I did not test. However, the code is almost unchanged in this regard. As long as I didn't make silly mistakes with the few little adjustments, everything should work as before.
• I thought of the main menu as an options menu. I considered the sidebar items to be "new activities" and the other ones to be options of the main activity. But at second glance the distinction really is not that clear. Maybe the only real option is "Here = Work". If you prefer, I will move all the items into the sidebar.

[mathisdt]

@dliw Sorry it took me so long to get back to you. Now I will test-drive this PR's code on my phone and merge it if all works as expected 😃.

Some notes:

1. Regarding the options menu vs. sidebar: I think the user could be unsettled when some ex-menu options are now in the sidebar and some remain in the menu. If you have the time, I'd really like all options in the sidebar. (It should not prevent me from merging if you only have the time later and not now.)

2. I don't completely understand the "Target" line in the main table. It seems that this is a monthly sum, while the whole table above it is week-oriented. If this is the case, there should (not "must") be an option to disable the "Target" line - for those who don't care about monthly sums.

3. If I tap on a date and make it a vacation day, it's surprisingly hard to revert that action. There is no type "normal working day" which sets the day target to the amount calculated from the weekly target. But as a workaround, I still can manually set the target to the amount that the calculation from the weekly target would produce.

[dliw]

As for the notes:

1. I will move all the options into the sidebar.
2. The target line is part of a feature to allow more flexible work time targets. However, it was / is a no more than a draft and shouldn't be in this pull request in the first place. I'll remove this line and the corresponding code.
3. Thanks for noticing. Well, if you know it, you can delete a target by long-pressing "Cancel". What would you prefer: An additional "normal day" type or an action item to delete the existing day target?

The fixes shouldn't be too much effort, so I think I will be able to push a new revision within the next few days.

[mathisdt]

Thanks for your work! I really appreciate your commitment. 👍

As to your question: Both options are fine by me - but (only) the long-press action on the "Cancel" button is a bit too hidden in my opinion.

[dliw]

With the latest revision

• all items are in the sidebar except the debug ones. This doesn't matter for the end user.
• no more target line in the table.
• action button to delete existing targets.

Once again I would like to remind you, that I changed the app version to 1.9 and that the upgrade text is more or less a placeholder. Feel free to change this as you see fit.

Due to the reworked time calculation, probably these bugs are fixed as well:
#7, #39, #91, #95, #119

[mathisdt]

Thanks again! 👍
I updated my phone with your PR's code and will test it.

[mathisdt]

Hi @dliw, I wanted to ask if the submodule you added could be incorporated into this project or alternatively turned into a dependency (ideally loaded from Maven Central, but e.g. Github Packages is also fine).
I ask this question because firstly, I don't want to make it complicated for new contributors to get this project running (and so abstained from using Project Lombok and the like which would require IDE plugins) and secondly, I'm about to change the build process from Travis-CI to Github Actions and using submodules could complicate things along the way.

[mathisdt]

I'm very content with your PR, but I'm sorry, I still found some problems. I was testing on Kitkat (API 19) but I don't think the API level had any impact on the findings.

1. Editing the settings and trying to enable e.g. "automatic pause" without setting any values in that section previously (app data was cleared before)

java.lang.RuntimeException: Unable to resume activity {org.zephyrsoft.trackworktime/org.zephyrsoft.trackworktime.MessageActivity}: java.lang.NullPointerException
	at android.app.ActivityThread.performResumeActivity(ActivityThread.java:2788)
	at android.app.ActivityThread.handleResumeActivity(ActivityThread.java:2817)
	...
Caused by: java.lang.NullPointerException
	at org.zephyrsoft.trackworktime.MessageActivity.onResume(MessageActivity.java:56)
	at android.app.Instrumentation.callActivityOnResume(Instrumentation.java:1192)
	at android.app.Activity.performResume(Activity.java:5310)
	at android.app.ActivityThread.performResumeActivity(ActivityThread.java:2778)
	... 12 more

---

2. Backup/restore functionality. I cleared the app's data, configured it again, added some events in the current week, created a backup, but when I wanted to restore backup.events.csv and backup.targets.csv, an exception occurred:

java.lang.RuntimeException: An error occured while executing doInBackground()
	at android.os.AsyncTask$3.done(AsyncTask.java:300)
	at java.util.concurrent.FutureTask.finishCompletion(FutureTask.java:355)
	...
Caused by: java.lang.IllegalArgumentException: unknown value
	at org.zephyrsoft.trackworktime.model.TargetEnum.byName(TargetEnum.java:110)
	at org.zephyrsoft.trackworktime.database.DAO.restoreTargetsFromReader(DAO.java:914)
	at org.zephyrsoft.trackworktime.WorkTimeTrackerActivity$3.doInBackground(WorkTimeTrackerActivity.java:893)
	at org.zephyrsoft.trackworktime.WorkTimeTrackerActivity$3.doInBackground(WorkTimeTrackerActivity.java:870)
	at android.os.AsyncTask$2.call(AsyncTask.java:288)
	at java.util.concurrent.FutureTask.run(FutureTask.java:237)
	... 4 more

[dliw]

I wanted to ask if the submodule you added could be incorporated into this project

Yes, of course. Feel free to copy / clone the submodule into your project. The reason for the separate submodule is that it allows better tracking upstream. As far as I know, the only change is compatibility with AndroidX.

The crashes should be fixed in the latest revision. I wonder where these come from. I'm rather sure that I have tested all of this. Probably they have crept in while adapting to upstream. Anyway, two stupid mistakes less.

[mathisdt]

I feel a bit like Columbo who also often comes back with "one last thing" - I was a bit puzzled by the fact that events I manually enter for the future are not displayed at all in the overview table. Also, events I clock via the buttons are not displayed until the next minute (when they are in the past). Is this intentional? I think most of the users will expect that their actions will reflect in the table immediately. I can live with future events not taken into account for the table (although I don't find it optimal), but events I enter with the buttons should be displayed immediately in the table.

Apart from that, I'm very happy - especially with the speed your modifications bring to the app!

[dliw]

The reason for this had been, that only events used by the time calculator were shown. I just changed this. Future clock-in events are now displayed (first after now if currently clocked-out, first of the day). This could be extended to also include clock-out events, but then it would be necessary to find related events. I don't think this complexity would be justified. In order to display all events in a meaningful way, they would have to be displayed in a timeline...

Apart from that, I noticed that the backup task often was called several hundred times in a short time span. It seems like this is a bug in the work manager for certain API levels. Anyway, I have made changes and will monitor if that has helped.

[mathisdt]

I tried to clock some time in the future - one period later today and one period tomorrow. This is what I got:

I think it would be better to display all future events. The way it works at the moment the user may come to the conclusion that no end event was entered - although it is there. The old version also displays future events. One possible use case is that a user worked "too much" on a day and wants to postpone the worked time, so he may manually enter events for the following day. In this case, the user should see the right sum in the table, even for future events/days.

[dliw]

The way the work time calculation is right now, it gives negative values for future events, because it calculates relative to the current time. If I understand correctly, you would like to have the full work time calculation for future days? If so, this means for me to go back to the drawing board. This will definitely take some time.

Edit: Thinking about it, I'm rather sure, the calculation in principle works the same way as it did in the original. Work time was calculated until CLOCK_OUT_NOW. I'm not sure, how this would work, if there are future clock-in and clock-out events to be considered....
And if it's just a question of display: which events should be displayed? There can be any number of events per day.

Edit2: I'm talking about the original version I started from almost 2 years ago. If something has changed in the meantime, then of course I didn't notice.

[mathisdt]

This is how the old version behaves:

The screenshot was taken with these events:

I'm reluctant to remove features some users may rely on, so I'd ask you to go the extra mile and make the PR reflect the legacy behaviour for the users.

Edit: Please note that the CLOCK_OUT_NOW event is only inserted if there is no CLOCK_OUT event present. Specifically in the case depicted here there is no CLOCK_OUT_NOW inserted but the time is calculated until the "real" CLOCK_OUT event (which is in the future).

[dliw]

I have found a quick way to achieve the desired behaviour after all. The only downside (currently) is, that calculation results from future are not cached. So the calculation takes longer the farther one gets into the future.

[mathisdt]

It's great, exactly what I meant. And I don't think the caching/speed is essential for events in the future, presumably the users won't use that excessively.

Thank you again for your contribution and for your endurance, this made TWT definitely much better! 👍

[WildOrangutan]

Can I just ask why timezone selecting is needed? Why is it not ok to use timezone from system?

[dliw]

In short, this is because the system's time zone changes when you move between time zones. Perhaps the following explanation will make it clearer: #146 (comment)

But if that still doesn't make sense to you, feel free to ask 😉

[WildOrangutan]

Ah okay, I understand it now. Not sure what confused me. Thanks.