Fix issue 4778: Do not show Active Line Highlight when editor has selection #4878
Conversation
ghost
assigned 
|
@lkcampbell In the future, I was wondering if you could use more descriptive pull request titles. It makes it easier to scan our overall list of PRs, which shows very little of the description (or the RepoMan view we sometimes use, which shows none of the description). Similarly, GitHub issue link tooltips and the like only show the title. |
|
@peterflynn, I updated the description. Thanks for the input. |
|
Thanks @lkcampbell! (Thanks for the fix here too btw :-) |
| editor._codeMirror.setOption("styleActiveLine", false); | ||
| } | ||
| } else { | ||
| editor._codeMirror.setOption("styleActiveLine", _styleActiveLine); |
There was a problem hiding this comment.
I'm seeing an issue with Highlight Active Line menu item unchecked after making a selection and then pressing left/right arrow to deselect. Even though we still see the active line being highlighted, the menu item is no longer checked. It seems that Editor.setShowActiveLine gets called at (or after) a selection is made and therefore _styleActiveLine becomes false.
Update: My steps above are wrong. You can see the issue with the following steps.
- Turn on Highlight Active Line in View menu if it is not enabled.
- Double-click on a word in the editor to select it.
- Go to View menu and uncheck Highlight Active Line while you still have selection in the editor.
- Press Left/Right arrow key to deselect the word in the editor.
- Open View menu and look at Highlight Active Line. It will be checked at this point but there is no active line highlight in the editor.
There was a problem hiding this comment.
@RaymondLim, I haven't been able to repro the problem using the steps you provided. Is there anything else you can provide me to help me recreate the issue?
There was a problem hiding this comment.
@lkcampbell
Sorry, my original steps may not be quite clear. I updated steps, especially adding step 1 to make sure that we start it with active line enabled.
There was a problem hiding this comment.
@RaymondLim, sorry I still can't repro the issue with your new steps. The test you are using is very basic so it surprises me that it is failing. It's not like you are jumping around between different main and inline editors or anything complex. Is it possible there is something out of sync with my pull request or your patched machine?
I'm going to try loading a fresh build and apply the patch myself and see if I can repro the issue that way. Would it be possible to have someone else try to patch on your end and see if they have the same problem?
There was a problem hiding this comment.
I strike out the first paragraph that is no longer true with my steps. And actually I should have posted the issue below on line 1570 (which I think is the culprit).
There was a problem hiding this comment.
I'm not even looking at the first paragraph and I'm not concerned about the line it was posted on.
What I am saying is I do the following steps (from your message above):
- Turn on Highlight Active Line in View menu if it is not enabled.
- Double-click on a word in the editor to select it.
- Go to View menu and uncheck Highlight Active Line while you still have selection in the editor.
- Press Left/Right arrow key to deselect the word in the editor.
- Open View menu and look at Highlight Active Line. It will be checked at this point but there is no active line highlight in the editor.
And I can't repro the issue.
First, we need to address the fact that you can repro problem and I can't.
There was a problem hiding this comment.
Sorry, my mistake. I was copying over your changes in Editor.js, but not your changes in EditorOptionHandlers.js. That's why I was seeing the issue that you can't reproduce.
|
Done with review. |
|
@RaymondLim, pull request updated. |
Fix issue 4778: Do not show Active Line Highlight when editor has selection
|
@lkcampbell |
|
@RaymondLim, good catch! Sorry about that. I did run unit tests but I was on the Unit tab instead of the All tab and I completely missed it. I will fix this tonight. |
Fix unit test failure from pull request #4878. Added unit test for hiding Active Line highlight when editor has selection.
This fix hides the Active Line Highlight when there is a selection in the current editor.
It does not address the general delay with the Shift-Down-Arrow, a different problem that started showing up in the Dev version of Sprint 30. It does, however, remove the really, really slow behavior associated with the having the Highlight Active Line turned on. The delay should now be the same, regardless of the Highlight Active Line setting.
I'm not sure I should be setting the CodeMirror Option on every single call to cursorActivity, but I didn't notice any performance issues by doing so. Let me know if you want me to set a flag or something to avoid excessive setOption calls.