On Wed, 2009-07-01 at 14:41 +0100, John-Mark Bell wrote: Overall, this is far improved. The API now appears to be fairly logical, which is good. Further comments below.
> Index: desktop/history_global_core.c > =================================================================== > --- /dev/null 2009-04-16 19:17:07.000000000 +0100 > +++ desktop/history_global_core.c 2009-07-01 14:22:40.000000000 +0100 > @@ -0,0 +1,418 @@ > +/* > + * Copyright 2005 Richard Wilson <[email protected]> > + * Copyright 2009 Paul Blokus <[email protected]> > + * > + * This file is part of NetSurf, http://www.netsurf-browser.org/ > + * > + * NetSurf is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; version 2 of the License. > + * > + * NetSurf is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program. If not, see <http://www.gnu.org/licenses/>. > + */ > + > + > +#include <stdlib.h> > + > +#include "content/urldb.h" > +#include "desktop/browser.h" > +#include "desktop/history_global_core.h" > +#include "desktop/options.h" > +#include "desktop/plotters.h" > +#include "desktop/tree.h" > +#include "desktop/tree_url_node.h" > +#include "utils/messages.h" > +#include "utils/utils.h" > +#include "utils/log.h" > + > +#define MAXIMUM_BASE_NODES 16 > +#define GLOBAL_HISTORY_RECENT_URLS 16 > +#define MAXIMUM_URL_LENGTH 1024 Can we please lose this static limit? > +bool history_global_initialise_nodes() Needs void parameter list. > +// > +// /** > +// * Saves the global history's recent URL data. > +// */ > +// void history_global_save(void) > +// { > +// FILE *fp; > +// int i; > +// > +// /* save recent URLs */ > +// fp = fopen(option_recent_file, "w"); > +// if (!fp) > +// LOG(("Failed to open file '%s' for writing", > +// option_recent_file)); > +// else { > +// for (i = global_history_recent_count - 1; i >= 0; i--) > +// if (strlen(global_history_recent_url[i]) < > +// MAXIMUM_URL_LENGTH) > +// fprintf(fp, "%s\n", > +// global_history_recent_url[i]); > +// fclose(fp); > +// } > +// } > +// I assume this will be reinstated? > +/** > + * Adds an URL to the recently used list > + * > + * \param url the URL to add (copied) > + */ > +void global_history_add_recent(const char *url) > +{ > + int i; > + int j = -1; > + char *current; > + > + /* try to find a string already there */ > + for (i = 0; i < global_history_recent_count; i++) > + if (global_history_recent_url[i] && > + !strcmp(global_history_recent_url[i], url)) > + j = i; > + > + /* already at head of list */ > + if (j == 0) > + return; > + > + if (j < 0) { > + /* add to head of list */ > + free(global_history_recent_url[ > + GLOBAL_HISTORY_RECENT_URLS - 1]); > + memmove(&global_history_recent_url[1], > + &global_history_recent_url[0], > + (GLOBAL_HISTORY_RECENT_URLS - 1) * > + sizeof(char *)); > + global_history_recent_url[0] = strdup(url); > + global_history_recent_count++; > + if (global_history_recent_count > GLOBAL_HISTORY_RECENT_URLS) > + global_history_recent_count = > + GLOBAL_HISTORY_RECENT_URLS; > + /* TODO: This would be best taken care of in the front end specific code > + as it seems to be necessary on risc os only */ > + /* if (global_history_recent_count == 1) > + ro_gui_window_prepare_navigate_all(); */ Probably. However, the frontend needs some way of being notified about this. I'm not entirely sure why this exists here, btw. > Index: desktop/history_global_core.h > =================================================================== > --- /dev/null 2009-04-16 19:17:07.000000000 +0100 > +++ desktop/history_global_core.h 2009-07-01 14:22:40.000000000 +0100 > + > +extern struct tree *global_history_tree; What requires access to this? Might it be better to hide it and provide accessors/mutators for the things that are needed? > Index: desktop/tree_url_node.c > =================================================================== > --- /dev/null 2009-04-16 19:17:07.000000000 +0100 > +++ desktop/tree_url_node.c 2009-07-01 14:22:43.000000000 +0100 > @@ -0,0 +1,187 @@ > +/* > + * Copyright 2005 Richard Wilson <[email protected]> > + * Copyright 2009 Paul Blokus <[email protected]> > + * > + * This file is part of NetSurf, http://www.netsurf-browser.org/ > + * > + * NetSurf is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; version 2 of the License. > + * > + * NetSurf is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program. If not, see <http://www.gnu.org/licenses/>. > + */ > + > +/** \file > + * Creation of URL nodes with use of trees public API (implementation) > + */ > + > + > +#include <assert.h> > + > +#include "content/urldb.h" > +#include "desktop/tree_url_node.h" > +#include "utils/messages.h" > +#include "utils/utils.h" > + > +/** > + * Creates a tree entry for a URL, and links it into the tree > + * > + * \param parent the node to link to > + * \param url the URL (copied) > + * \param data the URL data to use > + * \param title the custom title to use > + * \return the node created, or NULL for failure > + */ > +struct node *tree_create_URL_node(struct tree *tree, struct node *parent, > + const char *url, const struct url_data *data, const char *title, > + tree_node_user_callback user_callback, void *callback_data) > +{ > + struct node *node; > + struct node_element *element; > + char *title_cp; > + > + assert(data); > + > + title_cp = strdup(squash_whitespace(title ? title : url)); This leaks -- squash_whitespace returns an object allocated on the heap. > + node = tree_create_leaf_node(tree, parent, title_cp, true, false, false, > + TREE_ELEMENT_TITLE); > + if (!node) > + return NULL; Need to free(title_cp) on error. > Index: desktop/tree_url_node.h > =================================================================== > --- /dev/null 2009-04-16 19:17:07.000000000 +0100 > +++ desktop/tree_url_node.h 2009-07-01 14:22:43.000000000 +0100 > @@ -0,0 +1,50 @@ > +/* > + * Copyright 2005 Richard Wilson <[email protected]> > + * Copyright 2009 Paul Blokus <[email protected]> > + * > + * This file is part of NetSurf, http://www.netsurf-browser.org/ > + * > + * NetSurf is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; version 2 of the License. > + * > + * NetSurf is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program. If not, see <http://www.gnu.org/licenses/>. > + */ > + > +/** \file > + * Creation of URL nodes with use of trees public API > + */ > + > +#ifndef _NETSURF_DESKTOP_TREE_URL_NODE_H_ > +#define _NETSURF_DESKTOP_TREE_URL_NODE_H_ > + > + > +#include "desktop/tree.h" > + > +#define TREE_ELEMENT_URL 0x01 > +#define TREE_ELEMENT_LAST_VISIT 0x02 > +#define TREE_ELEMENT_VISITS 0x03 > +#define TREE_ELEMENT_TITLE 0x04 > +#define TREE_ELEMENT_THUMBNAIL 0x05 Do these need to be public? > Index: gtk/gtk_scaffolding.c > =================================================================== > --- gtk/gtk_scaffolding.c (revision 8239) > +++ gtk/gtk_scaffolding.c (working copy) > @@ -26,6 +26,7 @@ > #include "content/content.h" > #include "desktop/browser.h" > #include "desktop/history_core.h" > +#include "desktop/history_global_core.h" > #include "desktop/gui.h" > #include "desktop/netsurf.h" > #include "desktop/options.h" > @@ -37,6 +38,7 @@ > #endif > #include "desktop/selection.h" > #include "desktop/textinput.h" > +#include "desktop/tree.h" > #include "gtk/gtk_completion.h" > #include "gtk/dialogs/gtk_options.h" > #include "gtk/dialogs/gtk_about.h" > @@ -50,6 +52,7 @@ > #include "gtk/gtk_schedule.h" > #include "gtk/gtk_tabs.h" > #include "gtk/gtk_throbber.h" > +#include "gtk/gtk_treeview.h" > #include "gtk/gtk_window.h" > #include "gtk/options.h" > #include "render/box.h" > @@ -62,15 +65,6 @@ > > #include "utils/log.h" > > -struct gtk_history_window; > - > -struct gtk_history_window { > - struct gtk_scaffolding *g; > - GtkWindow *window; > - GtkScrolledWindow *scrolled; > - GtkDrawingArea *drawing_area; > -}; > - > struct menu_events { > const char *widget; > GCallback handler; > @@ -276,6 +270,9 @@ > if (g->history_window->window) { > gtk_widget_destroy(GTK_WIDGET(g->history_window->window)); > } > + if (global_history_window.window) { > + gtk_widget_destroy(GTK_WIDGET(global_history_window.window)); > + } > gtk_widget_destroy(GTK_WIDGET(g->window)); > > if (--open_windows == 0) > @@ -1120,9 +1117,18 @@ > > MENUHANDLER(global_history) > { > - gtk_widget_show(GTK_WIDGET(wndHistory)); > - gdk_window_raise(GTK_WIDGET(wndHistory)->window); > + //gtk_widget_show(GTK_WIDGET(wndHistory)); > + //gdk_window_raise(GTK_WIDGET(wndHistory)->window); Are these redundant? If so, lose them. > Index: desktop/tree.c > =================================================================== > --- desktop/tree.c (revision 8239) > +++ desktop/tree.c (working copy) > @@ -1,5 +1,6 @@ > /* > * Copyright 2004 Richard Wilson <[email protected]> > + * Copyright 2009 Paul Blokus <[email protected]> > * > * This file is part of NetSurf, http://www.netsurf-browser.org/ > * > @@ -25,657 +26,910 @@ > #include <stdio.h> > #include <stdlib.h> > #include <string.h> > -#include "content/urldb.h" > +#include "content/content.h" > +#include "content/fetchcache.h" > +#include "desktop/browser.h" > #include "desktop/tree.h" > #include "desktop/options.h" > +#include "desktop/plotters.h" > +#include "render/font.h" > #include "utils/log.h" > #include "utils/messages.h" > #include "utils/utils.h" > > + > +struct url_data; > +struct cookie_data; Why does the tree need to know about these? > +typedef enum { > + TREE_NO_DRAG = 0, > + TREE_SELECT_DRAG, > + TREE_MOVE_DRAG > +} tree_drag_type; > + > + > +#define MAXIMUM_URL_LENGTH 1024 Similarly, can we kill this? > +#define NODE_INSTEP 20 > +#define TREE_TEXT_HEIGHT 20 > +#define FURNITURE_COLOUR 0x888888 > +#define ICON_SIZE 16 > + > +struct node; > +struct tree; > + > +struct node_element_box { > + int x; /* X offset from origin */ > + int y; /* Y offset from origin */ > + int width; /* Element width */ > + int height; /* Element height */ > +}; > + > +struct node_element { > + struct node *parent; /* Parent node */ > + node_element_type type; /* Element type */ > + struct node_element_box box; /* Element bounding box */ > + const char *text; /* Text for the element */ > + void *bitmap; /* Bitmap for the element */ > + struct node_element *next; /* Next node element */ > + unsigned int flag; /* Client specified flag for data > + being represented */ Hm. Do these values have to be globally unique? > Index: desktop/tree.h > =================================================================== > --- desktop/tree.h (revision 8239) > +++ desktop/tree.h (working copy) > @@ -1,5 +1,6 @@ > /* > * Copyright 2004 Richard Wilson <[email protected]> > + * Copyright 2009 Paul Blokus <[email protected]> > * > * This file is part of NetSurf, http://www.netsurf-browser.org/ > * > @@ -26,157 +27,100 @@ > #include <stdbool.h> > #include <stdint.h> > > -struct url_data; > -struct cookie_data; > +#include "desktop/browser.h" > +#include "image/bitmap.h" > > -typedef enum { > - TREE_ELEMENT_URL, > - TREE_ELEMENT_ADDED, > - TREE_ELEMENT_LAST_VISIT, > - TREE_ELEMENT_VISITS, > - TREE_ELEMENT_VISITED, > - TREE_ELEMENT_THUMBNAIL, > - TREE_ELEMENT_TITLE, > - TREE_ELEMENT_NAME, > - TREE_ELEMENT_VALUE, > - TREE_ELEMENT_COMMENT, > - TREE_ELEMENT_DOMAIN, > - TREE_ELEMENT_PATH, > - TREE_ELEMENT_EXPIRES, > - TREE_ELEMENT_LAST_USED, > - TREE_ELEMENT_SECURE, > - TREE_ELEMENT_VERSION, > - TREE_ELEMENT_PERSISTENT, > - TREE_ELEMENT_SSL > -} node_element_data; > +/* Tree flags */ > +#define TREE_NO_DRAGS 0x01 > +#define TREE_NO_FURNITURE 0x02 > +#define TREE_SINGLE_SELECT 0x04 > +#define TREE_MOVABLE 0x08 > > -#define NODE_INSTEP 40 > +struct tree; > +struct node; > +struct node_element; > +struct cookie_data; Do we need to declare cookie_data here? > -struct node_sprite; > -struct toolbar; > - > typedef enum { > - NODE_ELEMENT_TEXT, /* <-- Text only */ > - NODE_ELEMENT_TEXT_PLUS_SPRITE, /* <-- Text and sprite */ > - NODE_ELEMENT_THUMBNAIL, /* <-- Bitmap only */ > + NODE_ELEMENT_TEXT, /* Text only */ > + NODE_ELEMENT_TEXT_PLUS_ICON, /* Text and icon */ > + NODE_ELEMENT_BITMAP /* Bitmap only */ > } node_element_type; > > +typedef enum { > + NODE_DELETE_ELEMENT, /* An element of the node is deleted */ > + NODE_LAUNCH /* The node has been launched */ > +} node_msg; > > -struct node_element_box { > - int x; /* <-- X offset from origin */ > - int y; /* <-- Y offset from origin */ > - int width; /* <-- Element width */ > - int height; /* <-- Element height */ > -}; > +typedef void (*tree_start_redraw_callback)(void *data); > +typedef void (*tree_end_redraw_callback)(void *data); > +/* TODO: where do I document callbacks? */ > +typedef bool (*tree_node_user_callback)(node_msg msg, void *user_data, > + void *node_data, unsigned int flag); Here :) > Index: desktop/options.c > =================================================================== > --- desktop/options.c (revision 8239) > +++ desktop/options.c (working copy) > @@ -37,6 +37,7 @@ > #include "css/css.h" > #include "desktop/options.h" > #include "desktop/tree.h" > +#include "desktop/tree_url_node.h" > #include "utils/log.h" > #include "utils/messages.h" > #include "utils/url.h" > @@ -144,6 +145,8 @@ > #else > unsigned int option_min_reflow_period = 25; /* time in cs */ > #endif > +char *option_recent_file = 0; > +char *option_tree_icons_dir = 0; > /** top margin of exported page*/ > int option_margin_top = DEFAULT_MARGIN_TOP_MM; > /** bottom margin of exported page*/ > @@ -243,6 +246,8 @@ > { "scale", OPTION_INTEGER, &option_scale }, > { "incremental_reflow", OPTION_BOOL, &option_incremental_reflow }, > { "min_reflow_period", OPTION_INTEGER, &option_min_reflow_period }, > + { "recent_file", OPTION_STRING, &option_recent_file }, > + { "tree_icons_dir", OPTION_STRING, &option_tree_icons_dir }, > /* Fetcher options */ > { "max_fetchers", OPTION_INTEGER, &option_max_fetchers }, > { "max_fetchers_per_host", > @@ -435,7 +440,8 @@ > xmlDoc *doc; > xmlNode *html, *body, *ul; > struct tree *tree; > - > + struct node *root; > + > doc = htmlParseFile(filename, "iso-8859-1"); > if (!doc) { > warn_user("HotlistLoadError", messages_get("ParsingFail")); > @@ -452,24 +458,16 @@ > return NULL; > } > > - tree = calloc(sizeof(struct tree), 1); > - if (!tree) { > + tree = tree_create(0, 0, 0, NULL, NULL, NULL, &root); > + if (tree == NULL) { > xmlFreeDoc(doc); > warn_user("NoMemory", 0); > return NULL; > } > - tree->root = tree_create_folder_node(NULL, "Root"); > - if (!tree->root) { > - free(tree); > - xmlFreeDoc(doc); > > - return NULL; > - } > + options_load_tree_directory(ul, root); > + tree_set_node_expanded(tree, root, true, false, false); > > - options_load_tree_directory(ul, tree->root); > - tree->root->expanded = true; > - tree_initialise(tree); > - > xmlFreeDoc(doc); > return tree; > } > @@ -522,7 +520,8 @@ > return; > } > > - dir = tree_create_folder_node(directory, title); > + dir = tree_create_folder_node(NULL, directory, title, > + false, false, false, 0); > if (!dir) { > free(title); > > @@ -602,11 +601,20 @@ > if (!data->title) > urldb_set_url_title(url, title); > > - entry = tree_create_URL_node(directory, url, data, title); > + entry = tree_create_URL_node(NULL, directory, url, data, title, NULL, > + NULL); > + if (entry == NULL) { > + /** \todo why isn't this fatal? */ > + warn_user("NoMemory", 0); > + } > if (entry == NULL) { > /** \todo why isn't this fatal? */ > warn_user("NoMemory", 0); > } > + if (entry == NULL) { > + /** \todo why isn't this fatal? */ > + warn_user("NoMemory", 0); > + } One check for this is enough, surely? > xmlFree(title); > free(url); > @@ -684,11 +692,11 @@ > return false; > } > > - if (!options_save_tree_directory(tree->root, body)) { > - warn_user("NoMemory", 0); > - xmlFreeDoc(doc); > - return false; > - } > + if (!options_save_tree_directory(tree_get_root(tree), body)) { > + warn_user("NoMemory", 0); > + xmlFreeDoc(doc); > + return false; > + } > > doc->charset = XML_CHAR_ENCODING_UTF8; > res = htmlSaveFileEnc(filename, doc, "iso-8859-1"); > @@ -710,25 +718,33 @@ > * \param node node to add ul to > * \return true on success, false on memory exhaustion > */ > -bool options_save_tree_directory(struct node *directory, xmlNode *node) { > +bool options_save_tree_directory(struct node *directory, xmlNode *node) > +{ > struct node *child; > + struct node_element *element; > xmlNode *ul, *h4; > + const char *text; > > ul = xmlNewChild(node, NULL, (const xmlChar *) "ul", NULL); > if (!ul) > return false; > > - for (child = directory->child; child; child = child->next) { > - if (!child->folder) { > + for (child = tree_node_get_child(directory); child; > + child = tree_node_get_next(child)) { > + if (!tree_node_is_folder(child)) { > /* entry */ > if (!options_save_tree_entry(child, ul)) > return false; > } else { > /* directory */ > /* invalid HTML */ > + element = tree_node_find_element(child, > + TREE_ELEMENT_TITLE); > + text = tree_node_element_get_text(element); > + > h4 = xmlNewTextChild(ul, NULL, > (const xmlChar *) "h4", > - (const xmlChar *) child->data.text); > + (const xmlChar *) text); > if (!h4) > return false; > > @@ -753,21 +769,24 @@ > xmlNode *li, *a; > xmlAttr *href; > struct node_element *element; > + const char *text; > > li = xmlNewChild(node, NULL, (const xmlChar *) "li", NULL); > if (!li) > return false; > > + element = tree_node_find_element(entry, TREE_ELEMENT_TITLE); > + text = tree_node_element_get_text(element); > a = xmlNewTextChild(li, NULL, (const xmlChar *) "a", > - (const xmlChar *) entry->data.text); > + (const xmlChar *) text); > if (!a) > return false; > > - element = tree_find_element(entry, TREE_ELEMENT_URL); > + element = tree_node_find_element(entry, TREE_ELEMENT_URL); > if (!element) > return false; > href = xmlNewProp(a, (const xmlChar *) "href", > - (const xmlChar *) element->text); > + (const xmlChar *) tree_node_element_get_text(element)); > if (!href) > return false; > return true; > Index: amiga/options.h > =================================================================== > --- amiga/options.h (revision 8239) > +++ amiga/options.h (working copy) > @@ -35,7 +35,6 @@ > extern bool option_use_os_pointers; > extern bool option_new_tab_active; > extern bool option_kiosk_mode; > -extern char *option_recent_file; > extern char *option_arexx_dir; > extern char *option_download_dir; > extern bool option_faster_scroll; > @@ -59,7 +58,6 @@ > bool option_use_os_pointers = true; \ > bool option_new_tab_active = false; \ > bool option_kiosk_mode = false; \ > -char *option_recent_file = 0; \ > char *option_arexx_dir = 0; \ > char *option_download_dir = 0; \ > bool option_faster_scroll = true; \ > @@ -83,7 +81,6 @@ > { "os_mouse_pointers", OPTION_BOOL, &option_use_os_pointers}, \ > { "new_tab_is_active", OPTION_BOOL, &option_new_tab_active}, \ > { "kiosk_mode", OPTION_BOOL, &option_kiosk_mode}, \ > -{ "recent_file", OPTION_STRING, &option_recent_file }, \ > { "arexx_dir", OPTION_STRING, &option_arexx_dir }, \ > { "download_dir", OPTION_STRING, &option_download_dir }, \ > { "faster_scroll", OPTION_BOOL, &option_faster_scroll}, \ > Index: amiga/tree.c > =================================================================== > --- amiga/tree.c (revision 8239) > +++ amiga/tree.c (working copy) > @@ -451,7 +451,7 @@ > if(!node->parent) flags |= LBFLG_HIDDEN; > > switch (element->type) { > - case NODE_ELEMENT_TEXT_PLUS_SPRITE: > +/* case NODE_ELEMENT_TEXT_PLUS_SPRITE:*/ Should this be replaced with NODE_ELEMENT_TEXT_PLUS_ICON? > case NODE_ELEMENT_TEXT: > if (lbnode = AllocListBrowserNode(3, > LBNA_UserData,node, J.
