support for retaining bookmarks order on deletion + printing multiline descriptions #818

LeXofLeviafan · 2025-02-26T19:09:00Z

implementing a retained_order parameter for deletion-related operations (when truthy, compactdb() updates multiple record indices at once in a way that prevents their reordering)
adding a --retained-order CLI option to allow for using said functionality from CLI
(...there doesn't seem to be any deletion support in the interactive shell 🤔)
changing Bukuserver to use this functionality by default
fixing a bug in delete_resultset()
ensuring that DB file is resized when committing (batch) deletions
additionally, fixing text wrapping during record printing (for proper output of multiline descriptions, and to allow for linebreaks between tags instead of hyphens)

@jarun honestly, I'd say it would make sense to always retain ordering in CLI deletions as well, since this at least guarantees consistent bookmarks order (and IDs are not consistent either way). …Alternatively we could drop the "old" DB compression logic altogether; though the new one is slower when deleting a large arbitrary resultset. (Then again, it's not like such updates need to be done very often.)

Screenshots

printing records with multiline data

(in case of tags, we're tricking textwrap into inserting linebreaks after commas rather than hyphens)

(an example of multiline description which looks better when rendered as such)

batch deletion of search results (with retained order of remaining records) + resized DB file

# deleting all https:// bookmarks from generated.db
yes | buku --nostdin --db generated.db --markers :https:// --delete --retain-order

(tail output after removing 9987 records)

(remaining records still retain their original order)

(the file size got reduced compared to the original database)

batch deletion of random records (with retained order)

# deleting a bunch of randomly picked bookmarks from generated.db
yes | buku --nostdin --db generated.db --np --markers :: --random 9995 --delete --retain-order

(tail output after removing 9995 records)

(remaining records still retain their original order)

batch deletion of an index range (with retained order)

yes | buku --nostdin --db generated.db --np --delete 3-9997 --retain-order

(tail output after removing 9995 sequential records)

(remaining records still retain their original order)

batch deletion of an index range without retained order

(removed indices are replaced in a single DB update)

(the last 10 records of resulting database)

(this also works when there's not enough records with larger indices to replace all that got removed)

(the last 4 records of resulting database)

webUI

(GUI deletion uses retained order now since it's a more convenient and intuitively expected UX)

LeXofLeviafan · 2025-02-26T19:11:11Z

buku

        fillwidth = columns - INDENT
+        desc_lines = [s for line in row.desc.splitlines()
+                        for s in textwrap.wrap(line, width=fillwidth) or ['']]


Splitting into lines then wrapping each separately (the fallback value is for empty lines since wrap() swallows them)

LeXofLeviafan · 2025-02-26T19:14:11Z

buku

+        desc_lines = [s for line in row.desc.splitlines()
+                        for s in textwrap.wrap(line, width=fillwidth) or ['']]
+        TR = str.maketrans(',-', '-,')  # we want breaks after commas rather than hyphens
+        tag_lines = textwrap.wrap(row.tags.translate(TR), width=fillwidth)


wrap() inserts linebreaks after hyphens by default; which is not really a good thing when displaying tags. On the other hand, it doesn't insert linebreaks after commas (unless those are followed by whitespace); so what we want is to replace - with , and vice versa before passing tags string to wrap() (then revert the change during the printing).

LeXofLeviafan · 2025-02-26T19:14:42Z

buku

                print(DESC_STR % line, end='')
-                ln_num += 1


Removed ln_num since it's simpler to just use enumerate()

LeXofLeviafan · 2025-02-26T19:18:07Z

buku

        with self.lock:
-            for pos, row in reversed(list(enumerate(results))):
-                self.delete_rec(row[0], delay_commit=True)
+            for pos, id in reversed(list(enumerate(ids))):


There's no actual guarantee that the input list is sorted by index and contains unique entries; which led to a bunch of warnings about missing indices (likely accompanied by removing wrong records) when I was testing this on certain searches.

LeXofLeviafan · 2025-02-26T19:18:50Z

buku

-                if idx not in ids:
-                    ids += (idx,)
-
+            ids = set(args.delete)


We don't actually care about the order, so using set() is a simpler approach here

LeXofLeviafan · 2025-02-26T19:20:26Z

buku

@@ -1456,17 +1464,18 @@ class BukuDb:
            if delay_delete:
                self.conn.commit()
            else:
-                self.commit_delete()
+                self.commit_delete(retain_order=retain_order)


In most cases, this value is passed directly as the respective parameter of calls which may result in record deletion

LeXofLeviafan · 2025-02-26T19:22:53Z

buku

                self.conn.commit()
+                self.cur.execute('VACUUM')


VACUUM is necessary to shrink the file after deletion (particularly when deleting a large amount of records). Note that it effectively creates (and commits) a transaction of its own, so placing it before conn.commit() results in an error.

LeXofLeviafan · 2025-02-26T19:24:55Z

buku

@@ -1841,28 +1850,34 @@ class BukuDb:
        delay_commit : bool
            True if record should not be committed to the DB,
            leaving commit responsibility to caller. Default is False.
+        upto : int, optional
+            If specified, multiple indices are moved at once.


This one is used specifically for optimization of range deletions (whether we're using retain_order or not, there's no reason to make multiple updates when a single one would suffice, without a worse outcome).

LeXofLeviafan · 2025-02-26T19:25:52Z

buku

        """

        # Return if the last index left in DB was just deleted
        max_id = self.get_max_id()
-        if not max_id:
+        if not max_id or (upto and upto < index):


If a range was specified, it needs to be non-empty

LeXofLeviafan · 2025-02-26T19:32:22Z

buku

+            with self.lock:
+                if retain_order or (upto or 0) > index:
+                    step = (max(max_id - upto, upto + 1 - index) if not retain_order else
+                            1 if not upto else upto + 1 - index)


There's 3 possibility here:

retain-order without upto

retain-order with upto

upto without retain-order

The 3rd case only differs from the 2nd one in that we want to support the sub-case where there's more records with larger indices remaining than was in the deleted range ("Indices 9998-10000 moved to 9990-9992"). This is covered by the max() subexpression.

LeXofLeviafan · 2025-02-26T19:33:21Z

buku

+                    msg = f'Indices {index+step}-{max_id} moved to {index}-{max_id-step}'
+                else:
+                    self.cur.execute('UPDATE bookmarks SET id = ? WHERE id = ?', (index, max_id))
+                    msg = f'Index {max_id} moved to {index}'


A single UPDATE should suffice for our purpose

LeXofLeviafan · 2025-02-26T19:35:02Z

buku

@@ -2114,9 +2138,6 @@ class BukuDb:
            return False

        if self.delete_rec_all():
-            with self.lock:
-                self.cur.execute('VACUUM')
-                self.conn.commit()


VACUUM is now invoked directly by delete_rec_all() after the commit (same as for other deletion operations)

LeXofLeviafan · 2025-02-26T19:35:53Z

buku

@@ -6332,22 +6355,17 @@ POSITIONAL ARGUMENTS:
            try:
                vals = [int(x) for x in args.delete[0].split('-')]
                if len(vals) == 2:
-                    bdb.delete_rec(0, vals[0], vals[1], True)
+                    bdb.delete_rec(0, vals[0], vals[1], is_range=True, retain_order=args.retain_order)


It's better to specify explicitly what this True actually stands for

fixed text wrapping during record printing

c869683

LeXofLeviafan commented Feb 26, 2025

View reviewed changes

buku

print(DESC_STR % line, end='')

ln_num += 1

Copy link

Collaborator Author

LeXofLeviafan Feb 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed ln_num since it's simpler to just use enumerate()

LeXofLeviafan commented Feb 26, 2025

View reviewed changes

implemented deletion with retained order

6a993a5

LeXofLeviafan commented Feb 26, 2025

View reviewed changes

LeXofLeviafan force-pushed the retain-order branch from a5462c1 to 6a993a5 Compare February 26, 2025 19:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support for retaining bookmarks order on deletion + printing multiline descriptions #818

support for retaining bookmarks order on deletion + printing multiline descriptions #818

LeXofLeviafan commented Feb 26, 2025

LeXofLeviafan Feb 26, 2025

LeXofLeviafan Feb 26, 2025

LeXofLeviafan Feb 26, 2025

LeXofLeviafan Feb 26, 2025

LeXofLeviafan Feb 26, 2025

LeXofLeviafan Feb 26, 2025

LeXofLeviafan Feb 26, 2025

LeXofLeviafan Feb 26, 2025

LeXofLeviafan Feb 26, 2025

LeXofLeviafan Feb 26, 2025

LeXofLeviafan Feb 26, 2025

LeXofLeviafan Feb 26, 2025

LeXofLeviafan Feb 26, 2025

support for retaining bookmarks order on deletion + printing multiline descriptions #818

Are you sure you want to change the base?

support for retaining bookmarks order on deletion + printing multiline descriptions #818

Conversation

LeXofLeviafan commented Feb 26, 2025

Screenshots

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment