Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
qingshuizheng opened this issue Oct 21, 2023 · 8 comments
Closed

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

qingshuizheng opened this issue Oct 21, 2023 · 8 comments

Comments

@qingshuizheng
Copy link
Contributor

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
Copy link
Owner

ahyatt commented Oct 21, 2023

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
Copy link
Contributor Author

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
Copy link
Owner

ahyatt commented Oct 22, 2023

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
Copy link
Contributor Author

I think the two title lines seem best.

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

ahyatt added a commit that referenced this issue Oct 29, 2023
Now properties can have multiple lines in the metadata section. The new const
ekg-property-multivalue-type controls how multivalued properties act. This fixes
#110.
@ahyatt
Copy link
Owner

ahyatt commented Oct 29, 2023

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

@qingshuizheng
Copy link
Contributor Author

qingshuizheng commented Oct 30, 2023

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
  1. open note:
Tags: date/2023-10-30, test 2, test 1
Title: title 2
Title: title 1
  1. 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
Copy link
Contributor Author

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

ahyatt added a commit that referenced this issue Oct 30, 2023
@qingshuizheng
Copy link
Contributor Author

qingshuizheng commented Oct 30, 2023

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)))))
  1. 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

ahyatt added a commit that referenced this issue Nov 11, 2023
Now properties can have multiple lines in the metadata section. The new const
ekg-property-multivalue-type controls how multivalued properties act. This fixes
#110.
ahyatt added a commit that referenced this issue Nov 11, 2023
ahyatt added a commit that referenced this issue Nov 21, 2023
Now properties can have multiple lines in the metadata section. The new const
ekg-property-multivalue-type controls how multivalued properties act. This fixes
#110.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants