Merge branch 'change-notes-order' of https://github.com/riyad/gitlabhq into...

Merge branch 'change-notes-order' of https://github.com/riyad/gitlabhq into riyad-change-notes-order

app/assets/javascripts/note.js

-var NoteList = {
-
-  notes_path: null,
-  target_params: null,
-  target_id: 0,
-  target_type: null,
-  first_id: 0,
-  last_id: 0,
-  disable:false,
-
-  init:
-    function(tid, tt, path) {
-      this.notes_path = path + ".js";
-      this.target_id = tid;
-      this.target_type = tt;
-      this.target_params = "&target_type=" + this.target_type + "&target_id=" + this.target_id;
-
-      // get notes
-      this.getContent();
-
-      // get new notes every n seconds
-      this.initRefresh();
-
-      $('.delete-note').live('ajax:success', function() {
-        $(this).closest('li').fadeOut(); });
-
-      $(".note-form-holder").live("ajax:before", function(){
-        $(".submit_note").disable()
-      })
-
-      $(".note-form-holder").live("ajax:complete", function(){
-        $(".submit_note").enable()
-      })
-
-      disableButtonIfEmptyField(".note-text", ".submit_note");
-
-      $(".note-text").live("focus", function(){
-        $(this).css("height", "80px");
-        $('.note_advanced_opts').show();
-      });
-
-      $("#note_attachment").change(function(e){
-        var val = $('.input-file').val();
-        var filename = val.replace(/^.*[\\\/]/, '');
-        $(".file_name").text(filename);
-      });
-
-    },
-
-
-  /**
-   * Load new notes to fresh list called 'new_notes_list': 
-   * - Replace 'new_notes_list' with new list every n seconds
-   * - Append new notes to this list after submit
-   */
-
-  initRefresh:
-    function() {
-      // init timer
-      var intNew = setInterval("NoteList.getNew()", 10000);
-    },
-
-  replace:
-    function(html) {
-      $("#new_notes_list").html(html);
-    },
-
-  prepend:
-    function(id, html) {
-      if(id != this.last_id) {
-        $("#new_notes_list").prepend(html);
-      }
-    },
-
-  getNew:
-    function() {
-      // refersh notes list
-      $.ajax({
-        type: "GET",
-      url: this.notes_path,
-      data: "last_id=" + this.last_id + this.target_params,
-      dataType: "script"});
-    },
-
-  refresh:
-    function() {
-      // refersh notes list
-      $.ajax({
-        type: "GET",
-      url: this.notes_path,
-      data: "first_id=" + this.first_id + "&last_id=" + this.last_id + this.target_params,
-      dataType: "script"});
-    },
-
-
-  /**
-   * Init load of notes: 
-   * 1. Get content with ajax call
-   * 2. Set content of notes list with loaded one
-   */
-
-
-  getContent: 
-    function() { 
-      $.ajax({
-        type: "GET",
-      url: this.notes_path,
-      data: "?" + this.target_params,
-      complete: function(){ $('.status').removeClass("loading")},
-      beforeSend: function() { $('.status').addClass("loading") },
-      dataType: "script"});
-    },
-
-  setContent:
-    function(fid, lid, html) {
-      this.last_id = lid;
-      this.first_id = fid;
-      $("#notes-list").html(html);
-
-      // Init infinite scrolling
-      this.initLoadMore();
-    },
-
-
-  /**
-   * Paging for old notes when scroll to bottom: 
-   * 1. Init scroll events with 'initLoadMore'
-   * 2. Load onlder notes with 'getOld' method
-   * 3. append old notes to bottom of list with 'append'
-   *
-   */
-  getOld:
-    function() {
-      $('.loading').show();
-      $.ajax({
-        type: "GET",
-        url: this.notes_path,
-        data: "first_id=" + this.first_id + this.target_params,
-        complete: function(){ $('.status').removeClass("loading")},
-        beforeSend: function() { $('.status').addClass("loading") },
-        dataType: "script"});
-    },
-
-  append:
-    function(id, html) {
-      if(this.first_id == id) { 
-        this.disable = true;
-      } else { 
-        this.first_id = id;
-        $("#notes-list").append(html);
-      }
-    },
-
-  initLoadMore:
-    function() {
-      $(document).endlessScroll({
-        bottomPixels: 400,
-      fireDelay: 1000,
-      fireOnce:true,
-      ceaseFire: function() { 
-        return NoteList.disable;
-      },
-      callback: function(i) {
-        NoteList.getOld();
-      }
-      });
-    }
-};
-
-var PerLineNotes = { 
-  init:
-    function() {
-      $(".line_note_link, .line_note_reply_link").live("click", function(e) {
-        var form = $(".per_line_form");
-        $(this).closest("tr").after(form);
-        form.find("#note_line_code").val($(this).attr("line_code"));
-        form.show();
-        return false;
-      });
-      disableButtonIfEmptyField(".line-note-text", ".submit_inline_note");
-    }
-}

app/assets/javascripts/notes.js

+var NoteList = {
+
+  notes_path: null,
+  target_params: null,
+  target_id: 0,
+  target_type: null,
+  top_id: 0,
+  bottom_id: 0,
+  loading_more_disabled: false,
+  reversed: false,
+
+  init:
+    function(tid, tt, path) {
+      this.notes_path = path + ".js";
+      this.target_id = tid;
+      this.target_type = tt;
+      this.reversed = $("#notes-list").hasClass("reversed");
+      this.target_params = "&target_type=" + this.target_type + "&target_id=" + this.target_id;
+
+      // get initial set of notes
+      this.getContent();
+
+      $("#notes-list, #new-notes-list").on("ajax:success", ".delete-note", function() {
+        $(this).closest('li').fadeOut();
+      });
+
+      $(".note-form-holder").on("ajax:before", function(){
+        $(".submit_note").disable()
+      })
+
+      $(".note-form-holder").on("ajax:complete", function(){
+        $(".submit_note").enable()
+      })
+
+      disableButtonIfEmptyField(".note-text", ".submit_note");
+
+      $(".note-text").on("focus", function(){
+        $(this).css("height", "80px");
+        $('.note_advanced_opts').show();
+      });
+
+      $("#note_attachment").change(function(e){
+        var val = $('.input-file').val();
+        var filename = val.replace(/^.*[\\\/]/, '');
+        $(".file_name").text(filename);
+      });
+    },
+
+
+  /**
+   * Handle loading the initial set of notes.
+   * And set up loading more notes when scrolling to the bottom of the page.
+   */
+
+
+  /**
+   * Gets an inital set of notes.
+   */
+  getContent:
+    function() {
+      $.ajax({
+        type: "GET",
+      url: this.notes_path,
+      data: "?" + this.target_params,
+      complete: function(){ $('.notes-status').removeClass("loading")},
+      beforeSend: function() { $('.notes-status').addClass("loading") },
+      dataType: "script"});
+    },
+
+  /**
+   * Called in response to getContent().
+   * Replaces the content of #notes-list with the given html.
+   */
+  setContent:
+    function(first_id, last_id, html) {
+      this.top_id = first_id;
+      this.bottom_id = last_id;
+      $("#notes-list").html(html);
+
+      // init infinite scrolling
+      this.initLoadMore();
+
+      // init getting new notes
+      if (this.reversed) {
+        this.initRefreshNew();
+      }
+    },
+
+
+  /**
+   * Handle loading more notes when scrolling to the bottom of the page.
+   * The id of the last note in the list is in this.bottom_id.
+   *
+   * Set up refreshing only new notes after all notes have been loaded.
+   */
+
+
+  /**
+   * Initializes loading more notes when scrolling to the bottom of the page.
+   */
+  initLoadMore:
+    function() {
+      $(document).endlessScroll({
+        bottomPixels: 400,
+        fireDelay: 1000,
+        fireOnce:true,
+        ceaseFire: function() {
+          return NoteList.loading_more_disabled;
+        },
+        callback: function(i) {
+          NoteList.getMore();
+        }
+      });
+    },
+
+  /**
+   * Gets an additional set of notes.
+   */
+  getMore:
+    function() {
+      // only load more notes if there are no "new" notes
+      $('.loading').show();
+      $.ajax({
+        type: "GET",
+        url: this.notes_path,
+        data: "loading_more=1&" + (this.reversed ? "before_id" : "after_id") + "=" + this.bottom_id + this.target_params,
+        complete: function(){ $('.notes-status').removeClass("loading")},
+        beforeSend: function() { $('.notes-status').addClass("loading") },
+        dataType: "script"});
+    },
+
+  /**
+   * Called in response to getMore().
+   * Append notes to #notes-list.
+   */
+  appendMoreNotes:
+    function(id, html) {
+      if(id != this.bottom_id) {
+        this.bottom_id = id;
+        $("#notes-list").append(html);
+      }
+    },
+
+  /**
+   * Called in response to getMore().
+   * Disables loading more notes when scrolling to the bottom of the page.
+   * Initalizes refreshing new notes.
+   */
+  finishedLoadingMore:
+    function() {
+      this.loading_more_disabled = true;
+
+      // from now on only get new notes
+      if (!this.reversed) {
+        this.initRefreshNew();
+      }
+    },
+
+
+  /**
+   * Handle refreshing and adding of new notes.
+   *
+   * New notes are all notes that are created after the site has been loaded.
+   * The "old" notes are in #notes-list the "new" ones will be in #new-notes-list.
+   * The id of the last "old" note is in this.bottom_id.
+   */
+
+
+  /**
+   * Initializes getting new notes every n seconds.
+   */
+  initRefreshNew:
+    function() {
+      setInterval("NoteList.getNew()", 10000);
+    },
+
+  /**
+   * Gets the new set of notes.
+   */
+  getNew:
+    function() {
+      $.ajax({
+        type: "GET",
+      url: this.notes_path,
+      data: "loading_new=1&after_id=" + (this.reversed ? this.top_id : this.bottom_id) + this.target_params,
+      dataType: "script"});
+    },
+
+  /**
+   * Called in response to getNew().
+   * Replaces the content of #new-notes-list with the given html.
+   */
+  replaceNewNotes:
+    function(html) {
+      $("#new-notes-list").html(html);
+    },
+
+  /**
+   * Adds a single note to #new-notes-list.
+   */
+  appendNewNote:
+    function(id, html) {
+      if (this.reversed) {
+        $("#new-notes-list").prepend(html);
+      } else {
+        $("#new-notes-list").append(html);
+      }
+    }
+};
+
+var PerLineNotes = {
+  init:
+    function() {
+      /**
+       * Called when clicking on the "add note" or "reply" button for a diff line.
+       *
+       * Shows the note form below the line.
+       * Sets some hidden fields in the form.
+       */
+      $(".diff_file_content").on("click", ".line_note_link, .line_note_reply_link", function(e) {
+        var form = $(".per_line_form");
+        $(this).closest("tr").after(form);
+        form.find("#note_line_code").val($(this).data("lineCode"));
+        form.show();
+        return false;
+      });
+
+      disableButtonIfEmptyField(".line-note-text", ".submit_inline_note");
+
+      /**
+       * Called in response to successfully deleting a note on a diff line.
+       *
+       * Removes the actual note from view.
+       * Removes the reply button if the last note for that line has been removed.
+       */
+      $(".diff_file_content").on("ajax:success", ".delete-note", function() {
+        var trNote = $(this).closest("tr");
+        trNote.fadeOut(function() {
+          $(this).remove();
+        });
+
+        // check if this is the last note for this line
+        // elements must really be removed for this to work reliably
+        var trLine = trNote.prev();
+        var trRpl  = trNote.next();
+        if (trLine.hasClass("line_holder") && trRpl.hasClass("reply")) {
+          trRpl.fadeOut(function() { $(this).remove(); });
+        }
+      });
+    }
+}

app/assets/stylesheets/sections/notes.scss

@@ -3,14 +3,17 @@
  *
  */
 #notes-list,
-#new_notes_list {
+#new-notes-list {
   display:block;
   list-style:none;
   margin:0px;
   padding:0px;
 }
 
-#new_notes_list li:last-child{
+#new-notes-list:not(.reversed) {
+  border-top:1px solid #aaa;
+}
+#new-notes-list.reversed {
   border-bottom:1px solid #aaa;
 }
 
@@ -48,7 +51,6 @@
 
 .note {
   padding: 8px 0;
-  border-bottom: 1px solid #eee;
   overflow: hidden;
   display: block;
   img {float: left; margin-right: 10px;}
@@ -70,6 +72,18 @@
     .delete-note { display:block; }
   }
 }
+#notes-list:not(.reversed) .note,
+#new-notes-list:not(.reversed) .note {
+  border-bottom: 1px solid #eee;
+}
+#notes-list.reversed .note,
+#new-notes-list.reversed .note {
+  border-top: 1px solid #eee;
+}
+
+.notes-status {
+  margin: 18px;
+}
 
 
 p.notify_controls input{

app/contexts/notes/load_context.rb

@@ -3,30 +3,31 @@ module Notes
     def execute
       target_type = params[:target_type]
       target_id   = params[:target_id]
-      first_id    = params[:first_id]
-      last_id     = params[:last_id]
+      after_id    = params[:after_id]
+      before_id   = params[:before_id]
 
 
       @notes = case target_type
-               when "commit" 
-                 then project.commit_notes(project.commit(target_id)).fresh.limit(20)
-               when "snippet"
-                 then  project.snippets.find(target_id).notes
-               when "wall"
-                 then project.common_notes.order("created_at DESC").fresh.limit(50)
+               when "commit"
+                 project.commit_notes(project.commit(target_id)).fresh.limit(20)
                when "issue"
-                 then project.issues.find(target_id).notes.inc_author.order("created_at DESC").limit(20)
+                 project.issues.find(target_id).notes.inc_author.fresh.limit(20)
                when "merge_request"
-                 then project.merge_requests.find(target_id).notes.inc_author.order("created_at DESC").limit(20)
+                 project.merge_requests.find(target_id).notes.inc_author.fresh.limit(20)
+               when "snippet"
+                 project.snippets.find(target_id).notes.fresh
+               when "wall"
+                 # this is the only case, where the order is DESC
+                 project.common_notes.order("created_at DESC, id DESC").limit(50)
                when "wiki"
-                 then project.wikis.reverse.map {|w| w.notes.fresh }.flatten[0..20]
+                 project.wikis.reverse.map {|w| w.notes.fresh }.flatten[0..20]
                end
 
-      @notes = if last_id
-                 @notes.where("id > ?", last_id)
-               elsif first_id
-                 @notes.where("id < ?", first_id)
-               else 
+      @notes = if after_id
+                 @notes.where("id > ?", after_id)
+               elsif before_id
+                 @notes.where("id < ?", before_id)
+               else
                  @notes
                end
     end

app/helpers/notes_helper.rb

+module NotesHelper
+  def loading_more_notes?
+    params[:loading_more].present?
+  end
+
+  def loading_new_notes?
+    params[:loading_new].present?
+  end
+end

app/models/note.rb

@@ -36,7 +36,7 @@ class Note < ActiveRecord::Base
   scope :today, where("created_at >= :date", date: Date.today)
   scope :last_week, where("created_at  >= :date", date: (Date.today - 7.days))
   scope :since, lambda { |day| where("created_at  >= :date", date: (day)) }
-  scope :fresh, order("created_at DESC")
+  scope :fresh, order("created_at ASC, id ASC")
   scope :inc_author_project, includes(:project, :author)
   scope :inc_author, includes(:author)
 

app/views/commits/_text_file.html.haml

@@ -13,14 +13,11 @@
         %td.old_line
           = link_to raw(type == "new" ? " " : line_old), "##{line_code}", id: line_code
           - if @comments_allowed
-            = link_to "", "#", class: "line_note_link", "line_code" => line_code, title: "Add note for this line"
+            = render "notes/per_line_note_link", line_code: line_code
         %td.new_line= link_to raw(type == "old" ? " " : line_new) , "##{line_code}", id: line_code
         %td.line_content{class: "noteable_line #{type} #{line_code}", "line_code" => line_code}= raw "#{line}  "
 
         - if @comments_allowed
-          - comments = @line_notes.select { |n| n.line_code == line_code }.sort_by(&:created_at).reverse
+          - comments = @line_notes.select { |n| n.line_code == line_code }.sort_by(&:created_at)
           - unless comments.empty?
-            - comments.each_with_index do |note, i|
-              = render "notes/reply_button", line_code: line_code if i.zero?
-              = render "notes/per_line_show", note: note
-              - @line_notes.reject!{ |n| n == note }
+            = render "notes/per_line_notes_with_reply", notes: comments

app/views/commits/show.html.haml

 = render "commits/commit_box"
 = render "commits/diffs", diffs: @commit.diffs
-= render "notes/notes", tid: @commit.id, tt: "commit"
+= render "notes/notes_with_form", tid: @commit.id, tt: "commit"
 = render "notes/per_line_form"
 
 

app/views/issues/show.html.haml

@@ -61,4 +61,4 @@
         = markdown @issue.description
 
 
-.issue_notes#notes= render "notes/notes", tid: @issue.id, tt: "issue"
+.issue_notes#notes= render "notes/notes_with_form", tid: @issue.id, tt: "issue"

app/views/merge_requests/_show.html.haml

@@ -16,7 +16,7 @@
         Diff
 
 .merge_request_notes#notes{ class: (controller.action_name == 'show') ? "" : "hide" }
-  = render("notes/notes", tid: @merge_request.id, tt: "merge_request")
+  = render("notes/notes_with_form", tid: @merge_request.id, tt: "merge_request")
 .merge-request-diffs
   = render "merge_requests/show/diffs" if @diffs
 .status

app/views/merge_requests/show.js.haml

 :plain
-  $(".merge-request-notes").html("#{escape_javascript(render("notes/notes", tid: @merge_request.id, tt: "merge_request"))}");
+  $(".merge-request-notes").html("#{escape_javascript(render notes/notes_with_form", tid: @merge_request.id, tt: "merge_request")}");

app/views/notes/_create_common.js.haml → app/views/notes/_create_common_note.js.haml

@@ -5,8 +5,9 @@
     $('.note-form-holder #preview-link').text('Preview');
     $('.note-form-holder #preview-note').hide();
     $('.note-form-holder').show();
-    NoteList.prepend(#{note.id}, "#{escape_javascript(render partial: "notes/show", locals: {note: note})}");
+    NoteList.appendNewNote(#{note.id}, "#{escape_javascript(render "notes/note", note: note)}");
+
 - else
   :plain
-    $(".note-form-holder").replaceWith("#{escape_javascript(render('form'))}");
+    $(".note-form-holder").replaceWith("#{escape_javascript(render 'form')}");
 

app/views/notes/_create_line.js.haml

-- if note.valid?
-  :plain
-    $(".per_line_form").hide();
-    $('.line-note-form-holder textarea').val("");
-    $("a.line_note_reply_link[line_code='#{note.line_code}']").closest("tr").remove();
-    var trEl = $(".#{note.line_code}").parent();
-    trEl.after("#{escape_javascript(render partial: "notes/per_line_show", locals: {note: note})}");
-    trEl.after("#{escape_javascript(render partial: "notes/reply_button", locals: {line_code: note.line_code})}");

app/views/notes/_create_per_line_note.js.haml

+- if note.valid?
+  :plain
+    // hide and reset the form
+    $(".per_line_form").hide();
+    $('.line-note-form-holder textarea').val("");
+
+    // find the reply button for this line
+    // (might not be there if this is the first note)
+    var trRpl = $("a.line_note_reply_link[data-line-code='#{note.line_code}']").closest("tr");
+    if (trRpl.size() == 0) {
+      // find the commented line ...
+      var trEl = $(".#{note.line_code}").parent();
+      // ... and insert the note and the reply button after it
+      trEl.after("#{escape_javascript(render "notes/per_line_reply_button", line_code: note.line_code)}");
+      trEl.after("#{escape_javascript(render "notes/per_line_note", note: note)}");
+    } else {
+      // instert new note before reply button
+      trRpl.before("#{escape_javascript(render "notes/per_line_note", note: note)}");
+    }

app/views/notes/_load.js.haml

-- unless @notes.blank?
-  - if params[:last_id]
-    :plain
-      NoteList.replace("#{escape_javascript(render(partial: 'notes/notes_list'))}");
-
-  - elsif params[:first_id]
-    :plain
-      NoteList.append(#{@notes.last.id}, "#{escape_javascript(render(partial: 'notes/notes_list'))}");
-
-  - else
-    :plain
-      NoteList.setContent(#{@notes.last.id}, #{@notes.first.id}, "#{escape_javascript(render(partial: 'notes/notes_list'))}");
-
-- else
-  - if params[:first_id]
-    :plain
-      NoteList.append(#{params[:first_id]}, "");

app/views/notes/_notes.html.haml

-- if can? current_user, :write_note, @project
-  = render "notes/form"
-.clear
-%hr
-%ul#new_notes_list
-%ul#notes-list
-.status
+- @notes.each do |note|
+  - next unless note.author
+  = render "note", note: note
 
-
-:javascript
-  $(function(){
-    NoteList.init("#{tid}", "#{tt}", "#{project_notes_path(@project)}");
-  });

app/views/notes/_notes_list.html.haml

-- @notes.each do |note|
-  - next unless note.author
-  = render partial: "notes/show", locals: {note: note}
-

app/views/notes/_notes_with_form.html.haml

+%ul#notes-list
+%ul#new-notes-list
+.notes-status
+
+- if can? current_user, :write_note, @project
+  = render "notes/common_form"
+
+:javascript
+  $(function(){
+    NoteList.init("#{tid}", "#{tt}", "#{project_notes_path(@project)}");
+  });

app/views/notes/_per_line_show.html.haml → app/views/notes/_per_line_note.html.haml

 %tr.line_notes_row
   %td{colspan: 3}
     %ul
-      = render partial: "notes/show", locals: {note: note}
+      = render "notes/note", note: note
 

app/views/notes/_per_line_note_link.html.haml

+= link_to "", "#", class: "line_note_link", data: { line_code: line_code }, title: "Add note for this line"

app/views/notes/_per_line_notes_with_reply.html.haml

+- notes.each do |note|
+  = render "notes/per_line_note", note: note
+= render "notes/per_line_reply_button", line_code: notes.first.line_code

app/views/notes/_reply_button.html.haml → app/views/notes/_per_line_reply_button.html.haml

 %tr.line_notes_row.reply
   %td{colspan: 3}
     %i.icon-comment
-    = link_to "Reply", "#", class: "line_note_reply_link", "line_code" => line_code, title: "Add note for this line"
+    = link_to "Reply", "#", class: "line_note_reply_link", data: { line_code: line_code }, title: "Add note for this line"

app/views/notes/_reversed_notes_with_form.html.haml

+- if can? current_user, :write_note, @project
+  = render "notes/common_form"
+
+%ul.reversed#new-notes-list
+%ul.reversed#notes-list
+.notes-status
+
+:javascript
+  $(function(){
+    NoteList.init("#{tid}", "#{tt}", "#{project_notes_path(@project)}");
+  });
\ No newline at end of file

app/views/notes/create.js.haml

 - if @note.line_code
-  = render "create_line", note: @note
+  = render "create_per_line_note", note: @note
 - else
-  = render "create_common", note: @note
+  = render "create_common_note", note: @note
 
 -# Enable submit button
 :plain

app/views/notes/index.js.haml

-= render "notes/load"
+- unless @notes.blank?
+  - if loading_more_notes?
+    :plain
+      NoteList.appendMoreNotes(#{@notes.last.id}, "#{escape_javascript(render 'notes/notes')}");
+
+  - elsif loading_new_notes?
+    :plain
+      NoteList.replaceNewNotes("#{escape_javascript(render 'notes/notes')}");
+
+  - else
+    :plain
+      NoteList.setContent(#{@notes.first.id}, #{@notes.last.id}, "#{escape_javascript(render 'notes/notes')}");
+
+- else
+  - if loading_more_notes?
+    :plain
+      NoteList.finishedLoadingMore();

app/views/projects/wall.html.haml

 %div.wall_page
-  = render "notes/notes", tid: nil, tt: "wall"
+  = render "notes/reversed_notes_with_form", tid: nil, tt: "wall"

app/views/snippets/show.html.haml

@@ -17,4 +17,4 @@
     %div{class: current_user.dark_scheme ? "black" : ""}
       = raw @snippet.colorize(options: { linenos: 'True'})
 
-= render "notes/notes", tid: @snippet.id, tt: "snippet"
+= render "notes/notes_with_form", tid: @snippet.id, tt: "snippet"

app/views/wikis/show.html.haml

@@ -21,4 +21,4 @@
     Delete this page
 
 %hr
-.wiki_notes#notes= render "notes/notes", tid: @wiki.id, tt: "wiki"
+.wiki_notes#notes= render "notes/notes_with_form", tid: @wiki.id, tt: "wiki"