ichernyshovvv / org-timeblock

Schedule your day visually, using timeblocking technique inside Emacs
GNU General Public License v3.0
275 stars 18 forks source link

Design suggestions #12

Closed progfolio closed 1 year ago

progfolio commented 1 year ago

Avoid this pattern:

(cl-macrolet ((on (accessor op lhs rhs)
                `(,op (,accessor ,lhs)
                      (,accessor ,rhs))))

    ;;wrapping defuns
)

It makes debugging the functions which use on much harder than necessary. It would be better to properly define the macro using defmacro and use it. Even better would be to rewrite on as a function so a macro is not necessary at all.

The macro also raises the following byte-compiler warnings:

org-timeblock.el  336  17 warning  e-f-b-c  Use of deprecated ((lambda (item) ...) ...) form
org-timeblock.el  341  17 warning  e-f-b-c  Use of deprecated ((lambda (item) ...) ...) form

Can this regexp be replaced with org-scheduled-regexp?

    (when (re-search-forward "^SCHEDULED:.+>\n" (1+ (line-end-position)) t)

Likewise, could these be replaced by org-tsr-regexp?:

(defconst org-ts-regexp0
  "\\(\\([0-9]\\{4\\}\\)-\\([0-9]\\{2\\}\\)-\\([0-9]\\{2\\}\\)\\( +[^]+0-9>\r\n -]+\\)?\\( +\\([0-9]\\{1,2\\}\\):\\([0-9]\\{2\\}\\)\\)?\\)"
  "Regular expression matching time strings for analysis.
This one does not require the space after the date, so it can be used
on a string that terminates immediately after the date.")

(defconst org-ts-regexp1 "\\(\\([0-9]\\{4\\}\\)-\\([0-9]\\{2\\}\\)-\\([0-9]\\{2\\}\\)\\(?: *\\([^]+0-9>\r\n -]+\\)\\)?\\( \\([0-9]\\{1,2\\}\\):\\([0-9]\\{2\\}\\)\\)?\\)"
  "Regular expression matching time strings for analysis.")

(defconst org-ts-regexp2 (concat "<" org-ts-regexp1 "[^>\n]\\{0,16\\}>")
  "Regular expression matching time stamps, with groups.")

(defconst org-ts-regexp3 (concat "[[<]" org-ts-regexp1 "[^]>\n]\\{0,16\\}[]>]")
  "Regular expression matching time stamps (also [..]), with groups.")

(defconst org-tr-regexp (concat org-ts-regexp "--?-?" org-ts-regexp)
  "Regular expression matching a time stamp range.")

Instead of searching through the text of the svg file, you should be able to give each element an id or classname when creating them and programatically manipulate them via the svg functions or the built-in dom.el.

      (while (re-search-forward "<rect width=\"\\([^\"]+\\)\" height=\"\\([^\"]+\\)\" x=\"\\([^\"]+\\)\" y=\"\\([^\"]+\\)\" id=\"\\([^\"]+\\)\"" nil t)
      ...
  (when (re-search-forward (format " fill=\"\\(%s\\)\"" ot-sel-block-color) nil t)
ichernyshovvv commented 1 year ago

Hi. Thank you for your contribution.

I've fixed compilation warnings, replaced all the manually constructed regexps with builtin constants and now I use ot-svg-obj global variable to search for a block under mouse cursor in ot-select-block-under-mouse. It's the only place I used this object though.

For searching over svg object, I used dom-search which returns all matched nodes. I think the dom.el library lacks of something like dom-find that returns only the first matched node (like seq-find). Maybe it's a good idea to send it in a patch.