Peter Dimov wrote:
> I finally got some spare time to look at that one again. I actually
> wanted to port my previous modifications to the latest revision in trunk
> but that required me to do some modifications in
> LogModule.process_request(), which was about 140 lines and called for
> refactoring!
Indeed ;-)
> Instead of writing big functions I normally like to write smaller, more
> cohesive ones, which do only a single coherent part of the whole work.
> The "big" function just handles the logic and delegates to the smaller
> functions accordingly. IMHO this way the source code becomes much more
> readable and easier to maintain. Thats definitely an approach that
> could be applied at different places in trac (from what Ive seen,
> LogModule and ChangesetModule at least).
>
I think it's a good idea to have a request handler object that can be
used to better encapsulate the processing of one request, as we can
store the request and the parameters and access them from processing
methods and helper methods. This is not possible to do at the level of
the module component itself, which is shared by different requests.
So I've started from your patch and made some further modification.
The LogModule.process_request just do:
return LogRequest(self, req).process()
instead of using an API of the LogRequest class. process() is a generic
method from the parent RequestHandler class, which will dispatch to the
appropriate method of the subclass according to the action parameter
('do_<action>' or 'render_<action>' ones, the former for GET requests
and the latter for POST requests). This is not that useful for the
LogRequest, where the only action is 'view' (the default), but would be
for other handlers.
> Ive refactored LogModule.process_request() having the above described
> considerations in mind. Thats just the first step and I guess it might
> make sense to further split some of the new functions. Ive added an new
> class LogRequest which handles a single request to the log module.
> __init__() parses the inputs and the object stores the request state
> between the invocations of the individual request processing steps. I
> havent thoroughly tested the patch, which means I may have forgotten to
> add a self. in front of a var somewhere.
>
Yes, the path_history mode hasn't been tested, I fixed it.
Also, self.revs was used in two different places for a different
purpose. I've used locals instead.
I also changed the methods a bit to make more apparent what step they
were doing, by using a more functional style:
def render_view(self):
history = self._get_history_provider()
items = self._get_log_items(history)
data = self._get_template_context(items)
if len(items) == self.limit+1:
...
instead of:
request.prepare_history()
request.extract_info()
request.extract_changes()
request.prepare_response()
return request.get_response()
> I plan to do similar refactoring to ChangesetModule before proceeding
> with the version control api refactoring. Actually I would further
> refactor LogRequest and ChangesetRequest to use the "perfect" version
> control api. I guess we would have some discussion about how perfect it
> is before actually going ahead and changing it ;-)
Please take a look at my updated patch. If we both feel we can achieve
something going on this way, maybe we should restart a vc-refactoring
branch ;-) As you proposed, we should first start there by some
clean-ups and refactoring of the current code base.
-- 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
-~----------~----~----~----~------~----~------~--~---
Index: trac/versioncontrol/web_ui/log.py
===================================================================
--- trac/versioncontrol/web_ui/log.py (revision 4941)
+++ trac/versioncontrol/web_ui/log.py (working copy)
@@ -35,6 +35,7 @@
Chrome
from trac.wiki import IWikiSyntaxProvider, WikiParser
+
class LogModule(Component):
implements(INavigationContributor, IPermissionRequestor, IRequestHandler,
@@ -69,175 +70,8 @@
def process_request(self, req):
req.perm.assert_permission('LOG_VIEW')
- mode = req.args.get('mode', 'stop_on_copy')
- path = req.args.get('path', '/')
- rev = req.args.get('rev')
- stop_rev = req.args.get('stop_rev')
- revs = req.args.get('revs', rev)
- format = req.args.get('format')
- verbose = req.args.get('verbose')
- limit = int(req.args.get('limit') or self.default_log_limit)
+ return LogRequest(self, req).process()
- repos = self.env.get_repository(req.authname)
- normpath = repos.normalize_path(path)
- revranges = None
- rev = revs
- if revs:
- try:
- revranges = Ranges(revs)
- rev = revranges.b
- except ValueError:
- pass
- rev = unicode(repos.normalize_rev(rev))
- path_links = get_path_links(req.href, path, rev)
- if path_links:
- add_link(req, 'up', path_links[-1]['href'], 'Parent directory')
-
- # The `history()` method depends on the mode:
- # * for ''stop on copy'' and ''follow copies'', it's `Node.history()`
- # unless explicit ranges have been specified
- # * for ''show only add, delete'' we're using
- # `Repository.get_path_history()`
- if mode == 'path_history':
- rev = revranges.b
- def history(limit):
- for h in repos.get_path_history(path, rev, limit):
- yield h
- else:
- if not revranges or revranges.a == revranges.b:
- history = get_existing_node(req, repos, path, rev).get_history
- else:
- def history(limit):
- prevpath = path
- ranges = list(revranges.pairs)
- ranges.reverse()
- for (a,b) in ranges:
- while b >= a:
- rev = repos.normalize_rev(b)
- node = get_existing_node(req, repos, prevpath, rev)
- node_history = list(node.get_history(2))
- p, rev, chg = node_history[0]
- if rev < a:
- yield (p, rev, None) # separator
- break
- yield node_history[0]
- prevpath = node_history[-1][0] # follow copy
- b = rev-1
- if b < a and len(node_history) > 1:
- p, rev, chg = node_history[1]
- yield (p, rev, None)
-
- # -- retrieve history, asking for limit+1 results
- info = []
- depth = 1
- fix_deleted_rev = False
- previous_path = normpath
- for old_path, old_rev, old_chg in history(limit+1):
- if fix_deleted_rev:
- fix_deleted_rev['existing_rev'] = old_rev
- fix_deleted_rev = False
- if stop_rev and repos.rev_older_than(old_rev, stop_rev):
- break
- old_path = repos.normalize_path(old_path)
-
- item = {
- 'path': old_path, 'rev': old_rev, 'existing_rev': old_rev,
- 'change': old_chg, 'depth': depth,
- }
-
- if old_chg == Changeset.DELETE:
- fix_deleted_rev = item
- if not (mode == 'path_history' and old_chg == Changeset.EDIT):
- info.append(item)
- if old_path and old_path != previous_path \
- and not (mode == 'path_history' and old_path == normpath):
- depth += 1
- item['depth'] = depth
- item['copyfrom_path'] = old_path
- if mode == 'stop_on_copy':
- break
- if len(info) > limit: # we want limit+1 entries
- break
- previous_path = old_path
- if info == []:
- # FIXME: we should send a 404 error here
- raise TracError("The file or directory '%s' doesn't exist "
- "at revision %s or at any previous revision."
- % (path, rev), 'Nonexistent path')
-
- def make_log_href(path, **args):
- link_rev = rev
- if rev == str(repos.youngest_rev):
- link_rev = None
- params = {'rev': link_rev, 'mode': mode, 'limit': limit}
- params.update(args)
- if verbose:
- params['verbose'] = verbose
- return req.href.log(path, **params)
-
- if len(info) == limit+1: # limit+1 reached, there _might_ be some more
- next_rev = info[-1]['rev']
- next_path = info[-1]['path']
- add_link(req, 'next', make_log_href(next_path, rev=next_rev),
- 'Revision Log (restarting at %s, rev. %s)'
- % (next_path, next_rev))
- # only show fully 'limit' results, use `change == None` as a marker
- info[-1]['change'] = None
-
- revs = [i['rev'] for i in info]
- changes = get_changes(repos, revs)
- extra_changes = {}
- email_map = {}
-
- if format == 'rss':
- # Get the email addresses of all known users
- if Chrome(self.env).show_email_addresses:
- for username,name,email in self.env.get_known_users():
- if email:
- email_map[username] = email
- elif format == 'changelog':
- for rev in revs:
- changeset = changes[rev]
- cs = {}
- cs['message'] = wrap(changeset.message, 70,
- initial_indent='\t',
- subsequent_indent='\t')
- files = []
- actions = []
- for path, kind, chg, bpath, brev in changeset.get_changes():
- files.append(chg == Changeset.DELETE and bpath or path)
- actions.append(chg)
- cs['files'] = files
- cs['actions'] = actions
- extra_changes[rev] = cs
- data = {
- 'context': Context(self.env, req, 'source', path),
- 'path': path, 'rev': rev, 'stop_rev': stop_rev,
- 'mode': mode, 'verbose': verbose,
- 'path_links': path_links, 'limit' : limit,
- 'items': info, 'changes': changes,
- 'email_map': email_map, 'extra_changes': extra_changes,
- 'wiki_format_messages':
- self.config['changeset'].getbool('wiki_format_messages')
- }
-
- if req.args.get('format') == 'changelog':
- return 'revisionlog.txt', data, 'text/plain'
- elif req.args.get('format') == 'rss':
- return 'revisionlog.rss', data, 'application/rss+xml'
-
- add_stylesheet(req, 'common/css/diff.css')
- add_stylesheet(req, 'common/css/browser.css')
-
- rss_href = make_log_href(path, format='rss', stop_rev=stop_rev)
- add_link(req, 'alternate', rss_href, 'RSS Feed', 'application/rss+xml',
- 'rss')
- changelog_href = make_log_href(path, format='changelog',
- stop_rev=stop_rev)
- add_link(req, 'alternate', changelog_href, 'ChangeLog', 'text/plain')
-
- return 'revisionlog.html', data, None
-
# IWikiSyntaxProvider methods
REV_RANGE = r"(?:%s|%s)" % (Ranges.RE_STR, ChangesetModule.CHANGESET_ID)
@@ -304,4 +138,247 @@
ranges = ''.join([str(rev)+sep for rev, sep in zip(revs, seps)])
revranges = Ranges(ranges)
return str(revranges) or None
-
+
+
+
+class RequestHandler(object):
+
+ default_action = 'view'
+
+ def __init__(self, env, req):
+ self.env = env
+ self.req = req
+
+ def process(self):
+ if self.req.method == 'POST':
+ prefix = 'do_'
+ else:
+ prefix = 'render_'
+ method = prefix + self.req.args.get('action', self.default_action)
+ if hasattr(self, method):
+ return getattr(self, method)()
+ raise TracError("The request handler '%s.%s' doesn't exist." %
+ (self.__class__.__name__, method))
+
+
+class LogRequest(RequestHandler):
+ """Completely handles a request to the LogModule."""
+
+ def __init__(self, module, req):
+ """Parses the data from req into member variables for use later."""
+ RequestHandler.__init__(self, module.env, req)
+
+ self.module = module
+ self.mode = req.args.get('mode', 'stop_on_copy')
+ self.path = req.args.get('path', '/')
+ self.rev = req.args.get('rev')
+ self.stop_rev = req.args.get('stop_rev')
+ revs = req.args.get('revs', self.rev)
+ self.format = req.args.get('format')
+ self.verbose = req.args.get('verbose')
+ self.limit = int(req.args.get('limit') or
+ self.module.default_log_limit)
+
+ self.repos = self.module.env.get_repository(req.authname)
+ self.normpath = self.repos.normalize_path(self.path)
+ self.revranges = None
+ self.rev = revs
+ if revs:
+ try:
+ self.revranges = Ranges(revs)
+ self.rev = self.revranges.b
+ except ValueError:
+ pass
+ self.rev = unicode(self.repos.normalize_rev(self.rev))
+
+ def render_view(self):
+ history = self.get_history_provider()
+ items = self.get_log_items(history)
+ data = self.get_template_context(items)
+
+ if len(items) == self.limit+1:
+ # limit+1 reached, there _might_ be some more
+ next_rev = items[-1]['rev']
+ next_path = items[-1]['path']
+ add_link(self.req, 'next',
+ self._make_log_href(next_path, rev=next_rev),
+ 'Revision Log (restarting at %s, rev. %s)' %
+ (next_path, next_rev))
+ # only show fully 'limit' results, use `change == None` as a marker
+ items[-1]['change'] = None
+
+ if self.format == 'changelog':
+ return 'revisionlog.txt', data, 'text/plain'
+ elif self.format == 'rss':
+ return 'revisionlog.rss', data, 'application/rss+xml'
+
+ add_stylesheet(self.req, 'common/css/diff.css')
+ add_stylesheet(self.req, 'common/css/browser.css')
+
+ rss_href = self._make_log_href(self.path, format='rss',
+ stop_rev=self.stop_rev)
+ add_link(self.req, 'alternate', rss_href, 'RSS Feed',
+ 'application/rss+xml', 'rss')
+ changelog_href = self._make_log_href(self.path, format='changelog',
+ stop_rev=self.stop_rev)
+ add_link(self.req, 'alternate', changelog_href, 'ChangeLog',
+ 'text/plain')
+
+ return 'revisionlog.html', data, None
+
+ def _get_path_history(self, limit):
+ """A generator yielding the changes history of a path, i.e. possibly
+ different files or directories that have been moved/copied to this
+ location in different revisions."""
+
+ for h in self.repos.get_path_history(self.path, self.rev, limit):
+ yield h
+
+ def _get_node_history(self, limit):
+ """Returns a gererator yielding the changes to a node."""
+
+ return get_existing_node(self.req, self.repos, \
+ self.path, self.rev).get_history(limit)
+
+ def _get_node_history_ranges(self, limit):
+ """A generator yielding the changes to a node, restricted to certain
+ revisions. The revisions outside the given ranges are filtered."""
+
+ prevpath = self.path
+ ranges = list(self.revranges.pairs)
+ ranges.reverse()
+ for (a,b) in ranges:
+ while b >= a:
+ rev = repos.normalize_rev(b) # \todo
+ node = get_existing_node(self.req, self.repos, prevpath, rev)
+ node_history = list(node.get_history(2))
+ p, rev, chg = node_history[0]
+ if rev < a:
+ yield (p, rev, None) # separator
+ break
+ yield node_history[0]
+ prevpath = node_history[-1][0] # follow copy
+ b = rev-1
+ if b < a and len(node_history) > 1:
+ p, rev, chg = node_history[1]
+ yield (p, rev, None)
+
+ def get_history_provider(self):
+ """Depending on the selected mode prepares and selects a generator
+ function, which will be used to extract the history in the next step.
+
+ * for ''stop on copy'' and ''follow copies'',
+ it's `Node.get_history()` unless explicit ranges have been
+ specified, in which case some of the entries are filtered.
+ * for ''show only add, delete'' we're using
+ `Repository.get_path_history()`"""
+
+ if self.mode == 'path_history':
+ return self._get_path_history
+ else:
+ if not self.revranges or self.revranges.a == self.revranges.b:
+ return self._get_node_history
+ else:
+ return self._get_node_history_ranges
+
+ def get_log_items(self, history):
+ """Retrieves history and extracts info for at most limit+1 results"""
+
+ info = []
+ depth = 1
+ fix_deleted_rev = False
+ previous_path = self.normpath
+ for old_path, old_rev, old_chg in history(self.limit+1):
+ if fix_deleted_rev:
+ fix_deleted_rev['existing_rev'] = old_rev
+ fix_deleted_rev = False
+ if self.stop_rev and self.repos.rev_older_than(old_rev,
+ self.stop_rev):
+ break
+ old_path = self.repos.normalize_path(old_path)
+
+ item = {
+ 'path': old_path, 'rev': old_rev, 'existing_rev': old_rev,
+ 'change': old_chg, 'depth': depth,
+ }
+
+ if old_chg == Changeset.DELETE:
+ fix_deleted_rev = item
+ if not (self.mode == 'path_history' and old_chg == Changeset.EDIT):
+ info.append(item)
+ if old_path and old_path != previous_path and \
+ not (self.mode == 'path_history' and
+ old_path == self.normpath):
+ depth += 1
+ item['depth'] = depth
+ item['copyfrom_path'] = old_path
+ if self.mode == 'stop_on_copy':
+ break
+ if len(info) > self.limit: # we want limit+1 entries
+ break
+ previous_path = old_path
+ if info == []:
+ # FIXME: we should send a 404 error here
+ raise TracError("The file or directory '%s' doesn't exist "
+ "at revision %s or at any previous revision."
+ % (path, rev), 'Nonexistent path')
+ else:
+ return info
+
+ def get_template_context(self, items):
+ """Prepares the data to be returned as a response to this request."""
+
+ # \todo I actually wanted this get_path_links part to be in response()
+ # but path_links is needed for self.data (see below).
+ self.path_links = get_path_links(self.req.href, self.path, self.rev)
+ if self.path_links:
+ add_link(self.req, 'up', self.path_links[-1]['href'],
+ 'Parent directory')
+
+ revs = [i['rev'] for i in items]
+ changes = get_changes(self.repos, revs)
+ extra_changes = {}
+ email_map = {}
+
+ if self.format == 'rss':
+ # Get the email addresses of all known users
+ if Chrome(self.module.env).show_email_addresses:
+ for username,name,email in self.module.env.get_known_users():
+ if email:
+ email_map[username] = email
+ elif self.format == 'changelog':
+ for rev in revs:
+ changeset = changes[rev]
+ cs = {}
+ cs['message'] = wrap(changeset.message, 70,
+ initial_indent='\t',
+ subsequent_indent='\t')
+ files = []
+ actions = []
+ for path, kind, chg, bpath, brev in changeset.get_changes():
+ files.append(chg == Changeset.DELETE and bpath or path)
+ actions.append(chg)
+ cs['files'] = files
+ cs['actions'] = actions
+ extra_changes[rev] = cs
+ return {
+ 'context': Context(self.module.env, self.req, 'source', self.path),
+ 'path': self.path, 'rev': self.rev, 'stop_rev': self.stop_rev,
+ 'mode': self.mode, 'verbose': self.verbose,
+ 'path_links': self.path_links, 'limit' : self.limit,
+ 'items': items, 'changes': changes,
+ 'email_map': email_map, 'extra_changes': extra_changes,
+ 'wiki_format_messages':
+ self.module.config['changeset'].getbool('wiki_format_messages')
+ }
+
+ def _make_log_href(self, path, **args):
+ link_rev = self.rev
+ if self.rev == str(self.repos.youngest_rev):
+ link_rev = None
+ params = {'rev': link_rev, 'mode': self.mode, 'limit': self.limit}
+ params.update(args)
+ if self.verbose:
+ params['verbose'] = self.verbose
+ return self.req.href.log(path, **params)
+
Index: trac/versioncontrol/templates/revisionlog.html
===================================================================
--- trac/versioncontrol/templates/revisionlog.html (revision 4941)
+++ trac/versioncontrol/templates/revisionlog.html (working copy)
@@ -30,7 +30,6 @@
<form id="prefs" action="" method="get">
<div>
- <input type="hidden" name="action" value="$mode" />
<div class="choice">
<fieldset>
<legend>Revision Log Mode:</legend>
