WIP: Retain the view position on update of FileViewer and GoToLine #5474

mstv · 2018-09-23T22:33:21Z

Fixes #5327
base functionality for #5328

Changes proposed in this pull request:

The following will be done in a dedicated PR for #5328:

show all assigned hotkeys in the context menus of the FileViewer
(optional?) show all assigned hotkeys in tooltips of toolbar buttons (Thoughts?)
check GoToLine after a .png was displayed
The context menu text shall be "Go to line... Ctrl+G", i.e. with dots "..." because it will open a subdialog.
The menu item shall be disabled if an image file or a binary diff is displayed.

Maybe a new issue:

in general? refresh FormCommit only if the GE application gets the focus again, not after own dialog window (Thoughts?)

Screenshots before and after (if PR changes UI):

none yet

What did I do to test the code and ensure quality:

manual testing see checklist above

Has been tested on (remove any that don't apply):

GIT 2.18.0.w.x64
Windows 7

…ons#5328, too

mstv · 2018-09-25T22:56:57Z

It is almost done. Are there concerns against my approach?
What else could be affected by removing FileViewer.SaveCurrentScrollPos, .ResetCurrentScrollPos, and .RestoreCurrentScrollPos?
@RussKie Who else should have a look at it?

RussKie · 2018-09-25T23:09:06Z

Apologies, I haven't had a chance to look, I have a lot of RL commitments atm that are taking precedence.
I will try to review it over the coming weekend.

mstv · 2018-09-25T23:23:09Z

I didn't want to push you. With the first commit and the WIP PR, I just wanted to indicate that I work on these issues.

Today, I mainly wanted to get some reinsurance. This can wait.

RussKie

I haven't had a chance to run it, just reviewed it in GH UI.

In general I very much like the idea of centralising the behaviour and removing it from consumers. I think it is a move in the right direction.

On a flip side I'm finding the new code somewhat difficult to comprehend. The lack of tests and testability (of the new code) is very concerning to me as well.
I totally appreciate the existing code isn't particularly extensible and doesn't lend itself well to testing, but we shouldn't continue with bad practices :)

At the moment I don't have any suggestions on how to restructure the code to be testable or easier to read, but I'll think about in coming days.

RussKie · 2018-09-27T11:54:05Z

GitUI/Editor/Diff/DiffViewerLineNumberControl.cs

        private bool _visible = true;

        public DiffViewerLineNumberControl(TextArea textArea) : base(textArea)
        {
            DiffLines = new Dictionary<int, DiffLineNum>();
        }

+        /// <summary>
+        /// returns the according line numbers or null if the caretLine is not mapped


Could you please rephrase the summary, it is quite confusing now

RussKie · 2018-09-27T12:10:10Z

GitUI/Editor/Diff/DiffViewerLineNumberControl.cs

@@ -14,21 +14,33 @@ internal class DiffViewerLineNumberControl : AbstractMargin

        private Dictionary<int, DiffLineNum> DiffLines { get; set; }

-        private int _maxValueOfLineNum;
+        public int MaxValueOfLineNum { get; private set; }


Please rename to MaxLineNumber to keep it consistent with DiffLineNumAnalyzer.Result.MaxLineNumber
Worth adding a comment explaining the purpose of the property

RussKie · 2018-09-27T12:28:23Z

GitUI/Editor/FileViewerInternal.cs


        public Action OpenWithDifftool { get; private set; }

+        private struct ViewPosition
+        {
+            internal string FirstLine; // contains the file names in case of a diff


Please change comments to proper xml-doc
Same for the rest of fields in the struct.

RussKie · 2018-09-27T12:28:48Z

GitUI/Editor/FileViewerInternal.cs

+        private struct ViewPosition
+        {
+            internal string FirstLine; // contains the file names in case of a diff
+            internal int TotalNumberOfLines; // if changed, CaretPosition and FirstVisibleLine must be ignored and the line number must be searched


The comment is not very clear on its intent

RussKie · 2018-09-27T12:52:45Z

GitUI/Editor/FileViewerInternal.cs

+                                _previousViewPosition.ActiveLineNum = null;
+                            }
+                        }
+                    }


We made a rule to keep all private functions (if they are necessary) at the end of parent functions, after return statement. If return statement is missing, it gets added.
Please move this function to the end.

RussKie · 2018-09-27T12:53:01Z

GitUI/Editor/FileViewerInternal.cs

+                    void SetActiveLineNum(int line)
+                    {
+                        _previousViewPosition.ActiveLineNum = _lineNumbersControl.GetLineNum(line);
+                        if (_previousViewPosition.ActiveLineNum != null)


Please invert all ifs to reduce nesting.
This applies to other cases as well.

RussKie · 2018-09-27T12:55:26Z

GitUI/Editor/FileViewerInternal.cs

+                int initialActiveLine = _previousViewPosition.CaretVisible ? _previousViewPosition.CaretPosition.Line : _previousViewPosition.FirstVisibleLine;
+                if (/*a diff was displayed*/!TextEditor.ShowLineNumbers)
+                {
+                    void SetActiveLineNum(int line)


I would make the method "purer", that is determine the value of ActiveLineNum and return it, rather than set it to _previousViewPosition.ActiveLineNum.

RussKie · 2018-09-27T13:14:53Z

GitUI/Editor/FileViewerInternal.cs

+            {
+                return lineNumber - 1;
+            }
+            else


else is redundant

RussKie · 2018-09-27T13:24:43Z

GitUI/Editor/FileViewerInternal.cs

+                _previousViewPosition.TotalNumberOfLines = TotalNumberOfLines;
+                _previousViewPosition.CaretPosition = TextEditor.ActiveTextAreaControl.Caret.Position;
+                _previousViewPosition.FirstVisibleLine = FirstVisibleLine;
+                _previousViewPosition.CaretVisible


This looks like the only place where CaretVisible is set. And for the most part it is based off what the struct already "knows" bar TextEditor.ActiveTextAreaControl.TextArea.TextView.VisibleLineCount.
It kind of feels it would be a good candidate for a method placed in ViewPosition however it also becomes dependent on the order in which other properties are set, which is equally bad...

* Rename DiffLineNumAnalyzer.Result -> DiffLineNumAnalyzer.DiffLinesInfo * Rename DiffLineNum -> DiffLineInfo * Rename DiffLineNum.DiffLineStyle -> DiffLineInfo.DiffLineType * Move DiffLineNumAnalyzer.AddToResult to DiffLinesInfo.Add * Rearrange order of few class members

RussKie · 2018-09-29T14:27:17Z

I had a little bit of time and tweaked the API slightly, these tweaks mainly deal with the overall hygiene of the code related to the PR (not necessarily originated by you).

* Extract DiffLinesInfo from DiffLineNumAnalyzer * Extract DiffLineType from DiffLineInfo

Extract functionality get/set of the current view position into own methods.

To faciliate testing `GitHighlightingStrategyBase` must depend on `IGitModule` instead of `GitModule`. This change requires a little hack because `IConfigFileSettings` does not expose `GetString(string, stirng)` that is exposed by `ConfigFileSettings` class. This will need to be reconciled separately later.

RussKie · 2018-09-30T02:52:31Z

Michael, please have a look at the refactor I pushed.
The code has been restructured to make it (more) modular, and that made the code testable.
I've also added a test scaffold that tests view position captures and restorations; it would be great if you could add tests for necessary use cases.

Eager to hear your thoughts.

GitUI/Editor/FileViewerInternal.cs

mstv · 2018-09-30T22:23:55Z

GitUI/Editor/FileViewerInternal.cs


-            if (isDiff)
-            {
                _lineNumbersControl.DisplayLineNumFor(text);


This might be too early now: _lineNumbersControl holds a reference to the TextEditor.ActiveTextAreaControl.TextArea but TextEditor.Text has not been updated yet.
I'll check whether this matters.

Please check, I grouped them together but the order may matter

GitUI/Editor/FileViewerInternal.cs

mstv · 2018-09-30T22:48:10Z

GitUI/Editor/RebaseTodoHighlightingStrategy.cs

-                            return false;
-                        }
+                            if (index == 1 && char.IsWhiteSpace(c))
+                            {


I guess, this additional indentation was not intended, was it?

No it wasn't. Looks like my VS settings are different

mstv · 2018-09-30T22:54:46Z

Thank you for the refactorings (I like the very descriptive identifier CurrentViewPositionCache) and for providing the test scaffold. It will make it much easier for me to write my first NUnit test cases.

RussKie · 2018-10-15T12:18:48Z

Superseded by #5563

mstv force-pushed the 5327_retain_diff_view_position branch from d7e3795 to a99382f Compare September 24, 2018 22:16

WIP: Implement gitextensions#5327, solves the main part of gitextensi…

4ea58a9

…ons#5328, too

mstv force-pushed the 5327_retain_diff_view_position branch from a99382f to 4ea58a9 Compare September 25, 2018 22:21

RussKie self-requested a review September 25, 2018 23:09

RussKie reviewed Sep 27, 2018

View reviewed changes

Tweak API

e98a0b4

* Rename DiffLineNumAnalyzer.Result -> DiffLineNumAnalyzer.DiffLinesInfo * Rename DiffLineNum -> DiffLineInfo * Rename DiffLineNum.DiffLineStyle -> DiffLineInfo.DiffLineType * Move DiffLineNumAnalyzer.AddToResult to DiffLinesInfo.Add * Rearrange order of few class members

RussKie added 2 commits September 30, 2018 09:21

Tweak API

84cd552

* Extract DiffLinesInfo from DiffLineNumAnalyzer * Extract DiffLineType from DiffLineInfo

Refactor FileViewerInternal.SetText

c292e7c

Extract functionality get/set of the current view position into own methods.

RussKie force-pushed the 5327_retain_diff_view_position branch from 232819f to c292e7c Compare September 29, 2018 23:21

RussKie added 2 commits September 30, 2018 12:44

Add test scaffold for CurrentViewPositionCache

af7fded

mstv commented Sep 30, 2018

View reviewed changes

RussKie closed this Oct 1, 2018

RussKie reopened this Oct 1, 2018

gitextensions deleted a comment from codecov bot Oct 1, 2018

RussKie mentioned this pull request Oct 3, 2018

Fix duplicate CRLF when copying #5506

Merged

RussKie mentioned this pull request Oct 15, 2018

5327 retain diff view position #5563

Merged

16 tasks

RussKie closed this in #5563 Oct 15, 2018

mstv deleted the 5327_retain_diff_view_position branch November 13, 2018 20:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Retain the view position on update of FileViewer and GoToLine #5474

WIP: Retain the view position on update of FileViewer and GoToLine #5474

mstv commented Sep 23, 2018 •

edited

Loading

mstv commented Sep 25, 2018

RussKie commented Sep 25, 2018

mstv commented Sep 25, 2018

RussKie left a comment

RussKie Sep 27, 2018

RussKie Sep 27, 2018

RussKie Sep 27, 2018

RussKie Sep 27, 2018

RussKie Sep 27, 2018

RussKie Sep 27, 2018

RussKie Sep 27, 2018

RussKie Sep 27, 2018

RussKie Sep 27, 2018

RussKie commented Sep 29, 2018

RussKie commented Sep 30, 2018

mstv Sep 30, 2018

RussKie Sep 30, 2018

mstv Sep 30, 2018

RussKie Sep 30, 2018

mstv commented Sep 30, 2018

RussKie commented Oct 15, 2018

WIP: Retain the view position on update of FileViewer and GoToLine #5474

WIP: Retain the view position on update of FileViewer and GoToLine #5474

Conversation

mstv commented Sep 23, 2018 • edited Loading

mstv commented Sep 25, 2018

RussKie commented Sep 25, 2018

mstv commented Sep 25, 2018

RussKie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RussKie commented Sep 29, 2018

RussKie commented Sep 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mstv commented Sep 30, 2018

RussKie commented Oct 15, 2018

mstv commented Sep 23, 2018 •

edited

Loading