Permalink
Browse files

Optimize layout lookup to avoid double calls.

  • Loading branch information...
1 parent efc28a7 commit 239262fee03096d1e52acf8fe69de736726d87e0 @josevalim josevalim committed Dec 8, 2011
@@ -168,11 +168,12 @@ module Layouts
included do
class_attribute :_layout_conditions
remove_possible_method :_layout_conditions
- delegate :_layout_conditions, :to => :'self.class'
self._layout_conditions = {}
_write_layout_method
end
+ delegate :_layout_conditions, :to => "self.class"
+
module ClassMethods
def inherited(klass)
super
@@ -188,7 +189,7 @@ module LayoutConditions
#
# ==== Returns
# * <tt> Boolean</tt> - True if the action has a layout, false otherwise.
- def action_has_layout?
+ def conditional_layout?
return unless super
conditions = _layout_conditions
@@ -244,7 +245,13 @@ def _implied_layout_name
def _write_layout_method
remove_possible_method(:_layout)
- prefixes = _implied_layout_name =~ /\blayouts/ ? [] : ["layouts"]
+ prefixes = _implied_layout_name =~ /\blayouts/ ? [] : ["layouts"]
+ name_clause = if name
+ <<-RUBY
+ lookup_context.find_all("#{_implied_layout_name}", #{prefixes.inspect}).first || super
+ RUBY
+ end
+
layout_definition = case defined?(@_layout) ? @_layout : nil
when String
@_layout.inspect
@@ -265,27 +272,15 @@ def _write_layout_method
when true
raise ArgumentError, "Layouts must be specified as a String, Symbol, false, or nil"
when nil
- if name
- <<-RUBY
- if template_exists?("#{_implied_layout_name}", #{prefixes.inspect})
- "#{_implied_layout_name}"
- else
- super
- end
- RUBY
- end
+ name_clause
end
self.class_eval <<-RUBY, __FILE__, __LINE__ + 1
def _layout
- if action_has_layout?
+ if conditional_layout?
#{layout_definition}
- elsif self.class.name
- if template_exists?("#{_implied_layout_name}", #{prefixes.inspect})
- "#{_implied_layout_name}"
- else
- super
- end
+ else
+ #{name_clause}
end
end
RUBY
@@ -298,8 +293,7 @@ def _normalize_options(options)
if _include_layout?(options)
layout = options.key?(:layout) ? options.delete(:layout) : :default
- value = _layout_for_option(layout)
- options[:layout] = (value =~ /\blayouts/ ? value : "layouts/#{value}") if value
+ options[:layout] = Proc.new { _normalize_layout(_layout_for_option(layout)) }
end
end
@@ -314,6 +308,10 @@ def action_has_layout?
@_action_has_layout
end
+ def conditional_layout?
+ true
+ end
+
private
# This will be overwritten by _write_layout_method
@@ -335,6 +333,10 @@ def _layout_for_option(name)
end
end
+ def _normalize_layout(value)
+ value.is_a?(String) && value !~ /\blayouts/ ? "layouts/#{value}" : value
+ end
+
# Returns the default layout for this controller.
# Optionally raises an exception if the layout could not be found.
#
@@ -346,17 +348,17 @@ def _layout_for_option(name)
# * <tt>template</tt> - The template object for the default layout (or nil)
def _default_layout(require_layout = false)
begin
- layout_name = _layout
+ value = _layout if action_has_layout?
rescue NameError => e
raise e, "Could not render layout: #{e.message}"
end
- if require_layout && action_has_layout? && !layout_name
+ if require_layout && action_has_layout? && !value
raise ArgumentError,
"There was no default layout for #{self.class} in #{view_paths.inspect}"
end
- layout_name
+ value
end
def _include_layout?(options)
@@ -10,7 +10,7 @@ module ViewPaths
self._view_paths.freeze
end
- delegate :find_template, :template_exists?, :view_paths, :formats, :formats=,
+ delegate :template_exists?, :view_paths, :formats, :formats=,
:locale, :locale=, :to => :lookup_context
module ClassMethods
@@ -227,7 +227,6 @@ def locale=(value)
end
# A method which only uses the first format in the formats array for layout lookup.
- # This method plays straight with instance variables for performance reasons.
def with_layout_format
if formats.size == 1
yield
@@ -58,14 +58,21 @@ def render_with_layout(path, locals) #:nodoc:
# context object. If no layout is found, it checks if at least a layout with
# the given name exists across all details before raising the error.
def find_layout(layout, keys)
- begin
- with_layout_format do
- layout =~ /^\// ?
- with_fallbacks { find_template(layout, nil, false, keys, @details) } : find_template(layout, nil, false, keys, @details)
+ with_layout_format { resolve_layout(layout, keys) }
+ end
+
+ def resolve_layout(layout, keys)
+ case layout
+ when String
+ if layout =~ /^\//
+ with_fallbacks { find_template(layout, nil, false, keys, @details) }
+ else
+ find_template(layout, nil, false, keys, @details)
end
- rescue ActionView::MissingTemplate
- all_details = @details.merge(:formats => @lookup_context.default_formats)
- raise unless template_exists?(layout, nil, false, keys, all_details)
+ when Proc
+ resolve_layout(layout.call, keys)
+ else
+ layout
end
end
end

2 comments on commit 239262f

Owner

pixeltrix replied Dec 17, 2012

@josevalim couple of things on this commit:

  1. You changed the method name from action_has_layout? to conditional_layout? but the documentation doesn't really match what the method does (it's whether it should use the implied layout or the layout definition). Did you change the behavior of the method or was the documentation always wrong? If the former, should this really be a public method? There doesn't seem to be much use for it other than it's use inside the class_eval.
  2. There's some code left for action_has_layout but it just seems to be hardcoded to true - is this left intentionally for backwards compatibility or is it an oversight?
Contributor

josevalim replied Dec 17, 2012

  1. I don't think it should be a public method. We could even underscore it (I would also put :nodoc: on all of those except layout, but leaving the comments)
  2. I think it is an oversight. It was probably renderered useless after the refactoring and I haven't noticed it.

Thanks!

Please sign in to comment.