orgzly-android icon indicating copy to clipboard operation
orgzly-android copied to clipboard

Add Clocking / Time capture implementation

Open aancel opened this issue 5 years ago • 43 comments

Hello,

This PR should fix #15. The basic functionalities should be covered. What is implemented:

  • [x] Quickbar (Book and Query) buttons to clock-{in,out,cancel}
  • [x] Clocking logic handling in OrgFormatter
    • [x] Allow to clock-in, only if a previous clocked-in entry is not already present
    • [x] Allow to clock-out, only if a previous clocked-in entry is present, and compute the spent time
    • [x] Allow to cancel the latest clock

What remains to be fixed:

  • [x] Buttons for clock-{in,out,cancel} are currently the same alarm icon
    • Maybe having a clock icon with an arrow going in for clock-in, a clock icon with with an arrow going out for clock-out and a button with the strikethrough clock for org-cancel ? => using the hourglass_{top, bottom, disabled} material icons
  • [ ] The regexes in OrgFormatter might be moved to OrgPatterns
  • [ ] Kotlin code needs some improvement, as I do not have much experience in it
  • [ ] Maybe adding buttons in the note edition UI to clock-{in,out,cancel} ?

Thanks for your comments and suggestions for improvements !

aancel avatar Apr 21 '20 10:04 aancel

@aancel is there an easy way for me to try your clocking functionality? I have never created an Android package myself, so I was wondering if you have package I can try via e.g. F-droid or directly as a .apk.

lckarssen avatar May 25 '20 08:05 lckarssen

Hello @lckarssen, Regarding feature testing, I don't think that it would be a good idea to install a custom debug build apk, if you already have Orgzly properly setup on your phone (It might probably not install or break the update to newer versions).

If you are willing to try via Android Studio, it is quite easy to test the feature via the android emulator.
You would need to:

  • Install Android Studio
  • Import the project via File -> New -> Project from version control -> Git, then type in the url of my fork
  • You would then need to make sure that the correct branch is selected via VCS -> Git
  • Then you can create a Virtual Machine via Tools -> AVD Manager (I usually use Pixel phones for this) or just plug in your phone
  • And then run the app (Should be bound to Shift+F10)

aancel avatar May 25 '20 10:05 aancel

Thank you for the instructions, @aancel. I'll have a look at Android Studio.

lckarssen avatar May 27 '20 10:05 lckarssen

@lckarssen In case you are having too much trouble setting everything up, I can still send you an apk if you would like (I'd just need a way to send it to you)

aancel avatar May 27 '20 11:05 aancel

@lckarssen you can use APK with @aancel's clocking feature created by the Workflow from #721

debanjum avatar May 27 '20 18:05 debanjum

@aancel Just in case you missed it, @nevenz added some comments on your PR.

allentiak avatar Sep 28 '20 00:09 allentiak

Sorry, life got a bit badly on the way those last months.
I should be able to start working on this feature again soon (in the coming days/weeks).
I still really want this feature too in Orgzly !

aancel avatar Oct 21 '20 19:10 aancel

@aancel Cool! Looking forward to see those changes!

allentiak avatar Oct 21 '20 22:10 allentiak

@nevenz I've noticed an unexpected behavior in the drawerPattern function in OrgFormatter:

    private fun drawerPattern(name: String) = Pattern.compile(
            """^[ \t]*:($name):[ \t]*\n(.*?)\n[ \t]*:END:[ \t]*$""",
            Pattern.CASE_INSENSITIVE or Pattern.MULTILINE or Pattern.DOTALL) 

If I do a clock-in, then clock-cancel, then clock-in, I end up having 2 LOGBOOK drawers:

:LOGBOOK:
CLOCK: [2020-10-25 Sun 17:43]
:END:

:LOGBOOK:
:END:

If I update the drawerPattern function to add a * after the second \n, I no longer have this behavior:

    private fun drawerPattern(name: String) = Pattern.compile(
            """^[ \t]*:($name):[ \t]*\n(.*?)\n*[ \t]*:END:[ \t]*$""",
            Pattern.CASE_INSENSITIVE or Pattern.MULTILINE or Pattern.DOTALL)

As this modification could have an impact elsewhere, I'd rather ask you whether I should push it or not

aancel avatar Oct 25 '20 16:10 aancel

If I do a clock-in, then clock-cancel

Org mode removes the drawer when it ends up being empty after this.

If I update the drawerPattern function to add a * after the second \n, I no longer have this behavior:

I think that would match:

:LOGBACK:
foo:END:

nevenz avatar Dec 24 '20 15:12 nevenz

When this branch is merged with master, the build fails with the following error:

Error while processing ..../orgzly-android/app/src/main/res/drawable/ic_hourglass_top.xml : Invalid color value ?attr/text_primary_color

Reverting this specific commit 7c6b75f6 - Update Gradle to v6.5 and plugin to v4.1.1 makes it work again. No idea why is this happening.

lytex avatar Dec 25 '20 16:12 lytex

This seems to be needed, due to the use of resources in vector drawables:

diff --git a/app/build.gradle b/app/build.gradle
index b4ce864b..ee8be29c 100644
--- a/app/build.gradle
+++ b/app/build.gradle
@@ -37,6 +37,8 @@ android {
                 ]
             }
         }
+
+        vectorDrawables.useSupportLibrary = true
     }
 
     buildFeatures {

nevenz avatar Dec 26 '20 12:12 nevenz

@nevenz I've updated the code following your latest reviews.

  • I've added the change you requested in the build.gradle file
  • I've also updated the clock cancel code to delete the drawer when it ends up being empty after a cancel (not so sure about the way I implemented it)

aancel avatar Dec 29 '20 21:12 aancel

Could we resolve or iterate on the discussions that have been addressed so I can see what remains to be done in this PR, please ? :thinking:

I think that there is at least this comment from @nevenz https://github.com/orgzly/orgzly-android/pull/691#pullrequestreview-433394906 that asks for several improvements.
TBH I think that I could handle all the UI related stuff, but it would probably take me some time to handle the part with the clock_range_id in the notes table, as I haven't touched any db code yet.
So how should we handle this ?

aancel avatar Dec 29 '20 22:12 aancel

I think that I could handle all the UI related stuff, but it would probably take me some time to handle the part with the clock_range_id in the notes table, as I haven't touched any db code yet.

I can do that part. I think it's important to be able to quickly jump to currently clocked note right from the start, which does require keeping DB in sync with CLOCK entries.

nevenz avatar Dec 31 '20 16:12 nevenz

Just curious... Any news regarding this?

allentiak avatar Jan 22 '21 19:01 allentiak

I cannot get this build to work if I don't include this changes, I think they are missing:

diff --git a/app/src/main/res/layout/list_widget.xml b/app/src/main/res/layout/list_widget.xml
index f60425f6..4b58bb7c 100644
--- a/app/src/main/res/layout/list_widget.xml
+++ b/app/src/main/res/layout/list_widget.xml
@@ -1,10 +1,10 @@
 <?xml version="1.0" encoding="utf-8"?>
 
-<LinearLayout
-    xmlns:android="http://schemas.android.com/apk/res/android"
+<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
     xmlns:tools="http://schemas.android.com/tools"
     android:layout_width="match_parent"
     android:layout_height="match_parent"
+    xmlns:app="http://schemas.android.com/apk/res-auto"
     android:orientation="vertical">
 
     <LinearLayout
@@ -22,7 +22,7 @@
             android:layout_width="48dp"
             android:layout_height="match_parent"
             android:layout_gravity="center_vertical"
-            android:src="@drawable/ic_logo_for_widget" />
+            app:srcCompat="@drawable/ic_logo_for_widget" />
 
         <LinearLayout
             android:id="@+id/list_widget_header_bar"

lytex avatar Feb 12 '21 22:02 lytex

One suggestion is to add a persistent notification when there is an active clock. Emacs have that in the mode-line to remind you. On Android it would be good to have a persistent notification to know there is an active clock going on. And that could have a button to quickly clock out.

xiaoruoruo avatar Feb 22 '21 06:02 xiaoruoruo

One suggestion is to add a persistent notification when there is an active clock.

Sure. But that can be requested separately. Otherwise, we may not live long enough to see this feature coming out.

alensiljak avatar Feb 27 '21 22:02 alensiljak

@aancel @nevenz Specifically... What do you think would be needed to move this conversation forward, so this PR could eventually be merged? Whereas I don't know much Kotlin, I might be able to help...

allentiak avatar Mar 11 '21 01:03 allentiak

Hello, Regarding this PR, I think that @debanjum still has a discussion opened, could you close it if the changes are ok for you ? Regarding your comment @lytex, I've just checked and it still seems to be building on my side, with the latest android-studio, are you up to date or maybe using a different setup that could cause the update you mention to be required ? 🤔 I think that the suggestion from @xiaoruoruo to have a persistent notification is a very good idea, maybe this could be a good idea to open an issue to keep this suggestion in the backlog ? And do this as a second step ! Please @nevenz do tell me, if I have to apply some additional modifications, I'll try my best to take some time for them !

aancel avatar Mar 29 '21 20:03 aancel

Hello, Regarding this PR, I think that @debanjum still has a discussion opened, could you close it if the changes are ok for you ?

Not sure how to close the discussion but that discussion shouldn't block this PR. And the changes look good to me! 😃

debanjum avatar Mar 29 '21 21:03 debanjum

I've resolved the discussion, it was indeed on an old diff so not blocking, sorry for the ping 😅

aancel avatar Mar 29 '21 21:03 aancel

Are there any remaining blockers on merging this feature? I'm very eager to see it released, though I'll likely build the APK for myself this week to start using it early.

sableseyler avatar Jun 26 '21 03:06 sableseyler

Can this feature be used for doing something like this https://github.com/orgzly/orgzly-android/issues/869 ?

michaelsjackson avatar Jul 15 '21 08:07 michaelsjackson

@nevenz Any news regarding this? https://github.com/orgzly/orgzly-android/pull/691#issuecomment-796368978

allentiak avatar Jul 27 '21 19:07 allentiak

I would really like to see this officially supported.

Since the CI artifacts can no longer be downloaded, I've merged this into master on my fork (see commit history here) and the apk generated from this action can be found on this page (different PR, but shas are identical). You can directly downloaded the generated apks here.

(heads up: took me a minute to realize that the quickbar is activated from swiping right, or left-right, on an item)

stites avatar Aug 18 '21 14:08 stites

@nevenz Do you think this PR could be merged as-is? BTW: Is there anyone else with the right permissions to do this?

@stites I would like to try your build. Which artifact should be installed? There are two: "fdroid" and "premium"...

allentiak avatar Sep 19 '21 19:09 allentiak

@allentiak premium is the paid version of this app which gives you access to the dropbox backend. Paying for the premium app through the app store is the only way to donate to this project (you can also try to ping @nevenz in https://github.com/orgzly/orgzly-android/issues/710 to change this). the fdroid version doesn't include the dropbox backend.

stites avatar Oct 01 '21 01:10 stites

Following up on @allentiak 's question, @nevenz it seems that everything is there, could this be merged? I tested the changes and everything seems to work fine.

schlagenhauf avatar Nov 16 '21 21:11 schlagenhauf