search

ahyatt / ekg

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

Don't split title with `ekg--split-metadata-string` #110

Closed qingshuizheng closed 10 months ago

qingshuizheng commented 10 months ago

Hello @ahyatt ,

https://github.com/ahyatt/ekg/blob/7811c17453fc6f59f16bb5fa04af06ebee50e00e/ekg.el#L1239C20-L1239C52

I don't think we need to use `ekg--split-metadata-string' for the title, or the title containing "," will be split.

(defun ekg--metadata-update-title (val)
  "Update the title field from the metadata VAL."
  (setf (ekg-note-properties ekg-note)
        (plist-put (ekg-note-properties ekg-note) :titled/title
                   (ekg--split-metadata-string val))))

Say, I have a note with title: "This is a test title, with a comma", after save I got this in db: (Note: the title is split into 2 parts)

sqlite> select * from triples where subject is 33419178152;
subject      predicate                   object                  properties
-----------  --------------------------  ----------------------  ----------
33419178152  base/type                   tagged                  ()        
33419178152  base/type                   text                    ()        
33419178152  base/type                   time-tracked            ()        
33419178152  base/type                   titled                  ()        
33419178152  tagged/tag                  "date/2023-10-22"       (:index 0)
33419178152  tagged/tag                  "test"                  (:index 1)
33419178152  text/mode                   org-mode                ()        
33419178152  text/text                   ""                      ()        
33419178152  time-tracked/creation-time  1697905670              ()        
33419178152  time-tracked/modified-time  1697905779              ()        
33419178152  titled/title                "This is a test title"  (:index 0)
33419178152  titled/title                "with a comma"          (:index 1)
sqlite>

Note that titled/title is split into 2 parts.

In this case, keep the original val is okay: 

(defun ekg--metadata-update-title (val)
  "Update the title field from the metadata VAL."
  (setf (ekg-note-properties ekg-note)
        (plist-put (ekg-note-properties ekg-note) :titled/title
                   ;;; (ekg--split-metadata-string val) ; <-- comment out
                   (list val) ; <-- change to this
                   )))

What do you think?

ahyatt commented 10 months ago

This is due to the consequence of titles not being unique, which was a deliberate (but perhaps wrong!) design decision. I do think there might be situations in which you want to have two different titles for a doc, although it hasn't actually been useful for me personally yet. We can reconsider that decision, which would necessitate a database upgrade of some sort.

qingshuizheng commented 10 months ago

Just wake up, sorry for late reply. :-)

there might be situations in which you want to have two different titles for a doc

Now I sort of understand the design and some errors I encountered before.

Multiple-title design could be useful for some users, you should keep it there.

Then it is just the "comma as separator" thing. The issue is that, commas are quite refrequently used in titles (generated by chatgpt):

1. "The Impact of Climate Change on Coastal Ecosystems: A Case Study of the Great Barrier Reef, Australia"
2. "Exploring the Relationship Between Sleep Quality, Stress Levels, and Academic Performance in College Students"
3. "Quantum Mechanics and Entanglement: Investigating the Role of Spontaneous Symmetry Breaking, Dark Energy, and Gravity"
4. "The Effects of Exercise, Diet, and Genetics on Obesity: A Review of Current Literature"
5. "Exploring the Use of Machine Learning Techniques for Data Analysis, Feature Extraction, and Prediction in Bioinformatics"

Perhaps we could replace comma with other separators, like "\\"? Or just one title at one line in metadata overlay? Both look good to me, but the latter would be better for multiple, long titles?

ahyatt commented 10 months ago

Yes, good point: a comma isn't great as a separator. I think both of your suggestions are reasonable, but I think the two title lines seem best. I'll experiment with that and see if something reasonable can be done. Thank you for raising this!

qingshuizheng commented 10 months ago

I think the two title lines seem best.

Yea, I would prefer this. Take your time. Thanks

ahyatt commented 10 months ago

I checked in a change to develop, PTAL and let me know if that works for you.

qingshuizheng commented 10 months ago

That's a lot of work!

It works pretty neat. Also the tags are able to merge if multiple-line. Great!

One small itch, the position of TAGs and TITLEs are rotating between each save, as shown beow:

  1. before save: 
    Tags: test 1, test 2, date/2023-10-30
Title: title 1
Title: title 2
  2. open note: 
    Tags: date/2023-10-30, test 2, test 1
Title: title 2
Title: title 1
  3. save again and open the note: 
    Tags: test 1, test 2, date/2023-10-30
Title: title 1
Title: title 2

    Need a `nreverse' somewhere?

Thanks!

qingshuizheng commented 10 months ago

BTW, in ekg-notes-mode, could we have each title at its own new line as well?

qingshuizheng commented 10 months ago

Hello @ahyatt ,

Currently (as of c0623a2), 2 tests failed:

Selector: "ekg-test-.*"
Passed:  24
Failed:  2 (2 unexpected)
Skipped: 0
Total:   26/26

Started at:   2023-10-31 00:28:49+0800
Finished.
Finished at:  2023-10-31 00:28:54+0800

..F......................F

F ekg-test-draft
    (ert-test-failed
     ((should
       (equal target-content (substring-no-properties (buffer-string))))
      :form
      (equal "Tags: test, date/2023-10-31\n\nfoo"
         "Tags: date/2023-10-31, test\n\nfoo")
      :value nil :explanation
      (array-elt 6 (different-atoms (116 "#x74" "?t") (100 "#x64" "?d")))))

F ekg-test-url-handling
    (ert-test-failed
     ((should
       (equal (ekg-document-titles)
          (list (cons "http://testurl" "A URL used for testing"))))
      :form
      (equal
       (("http://testurl" . 65) ("http://testurl" . 32)
    ("http://testurl" . 85) ("http://testurl" . 82)
    ("http://testurl" . 76) ("http://testurl" . 32)
    ("http://testurl" . 117) ("http://testurl" . 115)
    ("http://testurl" . 101) ("http://testurl" . 100) ...)
       (("http://testurl" . "A URL used for testing")))
      :value nil :explanation
      (proper-lists-of-different-length 22 1
                    (("http://testurl" . 65)
                     ("http://testurl" . 32)
                     ("http://testurl" . 85)
                     ("http://testurl" . 82)
                     ("http://testurl" . 76)
                     ("http://testurl" . 32)
                     ("http://testurl" . 117)
                     ("http://testurl" . 115)
                     ("http://testurl" . 101)
                     ("http://testurl" . 100) ...)
                    (("http://testurl" .
                      "A URL used for testing"))
                    first-mismatch-at 0)))

Also, for any ekg-capture related function, value of :titled/title should be a list.
Some modifications I made:

  1. In ekg.el, ekg-capture-url & ekg-capture-file, 

    modified   ekg.el
(defun ekg-capture-url ()
@@ -1041,8 +1041,7 @@ However, if URL already exists, we edit the existing note on it."
 (if existing
     (ekg-edit (ekg-get-note-with-id url))
   (ekg-capture :tags (list (concat "doc/" (downcase cleaned-title)))
-                   ;; Remove commas from the value.
-                   :properties `(:titled/title ,cleaned-title)
+                   :properties `(:titled/title ,(list title))
                :id url))))

(defun ekg-capture-file ()
@@ -1058,7 +1057,7 @@ file. If not, an error will be thrown."
   (if existing
       (ekg-edit (ekg-get-note-with-id file))
     (ekg-capture :tags (list (concat "doc/" (downcase (file-name-nondirectory file))))
-                     :properties `(:titled/title ,(file-name-nondirectory file))
+                     :properties `(:titled/title ,(list (file-name-nondirectory file)))
                  :id file)))))

  2. In ekg-logseq-test.el

modified   ekg-logseq-test.el

(ert-deftest ekg-logseq-test-note-with-resource-and-title-to-logseq ()
@@ -109,7 +109,7 @@
   (let ((note (ekg-note-create :text "line1\nline2\n" :mode 'org-mode :tags '("tag1" "tag2" "tag3"))))
     (setf (ekg-note-id note) "http://www.example.com")
     (setf (ekg-note-modified-time note) 123456789)
-    (setf (ekg-note-properties note) '(:titled/title "Title"))
+    (setf (ekg-note-properties note) '(:titled/title ("Title")))
     (should (equal
              "* Title\n:PROPERTIES:\n:ID: http://www.example.com\n:EKG_HASH: c696f3d4b296c737155637d3a708d2b986ab6f6f\n:END:\n#[[tag1]] #[[tag2]]\nhttp://www.example.com\nline1\nline2\n"
              (ekg-logseq-note-to-logseq-org note "tag3")))
  1. For listing titles in their own lines in ekg-notes-mode (if you'd like to to implement it :-)): 
    modified   ekg.el
(defun ekg-display-note-titled (note)
@@ -695,7 +695,7 @@ FORMAT-STR controls how the time is formatted."
(if-let (title (plist-get (ekg-note-properties note) :titled/title))
   (propertize (concat
            (mapconcat #'identity (plist-get (ekg-note-properties note) :titled/title)
-                          " / ") "\n")
+                          "\n") "\n")
           'face 'ekg-title)
 ""))

PLEASE NOTE: after these changes above, the test ekg-test-draft still fails.

Update: I created a PR, the above issues will all be addressed. Feel free to make any modifications.

Zheng