ahyatt / ekg

The emacs knowledge graph, app for notes and structured data.
GNU General Public License v3.0
230 stars 19 forks source link

Update metadata overlay and possible fix for #43 #50

Closed qingshuizheng closed 1 year ago

qingshuizheng commented 1 year ago
  1. update metadata overlay's read-only property for better interaction
  2. Possible fix #43

More details see commit messages.

ahyatt commented 1 year ago

Thank you for these changes, which look like interesting fixes! I like the ideas here. I'm going to try these out, but as written, they look like they should work.

ahyatt commented 1 year ago

A few comments (I probably should have tried this out before merging):

I'm trying this out now, but it seems like (propertize "\n" 'display "--text follows this line--\n\n" 'read-only t 'rear-nonsticky t) doesn't actually display "--text follows this line--", at least for me. I'm assuming this works for you, so I'm not sure what the difference is. I'm using Emacs 29 built a few months ago, perhaps this is functionality newer than that?

Also, two tests related to the overlay no longer pass, which needs to be fixed. Would you mind fixing those tests?

I'm not crazy about the warning about read-only when trying to delete backwards from the first actual line of text. Still, it does work, and org-insert-headline works now.

The label fixes work well, but I can press return on the first editable character and it beeps and complains about text-only properties but does insert the newline. It probably shouldn't; but then again, it almost certainly didn't protect against this before either.

All in all, these still look promising, and I'm interested to see if we can fix these issues - if not, we may have to revert the parts that are problematic (probably just the "--text follows this line--" part.

qingshuizheng commented 1 year ago

Ahh, I'm coming late.

This display property seems existing before I came to Emacs 3 years ago, but I'm not sure. I'm currently using Emacs 30 built one month ago, since it just branched out not long ago from emacs 29, I assumed there's no too much difference there. I can ask some others to test it out tho.

UPDATE: just tested it on Emacs 28.2 for macOS (universal), it works well.

(propertize "\n" 'display "--text follows this line--\n\n" 'read-only t 'rear-nonsticky t)

It should be inserted with (insert &args), dont' miss that:

(insert
 (propertize
  "\n"
  'display "--text follows this line--\n\n"
  'read-only t
  'rear-nonsticky t))

Here's a dummy gif: iShot_2023-05-09_17 18 22

Would you mind fixing those tests?

TBH I never write any test before, I can give it a go, but no promise.

qingshuizheng commented 1 year ago

Update:

After playing a little bit with ert, I do see the two overlay test failing due to the trailing "\n". I haven't figured out why, but will follow up on it.

The label fixes work well, but I can press return on the first editable character and it beeps and complains about text-only properties but does insert the newline

I update a new version of `ekg--metadata-string' to fix this, and now editing will only be allowed right after the whole label.

(defun ekg--metadata-string (property value)
  "Return a representation of PROPERTY with VALUE for the metadata.
This will be displayed at the top of the note buffer."
  (format "%s%s%s"
          (concat
           (propertize (concat property ":") 'face 'bold 'read-only t)
           (propertize " " 'read-only t 'rear-nonsticky t))
          value
          (propertize "\n" 'read-only t 'rear-nonsticky t)))

To elaborate:

Overlay for "label: " should be divided into 2 parts: "label:" and " ". For "label:", 'read-only t inhibits insertion from behind. For " ", extra 'rear-nonsticky t enables insertion from behind.

If it works at your end, feel free to commit it.

References:

  1. (elisp) Sticky Properties: https://www.gnu.org/software/emacs/manual/html_node/elisp/Sticky-Properties.html
  2. (elisp) Special Properties: https://www.gnu.org/software/emacs/manual/html_node/elisp/Special-Properties.html
ahyatt commented 1 year ago

Your example works perfectly, but for some reason your change is not working, and I have no idea why! I'm going to spend a bit more time looking into it. I appreciate your demonstration of this and further explanations.

Your new ekg--metadata-string does work, although it still has the same issue I mentioned: if I put the cursor on the first character of the field value, and press return, it will complain about a read-only property but then puts the field value on the following line. This isn't a big deal, and it probably has no good solution, but just wanted to mention it.

qingshuizheng commented 1 year ago

it will complain about a read-only property but then puts the field value on the following line.

This is truly annoying.

For the same reason I turned off all the visual bell and bell ring in my init.el, which is a trade-off:

(setq visible-bell nil) ; turn off blinking
(setq ring-bell-function #'ignore) ; turn off all alarming sounds, like from read-only, begining/end-of-buffer 

Therefore I didn't notice this sound before you mentioned it. 😆


Just now I experimented with the RET key and C-j at the beginning of field value: RET (org-mode) ==> newline insertion + read-only warning C-j (org-mode) ==> newline insertion, w/o warning

Then I turn off electric-indent-mode: RET (fundamental-mode) ==> newline insertion, w/o warning C-j (fundamental-mode) ==> go one character left + read-only warning

I'm assuming the read-only warning is triggered due to the electric-indent.

I'm having a family gathering later today, may not have time to look into it, but will update when I have additional info.