Re: [PATCH v10 05/13] ref-filter: implement an `align` atom

Karthik Nayak <karthik.188@gmail.com> writes:

Obviously not a problem with this step, but you need a SP before the open brace.

Minor nits on the design. %(align:<width>[,<position>]) would let us write %(align:16)...%(end) and use the "default position", which may be beneficial if one kind of alignment is prevalent (I guess all the internal users left-align?) %(align:<position>,<width>) forces users to spell both out all the time.

Mental note: it is not clear why you need 'end' field in the state. Perhaps it is an indication that the division of labor is poorly designed between the helper that updates the formatting state and the other helper that reflects the formatting state to the final string.

... and I think that is what I see. If this function knows that we are processing %(end), i.e. perform-state-formatting is called for each atom and receives atomv, there wouldn't have to be a code like this.

At the design level, I have a strong suspicion that it is a wrong way to go. It piles more "if (this state bit was left by the previous atom) then do this" on this function and will make an unmanageable mess.

You have a dictionary of all possible atoms somewhere. Why not hook a pointer to the "handler" function (or two) to each element in it, instead of duplicating "this one is special" information down to individual atom instantiations (i.e. atomv) as atomv.modifier_atom bit, an dstructure the caller more like this?

	get_ref_atom_value(info, parse_ref_filter_atom, &atomv);
        if (atomv->pre_handler)
        	atomv->pre_handler(atomv, &state);
	format_quote_value(atomv, &state);
        if (atomv->post_handler)
        	atomv->post_handler(atomv, &state);

Actually, each atom could just have a single handler; an atom like %(refname:short) whose sole effect is to append atomv->s to the state buffer can point a function to do so in its handler.

On the other hand, align atom's handler would push a new state on the stack, marking that it is the one to handle diverted output.

	align_atom_handler(atomv, state)
        {
        	struct format_state *new = push_new_state(state);
		strbuf_init(&new->output);
                new->atend = align_handler;
                new->return_to = atomv; /* or whatever that holds width,pos */
	}

Then end atom's handler would pop the state from the stack, and the processing to be done

	end_atom_handler(atomv, state)
	{
                state->atend(state);
                pop_state(state);
	}                

and the called align_handler would be something like:

	align_handler(state)
	{
                struct strbuf aligned = STRBUF_INIT;
		struct format_state *return_to = state->prev;
                struct atom_value *atomv = state->return_to;

                strbuf_utf8_align(&aligned,
                        atomv->align.pos, atomv->align.width,
                        state->output.buf);
		strbuf_addbuf(&return_to->output, &aligned);
                strbuf_release(&aligned);
	}

With an arrangement like that, the body of the loop in show_ref_array_item() could be as simple and regular as:

	get_ref_atom_value(info, parse_ref_filter_atom, &atomv);
       	atomv->handler(atomv, &state);

without any new "ah, this %(end) is special so we need a new mechanism to pass information between set_formatting_state and perform_formatting" logic introduced every time you add new things.