Hi,

That looks quite good to me :) I attach a couple of patches changing some
things, mainly simplifying the initial range setup. In detail:

  * added / fixed some comments

  * Simplified initial range setup. As we don't use the jumplist for state, we
    only need to look back one jump. I mean, when we start a new bisect, we do
    it between current page (which may or may not be current jump) and previous
    jump.

  * Save jump before page_set as Marwan suggests. This is relevant for the
    initial step, in the case current page != current jump.

    We don't have to check jumps -1 and -3 or anything like that though. We only
    need the previous jump, and only when beginning a bisect to set the initial
    range. In this case the previous jump is set by the user, and not the bisect
    algorithm, so we are good.

  * get rid of the cb_view_hadjustment_changed. This was there to recenter the
    page when zoom-center is enabled. Now, since the jumplist stores
    adjustments, this should be done right after every page_set, otherwise
    duplicte jumps may appear. I think it is better to call
    cb_view_hadjustment_changed from page_set. I attach a separate patch doing
    that.

The patches:
  page-set-fix.patch:        call cb_view_hadkustment_changed from page_set
  bisect-on-master.diff:     simplified bisect. diff on top of master
  bisect-on-sebastian.diff:  diff on top of Sebastian code, so it's easier to 
see
                             what I changed.

The order to apply them is first the page-set-fix and then the bisect-on-master.

Thank you both Sebastian and Marwan for taking the time to improve this!

Abdó Roig.

>From 5ed40a98551cb0492d801cfb2ec086156e62538f Mon Sep 17 00:00:00 2001
From: Abdo Roig-Maranges <abdo.r...@gmail.com>
Date: Sat, 6 Jul 2013 21:55:35 +0200
Subject: [PATCH] refresh horizontal position in page_set

After page_set cb_view_hadjustment_changed must be called so that when
zoom-center is enabled, the page is recentered.
---
 shortcuts.c | 7 -------
 zathura.c   | 3 +++
 2 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/shortcuts.c b/shortcuts.c
index bfb4cf3..7ec9fa1 100644
--- a/shortcuts.c
+++ b/shortcuts.c
@@ -325,9 +325,6 @@ sc_goto(girara_session_t* session, girara_argument_t* argument, girara_event_t*
     page_set(zathura, zathura_document_get_number_of_pages(zathura->document) - 1);
   }
 
-  /* adjust horizontal position */
-  GtkAdjustment* hadjustment = gtk_scrolled_window_get_hadjustment(GTK_SCROLLED_WINDOW(session->gtk.view));
-  cb_view_hadjustment_changed(hadjustment, zathura);
   zathura_jumplist_add(zathura);
 
   return false;
@@ -464,10 +461,6 @@ sc_navigate(girara_session_t* session, girara_argument_t* argument,
 
   page_set(zathura, new_page);
 
-  /* adjust horizontal position */
-  GtkAdjustment* hadjustment = gtk_scrolled_window_get_hadjustment(GTK_SCROLLED_WINDOW(session->gtk.view));
-  cb_view_hadjustment_changed(hadjustment, zathura);
-
   return false;
 }
 
diff --git a/zathura.c b/zathura.c
index 0165e40..d4d638d 100644
--- a/zathura.c
+++ b/zathura.c
@@ -1031,6 +1031,9 @@ page_set(zathura_t* zathura, unsigned int page_id)
   zathura_adjustment_set_value(view_hadjustment, offset.x);
   zathura_adjustment_set_value(view_vadjustment, offset.y);
 
+  /* refresh horizontal adjustment, to honor zoom-center */
+  cb_view_hadjustment_changed(view_hadjustment, zathura);
+
   statusbar_page_number_update(zathura);
 
   return true;
-- 
1.8.3.2

diff --git a/shortcuts.c b/shortcuts.c
index 7ec9fa1..244ada1 100644
--- a/shortcuts.c
+++ b/shortcuts.c
@@ -19,6 +19,14 @@
 #include "page-widget.h"
 #include "adjustment.h"
 
+#ifndef MIN
+#define MIN(a,b) (((a)<(b))?(a):(b))
+#endif
+
+#ifndef MAX
+#define MAX(a,b) (((a)>(b))?(a):(b))
+#endif
+
 /* Helper function; see sc_display_link and sc_follow. */
 static bool
 draw_links(zathura_t* zathura)
@@ -779,126 +787,115 @@ sc_bisect(girara_session_t* session, girara_argument_t* argument,
   g_return_val_if_fail(argument != NULL, false);
   g_return_val_if_fail(zathura->document != NULL, false);
 
-  unsigned int number_of_pages, cur_page, prev_page, prev2_page;
-  bool have_prev, have_prev2;
-
-  zathura_jump_t* prev_jump = NULL;
-  zathura_jump_t* prev2_jump = NULL;
-
-  number_of_pages= zathura_document_get_number_of_pages(zathura->document);
-  cur_page = zathura_document_get_current_page_number(zathura->document);
-
-  prev_page = prev2_page = 0;
-  have_prev = have_prev2 = false;
+  const unsigned int num_pages = zathura_document_get_number_of_pages(zathura->document);
+  const unsigned int cur_page = zathura_document_get_current_page_number(zathura->document);
 
   /* process arguments */
   int direction;
-  if (t > 0 && t <= number_of_pages) {
-    /* jump to page t, and bisect between cur_page and t */
-    zathura_jumplist_add(zathura);
-    page_set(zathura, t-1);
-    zathura_jumplist_add(zathura);
-    if (t-1 > cur_page) {
+  if (t > 0 && t <= num_pages) {
+    /* bisect between cur_page and t */
+    t -= 1;
+    if (t == cur_page) {
+      /* nothing to do */
+      return false;
+    }
+    else if (t > cur_page) {
+      zathura->bisect.start = cur_page;
+      zathura->bisect.end = t;
       direction = BACKWARD;
     } else {
+      zathura->bisect.start = t;
+      zathura->bisect.end = cur_page;
       direction = FORWARD;
     }
-
   } else if (argument != NULL)  {
     direction = argument->n;
 
-  } else {
-    return false;
-  }
+    /* setup initial bisect range */
+    zathura_jump_t* jump = zathura_jumplist_current(zathura);
+    if (jump == NULL) {
+      girara_debug("bisecting between first and last page because there are no jumps");
+      zathura->bisect.start = 0;
+      zathura->bisect.end = num_pages - 1;
+
+    } if (jump->page != cur_page || jump->page != zathura->bisect.last_jump) {
+      girara_debug("last jump doesn't match up, starting new bisecting");
+      zathura->bisect.start = 0;
+      zathura->bisect.end = num_pages - 1;
+
+      unsigned int prev_page;
+      if (direction == FORWARD) {
+        prev_page = num_pages - 1;
+      } else {
+        prev_page = 0;
+      }
 
-  cur_page = zathura_document_get_current_page_number(zathura->document);
+      /* check if we have previous jumps */
+      if (zathura_jumplist_has_previous(zathura) == true) {
+        zathura_jumplist_backward(zathura);
+        jump = zathura_jumplist_current(zathura);
+        if (jump != NULL) {
+          prev_page = jump->page;
+        }
+        zathura_jumplist_forward(zathura);
+      }
 
-  if (zathura_jumplist_has_previous(zathura)) {
-    /* If there is a previous jump, get its page */
-    zathura_jumplist_backward(zathura);
-    prev_jump = zathura_jumplist_current(zathura);
-    if (prev_jump) {
-      prev_page = prev_jump->page;
-      have_prev = true;
+      zathura->bisect.start = MIN(prev_page, cur_page);
+      zathura->bisect.end = MAX(prev_page, cur_page);
+      zathura->bisect.last_jump = cur_page;
     }
+  } else {
+    return false;
+  }
 
-    if (zathura_jumplist_has_previous(zathura)) {
-      /* If there is a second previous jump, get its page. */
-      zathura_jumplist_backward(zathura);
-      prev2_jump = zathura_jumplist_current(zathura);
-      if (prev2_jump) {
-        prev2_page = prev2_jump->page;
-        have_prev2 = true;
-      }
-      zathura_jumplist_forward(zathura);
-    }
-    zathura_jumplist_forward(zathura);
+  girara_debug("bisecting between %d and %d, at %d", zathura->bisect.start, zathura->bisect.end, cur_page);
+  if (zathura->bisect.start == zathura->bisect.end) {
+    /* nothing to do */
+    return false;
   }
 
-  /* now, we are back at the initial jump. prev_page and prev2_page contain
-   the pages for previous and second previous jump if they exist. */
+  unsigned int next_page = cur_page;
+  unsigned int next_start = zathura->bisect.start;
+  unsigned int next_end = zathura->bisect.end;
+
+  /* here we have next_start <= next_page <= next_end */
 
-  /* bisect */
+  /* bisect step */
   switch(direction) {
     case FORWARD:
-      if (have_prev && cur_page <= prev_page) {
-        /* add a new jump point */
-        if (cur_page < prev_page) {
-          zathura_jumplist_add(zathura);
-          page_set(zathura, (cur_page + prev_page)/2);
-          zathura_jumplist_add(zathura);
+      if (cur_page != zathura->bisect.end) {
+        next_page = (cur_page + zathura->bisect.end) / 2;
+        if (next_page == cur_page) {
+          ++next_page;
         }
-
-      } else if (have_prev2 && cur_page <= prev2_page) {
-        /* save current position at previous jump point */
-        if (cur_page < prev2_page) {
-          zathura_jumplist_backward(zathura);
-          zathura_jumplist_add(zathura);
-          zathura_jumplist_forward(zathura);
-
-          page_set(zathura, (cur_page + prev2_page)/2);
-          zathura_jumplist_add(zathura);
-        }
-      } else {
-        /* none of prev_page or prev2_page comes after cur_page */
-        zathura_jumplist_add(zathura);
-        page_set(zathura, (cur_page + number_of_pages - 1)/2);
-        zathura_jumplist_add(zathura);
+        next_start = cur_page;
       }
       break;
 
     case BACKWARD:
-      if (have_prev && prev_page <= cur_page) {
-        /* add a new jump point */
-        if (prev_page < cur_page) {
-          zathura_jumplist_add(zathura);
-          page_set(zathura, (cur_page + prev_page)/2);
-          zathura_jumplist_add(zathura);
+      if (cur_page != zathura->bisect.start) {
+        next_page = (cur_page + zathura->bisect.start) / 2;
+        if (next_page == cur_page) {
+          --next_page;
         }
-
-      } else if (have_prev2 && prev2_page <= cur_page) {
-        /* save current position at previous jump point */
-        if (prev2_page < cur_page) {
-          zathura_jumplist_backward(zathura);
-          zathura_jumplist_add(zathura);
-          zathura_jumplist_forward(zathura);
-
-          page_set(zathura, (cur_page + prev2_page)/2);
-          zathura_jumplist_add(zathura);
-        }
-
-      } else {
-        /* none of prev_page or prev2_page comes before cur_page */
-        zathura_jumplist_add(zathura);
-        page_set(zathura, cur_page/2);
-        zathura_jumplist_add(zathura);
+        next_end = cur_page;
       }
       break;
   }
 
-  /* adjust horizontal position */
-  GtkAdjustment* hadjustment = gtk_scrolled_window_get_hadjustment(GTK_SCROLLED_WINDOW(session->gtk.view));
-  cb_view_hadjustment_changed(hadjustment, zathura);
+  if (next_page == cur_page) {
+    /* nothing to do */
+    return false;
+  }
+
+  girara_debug("bisecting between %d and %d, jumping to %d", zathura->bisect.start, zathura->bisect.end, next_page);
+  zathura->bisect.last_jump = next_page;
+  zathura->bisect.start = next_start;
+  zathura->bisect.end = next_end;
+
+  zathura_jumplist_add(zathura);
+  page_set(zathura, next_page);
+  zathura_jumplist_add(zathura);
 
   return false;
 }
diff --git a/zathura.h b/zathura.h
index ac22636..18d3132 100644
--- a/zathura.h
+++ b/zathura.h
@@ -163,6 +163,15 @@ struct zathura_s
     unsigned int size;
     unsigned int num_cached_pages;
   } page_cache;
+
+  /**
+   * Bisect stage
+   */
+  struct {
+    unsigned int last_jump; /**<  */
+    unsigned int start; /**< Bisection range - start */
+    unsigned int end; /**< Bisection range - end */
+  } bisect;
 };
 
 /**
diff --git a/shortcuts.c b/shortcuts.c
index bca7621..244ada1 100644
--- a/shortcuts.c
+++ b/shortcuts.c
@@ -811,68 +811,38 @@ sc_bisect(girara_session_t* session, girara_argument_t* argument,
   } else if (argument != NULL)  {
     direction = argument->n;
 
+    /* setup initial bisect range */
     zathura_jump_t* jump = zathura_jumplist_current(zathura);
     if (jump == NULL) {
       girara_debug("bisecting between first and last page because there are no jumps");
       zathura->bisect.start = 0;
       zathura->bisect.end = num_pages - 1;
-    } else {
-      if (jump->page != cur_page || jump->page != zathura->bisect.last_jump) {
-        girara_debug("last jump doesn't match up, starting new bisecting");
-        zathura->bisect.start = 0;
-        zathura->bisect.end = num_pages - 1;
-
-        /* check if we have previous jumps */
-        if (zathura_jumplist_has_previous(zathura) == true) {
-          zathura_jumplist_backward(zathura);
-          jump = zathura_jumplist_current(zathura);
-
-          unsigned int prev_page = 0;
-          unsigned int prev_page2 = num_pages - 1;
-          if (jump != NULL) {
-            prev_page = jump->page;
-          }
 
-          prev_page2 = prev_page;
-          if (zathura_jumplist_has_previous(zathura) == true) {
-            zathura_jumplist_backward(zathura);
-            jump = zathura_jumplist_current(zathura);
-            if (jump != NULL) {
-              prev_page2 = jump->page;
-            }
-            zathura_jumplist_forward(zathura);
-          }
-          zathura_jumplist_forward(zathura);
-
-          if (prev_page == prev_page2) {
-            if (prev_page > cur_page) {
-              zathura->bisect.end = prev_page;
-            } else {
-              zathura->bisect.start = prev_page;
-            }
-          } else {
-            zathura->bisect.start = MIN(prev_page, prev_page2);
-            zathura->bisect.end = MAX(prev_page, prev_page2);
-          }
-        }
+    } if (jump->page != cur_page || jump->page != zathura->bisect.last_jump) {
+      girara_debug("last jump doesn't match up, starting new bisecting");
+      zathura->bisect.start = 0;
+      zathura->bisect.end = num_pages - 1;
 
-        /* some sanity checks on new bounds */
-        if (cur_page < zathura->bisect.start) {
-          if (direction == FORWARD) {
-            zathura->bisect.start = cur_page;
-          } else {
-            zathura->bisect.start = cur_page;
-            zathura->bisect.end = 0;
-          }
-        } else if (cur_page > zathura->bisect.end) {
-          if (direction == BACKWARD) {
-            zathura->bisect.end = cur_page;
-          } else {
-            zathura->bisect.start = cur_page;
-            zathura->bisect.end = num_pages - 1;
-          }
+      unsigned int prev_page;
+      if (direction == FORWARD) {
+        prev_page = num_pages - 1;
+      } else {
+        prev_page = 0;
+      }
+
+      /* check if we have previous jumps */
+      if (zathura_jumplist_has_previous(zathura) == true) {
+        zathura_jumplist_backward(zathura);
+        jump = zathura_jumplist_current(zathura);
+        if (jump != NULL) {
+          prev_page = jump->page;
         }
+        zathura_jumplist_forward(zathura);
       }
+
+      zathura->bisect.start = MIN(prev_page, cur_page);
+      zathura->bisect.end = MAX(prev_page, cur_page);
+      zathura->bisect.last_jump = cur_page;
     }
   } else {
     return false;
@@ -884,14 +854,13 @@ sc_bisect(girara_session_t* session, girara_argument_t* argument,
     return false;
   }
 
-  /* now, we are back at the initial jump. prev_page and prev2_page contain
-   the pages for previous and second previous jump if they exist. */
-
   unsigned int next_page = cur_page;
   unsigned int next_start = zathura->bisect.start;
   unsigned int next_end = zathura->bisect.end;
 
-  /* bisect */
+  /* here we have next_start <= next_page <= next_end */
+
+  /* bisect step */
   switch(direction) {
     case FORWARD:
       if (cur_page != zathura->bisect.end) {
@@ -924,13 +893,10 @@ sc_bisect(girara_session_t* session, girara_argument_t* argument,
   zathura->bisect.start = next_start;
   zathura->bisect.end = next_end;
 
+  zathura_jumplist_add(zathura);
   page_set(zathura, next_page);
   zathura_jumplist_add(zathura);
 
-  /* adjust horizontal position */
-  GtkAdjustment* hadjustment = gtk_scrolled_window_get_hadjustment(GTK_SCROLLED_WINDOW(session->gtk.view));
-  cb_view_hadjustment_changed(hadjustment, zathura);
-
   return false;
 }
 
_______________________________________________
zathura mailing list
zathura@lists.pwmt.org
http://lists.pwmt.org/mailman/listinfo/zathura

Reply via email to