From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dan Drake Subject: Re: [RFC PATCH] specify a time, not number of minutes to keep, with org-resolve-clock Date: Tue, 28 Jan 2020 20:11:42 -0600 Message-ID: References: <87sgk4o2j2.fsf@nicolasgoaziou.fr> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="00000000000045b9be059d3dddcc" Return-path: Received: from eggs.gnu.org ([2001:470:142:3::10]:44150) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1iwcph-0005YQ-Pc for emacs-orgmode@gnu.org; Tue, 28 Jan 2020 21:11:59 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1iwcpf-0000sK-Lu for emacs-orgmode@gnu.org; Tue, 28 Jan 2020 21:11:57 -0500 Received: from mail-il1-x144.google.com ([2607:f8b0:4864:20::144]:41395) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1iwcpf-0000s9-Dm for emacs-orgmode@gnu.org; Tue, 28 Jan 2020 21:11:55 -0500 Received: by mail-il1-x144.google.com with SMTP id f10so12561212ils.8 for ; Tue, 28 Jan 2020 18:11:55 -0800 (PST) In-Reply-To: <87sgk4o2j2.fsf@nicolasgoaziou.fr> List-Id: "General discussions about Org-mode." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-orgmode-bounces+geo-emacs-orgmode=m.gmane-mx.org@gnu.org Sender: "Emacs-orgmode" To: Dan Drake , emacs-orgmode@gnu.org --00000000000045b9be059d3dddcc Content-Type: multipart/alternative; boundary="00000000000045b9bb059d3dddca" --00000000000045b9bb059d3dddca Content-Type: text/plain; charset="UTF-8" Thank you, Nicolas, for the review of my first patch. I've updated my code and have the new patch attached. I didn't inline the "time to minutes to keep function"; yes, that function is very short, but the function name makes the intent/purpose of the code obvious, and the org-clock-resolve a bit more readable. If it's important to inline that -- performance concerns about the function call overhead, accepting org-mode/emacs coding style, etc. -- I'm willing to do that. I added an entry to ORG-NEWS for version 9.4. I didn't write any tests, as the change seems so simple, and UI-focused. But again, if that's a dealbreaker, I can work on doing that. Further comments welcome. On Fri, Jan 24, 2020 at 5:31 PM Nicolas Goaziou wrote: > Hello, > > Dan Drake writes: > > > I asked about a way to specify a time when using org-resolve-clock > instead > > of a number of minutes: > > > > https://lists.gnu.org/archive/html/emacs-orgmode/2020-01/msg00010.html > > > > I've implemented this myself and a patch is attached. Comments welcome -- > > my change works, but I'm not sure about coding style, and right now > there's > > no error checking. > > Thank you. Some comments follow. > > > I marked the patch as a tiny change, but it does add a new menu option > and > > behavior to org-resolve-clock, so there may be an argument that it's not, > > from a user perspective, a "tiny change", but code-wise it's quite > simple: > > the core logic really isn't more than "ask the user for a time and > > subtract". > > TINYCHANGE is only about the number of non-trivial lines of code in your > patch (15 or so). > > > +(defun time-to-mins-to-keep (start-time) > > + "Asks the user for a time and returns the number of minutes > > +from START-TIME to that time." > > + (floor (/ (float-time > > + (time-subtract (org-read-date t t) start-time)) 60))) > > The name of the function is wrong, it should start with "org-clock". > Also, docstring's first line must contain a full sentence. > > Anyway, this is used only once in your patch, I suggest to inline in > instead. > > + (or (and (memq ch '(?k ?K)) > > + (read-number "Keep how many minutes? " default)) > > + (and (memq ch '(?t ?T)) > > + (time-to-mins-to-keep last-valid)))) > > See above about inlining. > > Would you mind adding an ORG-NEWS entry, and, if possible, writing > a test? > > Regards, > > -- > Nicolas Goaziou > -- Ceci n'est pas une .signature. --00000000000045b9bb059d3dddca Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Thank you, Nicolas, for the review of my first patch.= I've updated my code and have the new patch attached.

I didn't inline the "time to minutes to keep function&quo= t;; yes, that function is very short, but the function name makes the inten= t/purpose of the code obvious, and the org-clock-resolve a bit more readabl= e. If it's important to inline that -- performance concerns about the f= unction call overhead, accepting org-mode/emacs coding style, etc. -- I'= ;m willing to do that.

I added an entry to ORG-NEW= S for version 9.4.

I didn't write any tes= ts, as the change seems so simple, and UI-focused. But again, if that's= a dealbreaker, I can work on doing that.

Further = comments welcome.

On Fri, Jan 24, 2020 at 5:31 PM Nicolas Goazi= ou <mail@nicolasgoaziou.fr= > wrote:
Hell= o,

Dan Drake <dan.= drake@gmail.com> writes:

> I asked about a way to specify a time when using org-resolve-clock ins= tead
> of a number of minutes:
>
> https://lists.gnu.org/arc= hive/html/emacs-orgmode/2020-01/msg00010.html
>
> I've implemented this myself and a patch is attached. Comments wel= come --
> my change works, but I'm not sure about coding style, and right no= w there's
> no error checking.

Thank you. Some comments follow.

> I marked the patch as a tiny change, but it does add a new menu option= and
> behavior to org-resolve-clock, so there may be an argument that it'= ;s not,
> from a user perspective, a "tiny change", but code-wise it&#= 39;s quite simple:
> the core logic really isn't more than "ask the user for a tim= e and
> subtract".

TINYCHANGE is only about the number of non-trivial lines of code in your patch (15 or so).

> +(defun time-to-mins-to-keep (start-time)
> +=C2=A0 "Asks the user for a time and returns the number of minut= es
> +from START-TIME to that time."
> +=C2=A0 (floor (/ (float-time
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (time-subtract (org-read-date t t)= start-time)) 60)))

The name of the function is wrong, it should start with "org-clock&quo= t;.
Also, docstring's first line must contain a full sentence.

Anyway, this is used only once in your patch, I suggest to inline in
instead.
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0(or (and (memq ch '(?k ?K))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (read-number = "Keep how many minutes? " default))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(and (memq ch '(?t ?T))<= br> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (time-to-mins= -to-keep last-valid))))

See above about inlining.

Would you mind adding an ORG-NEWS entry, and, if possible, writing
a test?

Regards,

--
Nicolas Goaziou


--
Ceci n'est pas une .signature.
--00000000000045b9bb059d3dddca-- --00000000000045b9be059d3dddcc Content-Type: text/x-patch; charset="US-ASCII"; name="0001-org-clock.el-add-t-option-to-org-clock-resolve.patch" Content-Disposition: attachment; filename="0001-org-clock.el-add-t-option-to-org-clock-resolve.patch" Content-Transfer-Encoding: base64 Content-ID: X-Attachment-Id: f_k5yobmej0 RnJvbSA1MmQ1M2RlNWY2N2U5YmExNmI2ZWZlOTFhYzc5YjM3MGQ4OWM5ZmVhIE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBEYW4gRHJha2UgPGRhbi5kcmFrZUBnbWFpbC5jb20+CkRhdGU6 IFN1biwgMTkgSmFuIDIwMjAgMDg6MjQ6MTIgLTA2MDAKU3ViamVjdDogW1BBVENIXSBvcmctY2xv Y2suZWw6IGFkZCBgdCcgb3B0aW9uIHRvIG9yZy1jbG9jay1yZXNvbHZlCgoqIG9yZy1jbG9jay5l bCAob3JnLWNsb2NrLXJlc29sdmUpOiBhZGQgYHQnIG9wdGlvbi4gVGhpcyB3b3JrcyBqdXN0IGxp a2UKYGsnLCBidXQgYXNrcyB0aGUgdXNlciB0byBzcGVjaWZ5IGEgdGltZSwgaW5zdGVhZCBvZiBh IG51bWJlciBvZgptaW51dGVzLgoKT2Z0ZW4gd2hlbiB5b3UgYXJlIGludGVycnVwdGVkIGF0IGEg dGFzayBhbmQgZ2V0IGJhY2sgdG8gaXQsIHlvdSBrbm93CndoYXQgdGltZSB0aGUgaW50ZXJydXB0 aW9uIGhhcHBlbmVkLiBUaGlzIG9wdGlvbiBtYWtlcyBpdCBlYXN5IHRvIHRlbGwKb3JnLXJlc29s dmUtY2xvY2tzIGFib3V0IHRoYXQuIEZvciBleGFtcGxlLCBzYXkgeW91IGNsb2NrZWQgaW50byB0 YXNrIEEKYXQsIHNheSwgOTozNzoKCiAgICAqIG9yaWdpbmFsIHRhc2sgQQogICAgICA6TE9HQk9P SzoKICAgICAgQ0xPQ0s6IFsyMDIwLTAxLTIxIE1vbiAwOTozN10KICAgICAgOkVORDoKCldoaWxl IHdvcmtpbmcgb24gdGFzayBBLCB5b3UgZ2V0IGEgcGhvbmUgY2FsbC4gV2hlbiB0aGUgY2FsbCBp cyBkb25lLAp5b3UnZCBsaWtlIHRvIHVwZGF0ZSB5b3VyIHRpbWUgbG9nZ2luZyB0byByZWZsZWN0 IHRoZSBwaG9uZSBjYWxsLiBZb3VyCnBob25lIHNheXMgdGhlIGNhbGwgd2FzIGF0IDExOjA5LgoK V2l0aCBDLWMgQy14IEMteiwgeW91IGNhbiB1c2UgdGhlIGBLJyBvcHRpb24sIGJ1dCB5b3UgbmVl ZCB0byBmaWd1cmUgb3V0CnRoZSBudW1iZXIgb2YgbWludXRlcyB0byBrZWVwLiBJdCdzIGVhc2ll ciB0byBsb29rIGF0IHRoZSBwaG9uZSwgb3IgdG8KbWVudGFsbHkgbm90ZSB0aGUgdGltZSB3aGVu IGFuIGludGVycnVwdGlvbiBzdGFydHMuIFdpdGggdGhlIG5ldyBvcHRpb24sCnlvdSBjYW4gc2Vs ZWN0IGBUJywgYW5kIGp1c3Qgc3BlY2lmeSBhIHRpbWUgb2YgMTE6MDkuIFRoZSBzdGF0ZSBpcyBu b3c6CgogICAgKiBvcmlnaW5hbCB0YXNrIEEKICAgICAgOkxPR0JPT0s6CiAgICAgIENMT0NLOiBb MjAyMC0wMS0yMSBNb24gMDk6MzddLS1bMjAyMC0wMS0yMSBNb24gMTE6MDldID0+IDE6MzIKICAg ICAgOkVORDoKCllvdSBhZGQgdGhlIHBob25lIGNhbGwgdG8geW91ciBvcmcgYnVmZmVyIGFuZCBk byBDLWMgQy14IEMtaSB0byBjbG9jawppbi4gT3JnIGFza3MgeW91IHRvIHN0YXJ0IHRoZSB0aW1l IGZyb20gd2hlbiB0aGUgcHJldmlvdXMgdGFzayBlbmRlZCwKeW91IHNheSB5ZXMsIGFuZCB0aGUg c3RhdGUgaXMgbm93OgoKICAgICogb3JpZ2luYWwgdGFzayBBCiAgICAgIDpMT0dCT09LOgogICAg ICBDTE9DSzogWzIwMjAtMDEtMjEgTW9uIDA5OjM3XS0tWzIwMjAtMDEtMjEgTW9uIDExOjA5XSA9 PiAxOjMyCiAgICAgIDpFTkQ6CiAgICAqIHRhc2sgQiwgcGhvbmUgY2FsbAogICAgICA6TE9HQk9P SzoKICAgICAgQ0xPQ0s6IFsyMDIwLTAxLTIxIE1vbiAxMTowOV0KICAgICAgOkVORDoKCkF0IHRo aXMgcG9pbnQsIHlvdSBjYW4gY2xvY2sgYmFjayBpbnRvIHRhc2sgQSwgb3IgYW55IG90aGVyIHRh c2suCgpUaGUga2V5IGZlYXR1cmUgaGVyZSBpcyB0byBiZSBhYmxlIHRvIGp1c3QgdHlwZSBpbiBh IHRpbWUgLS0gaW4gYW55CmZvcm1hdCBhY2NlcHRlZCBieSBvcmctcmVhZC1kYXRlIC0tIGluc3Rl YWQgb2Ygc3BlY2lmeWluZyBhIG51bWJlciBvZgptaW51dGVzLgoKVElOWUNIQU5HRQotLS0KIGV0 Yy9PUkctTkVXUyAgICAgIHwgIDUgKysrKysKIGxpc3Avb3JnLWNsb2NrLmVsIHwgMjIgKysrKysr KysrKysrKysrKy0tLS0tLQogMiBmaWxlcyBjaGFuZ2VkLCAyMSBpbnNlcnRpb25zKCspLCA2IGRl bGV0aW9ucygtKQoKZGlmZiAtLWdpdCBhL2V0Yy9PUkctTkVXUyBiL2V0Yy9PUkctTkVXUwppbmRl eCA2N2MzY2EyZWQuLjAwYzFmNDUzYSAxMDA2NDQKLS0tIGEvZXRjL09SRy1ORVdTCisrKyBiL2V0 Yy9PUkctTkVXUwpAQCAtMzUsNiArMzUsMTEgQEAgdmFsdWUgaW4gY2FsbCB0byA9amF2YT0uCiBB ZnRlciBlZGl0aW5nIGEgc291cmNlIGJsb2NrLCBPcmcgd2lsbCByZXN0b3JlIHRoZSB3aW5kb3cg bGF5b3V0IHdoZW4KIH5vcmctc3JjLXdpbmRvdy1zZXR1cH4gaXMgc2V0IHRvIGEgdmFsdWUgdGhh dCBtb2RpZmllcyB0aGUgbGF5b3V0LgogCisqKiogTmV3IG9wdGlvbiB0byByZXNvbHZlIG9wZW4g Y2xvY2sgYXQgYSBwcm92aWRlZCB0aW1lCit+b3JnLXJlc29sdmUtY2xvY2tzfiBub3cgaGFzIGEg YHQnIG9wdGlvbiwgd2hpY2ggd29ya3MganVzdCBsaWtlIHRoZQorYGsnIG9wdGlvbiwgYnV0IHRo ZSB1c2VyIHNwZWNpZmllcyBhIHRpbWUgb2YgZGF5LCBub3QgYSBudW1iZXIgb2YKK21pbnV0ZXMu CisKICoqIE5ldyBmdW5jdGlvbnMKICoqKiB+b3JnLWNvbHVtbnMtdG9nZ2xlLW9yLWNvbHVtbnMt cXVpdH4KID08Qy1jIEMtYz49IGJvdW5kIHRvIH5vcmctY29sdW1ucy10b2dnbGUtb3ItY29sdW1u cy1xdWl0fiByZXBsYWNlcyB0aGUKZGlmZiAtLWdpdCBhL2xpc3Avb3JnLWNsb2NrLmVsIGIvbGlz cC9vcmctY2xvY2suZWwKaW5kZXggMDZkZjJkNDk3Li4wNGUxNzY5NmIgMTAwNjQ0Ci0tLSBhL2xp c3Avb3JnLWNsb2NrLmVsCisrKyBiL2xpc3Avb3JnLWNsb2NrLmVsCkBAIC05ODYsNiArOTg2LDEx IEBAIENMT0NLIGlzIGEgY29ucyBjZWxsIG9mIHRoZSBmb3JtIChNQVJLRVIgU1RBUlQtVElNRSku IgogCQkgICAgIChvcmctZmxhZy1kcmF3ZXIgbmlsIGVsZW1lbnQpKQogCQkgICAodGhyb3cgJ2V4 aXQgbmlsKSkpKSkpKSkpKSkKIAorKGRlZnVuIG9yZy1jbG9jay10aW1lLXRvLW1pbnMtdG8ta2Vl cCAoc3RhcnQtdGltZSkKKyAgIkFzayB0aGUgdXNlciBmb3IgYSB0aW1lIGFuZCByZXR1cm4gdGhl IG51bWJlciBvZiBtaW51dGVzIGZyb20gU1RBUlQtVElNRSB0byB0aGF0IHRpbWUuIgorICAoZmxv b3IgKC8gKGZsb2F0LXRpbWUKKwkgICAgICh0aW1lLXN1YnRyYWN0IChvcmctcmVhZC1kYXRlIHQg dCkgc3RhcnQtdGltZSkpIDYwKSkpCisKIChkZWZ1biBvcmctY2xvY2stcmVzb2x2ZSAoY2xvY2sg Jm9wdGlvbmFsIHByb21wdC1mbiBsYXN0LXZhbGlkIGZhaWwtcXVpZXRseSkKICAgIlJlc29sdmUg YW4gb3BlbiBPcmcgY2xvY2suCiBBbiBvcGVuIGNsb2NrIHdhcyBmb3VuZCwgd2l0aCBgZGFuZ2xp bmcnIHBvc3NpYmx5IGJlaW5nIG5vbi1uaWwuCkBAIC0xMDIyLDYgKzEwMjcsOSBAQCBrL0sgICAg ICBLZWVwIFggbWludXRlcyBvZiB0aGUgaWRsZSB0aW1lIChkZWZhdWx0IGlzIGFsbCkuICBJZiB0 aGlzCiAgICAgICAgICB0aGF0IG1hbnkgbWludXRlcyBhZnRlciB0aGUgdGltZSB0aGF0IGlkbGlu ZyBiZWdhbiwgYW5kIHRoZW4KICAgICAgICAgIGNsb2NrZWQgYmFjayBpbiBhdCB0aGUgcHJlc2Vu dCB0aW1lLgogCit0L1QgICAgICBMaWtlIGBrJywgYnV0IHdpbGwgYXNrIHlvdSB0byBzcGVjaWZ5 IGEgdGltZSwgaW5zdGVhZCBvZiBhCisgICAgICAgICBudW1iZXIgb2YgbWludXRlcy4KKwogZy9H ICAgICAgSW5kaWNhdGUgdGhhdCB5b3UgXCJnb3QgYmFja1wiIFggbWludXRlcyBhZ28uICBUaGlz IGlzIHF1aXRlCiAgICAgICAgICBkaWZmZXJlbnQgZnJvbSBgayc6IGl0IGNsb2NrcyB5b3Ugb3V0 IGZyb20gdGhlIGJlZ2lubmluZyBvZgogICAgICAgICAgdGhlIGlkbGUgcGVyaW9kIGFuZCBjbG9j ayB5b3UgYmFjayBpbiBYIG1pbnV0ZXMgYWdvLgpAQCAtMTA0MSwxOSArMTA0OSwyMSBAQCB0byBi ZSBDTE9DS0VEIE9VVC4iKSkpKQogCQkod2hpbGUgKG9yIChudWxsIGNoYXItcHJlc3NlZCkKIAkJ CSAgIChhbmQgKG5vdCAobWVtcSBjaGFyLXByZXNzZWQKIAkJCQkJICAgJyg/ayA/SyA/ZyA/RyA/ cyA/UyA/QwotCQkJCQkJP2ogP0ogP2kgP3EpKSkKKwkJCQkJCT9qID9KID9pID9xID90ID9UKSkp CiAJCQkJKG9yIChkaW5nKSB0KSkpCiAJCSAgKHNldHEgY2hhci1wcmVzc2VkCiAJCQkocmVhZC1j aGFyIChjb25jYXQgKGZ1bmNhbGwgcHJvbXB0LWZuIGNsb2NrKQotCQkJCQkgICAiIFtqa0tnR1Nz Y0NpcV0/ICIpCisJCQkJCSAgICIgW2prS3RUZ0dTc2NDaXFdPyAiKQogCQkJCSAgIG5pbCA0NSkp KQogCQkoYW5kIChub3QgKG1lbXEgY2hhci1wcmVzc2VkICcoP2kgP3EpKSkgY2hhci1wcmVzc2Vk KSkpKSkKIAkgKGRlZmF1bHQKIAkgICAoZmxvb3IgKG9yZy10aW1lLWNvbnZlcnQtdG8taW50ZWdl ciAob3JnLXRpbWUtc2luY2UgbGFzdC12YWxpZCkpCiAJCSAgNjApKQogCSAoa2VlcAotCSAgKGFu ZCAobWVtcSBjaCAnKD9rID9LKSkKLQkgICAgICAgKHJlYWQtbnVtYmVyICJLZWVwIGhvdyBtYW55 IG1pbnV0ZXM/ICIgZGVmYXVsdCkpKQorCSAgKG9yIChhbmQgKG1lbXEgY2ggJyg/ayA/SykpCisJ CSAgIChyZWFkLW51bWJlciAiS2VlcCBob3cgbWFueSBtaW51dGVzPyAiIGRlZmF1bHQpKQorCSAg ICAgIChhbmQgKG1lbXEgY2ggJyg/dCA/VCkpCisJCSAgIChvcmctY2xvY2stdGltZS10by1taW5z LXRvLWtlZXAgbGFzdC12YWxpZCkpKSkKIAkgKGdvdGJhY2sKIAkgIChhbmQgKG1lbXEgY2ggJyg/ ZyA/RykpCiAJICAgICAgIChyZWFkLW51bWJlciAiR290IGJhY2sgaG93IG1hbnkgbWludXRlcyBh Z28/ICIgZGVmYXVsdCkpKQpAQCAtMTA2OCw3ICsxMDc4LDcgQEAgdG8gYmUgQ0xPQ0tFRCBPVVQu IikpKSkKIAkgIChvcmctY2xvY2stcmVzb2x2ZS1jbG9jayBjbG9jayAnbm93IG5pbCB0IG5pbCBm YWlsLXF1aWV0bHkpKQogICAgICAgKG9yZy1jbG9jay1qdW1wLXRvLWN1cnJlbnQtY2xvY2sgY2xv Y2spKQogICAgICAoKG9yIChudWxsIGNoKQotCSAgKG5vdCAobWVtcSBjaCAnKD9rID9LID9nID9H ID9zID9TID9DKSkpKQorCSAgKG5vdCAobWVtcSBjaCAnKD9rID9LID9nID9HID9zID9TID9DID90 ID9UKSkpKQogICAgICAgKG1lc3NhZ2UgIiIpKQogICAgICAodAogICAgICAgKG9yZy1jbG9jay1y ZXNvbHZlLWNsb2NrCkBAIC0xMDkyLDcgKzExMDIsNyBAQCB0byBiZSBDTE9DS0VEIE9VVC4iKSkp KQogCSAgICAgICh0CiAJICAgICAgIChlcnJvciAiVW5leHBlY3RlZCwgcGxlYXNlIHJlcG9ydCB0 aGlzIGFzIGEgYnVnIikpKQogICAgICAgIChhbmQgZ290YmFjayBsYXN0LXZhbGlkKQotICAgICAg IChtZW1xIGNoICcoP0sgP0cgP1MpKQorICAgICAgIChtZW1xIGNoICcoP0sgP0cgP1MgP1QpKQog ICAgICAgIChhbmQgc3RhcnQtb3ZlcgogCSAgICAobm90IChtZW1xIGNoICcoP0sgP0cgP1MgP0Mp KSkpCiAgICAgICAgZmFpbC1xdWlldGx5KSkpKSkKLS0gCjIuMTcuMQoK --00000000000045b9be059d3dddcc--