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

JSRefactoring Bugs Fix #14455

Merged
merged 6 commits into from
Aug 2, 2018
Merged

JSRefactoring Bugs Fix #14455

merged 6 commits into from
Aug 2, 2018

Conversation

niteskum
Copy link
Collaborator

@niteskum niteskum commented Jun 28, 2018

This PR includes Fix for JSRefactoring Bug which are reported in dreamweaver
ping @navch @sobisht @boopeshmahendran @raman211 for review

@boopeshmahendran
Copy link
Contributor

@niteskum Can you provide a short description of the bugs that you are fixing?

@niteskum niteskum changed the title JSRefactoring Bugs Fix <WIP>JSRefactoring Bugs Fix Jun 29, 2018
@niteskum
Copy link
Collaborator Author

niteskum commented Jun 29, 2018

Bug Descriptions:

  1. On Variable Rename function code view scrolled to last mentioned line
  2. Extract to Variable refactors code incorrectly if repeated value is present in function
  3. Extract to Variable refactors code incorrectly if value is repeated on one line
  4. Create Getters/Setters doesn't work for properties without indent
  5. Create Getters/Setters works incorrectly if Object definition is in one-line code

@@ -37,11 +37,11 @@ define(function(require, exports, module) {
* Does the actual extraction. i.e Replacing the text, Creating a variable
* and multi select variable names
*/
function extract(scopes, parentStatement, expns, text) {
function extract(scopes, parentStatement, expns, text, insertPostion) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: typo insertPosition

@@ -63,6 +63,17 @@ define(function(require, exports, module) {
expns[i].start = doc.adjustPosForChange(expns[i].start, varDeclaration.split("\n"), insertStartPos, insertStartPos);
expns[i].end = doc.adjustPosForChange(expns[i].end, varDeclaration.split("\n"), insertStartPos, insertStartPos);

/* If there are multiple expressions . then second Expression onward
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of adjusting the position for every replace, can you explore replacing backwards, so that the position need not be adjusted for every change. You can refer (and maybe use) this function

Copy link
Collaborator Author

@niteskum niteskum Jul 3, 2018

Choose a reason for hiding this comment

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

@boopeshmahendran We need to highlight all the replaced text also so replacing backwards, or "MultipleEdits" function won't help here

@niteskum niteskum changed the title <WIP>JSRefactoring Bugs Fix JSRefactoring Bugs Fix Jul 2, 2018
@niteskum
Copy link
Collaborator Author

ping @nethip for review

}
for(var i = 0; i < refsArray.length; ++i) {
var element = refsArray[i];
if((element.start.line === currentPosition.line || element.end.line === currentPosition.line)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you can use refsArray.find.

current.editor.displayErrorMessageAtCursor(Strings.ERROR_GETTERS_SETTERS);
return;
}

//Get Current Selected Property End Index;
var propertyNodeArray = parentNode.properties;
for(var i=0; i<propertyNodeArray.length; ++i) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Here also maybe you can use find method

editor.setSelections(refs);
} else {
editor.setSelections(refs.filter(function(element) {
var currentPosition = editor._codeMirror.posFromIndex(refsResp.offset),
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use editor.posFromIndex

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants