#11519 closed enhancement (fixed)
Allow default_handler to be set from the Basic Settings admin page
Reported by: | Ryan J Ollos | Owned by: | Ryan J Ollos |
---|---|---|---|
Priority: | normal | Milestone: | 1.1.2 |
Component: | admin/web | Version: | |
Severity: | normal | Keywords: | default_handler |
Cc: | Jun Omae | Branch: | |
Release Notes: |
The |
||
API Changes: |
The |
||
Internal Changes: |
Description
Some allowed values for default_handler
are documented at TracIni#trac-section. However, plugins may add IRequestHandler
s and some of the documented values may not be valid if the Components are disabled. We might consider trying to dynamically generate the list of IRequestHandler
s for the documentation. However, a simple solution would be to add a select to the Basic Settings admin page and allow the default_handler
to be set through the web ui.
Attachments (1)
Change History (31)
follow-up: 2 comment:1 by , 11 years ago
Milestone: | next-dev-1.1.x → 1.1.3 |
---|---|
Owner: | set to |
Status: | new → assigned |
follow-ups: 3 8 comment:2 by , 11 years ago
Replying to rjollos:
However, we could probably come up with better ways of indicating that a
Component
was suitable for use as a `default_handler. Any good ideas?
Similar to IWikiMacroProvider.is_inline() we could add (an optional) IRequestHandler.is_valid_default_handler()
.
Is there a better way to get the list of enabled
IRequestHandler
s than instantiating aRequestDispatcher
object?
Could you just repeat the handlers = ExtensionPoint(IRequestHandler)
declaration in BasicAdminPanel
?
follow-ups: 4 5 comment:3 by , 11 years ago
Replying to psuter:
Similar to IWikiMacroProvider.is_inline() we could add (an optional)
IRequestHandler.is_valid_default_handler()
.
I had been thinking of adding an attribute to the Interface definition. Perhaps default to False
, and implementations can set the property to True
if appropriate. Are there advantages to using a function rather than an attribute for this case?
One issue with the attribute is that it may not be obvious enough to the implementer of an IRequestHandler
that they should consider whether to override the default value.
Could you just repeat the
handlers = ExtensionPoint(IRequestHandler)
declaration inBasicAdminPanel
?
Yeah, that's a good point. A similar use is in trunk/trac/config.py@12432:725#L717.
comment:4 by , 11 years ago
Replying to rjollos:
Perhaps default to
False
, and implementations can set the property toTrue
if appropriate.
Defaulting to True
might be more sensible, since most IRequestHandler
s will probably be valid default_handler
s.
comment:5 by , 11 years ago
Replying to rjollos:
I had been thinking of adding an attribute to the Interface definition.
I guess we can't add it to the Interface
since the Interface
s aren't base classes.
Like the required attribute, we could just treat it as having a default value if it is not defined, and add it to the class when the class should have a non-default value.
I think having a method is more clear. The latest proposed changes can be found in log:rjollos.git:t11519.2.
follow-up: 7 comment:6 by , 11 years ago
Right. Following the precedent of the required
attribute would work as well I think.
The documentation sounds a bit misleading to me, given that the default is already True
. A more detailed comment like for get_extra_mimetypes would be nice. Maybe also mention what is required of a default_handler
.
comment:7 by , 11 years ago
Replying to psuter:
Right. Following the precedent of the
required
attribute would work as well I think.
I'm reconsidering. It might be bit cleaner to use an attribute, and we can document it in the IRequestHandler
interface documentation: trunk/trac/web/api.py@12527:51#L50.
I will give it some thought for another day or two, and wait to see if there is additional input on which direction to take.
I will also make sure to improve the documentation per your feedback. My understanding of the requirements for a default_handler
are:
- Returns a template representing an HTML document and the
data
dictionary needed to render the template (i.e. must satisfy the minimum requirements for an HTML document:html
,head
andbody
tags). - Does not require
match_request
to be called beforeprocess_request
(as an example,PreferencesModule
failed to meet this requirement prior to the fix in #11531: branches/1.0-stable/trac/web/main.py@12460:180-182#L171).
follow-up: 9 comment:8 by , 11 years ago
Similar to IWikiMacroProvider.is_inline() we could add (an optional)
IRequestHandler.is_valid_default_handler()
.
I like property rather than such a method, e.g. jquery_noconflict
property of IRequestHandler
class at tags/trac-1.0.1/trac/web/chrome.py@:674-677#L652.
comment:9 by , 11 years ago
Replying to jomae:
I like property rather than such a method, e.g.
jquery_noconflict
property ofIRequestHandler
class at tags/trac-1.0.1/trac/web/chrome.py@:674-677#L652.
That is an interesting case. It seems like a good time to document jquery_noconflict
in the IRequestHandler
interface definition as well.
comment:10 by , 11 years ago
Latest changes can be found in log:rjollos.git:t11519.3. is_valid_default_handler
is now a property.
follow-up: 12 comment:11 by , 11 years ago
Refactored in jomae.git@t11519.4, especially, accessing option properties is not cheap.
comment:12 by , 11 years ago
Replying to jomae:
Refactored in jomae.git@t11519.4, especially, accessing option properties is not cheap.
I was confused and had meant to use lazy
there rather than property
, but your changes are better. Thanks! I'll add functional tests in the coming days.
follow-up: 14 comment:13 by , 11 years ago
I tried some different ways of isolating the code that checks whether default_handler
is a valid value.
One was to define the class attribute as _default_handler
, and have a class property named default_handler
: [13144c03/rjollos.git]. Maybe that would be okay, but it doesn't look quite right. It would be nice if we could set a getter
when the ExtensionOption
is defined, but I haven't sorted out whether there is a way to make that work.
The other simple way to isolate the code would be to have a get_default_handler
method: [9b8b200b/rjollos.git].
Is either of these two better than previous changes?
follow-up: 15 comment:14 by , 11 years ago
Both look ok to me.
Another alternative would be to leave default_handler
as it is, and add the class property named valid_default_handler
. (But if that's really better is debatable.)
comment:15 by , 11 years ago
Replying to psuter:
Another alternative would be to leave
default_handler
as it is, and add the class property namedvalid_default_handler
. (But if that's really better is debatable.)
I like valid_default_handler
. I'll go ahead and make that change.
comment:16 by , 11 years ago
Milestone: | 1.1.3 → 1.1.2 |
---|
comment:17 by , 11 years ago
If I had tested the changes I proposed in comment:13, I would have realized that using a property is problematic because a Request
object is needed for constructing the href
to the Basic Settings admin page.
I tried instead using self.env.href
. That seems to work okay for the case that [trac] base_url
is not set, because of this assignment. However, if [trac] base_url
is set incorrectly, we land on the wrong page. I don't see any other cases of self.env.href
used to generate an href
, so I'm suspicious as to whether this is a good idea. Just the problem seen when [trac] base_url
is set incorrectly might push us to instead prefer: [7903703f/rjollos.git].
get_valid_default_handler
might be better anyway because in the future I'd like to propose adding default_handler
as a session preference, and when get_valid_default_handler
is modified to check the user's default_handler
preference, it will need access to the Request
object.
Proposed changes can be found in log:rjollos.git:t11519.5. I'm working on adding tests now.
comment:19 by , 11 years ago
API Changes: | modified (diff) |
---|---|
Release Notes: | modified (diff) |
Resolution: | → fixed |
Status: | assigned → closed |
Committed to trunk in [12624].
comment:20 by , 10 years ago
Olemis and I came up with the following refactoring after reviewing [12624] today:
-
trac/admin/web_ui.py
diff --git a/trac/admin/web_ui.py b/trac/admin/web_ui.py index a07c7e9..6c85851 100644
a b from trac.web import HTTPNotFound, IRequestHandler 42 42 from trac.web.chrome import add_notice, add_stylesheet, \ 43 43 add_warning, Chrome, INavigationContributor, \ 44 44 ITemplateProvider 45 from trac.web.api import is_valid_default_handler 45 46 from trac.wiki.formatter import format_to_html 46 47 47 48 try: … … class BasicsAdminPanel(Component): 212 213 yield ('general', _("General"), 'basics', _("Basic Settings")) 213 214 214 215 def render_admin_panel(self, req, cat, page, path_info): 215 valid_handlers = [h .__class__.__name__ for hin self.handlers216 if getattr(h, 'is_valid_default_handler', True)]216 valid_handlers = [hdlr.__class__.__name__ for hdlr in self.handlers 217 if is_valid_default_handler(hdlr)] 217 218 if Locale: 218 219 locale_ids = get_available_locales() 219 220 locales = [Locale.parse(locale) for locale in locale_ids] -
trac/web/api.py
diff --git a/trac/web/api.py b/trac/web/api.py index 8a9851a..8ace8fe 100644
a b class IRequestHandler(Interface): 87 87 """ 88 88 89 89 90 def is_valid_default_handler(handler): 91 return handler and getattr(handler, 'is_valid_default_handler', True) 92 93 90 94 class IRequestFilter(Interface): 91 95 """Enable components to interfere with the processing done by the 92 96 main handler, either before and/or after it enters in action. -
trac/web/main.py
diff --git a/trac/web/main.py b/trac/web/main.py index c546dde..bdab9f0 100644
a b class RequestDispatcher(Component): 267 267 268 268 def get_valid_default_handler(self, req): 269 269 handler = self.default_handler 270 if not handler or \ 271 not getattr(handler, 'is_valid_default_handler', True): 270 if not is_valid_default_handler(handler): 272 271 raise ConfigurationError( 273 272 tag_("%(handler)s is not a valid default handler. Please " 274 273 "update %(option)s through the %(page)s page or by "
The aim is to isolate to a single location in the codebase the definition of the default value for the is_valid_default_handler
attribute.
by , 10 years ago
Attachment: | t11519.diff added |
---|
comment:21 by , 10 years ago
Change from comment:20 committed in [12684].
comment:22 by , 10 years ago
Another thing I just noticed is that get_valid_default_handler
should probably be a private method: _get_valid_default_handler
.
comment:25 by , 10 years ago
comment:26 by , 10 years ago
Keywords: | default_handler added |
---|
comment:27 by , 10 years ago
follow-up: 29 comment:28 by , 10 years ago
Cc: | added |
---|
Location of default handler setting in user preferences feels strange to me while I was translating message catalogs. Currently, settings in General panel is for user information. However, default handler setting is not for user information.
Help in General panel says;
This information is used to associate your login name with your email address and full name, which is used for email notification and RSS feeds, for example.
I suppose it would be better to be located to User Interface rather than General panel. Thoughts?
comment:29 by , 10 years ago
Replying to jomae:
I suppose it would be better to be located to User Interface rather than General panel. Thoughts?
I chose General for lack of a better place. I agree that it's not ideal. #11597 is the ticket in which it was implemented.
User Interface doesn't feel like a better fit to me since it seems to be associated with visual presentation. I was hoping that a better location would be revealed as we add more preferences. #10543 could yield more preferences for a Navigation panel though I don't find the feature set compeling. A preference for mainnav ordering might be useful. Or we could do something like combine Keyboard Shortcuts with Default handler in a Navigation panel. Can we foresee any additional entries for Keyboard shortcuts? If not, then I'm not sure it deserves its own tab.
Maybe the thing to do is list preference we will likely add and try to come up with the best grouping that doesn't result in too many tabs.
comment:30 by , 10 years ago
#11813 will consider moving the Default handler preference to another tab.
The
IRequestHandler
s in a default Trac installation includes several Components that aren't usable as adefault_handler
. The list ofIRequestHandler
s is:Of those
InterTracDispatcher
,Chrome
,LoginModule
,PygmentsRenderer
,BatchModifyModule
andWikiRenderer
aren't usable as adefault_handler
(andPreferencesModule
needs the patch in #11531).If
BatchModifyModule
andLoginModule
were modified so that they would function correctly as adefault_handler
, then we could adopt a convention thatIRequestHandler
s ending inModule
should function as adefault_handler
. However, we could probably come up with better ways of indicating that aComponent
was suitable for use as a `default_handler. Any good ideas?Initial changes can be found in log:rjollos.git:t11519. Functional tests will be added later.
Is there a better way to get the list of enabled
IRequestHandler
s than instantiating aRequestDispatcher
object?