On 10/7/2010 1:40 AM, osimons wrote:
On Oct 6, 11:03 pm, "Mikael Relbe"<[email protected]> wrote:
Dear Trac developers, especially Christian (cboos) and Simon (osimons)
The discussion in ticket #6492 about timeline icons [1] has taken a
different turn than I expected, and I am afraid that my last comment,
comment:35, may be interpreted as I am not interested in the refactoring
problem.
I think it's perfectly fine to dissociate the two parts. As Simon says
later in his reply, maybe we will end up not doing this big refactoring
of the Timeline API to better separate data generation from rendering,
and the Timeline RPC interface will end up being done differently, we
don't know at this point. This is not a reason for not improving things
progressively. As long as we remain backward compatible (what the
current patch does), I have no problem in making intermediate steps on
trunk. The API changes can remain in flux during the -dev period, and
even if no "perfect" solution is found when approaching beta, well, no
big deal, that will become the "perfectible" API of 0.13. In the
meantime, Trac will provide a better experience for our end users, we
will have learned a thing or two, and have clearer ideas about what the
optimal solution could be (if there is ever one).
1. First we accept Simons proposal, by "reverting" the refactoring of the
timeline interface made in the patches that I have supplied so far.
Thereafter we re-factor the timeline event interface (coordinated by a
separate ticket).
No, the changes you did are quite fine, it's an improvement over the
existing state and the change is justified by the new feature and the
related upcoming "owned events" enhancement.
2. The opposite: first re-factor the timeline event interface (coordinated
by separate ticket), then a re-make of the timeline icons proposal.
I think such a change can happen afterward. If it happens in time for
0.13, fine, your API would have been transitional, never made "official"
and there will be no compatibility issue with plugins. If not, then I
don't see how it could harm either. Using a class is anyway more future
proof and leads to better API "version control" than using a tuple.
Now to the more "juicy" stuff ;-)
Quoting osimons:
...
The discussion on the ticket turned out to be a bit "deeper" than
expected. And certainly for Timeline that have an API that, in my
opinion, have been broken more or less forever by entangling the
"data" aspect of timeline events by their "presentation" (rendering).
Well, I still don't understand how you can do it differently, for the
kind of non-uniform rendering we do. Did you even read my comment:34 on
that ticket? The problem is a bit more complex than the way you present it.
If we really want a cleaner separation (and I can understand that need
for the RPC API) then we need 2 passes: Step 1) get the standardized
data that any consumer can understand; Step 2) for each such event, get
additional data according to the specific needs of the formatter
consumers (as there we have a tight binding between that additional data
and the way its presented).
IIUC, you pretend that we don't need 2), but that's simply not true, so
I must have misunderstood you ;-) The XmlRpcPlugin can certainly stop at
step 1), but in order to produce the exact same output we currently
produce in the timeline (i.e. the "no regression" scenario), we need
that second step.
And besides being an enabler for the XmlRpcPlugin, I see an other
advantage for this split: performance improvement. We currently gather
all the events (with all they're rendering specific data) from the
selected filters (*) in a given time span. In a second step, we apply
additional filtering, like filtering by author and even optionally only
keep a given max number of events. For all the discarded events, the
extra rendering data has been gathered for no use. Worse, some formats
(like RSS) won't use that extra rendering information anyway. So I think
it would make sense to only gather the extra rendering data when we're
sure we need it. This second round could eventually be made quite
efficient if we could do it in one call for all the events produced by a
same provider.
As mentioned, the issue arise from other needs, in particular the
TracRPC (XmlPrcPlugin) that really should support proper Timeline
information. But the output I get by looking at the events (html) is
not something I can pass on to machine-interface clients that I know
absolutely nothing about. Nor can I assume anything about the data and
try to fix it. Other than then the 'kind' that is currently used as
visual style class to make the icon, there is no underlying data or
domain knowledge to be found by querying the Timeline API. I will not
hard-code a mapping for core modules + any possible external plugin
that may provide Timeline events.
So, I'm frustrated. How should it work? The Events API should provide
pure event data - the 'when', 'who' and 'what'. I'm +1 on adding the
'action' as you suggest, that helps to understand the 'what'. However,
I would really want it to at least include the resource which would
provide the unique identification of the data as well as a means of
getting its title and description (via the Resource API).
I agree on this "step 1)" information gathering. But even at this stage,
we probably need to store some minimal amount of "opaque" data that will
make sense only for the specific renderers.
Typical example: when you gather ticket update changes, you know the
change number you're dealing with (cnum). When rendering the link, you
need that cnum information (#comment:$cnum). It would be highly
inefficient to /not/ store that information somewhere for the sake of
"clean" design, and have to recover that information later during
rendering, say from the timestamp of the event. That's what an "opaque"
data handle is for, it benefits those who need it, and those who don't
know about it can simply ignore it.
Then the
resource could perhaps be asked to render its transitional and finite
state by various methods at Resource level (and of course the actual
implementation by the provider of the resource realm).
This rendering necessarily needs to be dependent on the output format
(putting aside my remarks about an intermediate representation from
comment:34, which still holds). In some cases, the generic data of step
1) will not be enough and we'll need an extra round of fetching (typical
example, the changeset events with the file change information).
I see there is general positivity for an API change that changes all
this to just return some 'fluffy' Event object with all stuff attached
to it - "yield event". It certainly makes it a lot easier to attach
all kinds of new data and meaning onto such an object over time
(easily exendible),
Yes.
but personally I think that is bad API design as
it tells the consumer nothing about what to expect and the provider
nothing about what to deliver. How would you make breaking changes?
By introducing new methods and properties, and documenting them?
E.g. the "owner" property, different from the author (e.g. someone
commenting on a ticket you own is the "author", you're the "owner"). I
don't see the problem with such extensions.
By
adding various magic to the Event object to discover if its state of
data and methods is 'OK' according to the current executing visual
experience of the Trac TimelineModule? If I got a TimelineRPC
implementation off the ground, what can I expect to assume when my
code is to run on various versions of Trac and against a multitude of
plugins (also at varying versions) that yield such event objects?
Backward compatibility, as usual.
APIs
should break when there are new and better ways, and the breakage
should be obvious to see and obvious to debug. It will be a pain to
debug a yielded half-implemented event that crashes rendering at a
much later, disconnected, stage. Instead of just raising an exception
right at the initial call to fetch the event when things do not match
up.
Anyway, my proposal was to not break the API until we had a clear idea
of how the API should be broken and what the new API should be and
look like.
We can continue to discuss and refine such a new API. We could also take
this opportunity to stop talking about timeline "filters" when there are
actually *event providers*. Having real pluggable filters was discussed
a few times and would indeed allow us to better implement author
filtering, per-repository filtering, and new kind of filters which would
operate between step 1) and 2).
That is why I looked at a simple and non-obtrusive fix for
adding 'action' to support the improved icons.
Well, on the contrary I'm against continuing down that path and
extending or tweaking the tuple further, *that's* what I'd call bad
design...
The Event object
suggestion is a breaking change - as opposed to just gaining an
(optional) feature. When redefining the API in such a way, we need at
some point to deprecate the old interface.
We still support the 0.10 interface, at little cost. We can continue to
do so for 0.11 and 0.12.
However, I'm not so sure the underlying assumptions of the Timeline
code is worth changing at this point - too much is vested into the
entangled structure. It is perhaps best if I left it alone, and
instead pursued other ways of providing such a lower-level data
interface.
Well, I'm confident that the proposed approach (what I call step 1)
above) would meet your needs for the timeline RPC API.
BTW, remembering having seen #217 fly by recently, while I haven't yet
taken a close look at the XmlRpcPlugin, I wonder if it doesn't make
sense to have such an RPC API in the core, as it certainly needs to
follow the changes to the internals (looking... oh yes ;-) ).
-- Christian
--
You received this message because you are subscribed to the Google Groups "Trac
Development" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/trac-dev?hl=en.
