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/
signature.asc
Description: Digital signature
