From f3cfce4a93240fce264a25356d27303c749c472c Mon Sep 17 00:00:00 2001 From: David Taylor Date: Wed, 6 Feb 2019 15:51:23 +0000 Subject: [PATCH] FEATURE: Calculate sprite-sheet based on currently active themes (#6973) Previously there was only one sprite sheet, which always included icons from all themes even if they were disabled --- app/controllers/svg_sprite_controller.rb | 7 +- app/helpers/application_helper.rb | 6 +- app/models/theme.rb | 1 + app/models/theme_field.rb | 4 ++ config/routes.rb | 2 +- lib/svg_sprite/svg_sprite.rb | 65 ++++++++++--------- spec/components/svg_sprite/svg_sprite_spec.rb | 46 ++++++++++--- spec/requests/svg_sprite_controller_spec.rb | 12 +++- 8 files changed, 92 insertions(+), 51 deletions(-) diff --git a/app/controllers/svg_sprite_controller.rb b/app/controllers/svg_sprite_controller.rb index 2a8e05958bd..266db140138 100644 --- a/app/controllers/svg_sprite_controller.rb +++ b/app/controllers/svg_sprite_controller.rb @@ -10,12 +10,13 @@ class SvgSpriteController < ApplicationController no_cookies RailsMultisite::ConnectionManagement.with_hostname(params[:hostname]) do + theme_ids = params[:theme_ids].split(",").map(&:to_i) - if SvgSprite.version != params[:version] - return redirect_to path(SvgSprite.path) + if SvgSprite.version(theme_ids) != params[:version] + return redirect_to path(SvgSprite.path(theme_ids)) end - svg_sprite = "window.__svg_sprite = #{SvgSprite.bundle.inspect};" + svg_sprite = "window.__svg_sprite = #{SvgSprite.bundle(theme_ids).inspect};" response.headers["Last-Modified"] = 10.years.ago.httpdate response.headers["Content-Length"] = svg_sprite.bytesize.to_s diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index 4963b958322..38b826b9a5a 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -382,7 +382,7 @@ module ApplicationHelper def theme_ids if customization_disabled? - nil + [nil] else request.env[:resolved_theme_ids] end @@ -454,11 +454,11 @@ module ApplicationHelper asset_version: Discourse.assets_digest, disable_custom_css: loading_admin?, highlight_js_path: HighlightJs.path, - svg_sprite_path: SvgSprite.path, + svg_sprite_path: SvgSprite.path(theme_ids), } if Rails.env.development? - setup_data[:svg_icon_list] = SvgSprite.all_icons + setup_data[:svg_icon_list] = SvgSprite.all_icons(theme_ids) end if guardian.can_enable_safe_mode? && params["safe_mode"] diff --git a/app/models/theme.rb b/app/models/theme.rb index 3b0be21fef0..6a7854ede14 100644 --- a/app/models/theme.rb +++ b/app/models/theme.rb @@ -121,6 +121,7 @@ class Theme < ActiveRecord::Base end def self.transform_ids(ids, extend: true) + return [] if ids.nil? get_set_cache "#{extend ? "extended_" : ""}transformed_ids_#{ids.join("_")}" do next [] if ids.blank? diff --git a/app/models/theme_field.rb b/app/models/theme_field.rb index 34c24f1f496..740eaf4dd69 100644 --- a/app/models/theme_field.rb +++ b/app/models/theme_field.rb @@ -7,6 +7,10 @@ class ThemeField < ActiveRecord::Base belongs_to :upload has_one :javascript_cache, dependent: :destroy + after_commit do |field| + SvgSprite.expire_cache if field.target_id == Theme.targets[:settings] + end + scope :find_by_theme_ids, ->(theme_ids) { return none unless theme_ids.present? diff --git a/config/routes.rb b/config/routes.rb index 955d0b1c16a..dca765bef86 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -453,7 +453,7 @@ Discourse::Application.routes.draw do # in most production settings this is bypassed get "letter_avatar_proxy/:version/letter/:letter/:color/:size.png" => "user_avatars#show_proxy_letter" - get "svg-sprite/:hostname/svg-:version.js" => "svg_sprite#show", format: false, constraints: { hostname: /[\w\.-]+/, version: /\h{40}/ } + get "svg-sprite/:hostname/svg-:theme_ids-:version.js" => "svg_sprite#show", format: false, constraints: { hostname: /[\w\.-]+/, version: /\h{40}/, theme_ids: /([0-9]+(,[0-9]+)*)?/ } get "svg-sprite/search/:keyword" => "svg_sprite#search", format: false, constraints: { keyword: /[-a-z0-9\s\%]+/ } get "highlight-js/:hostname/:version.js" => "highlight_js#show", format: false, constraints: { hostname: /[\w\.-]+/ } diff --git a/lib/svg_sprite/svg_sprite.rb b/lib/svg_sprite/svg_sprite.rb index 3717d59944d..217fa9772ce 100644 --- a/lib/svg_sprite/svg_sprite.rb +++ b/lib/svg_sprite/svg_sprite.rb @@ -188,39 +188,37 @@ module SvgSprite SVG_SPRITE_PATHS = Dir.glob(["#{Rails.root}/vendor/assets/svg-icons/**/*.svg", "#{Rails.root}/plugins/*/svg-icons/*.svg"]) - def self.svg_sprite_cache - @svg_sprite_cache ||= DistributedCache.new('svg_sprite') + def self.all_icons(theme_ids = []) + get_set_cache("icons_#{Theme.transform_ids(theme_ids).join(',')}") do + Set.new() + .merge(settings_icons) + .merge(plugin_icons) + .merge(badge_icons) + .merge(group_icons) + .merge(theme_icons(theme_ids)) + .delete_if { |i| i.blank? || i.include?("/") } + .map! { |i| process(i.dup) } + .merge(SVG_ICONS) + .sort + end end - def self.all_icons - Set.new() - .merge(settings_icons) - .merge(plugin_icons) - .merge(badge_icons) - .merge(group_icons) - .merge(theme_icons) - .delete_if { |i| i.blank? || i.include?("/") } - .map! { |i| process(i.dup) } - .merge(SVG_ICONS) - .sort + def self.version(theme_ids = []) + get_set_cache("version_#{Theme.transform_ids(theme_ids).join(',')}") do + Digest::SHA1.hexdigest(all_icons(theme_ids).join('|')) + end end - def self.rebuild_cache - icons = all_icons - svg_sprite_cache['icons'] = icons - svg_sprite_cache['version'] = Digest::SHA1.hexdigest(icons.join('|')) + def self.path(theme_ids = []) + "/svg-sprite/#{Discourse.current_hostname}/svg-#{theme_ids&.join(",")}-#{version(theme_ids)}.js" end def self.expire_cache - svg_sprite_cache.clear + cache&.clear end - def self.version - svg_sprite_cache['version'] || rebuild_cache - end - - def self.bundle - icons = svg_sprite_cache['icons'] || all_icons + def self.bundle(theme_ids = []) + icons = all_icons(theme_ids) doc = File.open("#{Rails.root}/vendor/assets/svg-icons/fontawesome/solid.svg") { |f| Nokogiri::XML(f) } fa_license = doc.at('//comment()').text @@ -240,7 +238,6 @@ Discourse SVG subset of #{fa_license} svg_file.css('symbol').each do |sym| icon_id = prepare_symbol(sym, svg_filename) - if icons.include? icon_id sym.attributes['id'].value = icon_id sym.css('title').each(&:remove) @@ -286,10 +283,6 @@ Discourse SVG subset of #{fa_license} icon_id end - def self.path - "/svg-sprite/#{Discourse.current_hostname}/svg-#{version}.js" - end - def self.settings_icons # includes svg_icon_subset and any settings containing _icon (incl. plugin settings) site_setting_icons = [] @@ -319,11 +312,11 @@ Discourse SVG subset of #{fa_license} Group.where("flair_url LIKE '%fa-%'").pluck(:flair_url).uniq end - def self.theme_icons + def self.theme_icons(theme_ids) theme_icon_settings = [] - # Theme.all includes default values - Theme.all.each do |theme| + # Need to load full records for default values + Theme.where(id: Theme.transform_ids(theme_ids)).each do |theme| settings = theme.cached_settings.each do |key, value| if key.to_s.include?("_icon") && String === value theme_icon_settings |= value.split('|') @@ -347,4 +340,12 @@ Discourse SVG subset of #{fa_license} FA_ICON_MAP.each { |k, v| icon_name.sub!(k, v) } fa4_to_fa5_names[icon_name] || icon_name end + + def self.get_set_cache(key) + cache[key] ||= yield + end + + def self.cache + @cache ||= DistributedCache.new('svg_sprite') + end end diff --git a/spec/components/svg_sprite/svg_sprite_spec.rb b/spec/components/svg_sprite/svg_sprite_spec.rb index 1d0fad90ccf..b33fc70d41e 100644 --- a/spec/components/svg_sprite/svg_sprite_spec.rb +++ b/spec/components/svg_sprite/svg_sprite_spec.rb @@ -3,7 +3,7 @@ require 'rails_helper' describe SvgSprite do before do - SvgSprite.rebuild_cache + SvgSprite.expire_cache end it 'can generate a bundle' do @@ -12,6 +12,15 @@ describe SvgSprite do expect(bundle).to match(/angle-double-down/) end + it 'can generate paths' do + version = SvgSprite.version # Icons won't change for this test + expect(SvgSprite.path).to eq("/svg-sprite/#{Discourse.current_hostname}/svg--#{version}.js") + expect(SvgSprite.path([1, 2])).to eq("/svg-sprite/#{Discourse.current_hostname}/svg-1,2-#{version}.js") + + # Safe mode + expect(SvgSprite.path([nil])).to eq("/svg-sprite/#{Discourse.current_hostname}/svg--#{version}.js") + end + it 'can search for a specific FA icon' do expect(SvgSprite.search("fa-heart")).to match(/heart/) expect(SvgSprite.search("poo-storm")).to match(/poo-storm/) @@ -51,26 +60,43 @@ describe SvgSprite do it 'includes icons defined in theme settings' do theme = Fabricate(:theme) - theme.set_field(target: :settings, name: :yaml, value: "custom_icon: magic") + + # Works for default settings: + theme.set_field(target: :settings, name: :yaml, value: "custom_icon: dragon") theme.save! + expect(SvgSprite.all_icons([theme.id])).to include("dragon") - # TODO: add test for default settings values + # Automatically purges cache when default changes: + theme.set_field(target: :settings, name: :yaml, value: "custom_icon: gamepad") + theme.save! + expect(SvgSprite.all_icons([theme.id])).to include("gamepad") + # Works when applying override theme.update_setting(:custom_icon, "gas-pump") - expect(SvgSprite.all_icons).to include("gas-pump") + expect(SvgSprite.all_icons([theme.id])).to include("gas-pump") + # Works when changing override theme.update_setting(:custom_icon, "gamepad") - expect(SvgSprite.all_icons).to include("gamepad") - expect(SvgSprite.all_icons).not_to include("gas-pump") + expect(SvgSprite.all_icons([theme.id])).to include("gamepad") + expect(SvgSprite.all_icons([theme.id])).not_to include("gas-pump") # FA5 syntax theme.update_setting(:custom_icon, "fab fa-bandcamp") - expect(SvgSprite.all_icons).to include("fab-bandcamp") + expect(SvgSprite.all_icons([theme.id])).to include("fab-bandcamp") # Internal Discourse syntax + multiple icons theme.update_setting(:custom_icon, "fab-android|dragon") - expect(SvgSprite.all_icons).to include("fab-android") - expect(SvgSprite.all_icons).to include("dragon") + expect(SvgSprite.all_icons([theme.id])).to include("fab-android") + expect(SvgSprite.all_icons([theme.id])).to include("dragon") + + # Check themes don't leak into non-theme sprite sheet + expect(SvgSprite.all_icons).not_to include("dragon") + + # Check components are included + theme.update(component: true) + parent_theme = Fabricate(:theme) + parent_theme.add_child_theme!(theme) + expect(SvgSprite.all_icons([parent_theme.id])).to include("dragon") end it 'includes icons from SiteSettings' do @@ -82,10 +108,12 @@ describe SvgSprite do expect(all_icons).to include("fab-bandcamp") SiteSetting.svg_icon_subset = nil + SvgSprite.expire_cache expect(SvgSprite.all_icons).not_to include("drafting-compass") # does not fail on non-string setting SiteSetting.svg_icon_subset = false + SvgSprite.expire_cache expect(SvgSprite.all_icons).to be_truthy end diff --git a/spec/requests/svg_sprite_controller_spec.rb b/spec/requests/svg_sprite_controller_spec.rb index 1004c3b11d3..8eec7bcaf40 100644 --- a/spec/requests/svg_sprite_controller_spec.rb +++ b/spec/requests/svg_sprite_controller_spec.rb @@ -4,17 +4,23 @@ describe SvgSpriteController do context 'show' do before do - SvgSprite.rebuild_cache + SvgSprite.expire_cache end it "should return bundle when version is current" do - get "/svg-sprite/#{Discourse.current_hostname}/svg-#{SvgSprite.version}.js" + get "/svg-sprite/#{Discourse.current_hostname}/svg--#{SvgSprite.version}.js" + expect(response.status).to eq(200) + + theme = Fabricate(:theme) + theme.set_field(target: :settings, name: :yaml, value: "custom_icon: dragon") + theme.save! + get "/svg-sprite/#{Discourse.current_hostname}/svg-#{theme.id}-#{SvgSprite.version([theme.id])}.js" expect(response.status).to eq(200) end it "should redirect to current version" do random_hash = Digest::SHA1.hexdigest("somerandomstring") - get "/svg-sprite/#{Discourse.current_hostname}/svg-#{random_hash}.js" + get "/svg-sprite/#{Discourse.current_hostname}/svg--#{random_hash}.js" expect(response.status).to eq(302) expect(response.location).to include(SvgSprite.version)