Discussion:
[zathura] [Zathura PATCH v2] Bookmark the exact position, not just the page number (along with a number of fixes).
Marwan Tanager
2013-06-21 04:12:12 UTC
Permalink
This patch adds some enhancements/fixes to the bookmarking feature of Zathura:

- Bookmark the exact vertical and horizontal adjustments values, along with
the page number. This is done in a backward-compatible way for both the
plain and sqlite database backends, so that bookmarks that was taken
previously (bookmarking only the page number) will still work as expected
and won't be lost.

- Fix the issue of not being able to remove bookmarks from the plain
database; removing a bookmark in plain_remove_bookmark using
g_key_file_remove_key corrupts the bookmarks file. This is due to not
truncating the entire bookmarks file in zathura_db_write_key_file_to_file
prior to overriding it with a new one not containing the removed line.
This is why, I guess, plain_remove_bookmark hadn't been implemented as
expected using g_key_file_remove_key, because apparently, someone thought
that the problem is caused by this API.

- Fix not being able to update existing bookmarks persistently; updating a
bookmark works only during the current session, but after the file is
closed and reopened, the updated bookmark still has it's old value. This
is due to changing only the bookmark structure in memory; the proper way
to do it, is to call zathura_db_remove_bookmark on the old bookmark, then
follow that by a call to zathura_db_add_bookmark on a bookmark structure
containing the new position and the same old ID.

- Make zathura_bookmark_add updates the bookmark if it already exists,
rather than doing this externally in cmd_bookmark_create. This allows us
to have all the relevant girara_notify messages in cmd_bookmark_create.

---
bookmarks.c | 27 +++++++++++++++++++++++++--
bookmarks.h | 2 ++
commands.c | 31 +++++++++++++++++++++++--------
database-plain.c | 51 +++++++++++++++++++++++++++++++++++++++------------
database-sqlite.c | 29 ++++++++++++++++++++++++++---
5 files changed, 115 insertions(+), 25 deletions(-)

diff --git a/bookmarks.c b/bookmarks.c
index fc109d3..e7e792a 100644
--- a/bookmarks.c
+++ b/bookmarks.c
@@ -4,9 +4,11 @@
#include "bookmarks.h"
#include "database.h"
#include "document.h"
+#include "adjustment.h"

#include <girara/datastructures.h>
#include <girara/utils.h>
+#include <girara/session.h>

static int
bookmark_compare_find(const void* item, const void* data)
@@ -23,14 +25,35 @@ zathura_bookmark_add(zathura_t* zathura, const gchar* id, unsigned int page)
g_return_val_if_fail(zathura && zathura->document && zathura->bookmarks.bookmarks, NULL);
g_return_val_if_fail(id, NULL);

- zathura_bookmark_t* old = girara_list_find(zathura->bookmarks.bookmarks, bookmark_compare_find, id);
+ double x = zathura_adjustment_get_ratio(gtk_scrolled_window_get_hadjustment(GTK_SCROLLED_WINDOW(zathura->ui.session->gtk.view)));
+ double y = zathura_adjustment_get_ratio(gtk_scrolled_window_get_vadjustment(GTK_SCROLLED_WINDOW(zathura->ui.session->gtk.view)));
+ zathura_bookmark_t* old = zathura_bookmark_get(zathura, id);
+
if (old != NULL) {
- return NULL;
+ old->page = page;
+ old->x = x;
+ old->y = y;
+
+ if (zathura->database != NULL) {
+ const char* path = zathura_document_get_path(zathura->document);
+ if (zathura_db_remove_bookmark(zathura->database, path, old->id) == false) {
+ girara_warning("Failed to remove old bookmark from database.");
+ }
+
+ if (zathura_db_add_bookmark(zathura->database, path, old) == false) {
+ girara_warning("Failed to add new bookmark to database.");
+ }
+ }
+
+ return old;
}

zathura_bookmark_t* bookmark = g_malloc0(sizeof(zathura_bookmark_t));
+
bookmark->id = g_strdup(id);
bookmark->page = page;
+ bookmark->x = x;
+ bookmark->y = y;
girara_list_append(zathura->bookmarks.bookmarks, bookmark);

if (zathura->database != NULL) {
diff --git a/bookmarks.h b/bookmarks.h
index 7cd2698..980cca0 100644
--- a/bookmarks.h
+++ b/bookmarks.h
@@ -10,6 +10,8 @@ struct zathura_bookmark_s
{
gchar* id;
unsigned int page;
+ double x;
+ double y;
};

typedef struct zathura_bookmark_s zathura_bookmark_t;
diff --git a/commands.c b/commands.c
index 7ec73dc..76e2a27 100644
--- a/commands.c
+++ b/commands.c
@@ -18,6 +18,7 @@
#include "plugin.h"
#include "internal.h"
#include "render.h"
+#include "adjustment.h"

#include <girara/session.h>
#include <girara/settings.h>
@@ -44,19 +45,24 @@ cmd_bookmark_create(girara_session_t* session, girara_list_t* argument_list)

const char* bookmark_name = girara_list_nth(argument_list, 0);
zathura_bookmark_t* bookmark = zathura_bookmark_get(zathura, bookmark_name);
- if (bookmark != NULL) {
- bookmark->page = zathura_document_get_current_page_number(zathura->document) + 1;
- girara_notify(session, GIRARA_INFO, _("Bookmark successfuly updated: %s"), bookmark_name);
- return true;
- }
+ bool update = bookmark != NULL ? true : false;

bookmark = zathura_bookmark_add(zathura, bookmark_name, zathura_document_get_current_page_number(zathura->document) + 1);
if (bookmark == NULL) {
- girara_notify(session, GIRARA_ERROR, _("Could not create bookmark: %s"), bookmark_name);
+ if (update == true) {
+ girara_notify(session, GIRARA_ERROR, _("Could not update bookmark: %s"), bookmark_name);
+ } else {
+ girara_notify(session, GIRARA_ERROR, _("Could not create bookmark: %s"), bookmark_name);
+ }
return false;
+ } else {
+ if (update == true) {
+ girara_notify(session, GIRARA_INFO, _("Bookmark successfully updated: %s"), bookmark_name);
+ } else {
+ girara_notify(session, GIRARA_INFO, _("Bookmark successfully created: %s"), bookmark_name);
+ }
}

- girara_notify(session, GIRARA_INFO, _("Bookmark successfuly created: %s"), bookmark_name);
return true;
}

@@ -112,7 +118,16 @@ cmd_bookmark_open(girara_session_t* session, girara_list_t* argument_list)
}

zathura_jumplist_add(zathura);
- page_set(zathura, bookmark->page - 1);
+ if (bookmark->x != DBL_MIN && bookmark->y != DBL_MIN) {
+ GtkAdjustment* hadjustment = gtk_scrolled_window_get_hadjustment(GTK_SCROLLED_WINDOW(zathura->ui.session->gtk.view));
+ GtkAdjustment* vadjustment = gtk_scrolled_window_get_vadjustment(GTK_SCROLLED_WINDOW(zathura->ui.session->gtk.view));
+ zathura_adjustment_set_value_from_ratio(hadjustment, bookmark->x);
+ zathura_adjustment_set_value_from_ratio(vadjustment, bookmark->y);
+ zathura_document_set_current_page_number(zathura->document, bookmark->page - 1);
+ statusbar_page_number_update(zathura);
+ } else {
+ page_set(zathura, bookmark->page - 1);
+ }
zathura_jumplist_add(zathura);

return true;
diff --git a/database-plain.c b/database-plain.c
index 0282b71..fdb69e6 100644
--- a/database-plain.c
+++ b/database-plain.c
@@ -328,7 +328,18 @@ plain_add_bookmark(zathura_database_t* db, const char* file,
}

char* name = prepare_filename(file);
- g_key_file_set_integer(priv->bookmarks, name, bookmark->id, bookmark->page);
+ char* val_list[] = { g_strdup_printf("%d", bookmark->page),
+ g_ascii_dtostr(g_malloc(G_ASCII_DTOSTR_BUF_SIZE), G_ASCII_DTOSTR_BUF_SIZE, bookmark->x),
+ g_ascii_dtostr(g_malloc(G_ASCII_DTOSTR_BUF_SIZE), G_ASCII_DTOSTR_BUF_SIZE, bookmark->y) };
+
+ gsize num_vals = sizeof(val_list)/sizeof(char *);
+
+ g_key_file_set_string_list(priv->bookmarks, name, bookmark->id, (const char**)val_list, num_vals);
+
+ for (unsigned int i = 0; i < num_vals; ++i) {
+ g_free(val_list[i]);
+ }
+
g_free(name);

zathura_db_write_key_file_to_file(priv->bookmark_path, priv->bookmarks);
@@ -337,8 +348,7 @@ plain_add_bookmark(zathura_database_t* db, const char* file,
}

static bool
-plain_remove_bookmark(zathura_database_t* db, const char* file, const char*
- GIRARA_UNUSED(id))
+plain_remove_bookmark(zathura_database_t* db, const char* file, const char* id)
{
zathura_plaindatabase_private_t* priv = ZATHURA_PLAINDATABASE_GET_PRIVATE(db);
if (priv->bookmarks == NULL || priv->bookmark_path == NULL) {
@@ -347,12 +357,13 @@ plain_remove_bookmark(zathura_database_t* db, const char* file, const char*

char* name = prepare_filename(file);
if (g_key_file_has_group(priv->bookmarks, name) == TRUE) {
- g_key_file_remove_group(priv->bookmarks, name, NULL);
+ if (g_key_file_remove_key(priv->bookmarks, name, id, NULL) == TRUE) {

- zathura_db_write_key_file_to_file(priv->bookmark_path, priv->bookmarks);
- g_free(name);
+ zathura_db_write_key_file_to_file(priv->bookmark_path, priv->bookmarks);
+ g_free(name);

- return true;
+ return true;
+ }
}
g_free(name);

@@ -377,21 +388,37 @@ plain_load_bookmarks(zathura_database_t* db, const char* file)
zathura_bookmarks_compare, (girara_free_function_t)
zathura_bookmark_free);

- gsize length;
- char** keys = g_key_file_get_keys(priv->bookmarks, name, &length, NULL);
+ gsize num_keys;
+ char** keys = g_key_file_get_keys(priv->bookmarks, name, &num_keys, NULL);
if (keys == NULL) {
girara_list_free(result);
g_free(name);
return NULL;
}

- for (gsize i = 0; i < length; i++) {
+ char **val_list = NULL;
+ gsize num_vals = 0;
+
+ for (gsize i = 0; i < num_keys; i++) {
zathura_bookmark_t* bookmark = g_malloc0(sizeof(zathura_bookmark_t));

bookmark->id = g_strdup(keys[i]);
- bookmark->page = g_key_file_get_integer(priv->bookmarks, name, keys[i], NULL);
+ val_list = g_key_file_get_string_list(priv->bookmarks, name, keys[i], &num_vals, NULL);
+
+ bookmark->page = atoi(val_list[0]);
+
+ if (num_vals == 3) {
+ bookmark->x = g_ascii_strtod(val_list[1], NULL);
+ bookmark->y = g_ascii_strtod(val_list[2], NULL);
+ } else if (num_vals == 1) {
+ bookmark->x = DBL_MIN;
+ bookmark->y = DBL_MIN;
+ } else {
+ girara_debug("This must be a BUG");
+ }

girara_list_append(result, bookmark);
+ g_strfreev(val_list);
}

g_free(name);
@@ -579,7 +606,7 @@ zathura_db_write_key_file_to_file(const char* file, GKeyFile* key_file)
}

/* open file */
- int fd = open(file, O_RDWR);
+ int fd = open(file, O_RDWR | O_TRUNC);
if (fd == -1) {
g_free(content);
return;
diff --git a/database-sqlite.c b/database-sqlite.c
index 44829ed..a35d66a 100644
--- a/database-sqlite.c
+++ b/database-sqlite.c
@@ -146,6 +146,10 @@ sqlite_db_init(ZathuraSQLDatabase* db, const char* path)
static const char SQL_FILEINFO_ALTER2[] =
"ALTER TABLE fileinfo ADD COLUMN first_page_column INTEGER;";

+ static const char SQL_BOOKMARK_ALTER[] =
+ "ALTER TABLE bookmarks ADD COLUMN hadj_ratio FLOAT;"
+ "ALTER TABLE bookmarks ADD COLUMN vadj_ratio FLOAT;";
+
static const char SQL_HISTORY_INIT[] =
"CREATE TABLE IF NOT EXISTS history ("
"time TIMESTAMP,"
@@ -192,6 +196,15 @@ sqlite_db_init(ZathuraSQLDatabase* db, const char* path)
}
}

+ data_type = NULL;
+ if (sqlite3_table_column_metadata(session, NULL, "bookmarks", "hadj_ratio", &data_type, NULL, NULL, NULL, NULL) != SQLITE_OK &&
+ sqlite3_table_column_metadata(session, NULL, "bookmarks", "vadj_ratio", &data_type, NULL, NULL, NULL, NULL) != SQLITE_OK) {
+ girara_debug("old database table layout detected; updating ...");
+ if (sqlite3_exec(session, SQL_BOOKMARK_ALTER, NULL, 0, NULL) != SQLITE_OK) {
+ girara_warning("failed to update database table layout");
+ }
+ }
+
priv->session = session;
}

@@ -241,7 +254,7 @@ sqlite_add_bookmark(zathura_database_t* db, const char* file,
zathura_sqldatabase_private_t* priv = ZATHURA_SQLDATABASE_GET_PRIVATE(db);

static const char SQL_BOOKMARK_ADD[] =
- "REPLACE INTO bookmarks (file, id, page) VALUES (?, ?, ?);";
+ "REPLACE INTO bookmarks (file, id, page, hadj_ratio, vadj_ratio) VALUES (?, ?, ?, ?, ?);";

sqlite3_stmt* stmt = prepare_statement(priv->session, SQL_BOOKMARK_ADD);
if (stmt == NULL) {
@@ -250,7 +263,10 @@ sqlite_add_bookmark(zathura_database_t* db, const char* file,

if (sqlite3_bind_text(stmt, 1, file, -1, NULL) != SQLITE_OK ||
sqlite3_bind_text(stmt, 2, bookmark->id, -1, NULL) != SQLITE_OK ||
- sqlite3_bind_int(stmt, 3, bookmark->page) != SQLITE_OK) {
+ sqlite3_bind_int(stmt, 3, bookmark->page) != SQLITE_OK ||
+ sqlite3_bind_double(stmt, 4, bookmark->x) != SQLITE_OK ||
+ sqlite3_bind_double(stmt, 5, bookmark->y) != SQLITE_OK) {
+
sqlite3_finalize(stmt);
girara_error("Failed to bind arguments.");
return false;
@@ -295,7 +311,7 @@ sqlite_load_bookmarks(zathura_database_t* db, const char* file)
zathura_sqldatabase_private_t* priv = ZATHURA_SQLDATABASE_GET_PRIVATE(db);

static const char SQL_BOOKMARK_SELECT[] =
- "SELECT id, page FROM bookmarks WHERE file = ?;";
+ "SELECT id, page, hadj_ratio, vadj_ratio FROM bookmarks WHERE file = ?;";

sqlite3_stmt* stmt = prepare_statement(priv->session, SQL_BOOKMARK_SELECT);
if (stmt == NULL) {
@@ -316,6 +332,13 @@ sqlite_load_bookmarks(zathura_database_t* db, const char* file)

bookmark->id = g_strdup((const char*) sqlite3_column_text(stmt, 0));
bookmark->page = sqlite3_column_int(stmt, 1);
+ bookmark->x = sqlite3_column_double(stmt, 2);
+ bookmark->y = sqlite3_column_double(stmt, 3);
+
+ if (bookmark->page > 1) {
+ bookmark->x = bookmark->x == 0.0 ? DBL_MIN : bookmark->x;
+ bookmark->y = bookmark->y == 0.0 ? DBL_MIN : bookmark->y;
+ }

girara_list_append(result, bookmark);
}
--
1.7.10.4
Sebastian Ramacher
2013-06-21 07:51:22 UTC
Permalink
Thanks, applied now.
--
Sebastian Ramacher
Loading...