John-Mark Bell wrote:
> On Fri, 2009-07-31 at 19:14 +0200, Mark wrote:
>
>> John-Mark Bell wrote:
>>
> The correct way to parse @rel is to tokenise it using whitespace as the
> token separator. If any token is "icon", then treat it as an icon
> reference. This means that "apple-touch-icon" will be ignored (rightly
> so, imo).
>
I'd say recognise it when it's the only possibility found; the trouble
being that as was said at the time the alternative of looking for a
default location favicon *then* parsing the page for a reference is
wasting time
> As for multiple links, take the last one specified.
>
well now I've simplified the code, a simple scoring system should work
reasonably don't you think?
> Yes. If you're writing binary data, open files in binary mode.
>
well now I've made that code conditional; the comment was in case a bug
ever arose though testing seemed to have raised none
>> save_complete.h; as I recall, riscos sets the filetype *after* saving
>> for the base html document, so needs an additional call to set the
>> filetype
>>
> It may well do, but it doesn't need a separate function call to do that.
> It can equally well do it when saving the base html document.
>
for a reason perhaps obscure to non-riscos people, when the
save_complete code was written, the default filetype sent in
save_complete_gui_save is F79, it would seem for components, while the
main page is saved from libxml, needs its type changed afterwards to FAF
> That wasn't clear. Please document this before someone else gets
> confused.
>
I already did :-)
> you didn't factor in the length of "gtk default theme\n"
No, honestly, I already did that too :-D
>> currently it is although it's a delicate task of making sure the calls
>> are properly ordered; additional condition added
>>
> The word "delicate" worries me.
>
try to get more sleep at night :-) as I say I've added a condition so
now it is far less delicate :-D
> Let me reiterate, in case it wasn't already clear:
>
I've already understood, already acted accordingly; no need; thanks
though really :-)
> Please update the comment appropriately, then.
>
I already did
> Please use #defines instead of magic numbers. At least that way, you
> have an opportunity to make the defined name meaningful. "111" is not.
>
well apparently Fred Truman found it a very superstitious number :-)
defines added
> You call a function that can return NULL on failure.
>
> Then pass the result into another function without checking for this.
> At the very least, this needs some commentary explaining why it's safe
> to do so.
>
hmm, given how well documented gtk functions are, I would hope I could
let the reader check that in the manual normally; however comment added
> Why are they global? Is there any reason they can't be local variables
> in the function(s) that use them? Macros should operate only on their
> parameters.
>
understood, Macros amended accordingly
> why is it forward-declared here? It should be in a header.
>
it looks as though one of the 5 functions was static indeed, keyword added
> "For now" can easily turn into behaviour that remains unchanged for
> years.
>
I really feel as though the search code says nothing much to me; I've
tried to connect as much as possible of it although that is connecting
code stubs; connecting *to* it is a task as I say for whoever really
appreciates much of a purpose to a recent searches list at all;
> Well, it's the implementation of searching the web, not free text within
> a document.
>
quite right; I seem to have picked up a 24 hour bug Friday [luckily I
recovered very quickly] so I may have overlooked that, thanks
> Simpler than what?
>
changed to Enum then :-)
>> quite so; + '\0' - "%s"
>>
> Ugh. That was non-obvious.
>
I know, good huh? :-D documentation added incidentally
>> well the reason search_ico is global rather than being passed is that it may
>> be cached for asynchronous updates of the front
>>
> I really don't like this. It's horribly fragile.
>
Less fragile than it perhaps sounds; rather than needing to tour the
fetchcache code every time it needs to redraw the current search ico,
there is a global reference to it; the idea being that in a perfect
world NetSurf would permanently store search icos, however there is the
matter of copyright so we make the readily accessible cache
>>> It would be significantly better if there were a search object that
>>> contained all the above data. You'd have one such object per search
>>> context (i.e. one per window or tab).
>>>
>>> You'd need some API to create and destroy such objects as well as
>>> changes to all the other APIs to pass the search context around.
>>>
> Additionally, this impact of changing the search API is pretty minimal
> outside search.c
>
then please tinker with the search.c aspect in the gtkmain branch,
unless you want to merge then tinker; as you say the 5-year-old legacy
from riscos is less neat than it could be, my stamina for rearranging it
is limited; I've made 2 structs for now, 1 global 1 static to give it
some semblance of order, it could be a while until it's really neat
unless you are prepared to sort it; I'd rather it was treated as less
essential as I see the overview of merging the branch as quite important
in the sense of NetSurf progressing steadily, than the detail of
improving the search aspect from its neglected state
>>>> + /* check if we need to start a new search or continue an old one */
>>>> + if (!search_string || c != search_content || !search_found->next ||
>>>> + search_prev_case_sens != case_sens ||
>>>> + (case_sens && strcmp(string, search_string) != 0) ||
>>>> + (!case_sens && strcasecmp(string, search_string) != 0)) {
>>>>
>>> Dear lord this is awful. The frontend presumably knows this already, so
>>> can pass it in as a parameter, rather than have this hideous,
>>> potentially broken, mess.
>>>
more seriously looking carefully at that code, that is the point of
migrating the functionality to the core isn't it? that the front
essentially provides buttons while the core manages all of that, hence
the front has no responsibility to do all that kind of checking; I've
got a feeling that when you look at the detail of what needs improving
in the search code, it really is more a case of tidying than making
wholesale changes, the kind of tidying that you do so well while me,
well, let's say I have my moments of tidiness :-D
>>>> Index: desktop/save_complete.c
>>>> ===================================================================
>>>> --- /dev/null 2009-04-16 19:17:07.000000000 +0100
>>>> +++ desktop/save_complete.c 2009-07-10 12:50:20.000000000 +0100
>>>> +#ifdef RISCOS
>>>> + static char pathsep = '.';
>>>> +#else
>>>> + static char pathsep = '/';
>>>> +#endif
>>>>
>>> I don't like this. Nor, for that matter, do I understand why it exists.
>>>
>> save_complete_inventory needs it
>>
> I suggest using url_to_path, instead.
>
I'll look into it
>>>> +/** List of urls seen and saved so far. */
>>>> +static struct save_complete_entry *save_complete_list = 0;
>>>>
>>> I'd prefer this to not be global. Stick it on the stack in the top-level
>>> call, and pass a pointer to it around internally.
>>>
>> optimization?
>>
>
> No. As a global, you have the potential for dangling pointers. There's
> precisely no need for this to be global, so it shouldn't be.
>
I'll look into it
>>>> + gulong signalhandler[2];
>>>>
>>> What is this for? Why 2?
>>>
>> one for window clicks, one for repaints
>>
> Enum, please.
>
sounds involved an enum with 2 values; however consider it done
>> I simply moved them
>>
> So?
>
so what I would write may not be very accurately what they are supposed
to represent; however I'll look into it :-D
>> 2 indices slows it down?
>>
> I seriously doubt it.
>
it seems to me that speed as well as elegance are highly valued in such
kinds of functions; I daresay I would have been criticised either way,
so my own judgment would be to stick to how it is [as now improved to
check for NULL]
>>>> Index: utils/container.c
>>>> ===================================================================
>>>> --- utils/container.c (revision 8438)
>>>> +++ utils/container.c (working copy)
>>>> strncpy((char *)ctx->directory[ctx->entries - 1].filename,
>>>> - (char *)entryname, 16);
>>>> + (char *)entryname, 64);
>>>>
>>> use sizeof()
>>>
>> it all looks a bit delicate to me;
>>
> which is precisely why I suggested using sizeof()
I'll look into it :-)
> I'm not entirely sure why. It's perfectly possible to open a window or
> tab with no content in it. That should have the same effect as
> manufacturing a blank page.
>
not in gtk it isn't; so although there's possibly a way of painting a
blank page rather than seeing the beige canvas, the idea at the time had
been to display a blank page rather than looking at an empty beige canvas
> You seem to be confusing optimisation with writing code that doesn't
> fall over. They're completely orthogonal.
>
that search web redirect is perfectly working code, the idea of it
'falling over' seems to have no connection to what I see, albeit the
style is perhaps undesirable for the very long term; you seem to
consider the possibility of libcurl changing its error messages in a few
years' time as causing imminent instability in code that is the best
alternative that I could see, while although several people have chipped
in, no-one else has provided a more seriously workable suggestion; I
hope we shan't be ditching useful functionality simply for the sake of style
> I suggest, therefore, that you factor out the common parts into a helper
> function.
>
I'll be looking into it :-)
> J.
>
Best regards
Mark
http://www.halloit.com
Key ID 046B65CF
