browser-hist: Use a dynamic completion table #15

karthink · 2024-03-22T03:20:50Z

browser-hist.el (browser-hist--sqlite-open,
browser-hist--sqlite-select, browser-hist-minimum-query-length, browser-hist--db-fields, browser-hist--db-connection, browser-hist--send-query, browser-hist--completion-table, browser-hist-search):

Instead of querying the full history every time, use a dynamic completion table and query the history database as needed.
Don't include the annotation in the url as invisible text, this hack is no longer needed.
Only re-copy the history database to the temp directory if it's newer.
New variable browser-hist-minimum-query-length for minimum query string length -- otherwise the sql query will return everything.
Autoload browser-hist-search and make browser-hist a feature for easy package management.
Move checks (is sql built-in, etc) to the installation stage instead of the "runtime" stage.
Close the db connection after using completing read -- sqlite is C code, and opening too many simultaenous connections to the db can crash or mess up the Emacs session.

agzam · 2024-03-22T17:10:38Z

~~The PR breaks it for me in Emacs 30.~~

GNU Emacs 30.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.41, cairo version 1.18.0) of 2024-03-16

I'm trying to figure out why.

Oh wait. It doesn't break. It just didn't work the first time for some reason. I think it's due to lag between copying the db over and search you've mentioned.

agzam · 2024-03-22T17:26:49Z

Few things I noticed:

It breaks the multiword search. For example, before I could search for a website description by typing "foo bar zap". That no longer works. I think including the description as invisible text is still necessary. Oh, wait, I might be wrong, it could be the matter of tweaking the cl-loop logic.
There isn't a default list anymore. Previously, it was possible to see the last items in the history without any search - sometimes you just need to grab last item without knowing what it was. I know this is one of the shortcomings of the dynamic search approach. I'm wondering how difficult it would be to make it bring the last 10-20 items of the history by default.
Sometimes it fails with: Error in post-command-hook (vertico--exhibit): (sqlite-error ("SQL logic error" "no such table: urls" 1 1)), that's I think due to the delay between the DB copy and search.

Other than these, it works very nicely. I can start a develop branch and merge the PR there if you want, then once we address these items we can merge it all to the main. wdyt?

karthink · 2024-03-22T17:55:19Z

It breaks the multiword search. For example, before I could search for a website description by typing "foo bar zap". That no longer works. I think including the description as invisible text is still necessary. Oh, wait, I might be wrong, it could be the matter of tweaking the cl-loop logic.

I'm not sure why this is happening, I can find history items by description alone.

You can run a query directly (for testing) in the following way. In a lisp interaction buffer, run

(browser-hist--send-query "agzam dynamic")
;; produces =>
(("https://github.com/agzam/browser-hist.el/issues/14" . "Some changes (dynamic completion table, etc) · Issue #14 · agzam/browser-hist.el")
 ("https://github.com/agzam/browser-hist.el/issues/14#issuecomment-2014242357" . "Some changes (dynamic completion table, etc) · Issue #14 · agzam/browser-hist.el")
 ("https://github.com/agzam/browser-hist.el/pull/15" . "browser-hist: Use a dynamic completion table by karthink · Pull Request #15 · agzam/browser-hist.el"))

So it searched against the description as well.

karthink · 2024-03-22T17:55:51Z

There isn't a default list anymore. Previously, it was possible to see the last items in the history without any search - sometimes you just need to grab last item without knowing what it was. I know this is one of the shortcomings of the dynamic search approach. I'm wondering how difficult it would be to make it bring the last 10-20 items of the history by default.

Should be quite easy.

agzam · 2024-03-22T19:10:09Z

browser-hist.el

+                (cl-loop for s in (split-string strings)
+                         collect (format " ( %s LIKE '%%%s%%' OR %s LIKE '%%%s%%' ) "
+                                         title s url s)
+                         into queries
+                         finally return
+                         (concat (format "SELECT DISTINCT %s, %s FROM %s WHERE"
+                                  title url table)
+                                 (mapconcat #'identity queries " AND ")
+                                 rest)))


Wow, crafting beautiful sushi with cl-loop. As someone who never learned the magic and intricacies of cl-loop, I have great respect for people who know how to use it. Very nice.

Thanks, I just threw that one in without much thought. cl-loops are usually better than map in performance-critical "hot-loops", but it's probably not required here.

That said, the updated version reduces the whole function to two cl-loops, including handling the case of no input (where it returns the latest history).

agzam

Beautifully done. Thank you for such amazing work.

karthink · 2024-03-23T18:30:16Z

@agzam are you interested in this cache-timeout feature, either as part of this PR or as another one?

I use it since I don't want (almost) every invocation of the command to copy a 80 MB sqlite db first, and most of the time I'm searching for urls I visited a while ago. (If I need the immediate history I call browser-hist-search with a prefix arg to force the cache update.)

agzam · 2024-03-23T18:35:28Z

browser-hist.el

  (unless (member '(".*\t" . browser-hist--url-handler)
                  browse-url-handlers)
    (add-to-list 'browse-url-handlers '(".*\t" . browser-hist--url-handler)))

  (when (boundp 'embark-transformer-alist)
-    (unless (member '(url . browser-hist--url-transformer)
+    (unless (memq '(url . browser-hist--url-transformer)


Suggested change

(unless (memq '(url . browser-hist--url-transformer)

(let ((tfa '(url . browser-hist--url-transformer)))

(unless (memq tfa embark-transformer-alist)

I noticed that byte-compile throws warning: 'memq' called with literal list...

We could just go back to member then? memq is technically faster but embark-transformer-alist is tiny so it won't matter.

Whichever you say is the best.

I changed it back to member.

agzam · 2024-03-23T18:41:40Z

@agzam are you interested in this cache-timeout feature

To be honest, I'm not sure. It's a very good idea on one hand, but on the other, I don't want to disrupt the current behavior if people expect the most recent history items to appear immediately.

But I believe the custom variable can be set to zero in that case, right? So I think it's just a matter of choosing the initial default value?

Do you want to add this feature in the same PR or as a separate thing? It's up to you. Or I can try to grab your code and make the changes myself so you don't have to waste time on it.

karthink · 2024-03-23T18:43:41Z

But I believe the custom variable can be set to zero in that case, right? So I think it's just a matter of choosing the initial default value?

Yes, the default value should be zero so there is no breaking change to anyone's usage.

Do you want to add this feature in the same PR or as a separate thing? It's up to you.

I can add it to this PR. If you merge the PR via rebase it should appear as an atomic commit so it should be fine.

karthink · 2024-03-23T18:45:46Z

@karthink after the initial load it now reports:

browser-hist--send-query: Symbol’s function definition is void: browser-hist--sqlite-open.

autoload maybe is missing somewhere?

byte-compile throws a related warning:
browser-hist.el:136:34: Warning: the function ‘browser-hist--sqlite-select’
    might not be defined at runtime.

I updated it and checked that there are no byte-compile warnings here -- but byte-compile operations are a finicky Emacs state-dependent thing and I don't have one of those CI packages to sandbox things, so please let me know.

browser-hist.el (browser-hist--sqlite-open, browser-hist--sqlite-select, browser-hist-minimum-query-length, browser-hist--db-fields, browser-hist--db-connection, browser-hist--send-query, browser-hist--completion-table, browser-hist-search): - Instead of querying the full history every time, use a dynamic completion table and query the history database as needed. - Don't include the annotation in the url as invisible text, this hack is no longer needed. - Only re-copy the history database to the temp directory if it's newer. - New variable `browser-hist-minimum-query-length` for minimum query string length -- otherwise the sql query will return everything. - Autoload `browser-hist-search` and make `browser-hist` a feature for easy package management. - Move checks (is sql built-in, etc) to the installation stage instead of the "runtime" stage. - Close the db connection after using completing read -- sqlite is C code, and opening too many simultaenous connections to the db can crash or mess up the Emacs session.

browser-hist.el (browser-hist-db-fields, browser-hist--send-query, browser-hist--completion-table, browser-hist-search): When input is shorter than `browser-hist-minimum-query-length`, show the latest 100 items from the browser history instead. Additional changes: - Make browser-hist--send-query more efficient: `seq-remove` creates a duplicate, this is unnecessary. Convert result construction to a simpler cl-loop. - Don't require subr-x, it's no longer used. Require cl-lib but only at compile time, reducing the overall dependencies of the package. - Add a FORCE-UPDATE prefix arg to `browser-hist-search`: This is useless right now but can be used to force updating the browser history cache (via copying) in the future. - Enforce the basic completion style in `browser-hist-search` to avoid conflicts with users' Orderless (or other) configuration.

browser-hist.el (browser-hist--sqlite-open, browser-hist--sqlite-select): Make the inbuilt-sqlite vs sqlite.el dispatch decision a compile-time macro expansion operation, hopefully appeasing our byte-compiler lords.

browser-hist.el (browser-hist-cache-timeout, browser-hist--make-db-copy): Optionally work using an out of date cache to prevent copying the history db with each invocation. The default behavior is unchanged.

browser-hist.el (browser-hist--send-query): Filter results with empty titles in the SQL query instead of in elisp afterwards.

agzam · 2024-03-24T17:42:09Z

@karthink let me know when I can merge it, or feel free to merge it yourself.

karthink · 2024-03-24T20:39:02Z

@karthink let me know when I can merge it, or feel free to merge it yourself.

Oh wait. It doesn't break. It just didn't work the first time for some reason. I think it's due to lag between copying the db over and search you've mentioned.

Is the db access error gone? I don't experience it any more but I'd like to be sure before merging.

This comment was marked as resolved.

Sign in to view

agzam reviewed Mar 22, 2024

View reviewed changes

karthink force-pushed the main branch from 207243a to 15880d8 Compare March 23, 2024 06:14

agzam approved these changes Mar 23, 2024

View reviewed changes

This comment was marked as resolved.

Sign in to view

agzam reviewed Mar 23, 2024

View reviewed changes

karthink added 4 commits March 23, 2024 13:35

browser-hist: Macro-ize sqlite dispatch operations

5512ccc

browser-hist.el (browser-hist--sqlite-open, browser-hist--sqlite-select): Make the inbuilt-sqlite vs sqlite.el dispatch decision a compile-time macro expansion operation, hopefully appeasing our byte-compiler lords.

browser-hist: Add a cache-timeout option

9248853

browser-hist.el (browser-hist-cache-timeout, browser-hist--make-db-copy): Optionally work using an out of date cache to prevent copying the history db with each invocation. The default behavior is unchanged.

karthink force-pushed the main branch from 6462a08 to 9248853 Compare March 23, 2024 20:35

browser-hist: Filter empty links in SQL

21f1927

browser-hist.el (browser-hist--send-query): Filter results with empty titles in the SQL query instead of in elisp afterwards.

agzam merged commit 65ce9aa into agzam:main Mar 25, 2024

This was referenced Mar 25, 2024

Some changes (dynamic completion table, etc) #14

Closed

Feature: actual search #8

Closed

agzam mentioned this pull request Apr 9, 2024

Use sensible limits #22

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

browser-hist: Use a dynamic completion table #15

browser-hist: Use a dynamic completion table #15

karthink commented Mar 22, 2024

agzam commented Mar 22, 2024 •

edited

Loading

agzam commented Mar 22, 2024 •

edited

Loading

karthink commented Mar 22, 2024

karthink commented Mar 22, 2024

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

agzam Mar 22, 2024

karthink Mar 23, 2024

agzam left a comment

This comment was marked as resolved.

karthink commented Mar 23, 2024

This comment was marked as resolved.

agzam Mar 23, 2024

karthink Mar 23, 2024

agzam Mar 23, 2024

karthink Mar 23, 2024

agzam commented Mar 23, 2024

karthink commented Mar 23, 2024

karthink commented Mar 23, 2024

agzam commented Mar 24, 2024

karthink commented Mar 24, 2024

	(unless (memq '(url . browser-hist--url-transformer)
	(let ((tfa '(url . browser-hist--url-transformer)))
	(unless (memq tfa embark-transformer-alist)

browser-hist: Use a dynamic completion table #15

browser-hist: Use a dynamic completion table #15

Conversation

karthink commented Mar 22, 2024

agzam commented Mar 22, 2024 • edited Loading

agzam commented Mar 22, 2024 • edited Loading

karthink commented Mar 22, 2024

karthink commented Mar 22, 2024

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

agzam Mar 22, 2024

Choose a reason for hiding this comment

karthink Mar 23, 2024

Choose a reason for hiding this comment

agzam left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

karthink commented Mar 23, 2024

This comment was marked as resolved.

agzam Mar 23, 2024

Choose a reason for hiding this comment

karthink Mar 23, 2024

Choose a reason for hiding this comment

agzam Mar 23, 2024

Choose a reason for hiding this comment

karthink Mar 23, 2024

Choose a reason for hiding this comment

agzam commented Mar 23, 2024

karthink commented Mar 23, 2024

karthink commented Mar 23, 2024

agzam commented Mar 24, 2024

karthink commented Mar 24, 2024

agzam commented Mar 22, 2024 •

edited

Loading

agzam commented Mar 22, 2024 •

edited

Loading