Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Fix issue 4778: Do not show Active Line Highlight when editor has selection #4878

Merged
merged 3 commits into from
Aug 23, 2013
Merged

Fix issue 4778: Do not show Active Line Highlight when editor has selection #4878

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9aab054
Update toggling Highlight Active Line command
lkcampbell Aug 22, 2013
3222b1f
Add in cursorActivity listener and handler
lkcampbell Aug 22, 2013
aea644d
Added JSDoc and other comments
lkcampbell Aug 23, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion src/editor/Editor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,16 @@ define(function (require, exports, module) {
}
}

function _handleCursorActivity(jqEvent, editor, event) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to add JSDocs for the parameters.

if (editor.hasSelection()) {
if (editor._codeMirror.getOption("styleActiveLine")) {
editor._codeMirror.setOption("styleActiveLine", false);
}
} else {
editor._codeMirror.setOption("styleActiveLine", _styleActiveLine);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

  1. Turn on Highlight Active Line in View menu if it is not enabled.
  2. Double-click on a word in the editor to select it.
  3. Go to View menu and uncheck Highlight Active Line while you still have selection in the editor.
  4. Press Left/Right arrow key to deselect the word in the editor.
  5. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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):

  1. Turn on Highlight Active Line in View menu if it is not enabled.
  2. Double-click on a word in the editor to select it.
  3. Go to View menu and uncheck Highlight Active Line while you still have selection in the editor.
  4. Press Left/Right arrow key to deselect the word in the editor.
  5. 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}

function _handleKeyEvents(jqEvent, editor, event) {
_checkElectricChars(jqEvent, editor, event);

Expand Down Expand Up @@ -382,6 +392,7 @@ define(function (require, exports, module) {
this._installEditorListeners();

$(this)
.on("cursorActivity", _handleCursorActivity)
.on("keyEvent", _handleKeyEvents)
.on("change", this._handleEditorChange.bind(this));

Expand Down Expand Up @@ -1550,10 +1561,15 @@ define(function (require, exports, module) {
/**
* Sets show active line option and reapply it to all open editors.
* @param {boolean} value
* @param {Editor} Current editor
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: You need to have the param name editor after {Editor}.

*/
Editor.setShowActiveLine = function (value) {
Editor.setShowActiveLine = function (value, editor) {
_styleActiveLine = value;
_setEditorOptionAndPref(value, "styleActiveLine", "styleActiveLine");

if (editor.hasSelection()) {
editor._codeMirror.setOption("styleActiveLine", false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should not be overriding the option here. Instead, we should be checking editor.hasSelection() on line 1568 above and call _setEditorOptionAndPref() only if we don't have a selection. If we have a selection, then we only need to update preference, but not option since option was temporarily set to false when the user made a selection.

update: my mistake. Nothing wrong here.

}
};

/** @type {boolean} Returns true if show active line is enabled for all editors */
Expand Down
3 changes: 2 additions & 1 deletion src/editor/EditorOptionHandlers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ define(function (require, exports, module) {

var AppInit = require("utils/AppInit"),
Editor = require("editor/Editor").Editor,
EditorManager = require("editor/EditorManager"),
Commands = require("command/Commands"),
CommandManager = require("command/CommandManager"),
Strings = require("strings");
Expand All @@ -48,7 +49,7 @@ define(function (require, exports, module) {
* Activates/Deactivates showing active line option
*/
function _toggleActiveLine() {
Editor.setShowActiveLine(!Editor.getShowActiveLine());
Editor.setShowActiveLine(!Editor.getShowActiveLine(), EditorManager.getCurrentFullEditor());
CommandManager.get(Commands.TOGGLE_ACTIVE_LINE).setChecked(Editor.getShowActiveLine());
}

Expand Down