bookmarks: database structure

[Olli]

the current (0.4.0) database structure is impractical. i doesn’t even adheres to the first normal form.

i suggest to normalize it. this would also fix related bugs such as bug 700033 bug 725460 bug 711400

1th solution:
  folders | id (int primary), title (text), folder_id (int)
  bookmarks | id (int primary), title (text), folder_id (int), uri (text), desc (text), app (int), toolbar (int)
where folder.folder_id and bookmarks.folder_id refer to folder.id

2th solution:
  bookmarks | id (int primary), title (text), folder (int), uri (text), desc (text), app (int), toolbar (int)
where bookmarks.folder refers to bookmarks.id

[Olli (coderkun-deactivatedaccount)]

i prefer the 2th solution, cause it fits better in the current code.

CREATE TABLE IF NOT EXISTS bookmarks (
id integer primary key autoincrement,
parent integer,
title text,
uri text,
desc text,
app integer,
toolbar integer
)

i could provide a patch (by the end of the week or so), would this be appreciated?

[Olli (coderkun-deactivatedaccount)]

Main changes to implement this database structure.

The patch do not solve all bookmark related issues but the main targets and it is a good start for further improvements, I think.

[Olli (coderkun-deactivatedaccount)]

I renamed katze_item_(get|set)_name() to katze_item_(get|set)_title() (in katze/katze/item.(c|h) therefore a few more files have to be changed in order to apply the patch from the previous post.

[Olli (coderkun-deactivatedaccount)]

Another patch witch adds a Foreign Key to bookmarks.parent references bookmarks.id.

This takes care that all subfolders and containing bookmarks of an folder get deleted.

[Olli (coderkun-deactivatedaccount)]

With the patch from comment #3 the keys “name” in the file .config/midori/search has to be renamed to “title” for the search engines to show up properly (thanks to vcap).
Alternatively you could delete this file because new entries get set up correctly, but then you lose your configured search engines.

[Olli (coderkun-deactivatedaccount)]

Here is a patch so correct the default search engines configuration.

[Olli (coderkun-deactivatedaccount)]

And another patch ;) to automatically convert the old database structure into the now one when loading the sqlite database.

[vcap (vcappe)]

@gotik_,
i wonder if the name->title thing is a good idea, alltogether...

i mean, this bikeshedding tends to dominate your patch, which make me lose track of the substantial change. not to mention the compatibility issue with the search engines.

more to the point, every time i add or edit a bookmark i get a warning:
Gtk-WARNING **: gtkliststore.c:797: Invalid column number 170105136 added to iter (remember to end your list of columns with a -1)

eventually, this seems to lead to a crash (hard to reproduce) when adding a bookmark i got this:
<<<
Gtk-CRITICAL **: IA__gtk_tree_store_insert_with_values: assertion `GTK_IS_TREE_STORE (tree_store)' failed
Gtk-CRITICAL **: IA__gtk_tree_model_iter_n_children: assertion `GTK_IS_TREE_MODEL (tree_model)' failed
>>>

then a segfault:
<<<
Program terminated with signal 11, Segmentation fault.
#0 0xb76de23e in g_type_check_instance_cast () from /usr/lib/libgobject-2.0.so.0
(gdb) bt 2
#0 0xb76de23e in g_type_check_instance_cast () from /usr/lib/libgobject-2.0.so.0
#1 0x08070b9b in midori_bookmarks_add_item_cb (array=0x9389b18, item=0x97e4090,
    bookmarks=0x98beef0) at ../panels/midori-bookmarks.c:320
(More stack frames follow...)
(gdb) frame 1
#1 0x08070b9b in midori_bookmarks_add_item_cb (array=0x9389b18, item=0x97e4090,
    bookmarks=0x98beef0) at ../panels/midori-bookmarks.c:320
320 model = gtk_tree_view_get_model (GTK_TREE_VIEW (bookmarks->treeview));
(gdb) p bookmarks->treeview
$1 = (GtkWidget *) 0x7
(gdb) p *bookmarks->treeview
Cannot access memory at address 0x7
(gdb) p *bookmarks
$2 = {parent_instance = {box = {container = {widget = {object = {parent_instance = {
              g_type_instance = {g_class = 0x941f0f8}, ref_count = 5, qdata = 0x996b3f0},
            flags = 6359008}, private_flags = 3584, state = 0 '\000', saved_state = 0 '\000',
          name = 0x0, style = 0x9402400, requisition = {width = 104, height = 19}, allocation = {
            x = 0, y = 230, width = 183, height = 21}, window = 0x9920d80, parent = 0x97e8768},
        focus_child = 0x0, border_width = 0, need_resize = 0, resize_mode = 0,
        reallocate_redraws = 0, has_focus_chain = 0}, children = 0x9880900, spacing = 0,
      homogeneous = 0}}, toolbar = 0x992b218, edit = 0x15, delete = 0x0, treeview = 0x7,
  app = 0x0, array = 0x98c5400, filter_timeout = 0, filter = 0x0}
>>>

[vcap (vcappe)]

gdb log in attachment

[vcap (vcappe)]

your conversion code seems to work but it failed to get the content of my 'Arch Linux' folder, for some reason.

Note that "select * from bookmarks where folder='Arch Linux';" on the old database in a sqlite shell doesn't return anything either.

i guess this has to do with the space (this is my only folder with a space in it); but unless my understanding of sql is faulty, i should not have to escape anything, right?

(sqlite 3.7.11-2 from Arch Linux i686)

[vcap (vcappe)]

btw: "/* Foreign Keys require sqlite>3.6.19 */"

how old is 3.6.19?
is it reasonable as a hard requirement?

[vcap (vcappe)]

or 3.6.20 even, assuming you really mean '>' rather than ">=".

[Alexander Butenko (avb)]

Your patches looks great.
The only problem i see is that you renamed get|set_name API. and 'name' variable in search configs.

Id better rolled back this ones and replaced new katze_item_get_id with katze_item_get_meta_integer (item, "id");

Also note, that ~/.config/midori/search stays unchanged after midori upgrade so most likely you will break existing search configurations.

[Olli (coderkun-deactivatedaccount)]

Yes, the change of katze_item_get|set_name() is not good and not necessary.

I’m working on a new patch, which covers that and some more minor changes. So please forget the previous patches and wait with any further changes for the new one.

[Olli (coderkun-deactivatedaccount)]

Okay, here it is.

Forget all previous patches and try this one.

It contains a new function to update a database entry, but I’m not shure if it’s on the right place:
katze_array_update_item() in midori/midori-array.(c|h)

[vcap (vcappe)]

the warning "gtkliststore.c:797: Invalid column number 155650272 added to iter..." comes from the call to gtk_list_store_insert_with_values at midori-browser.c:693 (that's the first one in midori_bookmark_folder_button_new).

but then it's not quite clear how it comes up with such a value, given it's just:
gtk_list_store_insert_with_values (model, NULL, G_MAXINT,
        0, _("Toplevel folder"), 1, PANGO_ELLIPSIZE_END, 2, 0, -1);

my only guess is that the gtk_list_store stuff doesn't play well with G_TYPE_INT64.

[vcap (vcappe)]

Found the problem. the warning dissappear if i change it to:

gtk_list_store_insert_with_values (model, NULL, G_MAXINT,
        0, _("Toplevel folder"), 1, PANGO_ELLIPSIZE_END, 2, (gint64)0, -1);

hopefully, it fixes the crash too.

[Olli (coderkun-deactivatedaccount)]

Great, thanks.

[Alexander Butenko (avb)]

I think thats one more point you need to elaborate in your patch. I havent paid an attention that bookmarks parent != katzeItem parent. Thats for sure will be a problem in code readability after. Let me know what do you think

avb1: kalikiana: what do you think about katze_item_get_id?
avb1: he added new int64 member
avb1: i think we can go with katze_item_get_meta_integer
avb1: but both ways looks ok for me
avb1: except that im aware of some side effects from new member
kalikiana: avb1: my criteria would be 2: is it performance-critical? is it useful outside of bookmarks (history, search)?
kalikiana: if speed is not an issue, meta integer tends to make clear that it's an addon sort of thing
kalikiana: I wonder about the folder/parent change
kalikiana: I'm not sure if that is more than a rename
kalikiana: it could be misleading given that the parent normally is _get_parent, ie a real object
kalikiana: opinions on that?
natano: where can is see the patch?
avb1: kalikiana: agree
avb1: thats where was my concent
avb1: he is missing a fact that katzeitem is used not only for bookmarks
avb1: and yes, bookmarks parent != katzeItem parent

[Olli (coderkun-deactivatedaccount)]

“parent” is the column name in the sqlite-database. Do you have a better name for this? “folder” also suggest a full item rather than the id of the parent entry. I’m open to suggestions.

if you prefer dropping katze_item_get_id() and use katze_item_get_meta_integer() instead, let me know.

[Olli (coderkun-deactivatedaccount)]

This is the final patch, I guess.

Changes to the previous ones:
* implemented several annotations by vcap und avb
* katze_array_update_item() → midori_bookmarks_update_item_db() in ./panels/midori-bookmarks.c|h
* use of katze_item_get_meta_integer() instead of katze_item_get_id() (_get_id() dropped)
* renamed “parent” to “parentid” when it is no KatzeItem
* Im- and export tested

[Cris Dywan (kalikiana)]

This is looking very nice. I like how parent id works and the new folder handling in particular, we've had a lot of issues related to broken/ ambiguous parents.

If "TODO: Function never used" is true, by all means, let's drop the function.

I wonder, can the "if (convert) sqlite3_exec" with multiple statements in theory fail so that there's a new table without importing the old one? Should it use explicit commit statements?

Apart from that, I'd like to get this in shortly after release. Following that Vincent's test work.

[Cris Dywan (kalikiana)]

When I said "explicit commit statements" I was thinking of BEGIN, possibly also ON CONFLICT/ ROLLBACK, cf. http://www.sqlite.org/lang_transaction.html

[Alexander Butenko (avb)]

+1 Patch looks great. But agreee, its better to push it after release as its quite big, so some more testing will not hurt.

[vcap (vcappe)]

there was some breakage in menubar and bookmarkbar.

with the attached patch it should be on par with mainline midori (which is still very broken, btw. e.g. thing not updating when they should, bookmarkbar not recursing in subfolders).

[Cris Dywan (kalikiana)]

Vincent, are you suggesting to address this as part of the initial commit? It probably makes sense, though on the other hand it makes it difficult to get your test cases in.

[vcap (vcappe)]

> are you suggesting to address this as part of the initial commit?

the above patch fixes bugs in gotik's patch so they should get in together, no matter when it happens.
or do you mean something else?

[vcap (vcappe)]

I am unsure how that transaction stuff works.
Is that how it's done:

gboolean
midori_bookmarks_convert_table (sqlite3 *db)
{
    char *convert_errmsg;
    char *sqlcmd =
        "BEGIN TRANSACTION;"
            "ALTER TABLE bookmarks RENAME TO bookmarks_old;"
            "CREATE TABLE bookmarks"
                "(id INTEGER PRIMARY KEY AUTOINCREMENT, parentid INTEGER,"
                "title TEXT, uri TEXT, desc TEXT, app INTEGER, toolbar INTEGER,"
                "FOREIGN KEY(parentid) REFERENCES bookmarks(id) ON DELETE CASCADE);"
            "INSERT INTO bookmarks (parentid, title, uri, desc, app, toolbar) "
                "SELECT NULL AS parentid, title, uri, desc, app, toolbar "
                "FROM bookmarks_old; "
            "UPDATE bookmarks SET parentid = ("
                "SELECT id FROM bookmarks AS b1 WHERE b1.title = ("
                "SELECT folder FROM bookmarks_old WHERE title = bookmarks.title)); "
            "DROP TABLE bookmarks_old;"
        "COMMIT;";

    if (sqlite3_exec (db, sqlcmd, NULL, NULL, &convert_errmsg) != SQLITE_OK)
    {
        g_warning ("Failed to convert old bookmarks table: %s", convert_errmsg);
        sqlite3_free (convert_errmsg);

        if (sqlite3_exec (db, "ROLLBACK;", NULL, NULL, &convert_errmsg) != SQLITE_OK)
        {
            g_warning ("Could not revert changes: %s", convert_errmsg);
            sqlite3_free (convert_errmsg);
        }

        return FALSE;
    }

    return TRUE;
}

[vcap (vcappe)]

Same as above as attachment, so lines are not broken at inconvenient places.

[Olli (coderkun-deactivatedaccount)]

vcap:
fix_menubar.diff looks good. Thanks for this correction.

The transaction-stuff is a much cleaner way for converting the table. How do you plan to integrate midori_bookmarks_convert_table()?

[vcap (vcappe)]

> How do you plan to integrate midori_bookmarks_convert_table()?

It will be called from midori_bookmarks_initialize(). See attached patch.

I also added a function to identify the format of the table; that way midori_bookmarks_initialize is much more straigthforward (it is then just a switch on the table format).

(note: they all are in midori/midori-bookmarks.c because i moved the midori_bookmarks* functions that used to be in main.c there, see bug 983821).

[vcap (vcappe)]

Attached is gotik_'s patch on top of the change in bug 983821 (that is, it's the same patch as the one in comment #21 here, except the bookmarks functions from main.c are now in midori/midori-bookmarks.c).

[Olli (coderkun-deactivatedaccount)]

Last addition:
I added the column “position” (Integer) to the database. This can be used for sorting the bookmarks in the future without the need for another conversion.

@vcap: Can you add this to your bookmarks-conversion-code.patch/bookmarks_Database_structure_improved_on_top_of_983821.patch?

So that’s it.

[Olli (coderkun-deactivatedaccount)]

* Column “pos” splitted in “pos_panel” and “pos_bar” (both Integer).
* Applyed menu-/bookmarkbar fixes (patch https://bugs.launchpad.net/midori/+bug/836707/+attachment/3073792/+files/fix_menubar.diff)

[vcap (vcappe)]

Attached is gotik_'s patch on top of the change in bug 983821.

that is, it's the same patch as the one in comment #34 here, except the bookmarks functions from main.c are now in midori/midori-bookmarks.c

while i were at it i also replaced all spaces lines by empty lines and moved some variable declarations earlier to avoid "warning: ISO C90 forbids mixed declarations and code", but it's the same patch otherwise.

from here, the plan is to first commit the patches in bug 983821 (at least patch 1/4 and 4/4 there), then this one.

And then, commit 0001-Rework-bookmarks-conversion-code.patch once i'm finished polishing it (see comment #31 here to get the idea).

[vcap (vcappe)]

same as above with more cosmetic fixes (mostly space between function and opening parenthesis.

[vcap (vcappe)]

new version of my bookmarks conversion patch.

main difference from comment #31 is that it now report the error messages to the caller instead of using g_warning.

[vcap (vcappe)]

same as above with a slightly better error message when conversion failed and we could not rollback.

[vcap (vcappe)]

so, the easiest would be to commit patches 1/4 [1] and 4/4 [2] from bug 983821, then comment #36 [3] here, then comment #38 [4] here.

[1]
https://bugs.launchpad.net/midori/+bug/983821/+attachment/3082658/+files/0001-Move-bookmarks-code-out-of-midori-main.c.patch

[2]
https://bugs.launchpad.net/midori/+bug/983821/+attachment/3082678/+files/0004-Enable-sql-tracing-on-the-bookmarks-db.patch

[3]
https://bugs.launchpad.net/midori/+bug/836707/+attachment/3116815/+files/Bookmarks_Database_structure_improved_on_top_of_983821.patch

[4]
https://bugs.launchpad.net/midori/+bug/836707/+attachment/3120857/+files/0001-Rework-bookmarks-conversion-code.patch

[vcap (vcappe)]

So, i completly redone 0001-Rework-bookmarks-conversion-code.patch, so it now leaves the original bookmarks.db untouched.

instead we now have a bookmarks_v2.db which contains the new stuff. that way the user can hop back to previous version of midori and still find his old bookmarks (but not the one added after the switch to v2). it can also be useful in case the conversion fails.

also the conversion function no longer touch the database structure anymore. in effect, it's now an import function and has been renamed as such.

[vcap (vcappe)]

same patch as above with minor comestic fixes.

[Paweł Forysiuk (tuxator)]

Looks nice, good work!

I would sort two things before we commit latest patch:

1) Add separate field for bookmark title when it is in a bookmarkbar (user maybe want it to be shorter or even empty)

2) Populate pos_panel field when importing bookmarks, select count for given folder or simple counter should do i think

Any opinions and suggestions welcome.
Apart from that patch looks and works fine.

[Olli (coderkun-deactivatedaccount)]

Maybe a few additional fields would be nice.

On IRC there were the following suggestions:
* The date, when the bookmark was created
* The date when the user last visited the bookmark

 And I suggest:
* A counter of visits/clicks of the bookmark

Any other suggestions?

[Olli (coderkun-deactivatedaccount)]

Here is a new patch. It contains the following new database fields:

* created DATE DEFAULT CURRENT_TIMESTAMP
(default timestamp on insert)

* pos_panel INTEGER
(triggered on insert, update, delete)

* pos_bar INTEGER
(triggered on insert, update, delete)

* last_visit DATE
(unused)

* visit_count INTEGER DEFAULT 0
(unused)

* nick TEXT
(unused)

The pos-fields (pos_panel, pos_bar) are updated by triggers but are not yet used by the bookmarks panel.

@vcap: Can you adapt your bookmarks conversion patch?

[Olli (coderkun-deactivatedaccount)]

pfor merged the above patch with vcap’s bookmarks conversion patch and did a few things for readability.
I then updated it to work with the removal of a few old bookmark import lines by kalikiana, which has to be applied before this patch.

[vcap (vcappe)]

Doesn't build as-is with current git master, because main() still use the bookmarks_exist variable in the old conversion code. The "else if (!bookmarks_exist)" branch, there, should probably be removed too (which i assume is how it is on your local branch, but it's still there in master).

Also, the fact that midori_bookmarks_initialize doesn't take a filename anymore breaks my unit test code which relied upon being able to give ":memory" there (yeah, i am the one to blame for this).

What happened to the end of midori_bookmarks_import_from_old_db? Why are we g_printing the error message instead of setting the caller-provided errmsg pointer.

Sounds good, otherwise.

[vcap (vcappe)]

> I then updated it to work with the removal of a few old bookmark import lines by kalikiana, which has to be applied before this patch.

Ah! So, that's why i did not build out of the box for me.

[Cris Dywan (kalikiana)]

The prerequisite patch is in master now.

Updated patch:
 - don't use sqlite3_db_filename
 - require sqlite 3.6.19 due to foreign keys
 - use katze_str_non_null

So I don't know what's special about sqlite3_db_filename, but it doesn't work for me. My suggestion is henceforth to not use it, which is what I did here.
http://www.sqlite.org/c3ref/db_filename.html

As far as tests go, I'd say the easiest option is to adapt the required arguments after landing to whatever the tests may need.

[Cris Dywan (kalikiana)]

Two more things I forgot to mention:
 - The author name in the patch was bogus, and I put a FIXME there. Please use "git config user.email <email address hidden>" before generating patches in the future.
 - I fixed how errmsg was used, it was half uncommented.

[vcap (vcappe)]

Looking at the docs it seems like it should have been sqlite3_db_filename(db, "main") instead of sqlite3_db_filename(db, NULL).

That would not explain a build failure, though.

[vcap (vcappe)]

Attached is a patch to move the bookmarks db creation code out of main midori_bookmarks_initialize and into midori_bookmarks_create_db. That way it can be used from the unit test code. Also, it makes midori_bookmarks_initialize nicer.

Note that the unit test code is still broken due to it making assumptions on the internal db structure that don't hold anymore.

[vcap (vcappe)]

btw, do we really want to allow the caller to pass NULL as errmsg. Because, right now we are missing some checks (and that's even worse with my last patch).

[Cris Dywan (kalikiana)]

Vincent, you're right, we need to make a decision here. Part of the code checks it, most code doesn't. Overall I don't think ignoring errors is a good idea anywhere around bookmarks or history setup code, so I added return_if_fail in both now.

Pushed the database structure patch. I recall a dedicated bug for test cases, but can't find it. I'd like the follow up patches to go there, or a new bug, please, for clarity.