Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ui] Allow to setup the auxclasspath in the designer #1159

Merged
merged 20 commits into from Jun 8, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -23,6 +23,7 @@
import net.sourceforge.pmd.lang.ast.Node;
import net.sourceforge.pmd.lang.symboltable.NameDeclaration;
import net.sourceforge.pmd.lang.symboltable.NameOccurrence;
import net.sourceforge.pmd.util.fxdesigner.util.AuxClassPathController;
import net.sourceforge.pmd.util.fxdesigner.util.DesignerUtil;
import net.sourceforge.pmd.util.fxdesigner.util.LimitedSizeStack;
import net.sourceforge.pmd.util.fxdesigner.util.beans.SettingsOwner;
Expand Down Expand Up @@ -73,8 +74,11 @@ public class MainDesignerController implements Initializable, SettingsOwner {
*/
private final DesignerRoot designerRoot;


/* Menu bar */
@FXML
private MenuItem pmdconfig;
@FXML
private MenuItem openFileMenuItem;
@FXML
private MenuItem licenseMenuItem;
Expand Down Expand Up @@ -111,6 +115,8 @@ public class MainDesignerController implements Initializable, SettingsOwner {
private SourceEditorController sourceEditorController;
@FXML
private EventLogController eventLogPanelController;
@FXML
Copy link
Member

Choose a reason for hiding this comment

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

This should not be annotated @FXML. The annotation is only for fields that should be injected by the FxmlLoader. The other controllers are injected because their FXML fragment is included in designer.fxml. Here you must create a new controller yourself. I'll push to your branch to show you

private AuxClassPathController auxClassPathController;

// Other fields
private Stack<File> recentFiles = new LimitedSizeStack<>(5);
Expand Down Expand Up @@ -161,6 +167,16 @@ public void initialize(URL location, ResourceBundle resources) {
}
});

pmdconfig.setOnAction(e -> {
try {
auxClassPathController.showAuxPathWizard();
} catch (Exception e1) {
e1.printStackTrace();
}
});



sourceEditorController.refreshAST();
xpathPanelController.evaluateXPath(sourceEditorController.getCompilationUnit(),
getLanguageVersion());
Expand Down
@@ -0,0 +1,83 @@
/**
* BSD-style license; for more info see http://pmd.sourceforge.net/license.html
*/


package net.sourceforge.pmd.util.fxdesigner.util;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this package is the right one for this... we don't have any controllers in it, and controllers are not utilities... I'd move this up to net.sourceforge.pmd.util.fxdesigner or maybe even create a separate package for controllers that are modals or split by screen... @oowekyala ?

Copy link
Member

Choose a reason for hiding this comment

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

Oh yes good catch

Yeah making a new package for popups and such is a good idea.


import java.io.File;
import java.net.URL;
import java.util.ResourceBundle;

import net.sourceforge.pmd.util.fxdesigner.DesignerRoot;
import net.sourceforge.pmd.util.fxdesigner.MainDesignerController;
import net.sourceforge.pmd.util.fxdesigner.util.beans.SettingsOwner;

import javafx.fxml.FXML;
import javafx.fxml.FXMLLoader;
import javafx.fxml.Initializable;
import javafx.scene.Parent;
import javafx.scene.Scene;
import javafx.scene.control.Button;
import javafx.scene.control.TableColumn;
import javafx.scene.control.TableView;
import javafx.stage.FileChooser;
import javafx.stage.Stage;

public class AuxClassPathController implements Initializable, SettingsOwner {

private final DesignerRoot designerRoot;

@FXML
private Button removeFiles;

@FXML
private TableView fileTable;

@FXML
private Button selectFile;

@FXML
private TableColumn fileList;

@FXML
private TableColumn fileAdd;

@FXML
private final MainDesignerController parent;


public AuxClassPathController(DesignerRoot designerRoot, MainDesignerController parent) {
this.designerRoot = designerRoot;
this.parent = parent;
}


@Override
public void initialize(URL location, ResourceBundle resources) {

selectFile.setOnAction(e -> fileSeleced());

}


private void fileSeleced() {
Copy link
Member

Choose a reason for hiding this comment

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

Typo

FileChooser chooser = new FileChooser();
Copy link
Member

Choose a reason for hiding this comment

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

You should add an extension filter to only allow selecting jar files. There's an example here.

chooser.setTitle("Load source from file");
Copy link
Member

Choose a reason for hiding this comment

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

"Load source from file"? That's a copy-paste error

File file = chooser.showOpenDialog(designerRoot.getMainStage());
Copy link
Member

Choose a reason for hiding this comment

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

Also, you should use showOpenMultipleDialog

Copy link
Member

@jsotuyod jsotuyod May 30, 2018

Choose a reason for hiding this comment

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

and don't forget to filter file types:

            chooser.getExtensionFilters().addAll(
                new FileChooser.ExtensionFilter("Java JARs", "*.jar"),
                new FileChooser.ExtensionFilter("Java WARs", "*.war"),
                new FileChooser.ExtensionFilter("Java EARs", "*.ear"),
                new FileChooser.ExtensionFilter("Java class files", "*.class"),
            );


}


public void showAuxPathWizard() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

Encapsulating this logic here is actually a very nice idea. I'll refactor the other instances of this problem to follow that design principle


FXMLLoader fxmlLoader = new FXMLLoader(getClass().getResource("fxml/aux-controller.fxml"));
Parent root1 = (Parent) fxmlLoader.load();
Stage stage = new Stage();
Copy link
Member

Choose a reason for hiding this comment

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

And your stage should be modal

stage.setScene(new Scene(root1));
stage.show();

}


}
@@ -0,0 +1,57 @@
<?xml version="1.0" encoding="UTF-8"?>

<?import javafx.scene.control.Button?>
<?import javafx.scene.control.Label?>
<?import javafx.scene.control.TableColumn?>
<?import javafx.scene.control.TableView?>
<?import javafx.scene.control.TitledPane?>
<?import javafx.scene.effect.Blend?>
<?import javafx.scene.layout.AnchorPane?>
<?import javafx.scene.layout.BorderPane?>
<?import javafx.scene.layout.Pane?>
<AnchorPane prefHeight="444.0" prefWidth="594.0" xmlns="http://javafx.com/javafx/9.0.1"
xmlns:fx="http://javafx.com/fxml/1"
fx:controller="net.sourceforge.pmd.util.fxdesigner.util.AuxClassPathController">
<children>
<BorderPane layoutX="14.0" layoutY="14.0" prefHeight="374.0" prefWidth="571.0">
<top>
<TitledPane animated="false" prefHeight="420.0" prefWidth="571.0" text="PMD Configure"
textAlignment="CENTER" BorderPane.alignment="CENTER">
<content>
<AnchorPane minHeight="0.0" minWidth="0.0" prefHeight="542.0" prefWidth="569.0">
<children>
<Pane layoutX="17.0" layoutY="95.0" prefHeight="260.0" prefWidth="536.0">
<children>
<TableView id="fileSelected" layoutX="34.0" layoutY="30.0" prefHeight="200.0"
prefWidth="172.0">
<columns>
<TableColumn fx:id="fileList" prefWidth="171.0" text="Files Selected"/>
</columns>
Copy link
Member

Choose a reason for hiding this comment

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

The file name is wrong

The controller is the Java object of type XXXController. It adds logic to the UI. The FXML file doesn't control anything, instead it is controlled by the controller. It's the View in the Model-View-Controller pattern. This is a pretty basic concept which you should be aware of, see here and on the wikipedia page for example.

Consider naming it auxclasspath-setup-popup.fxml for example

Also, no offence but the ui is substandard for now

  • Your TitledPane should not be collapsible.
  • You've hardcoded every width and height. You should rather use anchorpane constraints to have nodes fit their parent. Otherwise resizing the popup is just hell. As a matter of fact, most of the time the only width and height you should explictly set is that of the root. Your labels don't fit their text because of that.
  • Why do you need two listviews ? One is enough. Allow items to be selected and have a remove button available when your selection is non empty.

</TableView>
<TableView fx:id="fileTable" id="fileTable" layoutX="247.0" layoutY="30.0"
prefHeight="200.0" prefWidth="200.0">
<columns>
<TableColumn fx:id="fileAdd" prefWidth="199.0"
text="Files to be Added"/>
</columns>
</TableView>
<Button fx:id="removeFiles" layoutX="463.0" layoutY="98.0"
mnemonicParsing="false" text="Remove"/>
</children>
</Pane>
<Label layoutX="47.0" layoutY="78.0" prefHeight="17.0" prefWidth="58.0"
text="Classpath">
<effect>
<Blend/>
</effect>
</Label>
<Button fx:id="selectFile" layoutX="39.0" layoutY="32.0" mnemonicParsing="false"
text="Select Files"/>
</children>
</AnchorPane>
</content>
</TitledPane>
</top>
</BorderPane>
</children>
</AnchorPane>
Expand Up @@ -18,14 +18,16 @@
<?import javafx.scene.layout.BorderPane?>
<?import javafx.scene.layout.HBox?>
<?import javafx.scene.layout.Pane?>

<AnchorPane prefHeight="750.0" prefWidth="1200.0" stylesheets="@../css/designer.css" xmlns="http://javafx.com/javafx/9" xmlns:fx="http://javafx.com/fxml/1" fx:controller="net.sourceforge.pmd.util.fxdesigner.MainDesignerController">
<AnchorPane prefHeight="750.0" prefWidth="1200.0" stylesheets="@../css/designer.css"
xmlns="http://javafx.com/javafx/8.0.121" xmlns:fx="http://javafx.com/fxml/1"
fx:controller="net.sourceforge.pmd.util.fxdesigner.MainDesignerController">
<children>
<BorderPane prefHeight="600.0" prefWidth="900.0" AnchorPane.bottomAnchor="0.0" AnchorPane.leftAnchor="0.0" AnchorPane.rightAnchor="0.0" AnchorPane.topAnchor="0.0">
<top>
<AnchorPane>
<children>
<MenuBar maxHeight="20.0" prefHeight="20.0" AnchorPane.bottomAnchor="0.0" AnchorPane.leftAnchor="0.0" AnchorPane.rightAnchor="0.0" AnchorPane.topAnchor="0.0">
<MenuBar layoutX="-11.0" maxHeight="20.0" prefHeight="20.0" AnchorPane.bottomAnchor="0.0"
AnchorPane.leftAnchor="-11.0" AnchorPane.rightAnchor="11.0" AnchorPane.topAnchor="0.0">
<menus>
<Menu fx:id="fileMenu" mnemonicParsing="false" text="File">
<items>
Expand All @@ -42,6 +44,12 @@
<MenuItem fx:id="licenseMenuItem" mnemonicParsing="false" text="License" />
</items>
</Menu>
<Menu mnemonicParsing="false" text="PMD Configuration">
<items>
<MenuItem fx:id="pmdconfig" mnemonicParsing="false" text="AuxClassPath SetUp"/>
Copy link
Member

Choose a reason for hiding this comment

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

The naming here is sloppy too.

text="AuxClassPath SetUp"

  • The capitalisation is wrong
  • All other menu items are named with the "verb + object" convention, eg "Open file" or "Export rule". This menu item should follow the same convention, e.g. be named "Set up the auxclasspath".

fx:id="pmdconfig"

You're using the name of the Menu as the id of the MenuItem. FX ids also have a naming convention, that is, they should be camelCase, and be suffixed with their type. You can see eg licenseMenuItem a few lines above. Rename that to setupAuxclasspathMenuItem

Conventions are important, and even though they're usually implicit, you can usually pick them up just by looking around

</items>
</Menu>

</menus>
</MenuBar>
</children>
Expand Down