Morgan Smith <morgan.j.sm...@outlook.com> writes: > I may have rewritten org-clock-sum yet again. See attached patch. > > * things I want you to tell me > 1. Does this look like something that could be eventually merged upstream or > am > I wasting my time?
Yes, it could be merged upstream. I do not see why not. > 2. Would you like me to do more performance testing? I basically only tested > my use case. If yes, should I create some test files for benchmarking that > can be shared? Your patch clearly provides more caching ability, so I anticipate an improvement. I will still need to test is on my side though to be sure. Having benchmarks would be nice, but optional. > 3. Do you want `org-element-cache-map' fixed before we merge this patch? If > yes, please be willing to wait. I have already spent probably about 8 > hours > looking into it and it still makes my head hurt. A fix would be nice, but it should not be a blocker for your patch. If necessary, we can discuss that function by screen sharing. > * todo > The patch is like 95% done. I still gotta > > 1. Write a decent docstring for `org-clock-ranges'. Maybe add a news entry > for > it too. Or make it internal. Then, no news entry will be required. I am not 100% sure if the return value is useful for generic use outside org-clock-sum. > 2. Check `org-clock-hd-marker' for open clock. You can simply compare it with org-element-begin for current headline. > 3. Figure out what to do about open clocks that aren't the current > one. Historically we ignored them so I guess I should just do that. Yes. Ideally, also document this in the docstring. > 4. Maybe test clocking in inlinetasks. I honestly don't even know what these > are. ********************************** TODO inline ********************************** END They can appear in parallel with paragraphs. Inlinetasks are an optional markup feature that is enabled by (require 'org-inlinetask) > * Downsides of my rewrite > > 1. Does it still perform better with the cache disabled? idk. Probably not. That should not be a problem. We are slowly moving Org code to use cache API everywhere. > 2. Radical change. Likely has bugs Then, it would be nice to add some test coverage. > 3. Dances around bugs in `org-element-cache-map' but does it actually dance > around all of them? It would be nice if you help with this by fixing known bugs and writing more tests, but it is generally not a concern _you_ need to worry about - I will (sooner or later) fix bugs in `org-element-cache-map' if they arise. -- Ihor Radchenko // yantar92, Org mode contributor, Learn more about Org mode at <https://orgmode.org/>. Support Org development at <https://liberapay.com/org-mode>, or support my work at <https://liberapay.com/yantar92>
