On Oct 28, 12:10 am, Remy Blank <[email protected]> wrote: > Hi Mikael, > > > I'm just bumping this thread since there has not been any response to the > > latest patch I supplied about three weeks ago > > (http://trac.edgewall.org/ticket/6492#comment:37). > > Yes, a reply was overdue, sorry about that. > > > I would like to see this feature in Trac 0.13, so what do you guys think is > > necessary for me to do to make the patches accepted? > > First, I like the visuals very much. The icons are nice and clean. I'm > not completely sure about the bold attribute for unseen events, but > that's a matter of taste, and I'm fine with it.
Agree. The moment we start discussing visuals of any kind, it becomes a a discussion of style and preference which is not so easy... I'm content with concluding that your sense of visuals is OK, and I trust your judgement :-) > > About the code, I have the following comments, of which only the first > one is important: > > - I like the idea of making the timeline even a class. However, I don't > think the rendering code should be moved to those classes. Timeline > events can be consumed by other components, possibly unrelated to HTML > rendering. So I would prefer the events to stay "pure data" objects, and > the rendering code to remain in the respective rendering components. > That would also cut down on the number of new classes, as most providers > would just yield TimelineEvent instances. I agree, and as said before I don't like the raw data being entangled directly with the various representations of them. I think we need to find a better solution to that when we progress on refactoring. We already have ways to call the Resource to render itself, and I'm mentally playing with the idea of creating change resources hierarchies where for instance a 'changeset' resource may contain 'file' and 'property' children depending on user and/or module settings for what to display. If we already now how to render a changeset resource at some given revision (as Resource(realm, id, version)), perhaps it is not so far fetched to add support for making the resource render the state change of becoming that version? I don't have any code for this yet - just an idea that needs more work... On the subject of entangling: New 'flags' being added to TimelineModule that do things such as "Highlight my tickets", has no place in Timeline code. The timeline code already have options, settings and code that deal with Trac-specific modules in ways that external plugins cannot. We should move away from this, and if anything provide extension points for plugins to add features or do post-processing on results before displaying them. Timeline should be completely module-agnostic. Anyway, as I said before: Great work, and much appreciated. I'm OK with adding the new 'status' marker to the events + improving icons (the original intension of the ticket). The other features and event refactoring still needs some work, in my opinion. :::simon -- 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.
