From 995fa6122b930e4b680b4987338442c13b33382a Mon Sep 17 00:00:00 2001 From: James Allen Date: Mon, 20 Feb 2017 12:56:26 +0100 Subject: [PATCH 1/4] Scroll the review panel past the limits of Ace --- .../ide/editor/directives/aceEditor.coffee | 17 ++++++++++ .../controllers/ReviewPanelController.coffee | 3 ++ .../directives/reviewPanelSorted.coffee | 31 ++++++++++--------- 3 files changed, 36 insertions(+), 15 deletions(-) diff --git a/services/web/public/coffee/ide/editor/directives/aceEditor.coffee b/services/web/public/coffee/ide/editor/directives/aceEditor.coffee index 9205cb7b87..b3c607455b 100644 --- a/services/web/public/coffee/ide/editor/directives/aceEditor.coffee +++ b/services/web/public/coffee/ide/editor/directives/aceEditor.coffee @@ -184,6 +184,23 @@ define [ session = editor.getSession() session.setScrollTop(session.getScrollTop() + newScreenPosition - previousScreenPosition) + scope.$on "#{scope.name}:set-scroll-size", (e, size) -> + # Make sure that the editor has enough scroll margin above and below + # to scroll the review panel with the given size + marginChanged = false + if editor.renderer.scrollMargin.top != size.overflowTop + window.editors[0].renderer.scrollMargin.top = size.overflowTop + marginChanged = true + + maxHeight = editor.renderer.layerConfig.maxHeight + marginBottom = Math.max(size.height - maxHeight, 0) + if editor.renderer.scrollMargin.bottom != marginBottom + editor.renderer.scrollMargin.bottom = marginBottom + marginChanged = true + + if marginChanged + editor.renderer.updateFull() + scope.$watch "theme", (value) -> editor.setTheme("ace/theme/#{value}") diff --git a/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee b/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee index 88a3233934..d0af889dda 100644 --- a/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee +++ b/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee @@ -39,6 +39,9 @@ define [ $timeout () -> $scope.$broadcast "review-panel:layout" + $scope.$on "review-panel:sizes", (e, sizes) -> + $scope.$broadcast "editor:set-scroll-size", sizes + $scope.$watch "ui.pdfLayout", (layout) -> $scope.reviewPanel.layoutToLeft = (layout == "flat") diff --git a/services/web/public/coffee/ide/review-panel/directives/reviewPanelSorted.coffee b/services/web/public/coffee/ide/review-panel/directives/reviewPanelSorted.coffee index 4028406713..551658eacb 100644 --- a/services/web/public/coffee/ide/review-panel/directives/reviewPanelSorted.coffee +++ b/services/web/public/coffee/ide/review-panel/directives/reviewPanelSorted.coffee @@ -15,9 +15,11 @@ define [ if scope.ui.reviewPanelOpen PADDING = 8 TOOLBAR_HEIGHT = 38 + OVERVIEW_TOGGLE_HEIGHT = 57 else PADDING = 4 TOOLBAR_HEIGHT = 4 + OVERVIEW_TOGGLE_HEIGHT = 0 entries = [] for el in element.find(".rp-entry-wrapper") @@ -50,19 +52,6 @@ define [ sl_console.log "focused_entry_index", focused_entry_index - # As we go backwards, we run the risk of pushing things off the top of the editor. - # If we go through the entries before and assume they are as pushed together as they - # could be, we can work out the 'ceiling' that each one can't go through. I.e. the first - # on can't go beyond the toolbar height, the next one can't go beyond the bottom of the first - # one at this minimum height, etc. - heights = (entry.$layout_el.height() for entry in entries_before) - previousMinTop = TOOLBAR_HEIGHT - min_tops = [] - for height in heights - min_tops.push previousMinTop - previousMinTop += PADDING + height - min_tops.reverse() - positionLayoutEl = ($callout_el, original_top, top) -> if original_top <= top $callout_el.removeClass("rp-entry-callout-inverted") @@ -72,7 +61,7 @@ define [ $callout_el.css(top: top + line_height, height: original_top - top) # Put the focused entry as close to where it wants to be as possible - focused_entry_top = Math.max(previousMinTop, focused_entry.scope.entry.screenPos.y) + focused_entry_top = focused_entry.scope.entry.screenPos.y focused_entry.$box_el.css(top: focused_entry_top) focused_entry.$indicator_el.css(top: focused_entry_top) positionLayoutEl(focused_entry.$callout_el, focused_entry.scope.entry.screenPos.y, focused_entry_top) @@ -87,6 +76,8 @@ define [ entry.$indicator_el.css(top: top) positionLayoutEl(entry.$callout_el, original_top, top) sl_console.log "ENTRY", {entry: entry.scope.entry, top} + + lastBottom = previousBottom previousTop = focused_entry_top entries_before.reverse() # Work through backwards, starting with the one just above @@ -95,12 +86,22 @@ define [ height = entry.$layout_el.height() original_bottom = original_top + height bottom = Math.min(original_bottom, previousTop - PADDING) - top = Math.max(bottom - height, min_tops[i]) + top = bottom - height previousTop = top entry.$box_el.css(top: top) entry.$indicator_el.css(top: top) positionLayoutEl(entry.$callout_el, original_top, top) sl_console.log "ENTRY", {entry: entry.scope.entry, top} + + lastTop = top + if lastTop < TOOLBAR_HEIGHT + overflowTop = -lastTop + TOOLBAR_HEIGHT + else + overflowTop = 0 + scope.$emit "review-panel:sizes", { + overflowTop: overflowTop, + height: previousBottom + OVERVIEW_TOGGLE_HEIGHT + } scope.$applyAsync () -> layout() From b52e4a5d1c40081744744703a47eedb4c33f06ff Mon Sep 17 00:00:00 2001 From: James Allen Date: Mon, 20 Feb 2017 15:46:53 +0100 Subject: [PATCH 2/4] Reset scroll margins when changing document --- .../ide/editor/directives/aceEditor.coffee | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/services/web/public/coffee/ide/editor/directives/aceEditor.coffee b/services/web/public/coffee/ide/editor/directives/aceEditor.coffee index b3c607455b..faa1159e00 100644 --- a/services/web/public/coffee/ide/editor/directives/aceEditor.coffee +++ b/services/web/public/coffee/ide/editor/directives/aceEditor.coffee @@ -187,20 +187,25 @@ define [ scope.$on "#{scope.name}:set-scroll-size", (e, size) -> # Make sure that the editor has enough scroll margin above and below # to scroll the review panel with the given size - marginChanged = false - if editor.renderer.scrollMargin.top != size.overflowTop - window.editors[0].renderer.scrollMargin.top = size.overflowTop - marginChanged = true - + marginTop = size.overflowTop maxHeight = editor.renderer.layerConfig.maxHeight marginBottom = Math.max(size.height - maxHeight, 0) + setScrollMargins(marginTop, marginBottom) + + setScrollMargins = (marginTop, marginBottom) -> + marginChanged = false + if editor.renderer.scrollMargin.top != marginTop + editor.renderer.scrollMargin.top = marginTop + marginChanged = true if editor.renderer.scrollMargin.bottom != marginBottom editor.renderer.scrollMargin.bottom = marginBottom marginChanged = true - if marginChanged editor.renderer.updateFull() + resetScrollMargins = () -> + setScrollMargins(0,0) + scope.$watch "theme", (value) -> editor.setTheme("ace/theme/#{value}") @@ -325,6 +330,8 @@ define [ sharejs_doc.attachToAce(editor) editor.initing = false + resetScrollMargins() + # need to set annotations after attaching because attaching # deletes and then inserts document content session.setAnnotations scope.annotations From ad05cc288cfcb70a795b981462b4d2c780c0cbee Mon Sep 17 00:00:00 2001 From: James Allen Date: Mon, 20 Feb 2017 16:22:18 +0100 Subject: [PATCH 3/4] Do DOM reads first to prevent thrashing --- .../ide/review-panel/directives/reviewPanelSorted.coffee | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/services/web/public/coffee/ide/review-panel/directives/reviewPanelSorted.coffee b/services/web/public/coffee/ide/review-panel/directives/reviewPanelSorted.coffee index 551658eacb..f64b014410 100644 --- a/services/web/public/coffee/ide/review-panel/directives/reviewPanelSorted.coffee +++ b/services/web/public/coffee/ide/review-panel/directives/reviewPanelSorted.coffee @@ -33,6 +33,7 @@ define [ entry.$layout_el = entry.$box_el else entry.$layout_el = entry.$indicator_el + entry.height = entry.$layout_el.height() # Do all of our DOM reads first for perfomance, see http://wilsonpage.co.uk/preventing-layout-thrashing/ entries.push entry entries.sort (a,b) -> a.scope.entry.offset - b.scope.entry.offset @@ -69,7 +70,7 @@ define [ previousBottom = focused_entry_top + focused_entry.$layout_el.height() for entry in entries_after original_top = entry.scope.entry.screenPos.y - height = entry.$layout_el.height() + height = entry.height top = Math.max(original_top, previousBottom + PADDING) previousBottom = top + height entry.$box_el.css(top: top) @@ -83,7 +84,7 @@ define [ entries_before.reverse() # Work through backwards, starting with the one just above for entry, i in entries_before original_top = entry.scope.entry.screenPos.y - height = entry.$layout_el.height() + height = entry.height original_bottom = original_top + height bottom = Math.min(original_bottom, previousTop - PADDING) top = bottom - height From f466be96dd76ab3f6067ca32b23cd08b8fc76702 Mon Sep 17 00:00:00 2001 From: James Allen Date: Tue, 21 Feb 2017 09:58:49 +0100 Subject: [PATCH 4/4] Make sure first change isn't hidden under toolbar --- .../coffee/ide/review-panel/directives/reviewPanelSorted.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/web/public/coffee/ide/review-panel/directives/reviewPanelSorted.coffee b/services/web/public/coffee/ide/review-panel/directives/reviewPanelSorted.coffee index f64b014410..b76f891e7f 100644 --- a/services/web/public/coffee/ide/review-panel/directives/reviewPanelSorted.coffee +++ b/services/web/public/coffee/ide/review-panel/directives/reviewPanelSorted.coffee @@ -62,7 +62,7 @@ define [ $callout_el.css(top: top + line_height, height: original_top - top) # Put the focused entry as close to where it wants to be as possible - focused_entry_top = focused_entry.scope.entry.screenPos.y + focused_entry_top = Math.max(focused_entry.scope.entry.screenPos.y, TOOLBAR_HEIGHT) focused_entry.$box_el.css(top: focused_entry_top) focused_entry.$indicator_el.css(top: focused_entry_top) positionLayoutEl(focused_entry.$callout_el, focused_entry.scope.entry.screenPos.y, focused_entry_top)