On Mon, 2009-03-09 at 22:11 +0100, Paweł Blokus wrote:
> I modified the patch to fit the suggestions You gave me on IRC. Now
> the scroll offsets are kept up-to-date all the time.
Thanks for this. Your approach is mostly fine. I've commented in detail
below.
Note for RISC OS, Amiga, BeOS/Haiku, Framebuffer port maintainers:
you'll want to ensure the scroll offsets are reported to the core
correctly once this patch goes in.
> I had problems with the scroll-child event in GtkScrolledWindow not
> getting called on any kind of scroll so I used the value-changed
> events of the two scrollbars.
I've little to no knowledge of the intricacies of GTK -- this is one for
Rob or Mike. (Similarly, below, I've not commented on your GTK frontend
changes at all)
> There is one thing that is still bothering me - doesn't calling the
> callback function every time you scroll have an impact on scroll
> efficiency?
In comparison to performing the required redraw, setting two integers in
a struct somewhere should have no performance impact at all.
> Index: desktop/history_core.c
> ===================================================================
> --- desktop/history_core.c (wersja 6748)
> +++ desktop/history_core.c (kopia robocza)
> @@ -29,6 +29,7 @@
> #include "content/content.h"
> #include "content/urldb.h"
> #include "css/css.h"
> +#include "desktop/browser.h"
> #include "desktop/gui.h"
> #include "desktop/history_core.h"
> #include "desktop/plotters.h"
> @@ -63,6 +64,8 @@
> int x; /**< Position of node. */
> int y; /**< Position of node. */
> struct bitmap *bitmap; /**< Thumbnail bitmap, or 0. */
> + int sx; /**< X scroll offset. */
> + int sy; /**< Y scroll offset. */
> };
>
> /** History tree for a window. */
> @@ -259,6 +262,8 @@
> entry->forward = entry->forward_pref = entry->forward_last = 0;
> entry->children = 0;
> entry->bitmap = 0;
> + entry->sx = 0;
> + entry->sy = 0;
> if (history->current) {
> if (history->current->forward_last)
> history->current->forward_last->next = entry;
> @@ -755,3 +760,58 @@
>
> return 0;
> }
> +
> +/**
> + * Save in the history the scroll offsets of the current page
> + * \param bw browser window with the page
> + */
> +
> +void history_set_current_scroll(struct browser_window *bw)
Could this not be void history_set_current_scroll(struct history
*history, int sx, int sy) ?
> +{
> + int sx, sy;
> + struct history *history = bw->history;
> +
> + if (!history || !history->current)
> + return;
> +
> + gui_window_get_scroll(bw->window, &sx, &sy);
With the API change suggested above, this upcall is redundant, as the
frontend will provide the scroll offsets directly.
> + history->current->sx = sx;
> + history->current->sy = sy;
> +}
> +
> +/**
> + * Load from the history the scroll offsets of the current page
> + * \param bw browser window with the page
> + * \param sx updated to x offset
> + * \param sy updated to y offset
> + * \return true on success
> + */
> +
> +bool history_get_current_scroll(struct browser_window *bw, int *sx, int *sy)
Similarly, s/struct browser_window *bw/struct history *history/
> +{
> + struct history *history = bw->history;
> +
> + if (!history || !history->current)
> + return false;
> +
> + *sx = history->current->sx;
> + *sy = history->current->sy;
> + return true;
> +}
> +
> +/**
> + * Clear the scroll offsets as if the page was never visited
> + * \param bw browser window with the page
> + */
> +
> +void history_clear_current_scroll(struct browser_window *bw)
s/struct browser_window *bw/struct history *history/
> +{
> + struct history *history = bw->history;
> +
> + if (!history || !history->current)
> + return;
> +
> + history->current->sx = -1;
> + history->current->sy = -1;
> +}
> +
> Index: desktop/history_core.h
> ===================================================================
> --- desktop/history_core.h (wersja 6748)
> +++ desktop/history_core.h (kopia robocza)
> @@ -46,5 +46,7 @@
> bool history_click(struct browser_window *bw, struct history *history,
> int x, int y, bool new_window);
> const char *history_position_url(struct history *history, int x, int y);
> -
> +void history_set_current_scroll(struct browser_window *bw);
> +bool history_get_current_scroll(struct browser_window *bw, int *sx, int *sy);
> +void history_clear_current_scroll(struct browser_window *bw);
As above.
> #endif
> Index: desktop/browser.c
> ===================================================================
> --- desktop/browser.c (wersja 6748)
> +++ desktop/browser.c (kopia robocza)
> @@ -347,7 +347,8 @@
> if (same_url && !post_urlenc && !post_multipart &&
> !strchr(url2, '?')) {
> free(url2);
> - browser_window_update(bw, false);
> + history_clear_current_scroll(bw);
> + browser_window_update(bw);
The clearing here is to ensure the fragment identifier wins, right?
> @@ -762,11 +763,10 @@
> * \param scroll_to_top move view to top of page
> */
>
> -void browser_window_update(struct browser_window *bw,
> - bool scroll_to_top)
> +void browser_window_update(struct browser_window *bw)
> {
> struct box *pos;
> - int x, y;
> + int x, y, sx, sy;
>
> if (!bw->current_content)
> return;
> @@ -778,17 +778,21 @@
>
> gui_window_update_extent(bw->window);
>
> - if (scroll_to_top)
> - gui_window_set_scroll(bw->window, 0, 0);
> -
> - /* todo: don't do this if the user has scrolled */
> + if (!history_get_current_scroll(bw, &sx, &sy))
> + sx = -1;
> +
> + /* if the page was scrolled before return to that position */
> + if (sx != -1) {
> + gui_window_set_scroll(bw->window, sx, sy);
> /* if frag_id exists, then try to scroll to it */
> - if (bw->frag_id && bw->current_content->type == CONTENT_HTML) {
> + } else if (bw->frag_id && bw->current_content->type == CONTENT_HTML) {
> if ((pos = box_find_by_id(bw->current_content->data.html.layout,
> bw->frag_id)) != 0) {
> box_coords(pos, &x, &y);
> gui_window_set_scroll(bw->window, x, y);
> }
> }
> + else
Small nit: make this "} else"
J.
