Community
Participate
Working Groups
With this code: int |buttonStyle = SWT.PUSH | SWT.WRAP; Button test = new Button(shell, buttonStyle); Activating Extract Constant from Ctrl-T menu next to the local variable buttonStyle (indicated by | character) causes an error message which says "Cannot extract this expression - it is not a valid static constant" to popup. Rather than an error message, it could instead activate the Convert Local Variable to Field refactoring dialog with the options pre-selected like this: Field name: changed to BUTTON_STYLE Initialize in: changed to Field declaration Declare field as 'static': set to checked Declare field as 'final': set to checked and Access modifier: not changed from default
It can be taken up as an enhancement, though it is not a very widely used scenario. If user invokes the Refactoring context menu and wishes to convert the local variable to a constant, then "Convert Local Variable to Field" refactoring can be directly selected instead of "Extract Constant".
Created attachment 242929 [details] Patch to implement the functionality I was about to start fiddling with the Convert Local Variable to Field dialog settings, again, but thought "enough is enough". Here's a patch which implements this functionality.
The patch invokes PromoteTempToFieldRefactoring on any VariableDeclaration where ExtractConstantRefactoring was invoked. For the example given below, if ExtractConstantRefactoring is invoked at 'test', it results in compile error: class S1 { private void foo() { int buttonStyle = SWT.PUSH | SWT.WRAP; Button test = new Button(null, buttonStyle); } } We also need to check all cases where ExtractConstantRefactoring can be invoked at a VariableDeclaration, and PromoteTempToFieldRefactoring is not feasible. See bug 436677 also. In a case like below, where the local variable is in an initializer, we still can't perform the refactoring. class S1 { { int |buttonStyle = SWT.PUSH | SWT.WRAP; Button test = new Button(null, buttonStyle); } }
Created attachment 244026 [details] Patch with added checks, initializer changes (In reply to comment #3) > The patch invokes PromoteTempToFieldRefactoring on any VariableDeclaration where > ExtractConstantRefactoring was invoked. > > For the example given below, if ExtractConstantRefactoring is invoked at 'test', > it results in compile error: > class S1 { > private void foo() { > int buttonStyle = SWT.PUSH | SWT.WRAP; > Button test = new Button(null, buttonStyle); > } > } This case can be fixed by adding checks for initial conditions and not setting options that aren't valid for the location. The convert temp refactoring can then proceed more like a regular Convert Local Variable to Field refactoring. > We also need to check all cases where ExtractConstantRefactoring can be invoked > at a VariableDeclaration, and PromoteTempToFieldRefactoring is not feasible. > See bug 436677 also. > > In a case like below, where the local variable is in an initializer, we still > can't perform the refactoring. > class S1 { > { > int |buttonStyle = SWT.PUSH | SWT.WRAP; > Button test = new Button(null, buttonStyle); > } > } This is a limitation in Convert Local Variable to Field refactoring. This can be fixed by relaxing restrictions to allow an initializer block in addition to method declaration as a parent AST node and checking the node type further down the line as needed.
The changes fixing this bug (from the last patch) are included in Evening-IDE, which is available here: http://overruler.github.io/Evening-IDE/
(In reply to Timo Kinnunen from comment #4) > Created attachment 244026 [details] [diff] > Patch with added checks, initializer changes I didn't go through the patch/result in detail. Here are some points based on initial observation that need to be fixed: - The patch doesn't extract a constant if the initializer/method is static: static { int |buttonStyle = SWT.PUSH | SWT.WRAP; Button test = new Button(null, buttonStyle); } - PromoteTempToFieldRefactoring.get*MethodDeclaration*() now returns a BodyDeclaration - Method name should also be changed. - In cases where extracting a constant is not possible (example: "test" above), it should specify that with the error message instead of opening the "Convert Local Variable to Field" wizard. - In cases where it extracts a constant via PromoteTempToFieldRefactoring (example: "buttonStyle" above), it is confusing that the user invokes "Extract Constant..." refactoring and the wizard title says "Convert Local Variable to Field". Probably the title can be changed accordingly. Also in such cases, we can disable the check boxes "Declare field as 'static'" and "Declare field as 'final'" that are checked, as the user already intended to make it static final by invoking "Extract Constant".
Created attachment 249301 [details] Patch 3 with initializer block improvements from review (In reply to comment #6) > I didn't go through the patch/result in detail. > Here are some points based on initial observation that need to be fixed: > > - The patch doesn't extract a constant if the initializer/method is static: > static { > int |buttonStyle = SWT.PUSH | SWT.WRAP; > Button test = new Button(null, buttonStyle); > } This worked for me with the earlier patch already. It also works for me in the attached patch. Please try expanding the selection (Shift-Alt-Up arrow) to be sure. > - PromoteTempToFieldRefactoring.get*MethodDeclaration*() now returns a > BodyDeclaration - Method name should also be changed. Addressed in the attached patch. > - In cases where extracting a constant is not possible (example: "test" above), > it should specify that with the error message instead of opening the "Convert > Local Variable to Field" wizard. Addressed in the attached patch by tweaking the logic which sets the values to the wizard. Extracting test in the example above is now possible without errors. > - In cases where it extracts a constant via PromoteTempToFieldRefactoring > (example: "buttonStyle" above), it is confusing that the user invokes "Extract > Constant..." refactoring and the wizard title says "Convert Local Variable to > Field". > Probably the title can be changed accordingly. Addressed in the attached patch by renaming the title of the wizard to "Convert Local Variable to Field or Constant" on account of a field that is static and final being known more commonly as a "constant". I also renamed the radio button "Current method" to "Current block" to make it better fit when used from static and instance initializer blocks. > Also in such cases, we can > disable the check boxes "Declare field as 'static'" and "Declare field as > 'final'" that are checked, as the user already intended to make it static final > by invoking "Extract Constant". Addressed partly in the attached patch by tweaking the logic which enables the radio buttons for where to initialize and check boxes for static and final modifiers. The patch errs on the permissive side where possible, so please try how it feels.
