On Mon, 2009-08-03 at 15:56 +0100, John-Mark Bell wrote:
> On Mon, 2009-08-03 at 15:16 +0200, Mark wrote:
> > John-Mark Bell wrote:
> > > "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;
>
> I'm not entirely sure what you're trying to say here -- can you clarify?
I've not seen any clarification of this, so I'm still confused :)
> > > > > 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
>
> My comments had nothing to do with neatness and everything to do with
> code stability. There are known cases where this code causes crashes
> that are the direct result of global pointers becoming stale. Therefore,
> this code is not appropriate for merging into the core in its present
> state.
I still have grave concerns about the current state of the free text
search, even though it's a little better than before.
I would much prefer to see something like this:
struct search_ctx;
typedef enum {
SEARCH_FLAG_NEW_SEARCH = (1 << 0),
SEARCH_FLAG_CASE_SENSITIVE = (1 << 1),
SEARCH_FLAG_FORWARDS = (1 << 2)
} search_flags_t;
struct search_ctx *search_create_context(struct browser_window *bw);
void search_destroy_context(struct search_ctx *ctx);
void search_text(struct search_ctx *ctx, const char *string,
search_flags_t flags);
Then, have search_ctx defined (privately) as:
struct search_ctx {
struct browser_window *bw; /**< Owning window */
/**
* Cursor for current active search
*/
struct {
unsigned start_idx; /**< Start index of match */
unsigned end_idx; /**< End index of match */
/* These two are for HTML contents only */
struct box *start_box; /**< First box for match */
struct box *end_box; /**< Last box for match */
struct selection *sel; /**< Selection object */
} cursor;
};
The implementation of search would have to change so that it no longer
creates a list of matches upfront, but actually performs the search in
the given direction on the live content data structure.
Each browser_window would have its own search_ctx object, thus removing
the ballache of current_search_window.
Other than the above, and assuming the nits in the other emails I've
sent get fixed, then this branch is ready for merge.
J.
