8:25 a.m.
Here we go with the next bit. Only tree.h to go, gonna try and sleep though, gnight
> +bool tree_url_load(const char *filename, struct tree *tree,
> + tree_node_user_callback callback, void *callback_data)
> +{
> + xmlDoc *doc;
> + xmlNode *html, *body, *ul;
> + struct node *root;
> +
> + doc = htmlParseFile(filename, "iso-8859-1");
Erm, eww!?!?! iso-8859-1 ?
Why the hell are we not storing all of our data in utf-8 ?
I have no idea
Also url_load and filename? Seems confusing to me.
as mentioned before i changed these to tree_urlfile_load
> +void tree_url_load_directory(xmlNode *ul, struct tree *tree,
> + struct node *directory, tree_node_user_callback callback,
> + void *callback_data)
> +{
> + char *title;
> + struct node *dir;
> + xmlNode *n;
> +
> + assert(ul);
Assert ul is what?
assert(ul != NULL);
> + assert(directory);
Assert directory is what?
assert(directory != NULL);
> + if (!n || strcmp((const char *) n->name, "ul") != 0) {
!n?
I chnaged it to xmlnode and that specifically to
if ((xmlnode == NULL) ||
strcmp((const char *)xmlnode->name, "ul") != 0) {
> +void tree_url_load_entry(xmlNode *li, struct tree *tree,
> + struct node *directory, tree_node_user_callback callback,
> + void *callback_data)
> + for (n = li->children; n; n = n->next) {
n?
xmlnode
> + if (!url1 || !title) {
!url1? !title?
I read that as "not hurl" I am only just complying
if ((url1 == NULL) || (title == NULL)) {
> +xmlNode *tree_url_find_xml_element(xmlNode *node, const char *name)
> +{
> + xmlNode *n;
changed n to xmlnode
> + if (!node)
> + return 0;
Erm, either assert(node != NULL) or compare to NULL and return NULL if not
available. This combination of treating node as a boolean and returning an
integer instead of a pointer truly upsets me.
xmlNode *xmlnode;
if (node == NULL)
return NULL;
> + for (n = node->children;
> + n && !(n->type == XML_ELEMENT_NODE &&
n?
xmlnode
> +bool tree_url_save(struct tree *tree, const char *filename,
> + const char *page_title)
> +{
> + /* Unfortunately the Browse Hotlist format is invalid HTML,
> + * so this is a lie. */
Aah so perhaps we're iso-8859-1 to be browse compatible. Is it time to drop
that crap and store something more efficient, correctly structured and utf-8 ?
possibly but thats a bit outside teh scope of this code review :-/
> + doc->charset = XML_CHAR_ENCODING_UTF8;
> + res = htmlSaveFileEnc(filename, doc, "iso-8859-1");
Urgh, ick, OMG, pain in my eyes as blood once again spewws forth.
yah...not fixed
> +bool tree_url_save_directory(struct node *directory, xmlNode
*node)
> +{
> + if (!ul)
ul is not what?
if (ul == NULL)
return false;
> + for (child = tree_node_get_child(directory); child;
child?
swhat it says...gimmie a clue and i can fix it
> + if (!h4)
h4 is not what?
if (h4 == NULL)
return false;
> +bool tree_url_save_entry(struct node *entry, xmlNode *node)
> +{
> + if (!li)
li is not what?
if (li == NULL)
return false;
> + if (!a)
a is not what?
if (a == NULL)
return false;
> + if (!href)
href is not what?
if (href == NULL)
return false;
> Index: !NetSurf/Resources/de/Messages
I'm going to skip all the messages files because I can't usefully review them at
this time.
> Index: Makefile.sources
Do we need an S_TREEVIEW ?
possibly, dunno what should go in it though
> Index: desktop/tree.c
> +struct node_element_box {
All these structs have comments, but aren't doccommented.
changed
> +struct node *tree_create_folder_node(struct tree *tree, struct node *parent,
> + const char *title, bool editable, bool retain_in_memory,
> + bool deleted)
> +{
> struct node *node;
>
> + assert(title);
assert that title is what?
you know you can go off people? changed
> +struct node *tree_create_leaf_node(struct tree *tree, struct node *parent,
> + const char *title, bool editable, bool retain_in_memory,
> + bool deleted)
> +{
> + struct node *node;
>
> + assert(title);
Assert title is what?
o/~ you can leave it all behind and sail and sail to Lahaina,
o/~ Just like the missionaries did, so many years ago
assert(title != NULL);
> +void tree_link_node(struct tree *tree, struct node *link,
struct node *node,
> + bool before)
> +{
>
> + struct node *parent;
> + bool sort = false;
> +
> + assert(link);
> assert(node);
Assert that these are what?
A rubber duckky!
assert(link != NULL);
assert(node != NULL);
> + if ((!link->folder) || (before)) {
link->folder is not what?
A spatula!
if ((link->folder == 0) || (before)) {
> /**
> + * Gets the redraw property of the given tree.
But what *is* the redraw property? It makes no sense.
nope, swhat it does though
> /**
> + * Sets an icon for a node
> *
> + * \param tree The tree to which node belongs, may be NULL
> + * \param node The node for which the icon is set
> + * \param icon the image to use
> + */
An indication of who owns the handle, is it cloned, blahblahblah needed here.
Millenium hand and shrimp
I have no bloody idea...I cannot figure out tree_update_node_element() intentions
I *think* it takes ownership...someone help?
> +void tree_set_node_sort_function(struct tree *tree, struct node
*node,
> + int (*sort) (struct node *, struct node *))
> +{
> + while (child) {
While child is what?
while (child != NULL) {
> /**
> + * Inserts a node into the correct place according to the parents sort function
parent's
changed
> +void tree_sort_insert(struct node *parent, struct node *node)
> + while (after && parent->sort(node, after) == -1)
while after is what?
while ((after != NULL) &&
(parent->sort(node, after) == -1))
after = after->previous;
> + if (parent->child)
if parent->child is what?
if (parent->child != NULL) {
parent->child->previous = node;
}
> +void tree_update_node_element(struct tree *tree, struct node_element *element,
> + const char *text, void *bitmap)
> +{
> + assert(element);
assert element is what?
assert(element != NULL);
> +/**
> + * Returns true if the tree is currently being edited
Returns whether the current tree is being edited at this time.
changed
> + return tree->editing == NULL ? false:true;
If you're going to space the ? then also space the :
changed
> +void tree_draw_node(struct tree *tree, struct node *node,
> + int tree_x, int tree_y,
> + int clip_x, int clip_y, int clip_width, int clip_height) {
My only concern with this function is that it has sufficient nested loops that
the code ends up smoosed up against the RH margin.
it is somewhat scary, yes
> +bool tree_mouse_action(struct tree *tree, browser_mouse_state
mouse, int x,
> + int y)
This function is huge and could do with being refactored at some point.
indeedy
> + /* TODO: the tree window has to scroll the tree when
> + mouse reaches border while dragging this isn't
> + solved for the browser window too.
> + */
/** @todo please
changed
> +void tree_handle_node_changed(struct tree *tree, struct node
*node,
> + bool recalculate_sizes, bool expansion)
> +{
> + int width, height, tree_height;
> +
> assert(node);
assert node is what?
assert(node != NULL);
> +void tree_recalculate_node_sizes(struct tree *tree, struct node
*node,
> + bool recalculate_sizes)
> +{
> + assert(node);
assert node is what?
assert(node != NULL);
> +hlcache_handle *tree_load_icon(const char *name)
> +{
> + char *url = NULL;
> + const char *icon_url = NULL;
> + int len;
> + hlcache_handle *c;
> +
> + /* TODO: something like bitmap_from_disc is needed here */
/** @todo
changed
> +
> + if (!strncmp(name, "file://", 7)) {
> + icon_url = name;
Erm? I'd prefer it if this took a "bool prepend_path" or similar, rather
than
this hideousness. Not least of which, in the future, we may want to retrieve
icons etc from a resources: or theme: scheme.
it does smell lots...can we have a decision on this?
> + /* Fetch the icon */
> + hlcache_handle_retrieve(icon_url, 0, 0, 0,
> + tree_icon_callback, 0, 0, 0, &c);
error check?
o/~ Well, my time went to quickly, I went lickety-splitly out to my old fifty-five
I added teh error check too!
--
Regards Vincent
http://www.kyllikki.org/