Improve UX of measure filters

* two different actions for "Save" (an existing filter) and "Save as" (a new filter)
* replace the link "Edit" by a pencil icon on the left

10 files changed, 117 insertions, 77 deletions

diff --git a/sonar-server/src/main/webapp/WEB-INF/app/controllers/measures_controller.rb b/sonar-server/src/main/webapp/WEB-INF/app/controllers/measures_controller.rb
index 391c759f859..79417be5417 100644
--- a/sonar-server/src/main/webapp/WEB-INF/app/controllers/measures_controller.rb
+++ b/sonar-server/src/main/webapp/WEB-INF/app/controllers/measures_controller.rb
@@ -31,7 +31,7 @@ class MeasuresController < ApplicationController
     else
       @filter = MeasureFilter.new
     end
-    @filter.criteria=(params.merge({:controller => nil, :action => nil, :search => nil, :widget_id => nil}))
+    @filter.criteria=criteria_params
     @filter.enable_default_display
     @filter.execute(self, :user => current_user)
 
@@ -47,25 +47,31 @@ class MeasuresController < ApplicationController
 
     @filter = find_filter(params[:id])
     @filter.load_criteria_from_data
+    @filter.enable_default_display
 
     # criteria can be overridden
-    @filter.override_criteria(params.reject{|k,v| k=='controller' || k=='action' || k=='id'})
+    @filter.override_criteria(criteria_params)
+
+    @filter.execute(self, :user => current_user)
+    @unchanged = true
 
-    redirect_to @filter.criteria.merge({:controller => 'measures', :action => 'search', :id => params[:id]})
+    render :action => 'search'
   end
 
-  def save_form
+  # GET /measures/save_as_form?[id=<id>][&criteria]
+  def save_as_form
     if params[:id].present?
       @filter = find_filter(params[:id])
     else
       @filter = MeasureFilter.new
     end
-    @filter.criteria=(params)
+    @filter.criteria=(criteria_params)
     @filter.convert_criteria_to_data
-    render :partial => 'measures/save_form'
+    render :partial => 'measures/save_as_form'
   end
 
-  def save
+  # POST /measures/save_as?[id=<id>]&name=<name>[&parameters]
+  def save_as
     verify_post_request
     access_denied unless logged_in?
 
@@ -85,10 +91,25 @@ class MeasuresController < ApplicationController
       current_user.favourited_measure_filters<<@filter if add_to_favourites
       render :text => @filter.id.to_s, :status => 200
     else
-      render :partial => 'measures/save_form', :status => 400
+      render :partial => 'measures/save_as_form', :status => 400
     end
   end
 
+  # POST /measures/save?id=<id>&[criteria]
+  def save
+    verify_post_request
+    require_parameters :id
+    access_denied unless logged_in?
+
+    @filter = find_filter(params[:id])
+    @filter.criteria=criteria_params
+    @filter.convert_criteria_to_data
+    unless @filter.save
+      flash[:error]='Error'
+    end
+    redirect_to :action => 'filter', :id => @filter.id
+  end
+
   # GET /measures/manage
   def manage
     access_denied unless logged_in?
@@ -199,4 +220,8 @@ class MeasuresController < ApplicationController
     access_denied unless filter.shared || filter.owner?(current_user)
     filter
   end
+
+  def criteria_params
+    params.merge({:controller => nil, :action => nil, :search => nil, :widget_id => nil, :edit => nil})
+  end
 end
diff --git a/sonar-server/src/main/webapp/WEB-INF/app/controllers/treemap_controller.rb b/sonar-server/src/main/webapp/WEB-INF/app/controllers/treemap_controller.rb
index a2db5e892e8..5213d582629 100644
--- a/sonar-server/src/main/webapp/WEB-INF/app/controllers/treemap_controller.rb
+++ b/sonar-server/src/main/webapp/WEB-INF/app/controllers/treemap_controller.rb
@@ -41,11 +41,11 @@ class TreemapController < ApplicationController
     resource = resource.permanent_resource
 
     filter = MeasureFilter.new
-    filter.set_criteria_value('baseId', resource.id)
-    filter.set_criteria_value('onBaseComponents', 'true')
-    filter.set_criteria_value('display', 'treemap')
-    filter.set_criteria_value('tmSize', size_metric.key) if size_metric
-    filter.set_criteria_value('tmColor', color_metric.key) if color_metric
+    filter.set_criteria_value(:baseId, resource.id)
+    filter.set_criteria_value(:onBaseComponents, 'true')
+    filter.set_criteria_value(:display, 'treemap')
+    filter.set_criteria_value(:tmSize, size_metric.key) if size_metric
+    filter.set_criteria_value(:tmColor, color_metric.key) if color_metric
     filter.execute(self, :user => current_user)
 
     render :text => filter.display.html
diff --git a/sonar-server/src/main/webapp/WEB-INF/app/models/measure_filter.rb b/sonar-server/src/main/webapp/WEB-INF/app/models/measure_filter.rb
index 587b4eea749..ca208d907bc 100644
--- a/sonar-server/src/main/webapp/WEB-INF/app/models/measure_filter.rb
+++ b/sonar-server/src/main/webapp/WEB-INF/app/models/measure_filter.rb
@@ -66,7 +66,6 @@ class MeasureFilter < ActiveRecord::Base
 
   attr_reader :pagination, :security_exclusions, :base_row, :rows, :display
 
-
   def sort_key
     criteria['sort']
   end
@@ -127,7 +126,8 @@ class MeasureFilter < ActiveRecord::Base
     @criteria ||= HashWithIndifferentAccess.new
     if key
       if value!=nil && value!='' && value!=['']
-        @criteria[key]=(value.kind_of?(Array) ? value : value.to_s)
+        value = (value.kind_of?(Array) ? value : value.to_s)
+        @criteria[key]=value
       else
         @criteria.delete(key)
       end
@@ -136,7 +136,17 @@ class MeasureFilter < ActiveRecord::Base
 
   # API used by Displays
   def set_criteria_default_value(key, value)
-    set_criteria_value(key, value) unless criteria.has_key?(key)
+    @criteria ||= HashWithIndifferentAccess.new
+    unless @criteria.has_key?(key)
+      if key
+        if value!=nil && value!='' && value!=['']
+          value = (value.kind_of?(Array) ? value : value.to_s)
+          @criteria[key]=value
+        else
+          @criteria.delete(key)
+        end
+      end
+    end
   end
 
   def load_criteria_from_data
diff --git a/sonar-server/src/main/webapp/WEB-INF/app/models/measure_filter_display_list.rb b/sonar-server/src/main/webapp/WEB-INF/app/models/measure_filter_display_list.rb
index 20a64295ace..2a109537790 100644
--- a/sonar-server/src/main/webapp/WEB-INF/app/models/measure_filter_display_list.rb
+++ b/sonar-server/src/main/webapp/WEB-INF/app/models/measure_filter_display_list.rb
@@ -20,6 +20,7 @@
 require 'set'
 class MeasureFilterDisplayList < MeasureFilterDisplay
   KEY = :list
+  MAX_PAGE_SIZE = 200
 
   class Column
     attr_reader :key, :metric, :period
@@ -85,9 +86,9 @@ class MeasureFilterDisplayList < MeasureFilterDisplay
     # default values
     filter.set_criteria_default_value(:cols, ['metric:alert_status', 'name', 'date', 'metric:ncloc', 'metric:violations', 'links'])
     filter.set_criteria_default_value(:sort, 'name')
-    filter.set_criteria_default_value(:asc, 'true')
-    filter.set_criteria_default_value(:pageSize, '30')
-    filter.set_criteria_value(:pageSize, [filter.criteria[:pageSize].to_i, 200].min)
+    filter.set_criteria_default_value(:asc, true)
+    filter.set_criteria_default_value(:pageSize, 30)
+    filter.set_criteria_value(:pageSize, MAX_PAGE_SIZE) if filter.criteria(:pageSize).to_i>MAX_PAGE_SIZE
 
     @columns = []
     metrics = []
diff --git a/sonar-server/src/main/webapp/WEB-INF/app/models/measure_filter_display_treemap.rb b/sonar-server/src/main/webapp/WEB-INF/app/models/measure_filter_display_treemap.rb
index f4d70b014d9..bbaf3e8b765 100644
--- a/sonar-server/src/main/webapp/WEB-INF/app/models/measure_filter_display_treemap.rb
+++ b/sonar-server/src/main/webapp/WEB-INF/app/models/measure_filter_display_treemap.rb
@@ -22,7 +22,7 @@ class MeasureFilterDisplayTreemap < MeasureFilterDisplay
   include ActionView::Helpers::UrlHelper
 
   KEY = :treemap
-  PROPERTY_KEYS = Set.new([:tmSize, :tmColor])
+  PROPERTY_KEYS = Set.new([:tmSize, :tmColor, :tmHeight])
   MAX_RESULTS = 1000
   DEFAULT_HEIGHT_PERCENTS = 55
   attr_reader :id, :size, :size_metric, :color_metric, :height_percents
@@ -37,8 +37,6 @@ class MeasureFilterDisplayTreemap < MeasureFilterDisplay
     @filter.metrics=([@size_metric, @color_metric].compact)
     @id_count = 0
 
-    filter.set_criteria_value(:sort, "metric:#{@size_metric.key}") if @size_metric
-    filter.set_criteria_value(:asc, 'true')
     filter.set_criteria_value(:pageSize, MAX_RESULTS)
     filter.set_criteria_value(:page, 1)
   end
diff --git a/sonar-server/src/main/webapp/WEB-INF/app/views/measures/_display_list.html.erb b/sonar-server/src/main/webapp/WEB-INF/app/views/measures/_display_list.html.erb
index 9df1f2374d8..83654fa0081 100644
--- a/sonar-server/src/main/webapp/WEB-INF/app/views/measures/_display_list.html.erb
+++ b/sonar-server/src/main/webapp/WEB-INF/app/views/measures/_display_list.html.erb
@@ -5,8 +5,10 @@
       $j('#measure_filter_foot<%= widget_id -%>_pages').hide();
       $j('#measure_filter_foot<%= widget_id -%>_loading').show();
 
-      var url = decodeURI('<%= url_for filter.criteria.merge(:controller => 'measures', :action => 'search', :page => nil, :sort => nil, :asc => nil, :search => nil, :widget_id => widget_id, :trailing_slash => true) -%>');
-      url += '&sort=' + sort + '&asc=' + asc + '&page=' + page;
+      filterCriteria['sort']=sort;
+      filterCriteria['asc']=asc;
+      filterCriteria['page']=page;
+      var url=baseUrl + '/measures/search/?' + $j.param(filterCriteria);
 
       <% if widget_id %>
       $j('#measure_filter_list<%= widget_id -%>').load(url);
@@ -22,33 +24,32 @@
    display_favourites = logged_in?
    colspan = filter.display.columns.size
    colspan += 1 if display_favourites
-   table_columns = []
-   table_columns << '' if display_favourites
-   table_columns.concat(filter.display.columns.map { |c| c.key })
    if edit_mode
      content_for :script do
 %>
     <script>
-      var cols = [<%= table_columns.map{|col| "'#{col}'"}.join(',')-%>];
-
+      var colOffset = <%= display_favourites ? 1 : 0 -%>;
       function leftCol(id) {
         var cell = $j('#measures-table tr td[index=' + id + ']');
         var columnIndex = cell.parent().children().index(cell);
-        if (columnIndex > 0) {
+        if (columnIndex-colOffset > 0) {
           moveCol(columnIndex - 1, columnIndex);
         }
       }
       function rightCol(id) {
+        var cols = filterCriteria['cols']||[];
         var cell = $j('#measures-table tr td[index=' + id + ']');
         var columnIndex = cell.parent().children().index(cell);
-        if (columnIndex < cols.length - 1) {
+
+        if (columnIndex-colOffset < cols.length - 1) {
           moveCol(columnIndex, columnIndex + 1);
         }
       }
       function moveCol(from, to) {
-        var temp = cols[from];
-        cols[from] = cols[to];
-        cols[to] = temp;
+        var cols = filterCriteria['cols']||[];
+        var temp = cols[from-colOffset];
+        cols[from-colOffset] = cols[to-colOffset];
+        cols[to-colOffset] = temp;
         $j('#measures-table thead tr').each(function () {
           var tr = $j(this);
           var td1 = tr.find('th:eq(' + from + ')');
@@ -63,9 +64,10 @@
         });
       }
       function deleteCol(id) {
+        var cols = filterCriteria['cols']||[];
         var cell = $j('#measures-table tr td[index=' + id + ']');
         var columnIndex = cell.parent().children().index(cell);
-        cols.splice(columnIndex, 1);
+        cols.splice(columnIndex-colOffset, 1);
         $j('#measures-table thead tr').each(function () {
           var tr = $j(this);
           var td1 = tr.find('th:eq(' + columnIndex + ')');
@@ -96,23 +98,19 @@
 
         });
         $j("#add-metric").on("click", function (e) {
+          var cols = filterCriteria['cols']||[];
           var columnKey = $j("#select-metric option:selected").val();
           var period = $j("#select-period option:selected").val();
           if (period.length > 0) {
             columnKey += ':' + period;
           }
           cols.push(columnKey);
-          window.location = removeUrlAttr(decodeURI(window.location.href), 'cols\\[\\]') + '&' + $j.map(cols,function (a) {
-            return a ? 'cols[]=' + a : null;
-          }).join('&');
+          filterCriteria['edit']='true';
+          window.location = baseUrl + '/measures/search/<%= filter.id -%>?' + $j.param(filterCriteria);
         });
         $j("#exit-edit").on("click", function (e) {
-          var url = removeUrlAttr(decodeURI(window.location.href), 'cols\\[\\]');
-          url = removeUrlAttr(url, 'edit');
-          url += '&' + $j.map(cols,function (a) {
-            return a ? 'cols[]=' + a : null;
-          }).join('&');
-          window.location = url;
+          delete filterCriteria['edit'];
+          window.location = baseUrl + '/measures/search/<%= filter.id -%>?' + $j.param(filterCriteria);
         });
       });
     </script>
@@ -138,9 +136,6 @@
       </td>
       <td class="right">
         <a href="#" class="button" id="exit-edit"><%= message 'close' -%></a>
-        <% if filter.owner?(current_user) %>
-          <a id="save-columns" href="<%= url_for filter.criteria.merge({:edit => nil, :action => 'save_form', :id => filter.id}) -%>" class="button open-modal"><%= message('measure_filter.close_and_save') -%></a>
-        <% end %>
       </td>
     </tr>
   </table>
diff --git a/sonar-server/src/main/webapp/WEB-INF/app/views/measures/_display_treemap.html.erb b/sonar-server/src/main/webapp/WEB-INF/app/views/measures/_display_treemap.html.erb
index 91f47ceae51..9715c75e694 100644
--- a/sonar-server/src/main/webapp/WEB-INF/app/views/measures/_display_treemap.html.erb
+++ b/sonar-server/src/main/webapp/WEB-INF/app/views/measures/_display_treemap.html.erb
@@ -47,9 +47,6 @@
       <td valign="bottom"><%= image_tag 'loading.gif', :id => "tm-loading-#{treemap_id}", :style => 'display:none' -%></td>
       <td class="right" valign="bottom">
         <a href="#" class="button" id="exit-edit"><%= message 'close' -%></a>
-        <% if filter.owner?(current_user) %>
-          <a id="save-columns" href="<%= url_for params.merge({:action => 'save_form', :id => filter.id}) -%>" class="button open-modal"><%= message('measure_filter.close_and_save') -%></a>
-        <% end %>
       </td>
     </tr>
   </table>
@@ -60,17 +57,17 @@
     <script>
       $j(document).ready(function () {
         $j("#update-treemap").on("click", function (e) {
-          var url = removeUrlAttr(decodeURI(window.location.href), 'tmSize');
-          url = removeUrlAttr(url, 'tmColor');
-          url += '&tmSize=' + $j('#select-tm-size').val();
+          filterCriteria['tmSize']=$j('#select-tm-size').val();
           var color = $j('#select-tm-color').val();
           if (color != null && color != '') {
-            url += '&tmColor=' + color;
+            filterCriteria['tmColor']=color;
           }
-          window.location = url;
+          filterCriteria['edit']=true;
+          window.location = baseUrl + '/measures/search/<%= filter.id -%>?' + $j.param(filterCriteria);
         });
         $j("#exit-edit").on("click", function (e) {
-          window.location = removeUrlAttr(decodeURI(window.location.href), 'edit');
+          delete filterCriteria['edit'];
+          window.location = baseUrl + '/measures/search/<%= filter.id -%>?' + $j.param(filterCriteria);
         });
       });
     </script>
@@ -80,7 +77,7 @@
 <% if filter.rows.empty? %>
   <p><%= message('no_data') -%></p>
 <% elsif filter.pagination.pages>1 %>
-  <p><%= message('measure_filter.errors.TOO_MANY_RESULTS') -%></p>
+  <p><%= message('measure_filter.error.TOO_MANY_RESULTS') -%></p>
 <% else %>
   <div id="tm-<%= treemap_id -%>" class="treemap width100">
     <%= filter.display.html -%>
diff --git a/sonar-server/src/main/webapp/WEB-INF/app/views/measures/_edit_form.html.erb b/sonar-server/src/main/webapp/WEB-INF/app/views/measures/_edit_form.html.erb
index 530cfefd2e4..14af4202db5 100644
--- a/sonar-server/src/main/webapp/WEB-INF/app/views/measures/_edit_form.html.erb
+++ b/sonar-server/src/main/webapp/WEB-INF/app/views/measures/_edit_form.html.erb
@@ -28,6 +28,8 @@
   </fieldset>
 </form>
 <script>
-  $j("#edit-filter-form").modalForm();
+  $j("#edit-filter-form").modalForm({success: function (data) {
+    window.location = baseUrl + '/measures/filter/' + data;
+  }});
   $j('#name').select();
 </script>
\ No newline at end of file
diff --git a/sonar-server/src/main/webapp/WEB-INF/app/views/measures/_save_form.html.erb b/sonar-server/src/main/webapp/WEB-INF/app/views/measures/_save_as_form.html.erb
index f848acdbd07..d834e3ab733 100644
--- a/sonar-server/src/main/webapp/WEB-INF/app/views/measures/_save_form.html.erb
+++ b/sonar-server/src/main/webapp/WEB-INF/app/views/measures/_save_as_form.html.erb
@@ -1,4 +1,4 @@
-<form id="save-filter-form" method="post" action="<%= ApplicationController.root_context -%>/measures/save">
+<form id="save-as-filter-form" method="post" action="<%= ApplicationController.root_context -%>/measures/save_as">
   <input type="hidden" name="id" value="<%= @filter.id -%>">
   <input type="hidden" name="data" value="<%= u(@filter.data) -%>">
   <fieldset>
@@ -24,13 +24,13 @@
       </div>
     </div>
     <div class="form-foot">
-      <input type="submit" value="<%= h message('save') -%>" id="save-submit"/>
-      <a href="#" onclick="return closeModalWindow()" id="save-cancel"><%= h message('cancel') -%></a>
+      <input type="submit" value="<%= h message('save') -%>" id="save-as-submit"/>
+      <a href="#" onclick="return closeModalWindow()" id="save-as-cancel"><%= h message('cancel') -%></a>
     </div>
   </fieldset>
 </form>
 <script>
-  $j("#save-filter-form").modalForm({success:function (data) {
+  $j("#save-as-filter-form").modalForm({success:function (data) {
     window.location = baseUrl + '/measures/filter/' + data;
   }});
   $j('#name').select();
diff --git a/sonar-server/src/main/webapp/WEB-INF/app/views/measures/search.html.erb b/sonar-server/src/main/webapp/WEB-INF/app/views/measures/search.html.erb
index 045cb0ac4d1..4e116fb0763 100644
--- a/sonar-server/src/main/webapp/WEB-INF/app/views/measures/search.html.erb
+++ b/sonar-server/src/main/webapp/WEB-INF/app/views/measures/search.html.erb
@@ -3,7 +3,9 @@
     <%= render :partial => 'measures/sidebar' -%>
   </div>
 
-  <% if @filter %>
+  <% if @filter && @filter.display %>
+    <script>var filterCriteria = <%= @filter.criteria.to_json -%>;</script>
+
     <div class="page-split-right">
       <div id="content">
         <div class="line-block marginbottom10">
@@ -11,32 +13,39 @@
 
             <%
                if @filter.display
-                edit_mode = (params[:edit]=='true')
-                unless edit_mode
+                 edit_mode = (params[:edit]=='true')
+                 unless edit_mode
             %>
+                <li>
+                  <a id="change-display" href="<%= url_for @filter.criteria.merge({:action => 'search', :edit => true, :id => @filter.id}) -%>"><%= message("measure_filter.#{@filter.display.key}.change") -%></a>
+                </li>
+              <% end %>
               <li>
-                <a id="change-display" href="<%= url_for params.merge({:edit => true, :id => @filter.id}) -%>"><%= message("measure_filter.#{@filter.display.key}.change") -%></a>
+                <%= message 'measure_filter.display_as' -%>:
+                <% MeasureFilterDisplay.keys.each do |display_key| %>
+                  <%= link_to_if display_key!=@filter.display.key, message("measure_filter.display.#{display_key}"), @filter.criteria.merge(:action => 'search', :display => display_key, :id => @filter.id), :id => "display_as_#{display_key}" -%>
+                <% end %>
               </li>
             <% end %>
-            <li>
-              <%= message 'measure_filter.display_as' -%>:
-              <% MeasureFilterDisplay.keys.each do |display_key| %>
-                <%= link_to_if display_key!=@filter.display.key, message("measure_filter.display.#{display_key}"), params.merge(:action => 'search', :display => display_key, :id => @filter.id), :id => "display_as_#{display_key}" -%>
-              <% end %>
-            </li>
-            <% end %>
 
             <% if logged_in? %>
               <% if @filter.id %>
-                <li><a id="copy" href="<%= url_for params.merge({:action => 'copy_form', :id => @filter.id}) -%>" class="link-action open-modal"><%= message('copy') -%></a></li>
+                <li><a id="copy" href="<%= url_for :action => 'copy_form', :id => @filter.id -%>" class="link-action open-modal"><%= message('copy') -%></a></li>
               <% end %>
-              <% if @filter.owner?(current_user) %>
-                <li><a id="save" href="<%= url_for params.merge({:action => 'save_form', :id => @filter.id}) -%>" class="link-action open-modal"><%= message('save') -%></a></li>
+              <% if !defined?(@unchanged) && @filter.id && @filter.owner?(current_user)%>
+                <li>
+                  <%= link_to message('save'), params.merge({:action => 'save', :id => @filter.id}), :class => 'link-action', :id => 'save', :method => :post -%>
+                </li>
+              <% end %>
+              <% unless @filter.id %>
+                <li>
+                  <a id="save_as" href="<%= url_for params.merge({:action => 'save_as_form', :id => @filter.id}) -%>" class="link-action open-modal"><%= message('save_as') -%></a>
+                </li>
               <% end %>
             <% end %>
           </ul>
           <div class="page_title" id="filter-title">
-            <% if @filter.name.present? %>
+            <% if @filter.id && @filter.name.present? %>
               <span class="h3"><%= h @filter.name -%></span>
               <span class="note">
                 <% if !@filter.shared %>
@@ -52,6 +61,9 @@
               <% if @filter.description.present? %>
                 - <span class="note"><%= h @filter.description -%></span>
               <% end %>
+              <% if @filter.owner?(current_user) %>
+                &nbsp;<a href="<%= url_for :action => 'edit_form', :id => @filter.id -%>" class="open-modal" id="edit-filter"><%= image_tag 'pencil-small.png', :alt => message('edit') -%></a>
+              <% end %>
             <% end %>
           </div>
         </div>