From 99632d5e8a456f27229c699c6298c8dcb071c39b Mon Sep 17 00:00:00 2001 From: evazion Date: Sun, 30 Sep 2018 11:32:30 -0500 Subject: [PATCH 1/4] TagSetPresenter: refactor to pass options explicitly. Refactor tag_list_html, split_tag_list_html, and inline_tag_list_html to take the `show_extra_links` and `current_query` options explicitly, rather than implicitly relying on CurrentUser or taking `params[:tags]` from the template. --- .../post_set_presenters/favorite.rb | 11 ++---- app/presenters/post_set_presenters/pool.rb | 4 --- app/presenters/post_set_presenters/post.rb | 8 ++--- app/presenters/tag_set_presenter.rb | 36 +++++++++---------- app/presenters/upload_presenter.rb | 7 ++++ .../comments/partials/index/_header.html.erb | 2 +- app/views/favorites/index.html.erb | 2 +- .../moderator/post/queues/_post.html.erb | 2 +- app/views/posts/index.html.erb | 2 +- app/views/posts/show.html+tooltip.erb | 2 +- app/views/posts/show.html.erb | 2 +- app/views/uploads/index.html.erb | 2 +- 12 files changed, 36 insertions(+), 44 deletions(-) diff --git a/app/presenters/post_set_presenters/favorite.rb b/app/presenters/post_set_presenters/favorite.rb index 0d726b276..f08e454a1 100644 --- a/app/presenters/post_set_presenters/favorite.rb +++ b/app/presenters/post_set_presenters/favorite.rb @@ -1,7 +1,8 @@ module PostSetPresenters class Favorite < Base attr_accessor :post_set, :tag_set_presenter - delegate :favorites, :to => :post_set + delegate :favorites, :posts, :to => :post_set + delegate :tag_list_html, to: :tag_set_presenter def initialize(post_set) @post_set = post_set @@ -11,13 +12,5 @@ module PostSetPresenters ).map {|x| x[0]} ) end - - def tag_list_html(template) - tag_set_presenter.tag_list_html(template) - end - - def posts - @posts ||= post_set.posts - end end end diff --git a/app/presenters/post_set_presenters/pool.rb b/app/presenters/post_set_presenters/pool.rb index 4bba321ed..798ec4d6b 100644 --- a/app/presenters/post_set_presenters/pool.rb +++ b/app/presenters/post_set_presenters/pool.rb @@ -12,10 +12,6 @@ module PostSetPresenters ) end - def tag_list_html(template) - tag_set_presenter.tag_list_html(template) - end - def post_previews_html(template) html = "" diff --git a/app/presenters/post_set_presenters/post.rb b/app/presenters/post_set_presenters/post.rb index fbb7b473d..c0d62b13b 100644 --- a/app/presenters/post_set_presenters/post.rb +++ b/app/presenters/post_set_presenters/post.rb @@ -67,12 +67,8 @@ module PostSetPresenters SavedSearch.labels_for(CurrentUser.user.id).map {|x| "search:#{x}"} end - def tag_list_html(template, options = {}) - if post_set.is_saved_search? - options[:name_only] = true - end - - tag_set_presenter.tag_list_html(template, options) + def tag_list_html(**options) + tag_set_presenter.tag_list_html(name_only: post_set.is_saved_search?, **options) end end end diff --git a/app/presenters/tag_set_presenter.rb b/app/presenters/tag_set_presenter.rb index 307964484..b5cadc582 100644 --- a/app/presenters/tag_set_presenter.rb +++ b/app/presenters/tag_set_presenter.rb @@ -5,16 +5,18 @@ =end class TagSetPresenter < Presenter - def initialize(tags) - @tags = tags + attr_reader :tag_names, :tags + + def initialize(tag_names) + @tag_names = tag_names end - def tag_list_html(template, options = {}) + def tag_list_html(current_query: "", show_extra_links: false, name_only: false) html = "" - if @tags.present? + if tag_names.present? html << '" end @@ -22,7 +24,7 @@ class TagSetPresenter < Presenter html.html_safe end - def split_tag_list_html(template, category_list: TagCategory.split_header_list, headers: true, **options) + def split_tag_list_html(headers: true, category_list: TagCategory.split_header_list, current_query: "", show_extra_links: false, name_only: false, humanize_tags: true) html = "" category_list.each do |category| @@ -31,7 +33,7 @@ class TagSetPresenter < Presenter html << TagCategory.header_mapping[category] if headers html << %{" end @@ -41,9 +43,9 @@ class TagSetPresenter < Presenter end # compact (horizontal) list, as seen in the /comments index. - def inline_tag_list_html(template, classes: "inline-tag-list", **options) - html = split_tag_list_html(template, category_list: TagCategory.categorized_list, headers: false, name_only: true, humanize_tags: true, **options) - template.tag.span(html, class: classes) + def inline_tag_list_html(humanize_tags: true) + html = split_tag_list_html(category_list: TagCategory.categorized_list, headers: false, show_extra_links: false, name_only: true, humanize_tags: humanize_tags) + %{#{html}}.html_safe end private @@ -51,27 +53,25 @@ class TagSetPresenter < Presenter def typed_tags(name) @typed_tags ||= {} @typed_tags[name] ||= begin - @tags.select do |tag| + tag_names.select do |tag| categories[tag] == TagCategory.mapping[name] end end end def categories - @categories ||= Tag.categories_for(@tags) + @categories ||= Tag.categories_for(tag_names) end def counts - @counts ||= Tag.counts_for(@tags).inject({}) do |hash, x| + @counts ||= Tag.counts_for(tag_names).inject({}) do |hash, x| hash[x["name"]] = x["post_count"] hash end end - def build_list_item(tag, template, name_only: false, humanize_tags: true, show_extra_links: CurrentUser.is_gold?) - html = "" - html << %{
  • } - current_query = template.params[:tags] || "" + def build_list_item(tag, name_only: false, humanize_tags: true, show_extra_links: false, current_query: "") + html = %{
  • } unless name_only if categories[tag] == Tag.categories.artist diff --git a/app/presenters/upload_presenter.rb b/app/presenters/upload_presenter.rb index 44a1cce2e..d132530eb 100644 --- a/app/presenters/upload_presenter.rb +++ b/app/presenters/upload_presenter.rb @@ -1,5 +1,12 @@ class UploadPresenter < Presenter + attr_reader :upload + delegate :inline_tag_list_html, to: :tag_set_presenter + def initialize(upload) @upload = upload end + + def tag_set_presenter + @tag_set_presenter ||= TagSetPresenter.new(upload.tag_string.split) + end end diff --git a/app/views/comments/partials/index/_header.html.erb b/app/views/comments/partials/index/_header.html.erb index 83c48d12a..735bc541c 100644 --- a/app/views/comments/partials/index/_header.html.erb +++ b/app/views/comments/partials/index/_header.html.erb @@ -26,7 +26,7 @@
    Tags - <%= post.presenter.inline_tag_list_html(self) %> + <%= post.presenter.inline_tag_list_html %>
    diff --git a/app/views/favorites/index.html.erb b/app/views/favorites/index.html.erb index 630e0fcbe..ea14cc280 100644 --- a/app/views/favorites/index.html.erb +++ b/app/views/favorites/index.html.erb @@ -9,7 +9,7 @@

    Tags

    - <%= @favorite_set.presenter.tag_list_html(self) %> + <%= @favorite_set.presenter.tag_list_html %>
    diff --git a/app/views/posts/index.html.erb b/app/views/posts/index.html.erb index bae084e0c..a7a2ca72a 100644 --- a/app/views/posts/index.html.erb +++ b/app/views/posts/index.html.erb @@ -9,7 +9,7 @@

    Tags

    - <%= @post_set.presenter.tag_list_html(self) %> + <%= @post_set.presenter.tag_list_html(current_query: params[:tags], show_extra_links: CurrentUser.user.is_gold?) %>
    <%= render "posts/partials/index/options" %> diff --git a/app/views/posts/show.html+tooltip.erb b/app/views/posts/show.html+tooltip.erb index d9d896c33..a4bf9f04c 100644 --- a/app/views/posts/show.html+tooltip.erb +++ b/app/views/posts/show.html+tooltip.erb @@ -46,5 +46,5 @@ <% end %> - <%= @post.presenter.inline_tag_list_html(self, humanize_tags: false) %> + <%= @post.presenter.inline_tag_list_html(humanize_tags: false) %> diff --git a/app/views/posts/show.html.erb b/app/views/posts/show.html.erb index dddda0107..4e9e6a440 100644 --- a/app/views/posts/show.html.erb +++ b/app/views/posts/show.html.erb @@ -6,7 +6,7 @@ <%= render "posts/partials/index/blacklist" %>
    - <%= @post.presenter.split_tag_list_html(self) %> + <%= @post.presenter.split_tag_list_html(current_query: params[:tags], show_extra_links: CurrentUser.user.is_gold?) %>
    diff --git a/app/views/uploads/index.html.erb b/app/views/uploads/index.html.erb index dc526624b..70dbe995d 100644 --- a/app/views/uploads/index.html.erb +++ b/app/views/uploads/index.html.erb @@ -57,7 +57,7 @@ Tags - <%= TagSetPresenter.new(upload.tag_string.split).inline_tag_list_html(self) %> + <%= upload.presenter.inline_tag_list_html %> From b1f2096d721a5dc93a155e162fda5ef29099aee2 Mon Sep 17 00:00:00 2001 From: evazion Date: Sun, 30 Sep 2018 13:21:12 -0500 Subject: [PATCH 2/4] TagSetPresenter: refactor *_tag_list_html to avoid memcache calls. Refactor the tag set presenter to get both the tag categories and the tag counts in the same call to the database, instead of getting the counts from the db and the categories from memcache. --- app/models/tag.rb | 4 -- app/presenters/tag_set_presenter.rb | 79 ++++++++++++++--------------- 2 files changed, 38 insertions(+), 45 deletions(-) diff --git a/app/models/tag.rb b/app/models/tag.rb index 2d3ad47c3..135bd3c46 100644 --- a/app/models/tag.rb +++ b/app/models/tag.rb @@ -56,10 +56,6 @@ class Tag < ApplicationRecord extend ActiveSupport::Concern module ClassMethods - def counts_for(tag_names) - select_all_sql("SELECT name, post_count FROM tags WHERE name IN (?)", tag_names) - end - def highest_post_count Cache.get("highest-post-count", 4.hours) do select("post_count").order("post_count DESC").first.post_count diff --git a/app/presenters/tag_set_presenter.rb b/app/presenters/tag_set_presenter.rb index b5cadc582..e8b499010 100644 --- a/app/presenters/tag_set_presenter.rb +++ b/app/presenters/tag_set_presenter.rb @@ -5,7 +5,8 @@ =end class TagSetPresenter < Presenter - attr_reader :tag_names, :tags + extend Memoist + attr_reader :tag_names def initialize(tag_names) @tag_names = tag_names @@ -13,9 +14,10 @@ class TagSetPresenter < Presenter def tag_list_html(current_query: "", show_extra_links: false, name_only: false) html = "" - if tag_names.present? + + if ordered_tags.present? html << '
      ' - tag_names.each do |tag| + ordered_tags.each do |tag| html << build_list_item(tag, current_query: current_query, show_extra_links: show_extra_links, name_only: name_only) end html << "
    " @@ -28,7 +30,8 @@ class TagSetPresenter < Presenter html = "" category_list.each do |category| - typetags = typed_tags(category) + typetags = ordered_tags.select { |tag| tag.category == Tag.categories.value_for(category) } + if typetags.any? html << TagCategory.header_mapping[category] if headers html << %{
      } @@ -50,60 +53,54 @@ class TagSetPresenter < Presenter private - def typed_tags(name) - @typed_tags ||= {} - @typed_tags[name] ||= begin - tag_names.select do |tag| - categories[tag] == TagCategory.mapping[name] - end - end - end - - def categories - @categories ||= Tag.categories_for(tag_names) - end - - def counts - @counts ||= Tag.counts_for(tag_names).inject({}) do |hash, x| - hash[x["name"]] = x["post_count"] - hash + def tags + Tag.where(name: tag_names).select(:name, :post_count, :category) + end + memoize :tags + + def ordered_tags + names_to_tags = tags.map { |tag| [tag.name, tag] }.to_h + + tag_names.map do |name| + names_to_tags[name] || Tag.new(name: name).freeze end end + memoize :ordered_tags def build_list_item(tag, name_only: false, humanize_tags: true, show_extra_links: false, current_query: "") - html = %{
    • } + name = tag.name + count = tag.post_count + category = tag.category + + html = %{
    • } unless name_only - if categories[tag] == Tag.categories.artist - html << %{? } + if category == Tag.categories.artist + html << %{? } else - html << %{? } + html << %{? } end if show_extra_links && current_query.present? - html << %{+ } - html << %{– } + html << %{+ } + html << %{– } end end - humanized_tag = humanize_tags ? tag.tr("_", " ") : tag - if categories[tag] == Tag.categories.artist - itemprop = 'itemprop="author"' - else - itemprop = nil - end - html << %{#{h(humanized_tag)} } + humanized_tag = humanize_tags ? name.tr("_", " ") : name + itemprop = 'itemprop="author"' if category == Tag.categories.artist + html << %{#{h(humanized_tag)} } - unless name_only - if counts[tag].to_i >= 10_000 - post_count = "#{counts[tag].to_i / 1_000}k" - elsif counts[tag].to_i >= 1_000 - post_count = "%.1fk" % (counts[tag].to_f / 1_000) + unless name_only || tag.new_record? + if count >= 10_000 + post_count = "#{count / 1_000}k" + elsif count >= 1_000 + post_count = "%.1fk" % (count / 1_000.0) else - post_count = counts[tag].to_s + post_count = count end - is_underused_tag = counts[tag].to_i <= 1 && categories[tag] == Tag.categories.general + is_underused_tag = count <= 1 && category == Tag.categories.general klass = "post-count#{is_underused_tag ? " low-post-count" : ""}" title = "New general tag detected. Check the spelling or populate it now." From 739bb1270c4ee139e81c20481691f959e3586325 Mon Sep 17 00:00:00 2001 From: evazion Date: Sun, 30 Sep 2018 17:46:33 -0500 Subject: [PATCH 3/4] TagSetPresenter: refactor tag string for post edit form. Move PostPresenter#categorized_tag_groups to TagSetPresenter#split_tag_list_text. This allows split_tag_list_text to reuse the same set of tags already fetched by the tag set presenter for the sidebar. This avoids a memcache call to get the tag categories when rendering the tag string for the post edit form. --- app/presenters/post_presenter.rb | 18 +------------- app/presenters/tag_set_presenter.rb | 25 +++++++++++++++++--- app/views/posts/partials/show/_edit.html.erb | 2 +- 3 files changed, 24 insertions(+), 21 deletions(-) diff --git a/app/presenters/post_presenter.rb b/app/presenters/post_presenter.rb index f90d44789..e38e24690 100644 --- a/app/presenters/post_presenter.rb +++ b/app/presenters/post_presenter.rb @@ -1,6 +1,6 @@ class PostPresenter < Presenter attr_reader :pool, :next_post_in_pool - delegate :tag_list_html, :split_tag_list_html, :inline_tag_list_html, to: :tag_set_presenter + delegate :tag_list_html, :split_tag_list_html, :split_tag_list_text, :inline_tag_list_html, to: :tag_set_presenter def self.preview(post, options = {}) if post.nil? @@ -151,22 +151,6 @@ class PostPresenter < Presenter "#{humanized_essential_tag_string} - #{@post.md5}.#{@post.file_ext}" end - def categorized_tag_groups - string = [] - - TagCategory.categorized_list.each do |category| - if @post.typed_tags(category).any? - string << @post.typed_tags(category).join(" ") - end - end - - string - end - - def categorized_tag_string - categorized_tag_groups.join(" \n") - end - def safe_mode_message(template) html = ["This image is unavailable on safe mode (#{Danbooru.config.app_name}). Go to "] html << template.link_to("Danbooru", "https://danbooru.donmai.us") # XXX don't hardcode. diff --git a/app/presenters/tag_set_presenter.rb b/app/presenters/tag_set_presenter.rb index e8b499010..aaa83655c 100644 --- a/app/presenters/tag_set_presenter.rb +++ b/app/presenters/tag_set_presenter.rb @@ -8,6 +8,9 @@ class TagSetPresenter < Presenter extend Memoist attr_reader :tag_names + # @param [Array] a list of tags to present. Tags will be presented in + # the order given. The list should not contain duplicates. The list may + # contain tags that do not exist in the tags table, such as metatags. def initialize(tag_names) @tag_names = tag_names end @@ -30,7 +33,7 @@ class TagSetPresenter < Presenter html = "" category_list.each do |category| - typetags = ordered_tags.select { |tag| tag.category == Tag.categories.value_for(category) } + typetags = tags_for_category(category) if typetags.any? html << TagCategory.header_mapping[category] if headers @@ -51,12 +54,27 @@ class TagSetPresenter < Presenter %{#{html}}.html_safe end + # the list of tags inside the tag box in the post edit form. + def split_tag_list_text(category_list: TagCategory.categorized_list) + category_list.map do |category| + tags_for_category(category).map(&:name).join(" ") + end.join(" \n") + end + private def tags Tag.where(name: tag_names).select(:name, :post_count, :category) end - memoize :tags + + def tags_by_category + ordered_tags.group_by(&:category) + end + + def tags_for_category(category_name) + category = TagCategory.mapping[category_name.downcase] + tags_by_category[category] || [] + end def ordered_tags names_to_tags = tags.map { |tag| [tag.name, tag] }.to_h @@ -65,7 +83,6 @@ class TagSetPresenter < Presenter names_to_tags[name] || Tag.new(name: name).freeze end end - memoize :ordered_tags def build_list_item(tag, name_only: false, humanize_tags: true, show_extra_links: false, current_query: "") name = tag.name @@ -110,4 +127,6 @@ class TagSetPresenter < Presenter html << "
      • " html end + + memoize :tags, :tags_by_category, :ordered_tags end diff --git a/app/views/posts/partials/show/_edit.html.erb b/app/views/posts/partials/show/_edit.html.erb index 2b5ddc82a..c838e2ecd 100644 --- a/app/views/posts/partials/show/_edit.html.erb +++ b/app/views/posts/partials/show/_edit.html.erb @@ -78,7 +78,7 @@
      <%= f.label :tag_string, "Tags" %> - <%= f.text_area :tag_string, :size => "50x5", :value => post.presenter.categorized_tag_string + " ", :data => { :autocomplete => "tag-edit" } %> + <%= f.text_area :tag_string, :size => "50x5", :value => post.presenter.split_tag_list_text + " ", :data => { :autocomplete => "tag-edit" } %>
      From 88a177e1d58f6cb60c690a9627802c6153a68fb1 Mon Sep 17 00:00:00 2001 From: evazion Date: Sun, 30 Sep 2018 20:39:12 -0500 Subject: [PATCH 4/4] TagSetPresenter: refactor humanized_essential_tag_string. Move Post#humanized_essential_tag_string to TagSetPresenter#humanized_essential_tag_string. This allows humanized_essential_tag_string to reuse the same set of tags already fetched by the tag set presenter for the sidebar. This avoids fetching the tag categories from memcache again (via Post#typed_tags) when we're already fetched the tags once before. This also means it's no longer necessary to cache humanized_essential_tag_string itself in memcache, since it can be generated as quickly as the sidebar taglist. --- app/logical/storage_manager.rb | 2 +- app/models/post.rb | 31 -------------------------- app/presenters/post_presenter.rb | 2 +- app/presenters/tag_set_presenter.rb | 32 ++++++++++++++++++++++++++- app/views/comments/index.atom.builder | 2 +- 5 files changed, 34 insertions(+), 35 deletions(-) diff --git a/app/logical/storage_manager.rb b/app/logical/storage_manager.rb index 2937d6b3c..121887a0e 100644 --- a/app/logical/storage_manager.rb +++ b/app/logical/storage_manager.rb @@ -115,7 +115,7 @@ class StorageManager def seo_tags(post) return "" if !tagged_filenames - tags = post.humanized_essential_tag_string.gsub(/[^a-z0-9]+/, "_").gsub(/(?:^_+)|(?:_+$)/, "").gsub(/_{2,}/, "_") + tags = post.presenter.humanized_essential_tag_string.gsub(/[^a-z0-9]+/, "_").gsub(/(?:^_+)|(?:_+$)/, "").gsub(/_{2,}/, "_") "__#{tags}__" end end diff --git a/app/models/post.rb b/app/models/post.rb index 93944b4a9..477380a4f 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -36,7 +36,6 @@ class Post < ApplicationRecord after_save :create_version after_save :update_parent_on_save after_save :apply_post_metatags - after_save :expire_essential_tag_string_cache after_commit :delete_files, :on => :destroy after_commit :remove_iqdb_async, :on => :destroy after_commit :update_iqdb_async, :on => :create @@ -936,36 +935,6 @@ class Post < ApplicationRecord end end - def expire_essential_tag_string_cache - Cache.delete("hets-#{id}") - end - - def humanized_essential_tag_string - @humanized_essential_tag_string ||= Cache.get("hets-#{id}", 1.hour.to_i) do - string = [] - - TagCategory.humanized_list.each do |category| - typetags = typed_tags(category) - TagCategory.humanized_mapping[category]["exclusion"] - if TagCategory.humanized_mapping[category]["slice"] > 0 - typetags = typetags.slice(0,TagCategory.humanized_mapping[category]["slice"]) + (typetags.length > TagCategory.humanized_mapping[category]["slice"] ? ["others"] : []) - end - if TagCategory.humanized_mapping[category]["regexmap"] != // - typetags = typetags.map do |tag| - tag.match(TagCategory.humanized_mapping[category]["regexmap"])[1] - end - end - if typetags.any? - if category != "copyright" || typed_tags("character").any? - string << TagCategory.humanized_mapping[category]["formatstr"] % typetags.to_sentence - else - string << typetags.to_sentence - end - end - end - string.empty? ? "##{id}" : string.join(" ").tr("_", " ") - end - end - TagCategory.categories.each do |category| define_method("tag_string_#{category}") do typed_tags(category).join(" ") diff --git a/app/presenters/post_presenter.rb b/app/presenters/post_presenter.rb index e38e24690..f58fa782d 100644 --- a/app/presenters/post_presenter.rb +++ b/app/presenters/post_presenter.rb @@ -144,7 +144,7 @@ class PostPresenter < Presenter end def humanized_essential_tag_string - @post.humanized_essential_tag_string + @humanized_essential_tag_string ||= tag_set_presenter.humanized_essential_tag_string(default: "##{@post.id}") end def filename_for_download diff --git a/app/presenters/tag_set_presenter.rb b/app/presenters/tag_set_presenter.rb index aaa83655c..42432d778 100644 --- a/app/presenters/tag_set_presenter.rb +++ b/app/presenters/tag_set_presenter.rb @@ -61,6 +61,36 @@ class TagSetPresenter < Presenter end.join(" \n") end + def humanized_essential_tag_string(category_list: TagCategory.humanized_list, default: "") + strings = category_list.map do |category| + mapping = TagCategory.humanized_mapping[category] + max_tags = mapping["slice"] + regexmap = mapping["regexmap"] + formatstr = mapping["formatstr"] + excluded_tags = mapping["exclusion"] + + type_tags = tags_for_category(category).map(&:name) - excluded_tags + next if type_tags.empty? + + if max_tags > 0 && type_tags.length > max_tags + type_tags = type_tags.take(max_tags) + ["others"] + end + + if regexmap != // + type_tags = type_tags.map { |tag| tag.match(regexmap)[1] } + end + + if category == "copyright" && tags_for_category("character").blank? + type_tags.to_sentence + else + formatstr % type_tags.to_sentence + end + end + + strings = strings.compact.join(" ").tr("_", " ") + strings.blank? ? default : strings + end + private def tags @@ -128,5 +158,5 @@ class TagSetPresenter < Presenter html end - memoize :tags, :tags_by_category, :ordered_tags + memoize :tags, :tags_by_category, :ordered_tags, :humanized_essential_tag_string end diff --git a/app/views/comments/index.atom.builder b/app/views/comments/index.atom.builder index 19e1c8b75..14508448b 100644 --- a/app/views/comments/index.atom.builder +++ b/app/views/comments/index.atom.builder @@ -9,7 +9,7 @@ atom_feed(root_url: comments_url(host: Danbooru.config.hostname)) do |feed| @comments.each do |comment| feed.entry(comment, published: comment.created_at, updated: comment.updated_at) do |entry| - entry.title("@#{comment.creator_name} on post ##{comment.post_id} (#{comment.post.humanized_essential_tag_string})") + entry.title("@#{comment.creator_name} on post ##{comment.post_id} (#{comment.post.presenter.humanized_essential_tag_string})") entry.content(<<-EOS.strip_heredoc, type: "html")