Ihor Radchenko <[email protected]> writes:
> display-column-overlay-here, put-column-overlay, install-column-overlay.
Thanks for the suggestions. I looked into the existing conventions
across org-mode and here's what I found.
1. Choosing the right verb for overlay creation
A survey of org-mode source reveals a clear convention for overlay
creation verbs:
| Function | Verb | File
|
|--------------------------------------------------+----------+------------------|
| org-num--make-overlay | make | org-num.el
|
| org--make-preview-overlay | make | org.el
|
| org-src--make-source-overlay | make | org-src.el
|
| org-table--make-shrinking-overlay | make | org-table.el
|
| org-columns--new-overlay | new |
org-colview.el |
| org-clock-put-overlay | put | org-clock.el
|
| org-table-add-rectangle-overlay | add | org-table.el
|
| org-fold-core--create-isearch-overlays | create |
org-fold-core.el |
The dominant pattern is --make-<descriptor>-overlay (4 out of 8).
Notably, install is absent from org-mode entirely for overlays — it
appears only in org-install-agenda-files-menu (unrelated usage).
'make' also pairs naturally with the existing org-columns--new-overlay:
- new-overlay → bare allocation
- make-cell-overlay → full construction (calls new-overlay, then
sets keymap, org-columns-key, org-columns-value,
org-columns-value-modified, org-columns-format, line-prefix,
wrap-prefix, and pushes to org-columns-overlays)
So I'd go with 'make' rather than put or install.
2. Removing the word "here"
The word here is redundant — all these functions operate at point by
convention. It adds noise without disambiguating. I'd drop it.
3. Renaming "column" to "cell"
These names have confused me since I started reading this code, and
they still do. The problem is that everything is called "column",
giving the impression that we're building whole columns.
Looking at the actual rendering loop in
`org-columns--display-columns':
(dolist (column columns) ; columns = ((spec val displayed) ...) for this
entry
(pcase column
(`(,spec ,original ,value)
(let* ((property (car spec))
(width (aref org-columns-current-maxwidths i))
(fmt (org-columns--overlay-fmt width (= i last))))
(org-columns--display-here-column ; installs ONE cell overlay
value fmt width property original face))))
(forward-char) ; advance one character for next cell
(cl-incf i))
Each overlay covers a single character representing one property
value for one heading. Then point advances by one character. This
is rendering:
cell₁ cell₂ cell₃ ... → one row
cell₁ cell₂ cell₃ ... → next row
In spreadsheet terms, each overlay is a cell — the intersection of
a row (heading) and a column (property). The entire loop renders
one row.
So the current name `org-columns-display-here-column' is misleading: it
suggests
installing a whole column (all rows for one property), when it
actually installs a single cell.
Proposed renames:
Current Proposed
------------------------------------ ---------------------------------
`org-columns--display-here-column' org-columns--make-cell-overlay
`org-columns--display-columns' org-columns--make-row-cells
`org-columns--display-here' could also benefit from renaming to
org-columns--display-row but I'll leave those for a separate
discussion.
Best,
--
Slawomir Grochowski
